[PATCH 0/3] 32 bit PCI domain patches

[PATCH 0/3] 32 bit PCI domain patches

[Stephen Hemminger]

These patches address a potential future issue when devices are passed through
to DPDK applications on Azure. Since PCI bus is changing in current release
it makes sense to change now.

Stephen Hemminger (3):
  pci: remove unnecessary casts from strtoul
  eal: PCI domain should be 32 bits
  mlx5: handle 32 bit PCI domain

 drivers/net/mlx5/mlx5_ethdev.c          | 2 +-
 lib/librte_eal/common/include/rte_pci.h | 2 +-
 lib/librte_eal/linuxapp/eal/eal_pci.c   | 8 ++++----
 3 files changed, 6 insertions(+), 6 deletions(-)

-- 
2.11.0

[PATCH 1/3] pci: remove unnecessary casts from strtoul

[Stephen Hemminger]

The function strtoul returns unsigned long and can be directly
assigned to a smaller type. Removing the casts allows easier
expansion of PCI domain.

Signed-off-by: Stephen Hemminger <sthemmin@microsoft.com>
---
 lib/librte_eal/linuxapp/eal/eal_pci.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/lib/librte_eal/linuxapp/eal/eal_pci.c b/lib/librte_eal/linuxapp/eal/eal_pci.c
index 595622b2129f..cfd3a8e92839 100644
--- a/lib/librte_eal/linuxapp/eal/eal_pci.c
+++ b/lib/librte_eal/linuxapp/eal/eal_pci.c
@@ -430,10 +430,10 @@ parse_pci_addr_format(const char *buf, int bufsize, struct rte_pci_addr *addr)
 
 	/* now convert to int values */
 	errno = 0;
-	addr->domain = (uint16_t)strtoul(splitaddr.domain, NULL, 16);
-	addr->bus = (uint8_t)strtoul(splitaddr.bus, NULL, 16);
-	addr->devid = (uint8_t)strtoul(splitaddr.devid, NULL, 16);
-	addr->function = (uint8_t)strtoul(splitaddr.function, NULL, 10);
+	addr->domain = strtoul(splitaddr.domain, NULL, 16);
+	addr->bus = strtoul(splitaddr.bus, NULL, 16);
+	addr->devid = strtoul(splitaddr.devid, NULL, 16);
+	addr->function = strtoul(splitaddr.function, NULL, 10);
 	if (errno != 0)
 		goto error;
 
-- 
2.11.0

[PATCH 2/3] eal: PCI domain should be 32 bits

[Stephen Hemminger]

In some environments, the PCI domain can be larger than 16 bits.
For example, a PCI device passed through in Azure gets a synthetic domain
id  which is internally generated based on GUID. The PCI standard does
not restrict domain to be 16 bits.

This change breaks ABI for API's that expose PCI address structure.

Signed-off-by: Stephen Hemminger <sthemmin@microsoft.com>
---
 lib/librte_eal/common/include/rte_pci.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/librte_eal/common/include/rte_pci.h b/lib/librte_eal/common/include/rte_pci.h
index 0284a6208aa5..8b549aadfbe6 100644
--- a/lib/librte_eal/common/include/rte_pci.h
+++ b/lib/librte_eal/common/include/rte_pci.h
@@ -112,7 +112,7 @@ struct rte_pci_id {
  * A structure describing the location of a PCI device.
  */
 struct rte_pci_addr {
-	uint16_t domain;                /**< Device domain */
+	uint32_t domain;                /**< Device domain */
 	uint8_t bus;                    /**< Device bus */
 	uint8_t devid;                  /**< Device ID */
 	uint8_t function;               /**< Device function. */
-- 
2.11.0

Re: [PATCH 2/3] eal: PCI domain should be 32 bits

[Chang, Cunyin]

I think the series patches does not cover all area which need to adapt to u32 PCI domain,
We still need some other work to do:
we need define another macro such as PCI_PRI_FMT. Something like:
#define PCI_XXX_PRI_FMT "%.5" PRIx32 ":%.2" PRIx8 ":%.2" PRIx8 ".%" PRIx8

PCI_PRI_STR_SIZE also need to be modified:
#define PCI_PRI_STR_SIZE sizeof("XXXXX:XX:XX.X")

The macro PCI_PRI_FMT will not works if
The domain exceed 16bits. It will impact the following functions:
1  RTE_LOG function, there a lots of RTE_LOG such as:
RTE_LOG(WARNING, EAL,
			"Requested device " PCI_PRI_FMT " cannot be used\n",
			addr->domain, addr->bus, addr->devid, addr->function);
2  pci_dump_one_device().
3 rte_eal_pci_device_name()
4 pci_update_device()
5 pci_ioport_map()
6 pci_get_uio_dev()
7 pci_uio_map_resource_by_index()
8 pci_uio_ioport_map()
9 pci_vfio_map_resource()
10 pci_vfio_unmap_resource()
All the above functions will related with the macro PCI_PRI_FMT, so I think they need to be modified too.

There are some other code need modify:
In function rte_eal_compare_pci_addr(), we need do the following work:
dev_addr = ((uint64_t)addr->domain << 24) | ((uint64_t)addr->bus << 16) |
				((uint64_t)addr->devid << 8) | (uint64_t)addr->function;
dev_addr2 = ((uint64_t)addr2->domain << 24) | ((uint64_t)addr2->bus << 16) |
				((uint64_t)addr2->devid << 8) | (uint64_t)addr2->function;

In function eal_parse_pci_BDF(), we need do the following work:
GET_PCIADDR_FIELD(input, dev_addr->domain, UINT32_MAX, ':');

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Stephen
> Hemminger
> Sent: Thursday, June 22, 2017 12:36 AM
> To: dev@dpdk.org
> Cc: Stephen Hemminger <stephen@networkplumber.org>; Stephen
> Hemminger <sthemmin@microsoft.com>
> Subject: [dpdk-dev] [PATCH 2/3] eal: PCI domain should be 32 bits
> 
> In some environments, the PCI domain can be larger than 16 bits.
> For example, a PCI device passed through in Azure gets a synthetic domain id
> which is internally generated based on GUID. The PCI standard does not
> restrict domain to be 16 bits.
> 
> This change breaks ABI for API's that expose PCI address structure.
> 
> Signed-off-by: Stephen Hemminger <sthemmin@microsoft.com>
> ---
>  lib/librte_eal/common/include/rte_pci.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/lib/librte_eal/common/include/rte_pci.h
> b/lib/librte_eal/common/include/rte_pci.h
> index 0284a6208aa5..8b549aadfbe6 100644
> --- a/lib/librte_eal/common/include/rte_pci.h
> +++ b/lib/librte_eal/common/include/rte_pci.h
> @@ -112,7 +112,7 @@ struct rte_pci_id {
>   * A structure describing the location of a PCI device.
>   */
>  struct rte_pci_addr {
> -	uint16_t domain;                /**< Device domain */
> +	uint32_t domain;                /**< Device domain */
>  	uint8_t bus;                    /**< Device bus */
>  	uint8_t devid;                  /**< Device ID */
>  	uint8_t function;               /**< Device function. */
> --
> 2.11.0

Re: [PATCH 2/3] eal: PCI domain should be 32 bits

[Stephen Hemminger]

On Thu, 22 Jun 2017 09:28:31 +0000
"Chang, Cunyin" <cunyin.chang@intel.com> wrote:

> I think the series patches does not cover all area which need to adapt to u32 PCI domain,
> We still need some other work to do:
> we need define another macro such as PCI_PRI_FMT. Something like:
> #define PCI_XXX_PRI_FMT "%.5" PRIx32 ":%.2" PRIx8 ":%.2" PRIx8 ".%" PRIx8
> 
> PCI_PRI_STR_SIZE also need to be modified:
> #define PCI_PRI_STR_SIZE sizeof("XXXXX:XX:XX.X")
> 
> The macro PCI_PRI_FMT will not works if
> The domain exceed 16bits. It will impact the following functions:
> 1  RTE_LOG function, there a lots of RTE_LOG such as:
> RTE_LOG(WARNING, EAL,
> 			"Requested device " PCI_PRI_FMT " cannot be used\n",
> 			addr->domain, addr->bus, addr->devid, addr->function);
> 2  pci_dump_one_device().
> 3 rte_eal_pci_device_name()
> 4 pci_update_device()
> 5 pci_ioport_map()
> 6 pci_get_uio_dev()
> 7 pci_uio_map_resource_by_index()
> 8 pci_uio_ioport_map()
> 9 pci_vfio_map_resource()
> 10 pci_vfio_unmap_resource()
> All the above functions will related with the macro PCI_PRI_FMT, so I think they need to be modified too.
> 
> There are some other code need modify:
> In function rte_eal_compare_pci_addr(), we need do the following work:
> dev_addr = ((uint64_t)addr->domain << 24) | ((uint64_t)addr->bus << 16) |
> 				((uint64_t)addr->devid << 8) | (uint64_t)addr->function;
> dev_addr2 = ((uint64_t)addr2->domain << 24) | ((uint64_t)addr2->bus << 16) |
> 				((uint64_t)addr2->devid << 8) | (uint64_t)addr2->function;
> 
> In function eal_parse_pci_BDF(), we need do the following work:
> GET_PCIADDR_FIELD(input, dev_addr->domain, UINT32_MAX, ':');

Good catch, the string size must be increased.

It turns out that you don't need to change the PCI print format. Printing the domain with %.4x
works correctly with 32 bit. It just gets wider. This is how pciutils works, so no change
is necessary there.

For normal 16 bit domain, the print will be:
	0000:05.00.0
and for these 32 bit values
	100000:05:00.0

Yes, the compare needs domain cast to 64 bit. The bus/devid/function don't need cast.

Re: [PATCH 2/3] eal: PCI domain should be 32 bits

[Chang, Cunyin]

> -----Original Message-----
> From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> Sent: Thursday, June 22, 2017 11:52 PM
> To: Chang, Cunyin <cunyin.chang@intel.com>
> Cc: dev@dpdk.org; Stephen Hemminger <sthemmin@microsoft.com>
> Subject: Re: [dpdk-dev] [PATCH 2/3] eal: PCI domain should be 32 bits
> 
> On Thu, 22 Jun 2017 09:28:31 +0000
> "Chang, Cunyin" <cunyin.chang@intel.com> wrote:
> 
> > I think the series patches does not cover all area which need to adapt
> > to u32 PCI domain, We still need some other work to do:
> > we need define another macro such as PCI_PRI_FMT. Something like:
> > #define PCI_XXX_PRI_FMT "%.5" PRIx32 ":%.2" PRIx8 ":%.2" PRIx8 ".%"
> > PRIx8
> >
> > PCI_PRI_STR_SIZE also need to be modified:
> > #define PCI_PRI_STR_SIZE sizeof("XXXXX:XX:XX.X")
> >
> > The macro PCI_PRI_FMT will not works if The domain exceed 16bits. It
> > will impact the following functions:
> > 1  RTE_LOG function, there a lots of RTE_LOG such as:
> > RTE_LOG(WARNING, EAL,
> > 			"Requested device " PCI_PRI_FMT " cannot be
> used\n",
> > 			addr->domain, addr->bus, addr->devid, addr-
> >function);
> > 2  pci_dump_one_device().
> > 3 rte_eal_pci_device_name()
> > 4 pci_update_device()
> > 5 pci_ioport_map()
> > 6 pci_get_uio_dev()
> > 7 pci_uio_map_resource_by_index()
> > 8 pci_uio_ioport_map()
> > 9 pci_vfio_map_resource()
> > 10 pci_vfio_unmap_resource()
> > All the above functions will related with the macro PCI_PRI_FMT, so I think
> they need to be modified too.
> >
> > There are some other code need modify:
> > In function rte_eal_compare_pci_addr(), we need do the following work:
> > dev_addr = ((uint64_t)addr->domain << 24) | ((uint64_t)addr->bus << 16) |
> > 				((uint64_t)addr->devid << 8) |
> (uint64_t)addr->function;
> > dev_addr2 = ((uint64_t)addr2->domain << 24) | ((uint64_t)addr2->bus <<
> 16) |
> > 				((uint64_t)addr2->devid << 8) |
> (uint64_t)addr2->function;
> >
> > In function eal_parse_pci_BDF(), we need do the following work:
> > GET_PCIADDR_FIELD(input, dev_addr->domain, UINT32_MAX, ':');
> 
> Good catch, the string size must be increased.
> 
> It turns out that you don't need to change the PCI print format. Printing the
> domain with %.4x works correctly with 32 bit. It just gets wider. This is how
> pciutils works, so no change is necessary there.

I suppose we should use %4x, not %.4x?, the %.4x will cut the 10000:05:00.0 as 0000:05:00.0.
So the macro:
#define PCI_PRI_FMT "%.4" PRIx32 ":%.2" PRIx8 ":%.2" PRIx8 ".%" PRIx8
Should be:
#define PCI_PRI_FMT "%4" PRIx32 ":%.2" PRIx8 ":%.2" PRIx8 ".%" PRIx8

Make sense?

> 
> For normal 16 bit domain, the print will be:
> 	0000:05.00.0
> and for these 32 bit values
> 	100000:05:00.0
> 
> Yes, the compare needs domain cast to 64 bit. The bus/devid/function don't
> need cast.

Re: [PATCH 2/3] eal: PCI domain should be 32 bits

[Stephen Hemminger]

On Fri, 23 Jun 2017 00:41:43 +0000
"Chang, Cunyin" <cunyin.chang@intel.com> wrote:

> > -----Original Message-----
> > From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> > Sent: Thursday, June 22, 2017 11:52 PM
> > To: Chang, Cunyin <cunyin.chang@intel.com>
> > Cc: dev@dpdk.org; Stephen Hemminger <sthemmin@microsoft.com>
> > Subject: Re: [dpdk-dev] [PATCH 2/3] eal: PCI domain should be 32 bits
> > 
> > On Thu, 22 Jun 2017 09:28:31 +0000
> > "Chang, Cunyin" <cunyin.chang@intel.com> wrote:
> >   
> > > I think the series patches does not cover all area which need to adapt
> > > to u32 PCI domain, We still need some other work to do:
> > > we need define another macro such as PCI_PRI_FMT. Something like:
> > > #define PCI_XXX_PRI_FMT "%.5" PRIx32 ":%.2" PRIx8 ":%.2" PRIx8 ".%"
> > > PRIx8
> > >
> > > PCI_PRI_STR_SIZE also need to be modified:
> > > #define PCI_PRI_STR_SIZE sizeof("XXXXX:XX:XX.X")
> > >
> > > The macro PCI_PRI_FMT will not works if The domain exceed 16bits. It
> > > will impact the following functions:
> > > 1  RTE_LOG function, there a lots of RTE_LOG such as:
> > > RTE_LOG(WARNING, EAL,
> > > 			"Requested device " PCI_PRI_FMT " cannot be  
> > used\n",  
> > > 			addr->domain, addr->bus, addr->devid, addr-
> > >function);
> > > 2  pci_dump_one_device().
> > > 3 rte_eal_pci_device_name()
> > > 4 pci_update_device()
> > > 5 pci_ioport_map()
> > > 6 pci_get_uio_dev()
> > > 7 pci_uio_map_resource_by_index()
> > > 8 pci_uio_ioport_map()
> > > 9 pci_vfio_map_resource()
> > > 10 pci_vfio_unmap_resource()
> > > All the above functions will related with the macro PCI_PRI_FMT, so I think  
> > they need to be modified too.  
> > >
> > > There are some other code need modify:
> > > In function rte_eal_compare_pci_addr(), we need do the following work:
> > > dev_addr = ((uint64_t)addr->domain << 24) | ((uint64_t)addr->bus << 16) |
> > > 				((uint64_t)addr->devid << 8) |  
> > (uint64_t)addr->function;  
> > > dev_addr2 = ((uint64_t)addr2->domain << 24) | ((uint64_t)addr2->bus <<  
> > 16) |  
> > > 				((uint64_t)addr2->devid << 8) |  
> > (uint64_t)addr2->function;  
> > >
> > > In function eal_parse_pci_BDF(), we need do the following work:
> > > GET_PCIADDR_FIELD(input, dev_addr->domain, UINT32_MAX, ':');  
> > 
> > Good catch, the string size must be increased.
> > 
> > It turns out that you don't need to change the PCI print format. Printing the
> > domain with %.4x works correctly with 32 bit. It just gets wider. This is how
> > pciutils works, so no change is necessary there.  
> 
> I suppose we should use %4x, not %.4x?, the %.4x will cut the 10000:05:00.0 as 0000:05:00.0.
> So the macro:
> #define PCI_PRI_FMT "%.4" PRIx32 ":%.2" PRIx8 ":%.2" PRIx8 ".%" PRIx8
> Should be:
> #define PCI_PRI_FMT "%4" PRIx32 ":%.2" PRIx8 ":%.2" PRIx8 ".%" PRIx8
> 
> Make sense?

No, that format would not be correct. I want to keep the visible output the
same for the normal case of 16 bit domains.  Output of printf test program
shows that %.4x is the correct format to use.

Domain            %4x       %.4x      %4.4x
0                   0       0000       0000
0x1                 1       0001       0001
0x1000           1000       1000       1000
0x10000         10000      10000      10000
0x12345678   12345678   12345678   12345678
0xdeadbeef   deadbeef   deadbeef   deadbeef

Re: [PATCH 2/3] eal: PCI domain should be 32 bits

[Chang, Cunyin]

> -----Original Message-----
> From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> Sent: Saturday, June 24, 2017 1:47 AM
> To: Chang, Cunyin <cunyin.chang@intel.com>
> Cc: dev@dpdk.org; Stephen Hemminger <sthemmin@microsoft.com>
> Subject: Re: [dpdk-dev] [PATCH 2/3] eal: PCI domain should be 32 bits
> 
> On Fri, 23 Jun 2017 00:41:43 +0000
> "Chang, Cunyin" <cunyin.chang@intel.com> wrote:
> 
> > > -----Original Message-----
> > > From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> > > Sent: Thursday, June 22, 2017 11:52 PM
> > > To: Chang, Cunyin <cunyin.chang@intel.com>
> > > Cc: dev@dpdk.org; Stephen Hemminger <sthemmin@microsoft.com>
> > > Subject: Re: [dpdk-dev] [PATCH 2/3] eal: PCI domain should be 32
> > > bits
> > >
> > > On Thu, 22 Jun 2017 09:28:31 +0000
> > > "Chang, Cunyin" <cunyin.chang@intel.com> wrote:
> > >
> > > > I think the series patches does not cover all area which need to
> > > > adapt to u32 PCI domain, We still need some other work to do:
> > > > we need define another macro such as PCI_PRI_FMT. Something like:
> > > > #define PCI_XXX_PRI_FMT "%.5" PRIx32 ":%.2" PRIx8 ":%.2" PRIx8 ".%"
> > > > PRIx8
> > > >
> > > > PCI_PRI_STR_SIZE also need to be modified:
> > > > #define PCI_PRI_STR_SIZE sizeof("XXXXX:XX:XX.X")
> > > >
> > > > The macro PCI_PRI_FMT will not works if The domain exceed 16bits.
> > > > It will impact the following functions:
> > > > 1  RTE_LOG function, there a lots of RTE_LOG such as:
> > > > RTE_LOG(WARNING, EAL,
> > > > 			"Requested device " PCI_PRI_FMT " cannot be
> > > used\n",
> > > > 			addr->domain, addr->bus, addr->devid, addr-
> function);
> > > > 2  pci_dump_one_device().
> > > > 3 rte_eal_pci_device_name()
> > > > 4 pci_update_device()
> > > > 5 pci_ioport_map()
> > > > 6 pci_get_uio_dev()
> > > > 7 pci_uio_map_resource_by_index()
> > > > 8 pci_uio_ioport_map()
> > > > 9 pci_vfio_map_resource()
> > > > 10 pci_vfio_unmap_resource()
> > > > All the above functions will related with the macro PCI_PRI_FMT,
> > > >so I think
> > > they need to be modified too.
> > > >
> > > > There are some other code need modify:
> > > > In function rte_eal_compare_pci_addr(), we need do the following
> work:
> > > > dev_addr = ((uint64_t)addr->domain << 24) | ((uint64_t)addr->bus <<
> 16) |
> > > > 				((uint64_t)addr->devid << 8) |
> > > (uint64_t)addr->function;
> > > > dev_addr2 = ((uint64_t)addr2->domain << 24) |
> > > > ((uint64_t)addr2->bus <<
> > > 16) |
> > > > 				((uint64_t)addr2->devid << 8) |
> > > (uint64_t)addr2->function;
> > > >
> > > > In function eal_parse_pci_BDF(), we need do the following work:
> > > > GET_PCIADDR_FIELD(input, dev_addr->domain, UINT32_MAX, ':');
> > >
> > > Good catch, the string size must be increased.
> > >
> > > It turns out that you don't need to change the PCI print format.
> > > Printing the domain with %.4x works correctly with 32 bit. It just
> > > gets wider. This is how pciutils works, so no change is necessary there.
> >
> > I suppose we should use %4x, not %.4x?, the %.4x will cut the
> 10000:05:00.0 as 0000:05:00.0.
> > So the macro:
> > #define PCI_PRI_FMT "%.4" PRIx32 ":%.2" PRIx8 ":%.2" PRIx8 ".%" PRIx8
> > Should be:
> > #define PCI_PRI_FMT "%4" PRIx32 ":%.2" PRIx8 ":%.2" PRIx8 ".%" PRIx8
> >
> > Make sense?
> 
> No, that format would not be correct. I want to keep the visible output the
> same for the normal case of 16 bit domains.  Output of printf test program
> shows that %.4x is the correct format to use.
> 
> Domain            %4x       %.4x      %4.4x
> 0                   0       0000       0000
> 0x1                 1       0001       0001
> 0x1000           1000       1000       1000
> 0x10000         10000      10000      10000
> 0x12345678   12345678   12345678   12345678
> 0xdeadbeef   deadbeef   deadbeef   deadbeef
> 
Looks good. No more questions about this.

[PATCH 3/3] mlx5: handle 32 bit PCI domain

[Stephen Hemminger]

The PCI domain in Azure maybe 32 bits. When device is passed through
the domain is synthesize from the internal GUID.

Signed-off-by: Stephen Hemminger <sthemmin@microsoft.com>
---
 drivers/net/mlx5/mlx5_ethdev.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/mlx5/mlx5_ethdev.c b/drivers/net/mlx5/mlx5_ethdev.c
index eadf452b9e95..6a5733354b2c 100644
--- a/drivers/net/mlx5/mlx5_ethdev.c
+++ b/drivers/net/mlx5/mlx5_ethdev.c
@@ -1185,7 +1185,7 @@ mlx5_ibv_device_to_pci_addr(const struct ibv_device *device,
 		/* Extract information. */
 		if (sscanf(line,
 			   "PCI_SLOT_NAME="
-			   "%" SCNx16 ":%" SCNx8 ":%" SCNx8 ".%" SCNx8 "\n",
+			   "%" SCNx32 ":%" SCNx8 ":%" SCNx8 ".%" SCNx8 "\n",
 			   &pci_addr->domain,
 			   &pci_addr->bus,
 			   &pci_addr->devid,
-- 
2.11.0