The parsing code was bailing on domains greater than UINT16_MAX, but domain numbers like that are still valid and present on some systems. One example is Intel VMD (Volume Management Device), which acts somewhat as a software-managed PCI switch and its upstream linux driver assigns all downstream devices a PCI domain of 0x10000. Parsing a BDF like 10000:01:00.0 was failing before. To fix it, increase the upper limit of domain number to UINT32_MAX. This matches the size of struct rte_pci_addr->domain (uint32). Signed-off-by: Darek Stojaczyk <dariusz.stojaczyk@intel.com> --- lib/librte_pci/rte_pci.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/librte_pci/rte_pci.c b/lib/librte_pci/rte_pci.c index d1ab6b414d..ad2cdfebb2 100644 --- a/lib/librte_pci/rte_pci.c +++ b/lib/librte_pci/rte_pci.c @@ -72,9 +72,9 @@ pci_dbdf_parse(const char *input, struct rte_pci_addr *dev_addr) errno = 0; val = strtoul(in, &end, 16); - if (errno != 0 || end[0] != ':' || val > UINT16_MAX) + if (errno != 0 || end[0] != ':' || val > UINT32_MAX) return -EINVAL; - dev_addr->domain = (uint16_t)val; + dev_addr->domain = (uint32_t)val; in = end + 1; in = get_u8_pciaddr_field(in, &dev_addr->bus, ':'); if (in == NULL) -- 2.17.1
On 12-May-20 2:30 PM, Darek Stojaczyk wrote:
> The parsing code was bailing on domains greater than UINT16_MAX,
> but domain numbers like that are still valid and present on some systems.
> One example is Intel VMD (Volume Management Device), which acts somewhat
> as a software-managed PCI switch and its upstream linux driver assigns
> all downstream devices a PCI domain of 0x10000.
>
> Parsing a BDF like 10000:01:00.0 was failing before. To fix it, increase
> the upper limit of domain number to UINT32_MAX. This matches the size of
> struct rte_pci_addr->domain (uint32).
>
> Signed-off-by: Darek Stojaczyk <dariusz.stojaczyk@intel.com>
> ---
Cc: stable?
--
Thanks,
Anatoly
On Tue, May 12, 2020 at 3:30 pm, Darek Stojaczyk <dariusz.stojaczyk@intel.com> wrote: > The parsing code was bailing on domains greater than UINT16_MAX, > but domain numbers like that are still valid and present on some > systems. > One example is Intel VMD (Volume Management Device), which acts > somewhat > as a software-managed PCI switch and its upstream linux driver assigns > all downstream devices a PCI domain of 0x10000. > > Parsing a BDF like 10000:01:00.0 was failing before. To fix it, > increase > the upper limit of domain number to UINT32_MAX. This matches the size > of > struct rte_pci_addr->domain (uint32). > > Signed-off-by: Darek Stojaczyk <dariusz.stojaczyk@intel.com > <mailto:dariusz.stojaczyk@intel.com>> > --- > lib/librte_pci/rte_pci.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/lib/librte_pci/rte_pci.c b/lib/librte_pci/rte_pci.c > index d1ab6b414d..ad2cdfebb2 100644 > --- a/lib/librte_pci/rte_pci.c > +++ b/lib/librte_pci/rte_pci.c > @@ -72,9 +72,9 @@ pci_dbdf_parse(const char *input, struct > rte_pci_addr *dev_addr) > > errno = 0; > val = strtoul(in, &end, 16); > - if (errno != 0 || end[0] != ':' || val > UINT16_MAX) > + if (errno != 0 || end[0] != ':' || val > UINT32_MAX) > return -EINVAL; > - dev_addr->domain = (uint16_t)val; > + dev_addr->domain = (uint32_t)val; > in = end + 1; > in = get_u8_pciaddr_field(in, &dev_addr->bus, ':'); > if (in == NULL) > -- > 2.17.1 Agree this came up before on Hyper-V as well. It meant fixing libpci. Not sure the cast of val is necessary, other than an attempt to silence some static checker about implicit type conversion causing loss of precision. >
On 12/05/20 11:16 -0700, Stephen Hemminger wrote: > > > On Tue, May 12, 2020 at 3:30 pm, Darek Stojaczyk > <dariusz.stojaczyk@intel.com> wrote: > > The parsing code was bailing on domains greater than UINT16_MAX, > > but domain numbers like that are still valid and present on some > > systems. > > One example is Intel VMD (Volume Management Device), which acts somewhat > > as a software-managed PCI switch and its upstream linux driver assigns > > all downstream devices a PCI domain of 0x10000. > > > > Parsing a BDF like 10000:01:00.0 was failing before. To fix it, increase > > the upper limit of domain number to UINT32_MAX. This matches the size of > > struct rte_pci_addr->domain (uint32). > > > > Signed-off-by: Darek Stojaczyk <dariusz.stojaczyk@intel.com > > <mailto:dariusz.stojaczyk@intel.com>> The original code predates the change from macro in commit c742e8d3110b. Fixes: af75078fece3 ("first public release") Cc: stable@dpdk.org Thanks for the fix, Acked-by: Gaetan Rivet <grive@u256.net> > > --- > > lib/librte_pci/rte_pci.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/lib/librte_pci/rte_pci.c b/lib/librte_pci/rte_pci.c > > index d1ab6b414d..ad2cdfebb2 100644 > > --- a/lib/librte_pci/rte_pci.c > > +++ b/lib/librte_pci/rte_pci.c > > @@ -72,9 +72,9 @@ pci_dbdf_parse(const char *input, struct rte_pci_addr > > *dev_addr) > > > > errno = 0; > > val = strtoul(in, &end, 16); > > - if (errno != 0 || end[0] != ':' || val > UINT16_MAX) > > + if (errno != 0 || end[0] != ':' || val > UINT32_MAX) > > return -EINVAL; > > - dev_addr->domain = (uint16_t)val; > > + dev_addr->domain = (uint32_t)val; > > in = end + 1; > > in = get_u8_pciaddr_field(in, &dev_addr->bus, ':'); > > if (in == NULL) > > -- > > 2.17.1 > > > Agree this came up before on Hyper-V as well. It meant fixing libpci. > > Not sure the cast of val is necessary, other than an attempt to silence some > static checker > about implicit type conversion causing loss of precision. > > > > > The cast is useless indeed. Remnants from the original macro. For now best to leave it as-is, make another patch to remove those casts. There are other potential bugs in parsing, -FFFFFFFFFFFF0001 is considered valid (-FFFFFFFF00000001 with this patch) as well as an empty domain. I will send a fix for those. -- Gaëtan
> -----Original Message----- > From: Gaëtan Rivet <grive@u256.net> > Sent: Wednesday, May 13, 2020 11:04 AM > To: Stephen Hemminger <stephen@networkplumber.org> > Cc: Stojaczyk, Dariusz <dariusz.stojaczyk@intel.com>; dev@dpdk.org > Subject: Re: [dpdk-dev] [PATCH] pci: properly parse 32-bit domain numbers > > [SNIP] > > The original code predates the change from macro in commit c742e8d3110b. > > Fixes: af75078fece3 ("first public release") > Cc: stable@dpdk.org > > Thanks for the fix, > Acked-by: Gaetan Rivet <grive@u256.net> > Yes, I wasn't sure if this is a fix or more of a feature. Well, it *could* be backported, I guess. D. > > > --- > > > lib/librte_pci/rte_pci.c | 4 ++-- > > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > > > diff --git a/lib/librte_pci/rte_pci.c b/lib/librte_pci/rte_pci.c > > > index d1ab6b414d..ad2cdfebb2 100644 > > > --- a/lib/librte_pci/rte_pci.c > > > +++ b/lib/librte_pci/rte_pci.c > > > @@ -72,9 +72,9 @@ pci_dbdf_parse(const char *input, struct rte_pci_addr > > > *dev_addr) > > > > > > errno = 0; > > > val = strtoul(in, &end, 16); > > > - if (errno != 0 || end[0] != ':' || val > UINT16_MAX) > > > + if (errno != 0 || end[0] != ':' || val > UINT32_MAX) > > > return -EINVAL; > > > - dev_addr->domain = (uint16_t)val; > > > + dev_addr->domain = (uint32_t)val; > > > in = end + 1; > > > in = get_u8_pciaddr_field(in, &dev_addr->bus, ':'); > > > if (in == NULL) > > > -- > > > 2.17.1 > > > > > > Agree this came up before on Hyper-V as well. It meant fixing libpci. > > > > Not sure the cast of val is necessary, other than an attempt to silence some > > static checker > > about implicit type conversion causing loss of precision. > > > > > > > > > > > The cast is useless indeed. Remnants from the original macro. > For now best to leave it as-is, make another patch to remove those > casts. > > There are other potential bugs in parsing, -FFFFFFFFFFFF0001 is > considered valid (-FFFFFFFF00000001 with this patch) as well as an empty > domain. > I will send a fix for those. > > -- > Gaëtan
On Tue, May 12, 2020 at 3:31 PM Darek Stojaczyk <dariusz.stojaczyk@intel.com> wrote: > > The parsing code was bailing on domains greater than UINT16_MAX, > but domain numbers like that are still valid and present on some systems. > One example is Intel VMD (Volume Management Device), which acts somewhat > as a software-managed PCI switch and its upstream linux driver assigns > all downstream devices a PCI domain of 0x10000. > > Parsing a BDF like 10000:01:00.0 was failing before. To fix it, increase > the upper limit of domain number to UINT32_MAX. This matches the size of > struct rte_pci_addr->domain (uint32). Fixes: af75078fece3 ("first public release") Cc: stable@dpdk.org > Signed-off-by: Darek Stojaczyk <dariusz.stojaczyk@intel.com> Acked-by: Gaetan Rivet <grive@u256.net> Applied, thanks. -- David Marchand