[PATCH v2 0/3] 32 bit PCI domain support

[PATCH v2 0/3] 32 bit PCI domain support

[Stephen Hemminger]

The PCI address consists of  domain, bus, device, function.
The domain value is limited to 16 bits when received from ACPI,
but on virtual environments the host may use the full 32 bits to
distinguish direct hardware from synthetic buses.

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 | 12 ++++++------
 lib/librte_eal/linuxapp/eal/eal_pci.c   |  8 ++++----
 3 files changed, 11 insertions(+), 11 deletions(-)

-- 
2.11.0

[PATCH v2 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 v2 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>
---
v2 -- need to expand string size and shifts in compare function

 lib/librte_eal/common/include/rte_pci.h | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/lib/librte_eal/common/include/rte_pci.h b/lib/librte_eal/common/include/rte_pci.h
index 0284a6208aa5..e416714b32ff 100644
--- a/lib/librte_eal/common/include/rte_pci.h
+++ b/lib/librte_eal/common/include/rte_pci.h
@@ -63,7 +63,7 @@ const char *pci_get_sysfs_path(void);
 
 /** Formatting string for PCI device identifier: Ex: 0000:00:01.0 */
 #define PCI_PRI_FMT "%.4" PRIx16 ":%.2" PRIx8 ":%.2" PRIx8 ".%" PRIx8
-#define PCI_PRI_STR_SIZE sizeof("XXXX:XX:XX.X")
+#define PCI_PRI_STR_SIZE sizeof("XXXXXXXX:XX:XX.X")
 
 /** Short formatting string, without domain, for PCI device: Ex: 00:01.0 */
 #define PCI_SHORT_PRI_FMT "%.2" PRIx8 ":%.2" PRIx8 ".%" PRIx8
@@ -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. */
@@ -346,10 +346,10 @@ rte_eal_compare_pci_addr(const struct rte_pci_addr *addr,
 	if ((addr == NULL) || (addr2 == NULL))
 		return -1;
 
-	dev_addr = (addr->domain << 24) | (addr->bus << 16) |
-				(addr->devid << 8) | addr->function;
-	dev_addr2 = (addr2->domain << 24) | (addr2->bus << 16) |
-				(addr2->devid << 8) | addr2->function;
+	dev_addr = ((uint64_t)addr->domain << 24) |
+		(addr->bus << 16) | (addr->devid << 8) | addr->function;
+	dev_addr2 = ((uint64_t)addr2->domain << 24) |
+		(addr2->bus << 16) | (addr2->devid << 8) | addr2->function;
 
 	if (dev_addr > dev_addr2)
 		return 1;
-- 
2.11.0

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

[Thomas Monjalon]

22/06/2017 17:56, 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>
> ---
[...]
> --- a/lib/librte_eal/common/include/rte_pci.h
> +++ b/lib/librte_eal/common/include/rte_pci.h
> @@ -63,7 +63,7 @@ const char *pci_get_sysfs_path(void);
>  
>  /** Formatting string for PCI device identifier: Ex: 0000:00:01.0 */
>  #define PCI_PRI_FMT "%.4" PRIx16 ":%.2" PRIx8 ":%.2" PRIx8 ".%" PRIx8
> -#define PCI_PRI_STR_SIZE sizeof("XXXX:XX:XX.X")
> +#define PCI_PRI_STR_SIZE sizeof("XXXXXXXX:XX:XX.X")

I think you need to change PCI_PRI_FMT accordingly.

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

[Stephen Hemminger]

On Thu, 22 Jun 2017 18:05:42 +0200
Thomas Monjalon <thomas@monjalon.net> wrote:

> 22/06/2017 17:56, 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>
> > ---  
> [...]
> > --- a/lib/librte_eal/common/include/rte_pci.h
> > +++ b/lib/librte_eal/common/include/rte_pci.h
> > @@ -63,7 +63,7 @@ const char *pci_get_sysfs_path(void);
> >  
> >  /** Formatting string for PCI device identifier: Ex: 0000:00:01.0 */
> >  #define PCI_PRI_FMT "%.4" PRIx16 ":%.2" PRIx8 ":%.2" PRIx8 ".%" PRIx8
> > -#define PCI_PRI_STR_SIZE sizeof("XXXX:XX:XX.X")
> > +#define PCI_PRI_STR_SIZE sizeof("XXXXXXXX:XX:XX.X")  
> 
> I think you need to change PCI_PRI_FMT accordingly.

No. I don't want all outputs to have extra leading zeros on other platforms
The existing format works:

Example:
--- cut here ---

struct rte_pci_addr {
	uint32_t domain;                /**< Device domain */
	uint8_t bus;                    /**< Device bus */
	uint8_t devid;                  /**< Device ID */
	uint8_t function;               /**< Device function. */
};

#define PCI_PRI_FMT "%.4" PRIx16 ":%.2" PRIx8 ":%.2" PRIx8 ".%" PRIx8

int main(void)
{
	struct rte_pci_addr pci_addr = { 0, 5, 0, 0 };

	printf(PCI_PRI_FMT "\n",
		       pci_addr.domain,
		       pci_addr.bus, pci_addr.devid,
		       pci_addr.function);

	pci_addr.domain = 0xdeadbeef;
	
	printf(PCI_PRI_FMT "\n",
		       pci_addr.domain,
		       pci_addr.bus, pci_addr.devid,
		       pci_addr.function);
	
	return 0;
}

--- output ---
0000:05:00.0
deadbeef:05:00.0

[PATCH v2 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

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

[Thomas Monjalon]

22/06/2017 17:56, 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(-)

Do you want to update other drivers, or should we ask to
PMD maintainers to do the update of their driver?

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

[Stephen Hemminger]

On Thu, 22 Jun 2017 18:17:38 +0200
Thomas Monjalon <thomas@monjalon.net> wrote:

> 22/06/2017 17:56, 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(-)  
> 
> Do you want to update other drivers, or should we ask to
> PMD maintainers to do the update of their driver?
> 

Only mlx5 driver used sscanf, others did not do that.

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

[Stephen Hemminger]

On Thu, 22 Jun 2017 18:17:38 +0200
Thomas Monjalon <thomas@monjalon.net> wrote:

> 22/06/2017 17:56, 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(-)  
> 
> Do you want to update other drivers, or should we ask to
> PMD maintainers to do the update of their driver?
> 

Looks like mlx4 is only other driver looking for PCI_SLOT_NAME

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

[Thomas Monjalon]

22/06/2017 18:42, Stephen Hemminger:
> On Thu, 22 Jun 2017 18:17:38 +0200
> Thomas Monjalon <thomas@monjalon.net> wrote:
> 
> > 22/06/2017 17:56, 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(-)  
> > 
> > Do you want to update other drivers, or should we ask to
> > PMD maintainers to do the update of their driver?
> > 
> 
> Looks like mlx4 is only other driver looking for PCI_SLOT_NAME

Yes the same change must be done for mlx4.