[dpdk-dev] [PATCH] bus/pci: nvme on Windows requires class id and bus

[dpdk-dev] [PATCH] bus/pci: nvme on Windows requires class id and bus

[Nick Connolly]

Attaching to an NVMe disk on Windows using SPDK requires the
PCI class ID and device.bus fields. Decode the class ID from the PCI
device info strings if it is present and set device.bus.

Signed-off-by: Nick Connolly <nick.connolly@mayadata.io>
---
 drivers/bus/pci/windows/pci.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/bus/pci/windows/pci.c b/drivers/bus/pci/windows/pci.c
index f66258452..3d11444c2 100644
--- a/drivers/bus/pci/windows/pci.c
+++ b/drivers/bus/pci/windows/pci.c
@@ -280,17 +280,24 @@ parse_pci_hardware_id(const char *buf, struct rte_pci_id *pci_id)
 {
 	int ids = 0;
 	uint16_t vendor_id, device_id;
-	uint32_t subvendor_id = 0;
+	uint32_t subvendor_id = 0, class_id = 0;
+	const char *cp;
 
 	ids = sscanf_s(buf, "PCI\\VEN_%" PRIx16 "&DEV_%" PRIx16 "&SUBSYS_%"
 		PRIx32, &vendor_id, &device_id, &subvendor_id);
 	if (ids != 3)
 		return -1;
 
+	/* Try and find PCI class ID */
+	for (cp = buf; !(cp[0] == 0 && cp[1] == 0); cp++)
+		if (*cp == '&' && sscanf_s(cp, "&CC_%" PRIx32, &class_id) == 1)
+			break;
+
 	pci_id->vendor_id = vendor_id;
 	pci_id->device_id = device_id;
 	pci_id->subsystem_device_id = subvendor_id >> 16;
 	pci_id->subsystem_vendor_id = subvendor_id & 0xffff;
+	pci_id->class_id = class_id;
 	return 0;
 }
 
@@ -339,6 +346,7 @@ pci_scan_one(HDEVINFO dev_info, PSP_DEVINFO_DATA device_info_data)
 	if (ret != 0)
 		goto end;
 
+	dev->device.bus = &rte_pci_bus.bus;
 	dev->addr = addr;
 	dev->id = pci_id;
 	dev->max_vfs = 0; /* TODO: get max_vfs */
-- 
2.25.1

Re: [dpdk-dev] [PATCH] bus/pci: nvme on Windows requires class id and bus

[Tal Shnaiderman]

> Subject: [PATCH] bus/pci: nvme on Windows requires class id and bus
> 
> External email: Use caution opening links or attachments
> 
> 
> Attaching to an NVMe disk on Windows using SPDK requires the PCI class ID
> and device.bus fields. Decode the class ID from the PCI device info strings if it
> is present and set device.bus.
> 
> Signed-off-by: Nick Connolly <nick.connolly@mayadata.io>
> ---
>  drivers/bus/pci/windows/pci.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/bus/pci/windows/pci.c b/drivers/bus/pci/windows/pci.c
> index f66258452..3d11444c2 100644
> --- a/drivers/bus/pci/windows/pci.c
> +++ b/drivers/bus/pci/windows/pci.c
> @@ -280,17 +280,24 @@ parse_pci_hardware_id(const char *buf, struct
> rte_pci_id *pci_id)  {
>         int ids = 0;
>         uint16_t vendor_id, device_id;
> -       uint32_t subvendor_id = 0;
> +       uint32_t subvendor_id = 0, class_id = 0;
> +       const char *cp;
> 
>         ids = sscanf_s(buf, "PCI\\VEN_%" PRIx16 "&DEV_%" PRIx16
> "&SUBSYS_%"
>                 PRIx32, &vendor_id, &device_id, &subvendor_id);
>         if (ids != 3)
>                 return -1;
> 
> +       /* Try and find PCI class ID */
> +       for (cp = buf; !(cp[0] == 0 && cp[1] == 0); cp++)

How about 
for (cp = buf; cp[0] || cp[1]; cp++)

> +               if (*cp == '&' && sscanf_s(cp, "&CC_%" PRIx32, &class_id) == 1)

Could there be a case where PCI\\VEN_v(4)&DEV_d(4)&CC_c(2)s(2) exist but PCI\\VEN_v(4)&DEV_d(4)&CC_c(2)s(2)p(2) doesn't? In that case the parsing would be incorrect.

> +                       break;
> +
>         pci_id->vendor_id = vendor_id;
>         pci_id->device_id = device_id;
>         pci_id->subsystem_device_id = subvendor_id >> 16;
>         pci_id->subsystem_vendor_id = subvendor_id & 0xffff;
> +       pci_id->class_id = class_id;
>         return 0;
>  }
> 
> @@ -339,6 +346,7 @@ pci_scan_one(HDEVINFO dev_info,
> PSP_DEVINFO_DATA device_info_data)
>         if (ret != 0)
>                 goto end;
> 
> +       dev->device.bus = &rte_pci_bus.bus;
>         dev->addr = addr;
>         dev->id = pci_id;
>         dev->max_vfs = 0; /* TODO: get max_vfs */
> --
> 2.25.1

Re: [dpdk-dev] [PATCH] bus/pci: nvme on Windows requires class id and bus

[Nick Connolly]

Hi Tal,

Thanks for the comments.

>> +       /* Try and find PCI class ID */
>> +       for (cp = buf; !(cp[0] == 0 && cp[1] == 0); cp++)
> How about
> for (cp = buf; cp[0] || cp[1]; cp++)
That would be my preferred idiom, but the DPDK coding style (1.9.1) says 
'do not use ! for tests unless it is a boolean' (but somewhat 
confusingly does so in a section on NULL pointers).  I interpreted it as 
a general prohibition on conditionals without an explicit operator 
(except for booleans).  I'd love to be corrected here!

>> +               if (*cp == '&' && sscanf_s(cp, "&CC_%" PRIx32, &class_id) == 1)
> Could there be a case where PCI\\VEN_v(4)&DEV_d(4)&CC_c(2)s(2) exist but PCI\\VEN_v(4)&DEV_d(4)&CC_c(2)s(2)p(2) doesn't? In that case the parsing would be incorrect.
The MSDN documentation says that the most specific string will be 
returned first, in this case &CC_c(2)s(2)p(2), so if there is a 6-digit 
class ID we will return it.  That left me wondering what to do if we 
only encounter a 4-digit class ID.  If we ignore it, then we won't be 
able to match on the class ID.  We could parse it as 4-digits and supply 
zero for the p(2) field, but that left me wondering why Windows wouldn't 
have already done that for us.  What I see on my own systems is that 
there is always a 6-digit class ID defined, even if the pci-ids 
repository doesn't list any values for p(2) (e.g. 
https://pci-ids.ucw.cz/read/PD/07/80 reports as &CC_078000).

My conclusion was that was probably best just to return the first class 
ID that Windows reports even if it's only 4-digits.  That way the device 
can still be found and will match the CC_ info displayed in device manager.

Thanks,
Nick

Re: [dpdk-dev] [PATCH] bus/pci: nvme on Windows requires class id and bus

[Thomas Monjalon]

26/01/2021 19:18, Nick Connolly:
> Hi Tal,
> 
> Thanks for the comments.
> 
> >> +       /* Try and find PCI class ID */
> >> +       for (cp = buf; !(cp[0] == 0 && cp[1] == 0); cp++)
> > How about
> > for (cp = buf; cp[0] || cp[1]; cp++)
> That would be my preferred idiom, but the DPDK coding style (1.9.1) says 
> 'do not use ! for tests unless it is a boolean' (but somewhat 
> confusingly does so in a section on NULL pointers).  I interpreted it as 
> a general prohibition on conditionals without an explicit operator 
> (except for booleans).  I'd love to be corrected here!

That's true. Comparisons should be explicit.

Re: [dpdk-dev] [PATCH] bus/pci: nvme on Windows requires class id and bus

[Nick Connolly]

On 26/01/2021 22:41, Thomas Monjalon wrote:
> That's true. Comparisons should be explicit.

Thanks Thomas,
Nick

Re: [dpdk-dev] [PATCH] bus/pci: nvme on Windows requires class id and bus

[Tal Shnaiderman]

> Subject: Re: [PATCH] bus/pci: nvme on Windows requires class id and bus
> 
> External email: Use caution opening links or attachments
> 
> 
> Hi Tal,
> 
> Thanks for the comments.
> 
> >> +       /* Try and find PCI class ID */
> >> +       for (cp = buf; !(cp[0] == 0 && cp[1] == 0); cp++)
> > How about
> > for (cp = buf; cp[0] || cp[1]; cp++)
> That would be my preferred idiom, but the DPDK coding style (1.9.1) says 'do
> not use ! for tests unless it is a boolean' (but somewhat confusingly does so
> in a section on NULL pointers).  I interpreted it as a general prohibition on
> conditionals without an explicit operator (except for booleans).  I'd love to be
> corrected here!
> 
> >> +               if (*cp == '&' && sscanf_s(cp, "&CC_%" PRIx32,
> >> + &class_id) == 1)
> > Could there be a case where PCI\\VEN_v(4)&DEV_d(4)&CC_c(2)s(2) exist
> but PCI\\VEN_v(4)&DEV_d(4)&CC_c(2)s(2)p(2) doesn't? In that case the
> parsing would be incorrect.
> The MSDN documentation says that the most specific string will be returned
> first, in this case &CC_c(2)s(2)p(2), so if there is a 6-digit class ID we will
> return it.  That left me wondering what to do if we only encounter a 4-digit
> class ID.  If we ignore it, then we won't be able to match on the class ID.  We
> could parse it as 4-digits and supply zero for the p(2) field, but that left me
> wondering why Windows wouldn't have already done that for us.  What I see
> on my own systems is that there is always a 6-digit class ID defined, even if
> the pci-ids repository doesn't list any values for p(2) (e.g.
> https://pci-ids.ucw.cz/read/PD/07/80 reports as &CC_078000).
> 
> My conclusion was that was probably best just to return the first class ID that
> Windows reports even if it's only 4-digits.  That way the device can still be
> found and will match the CC_ info displayed in device manager.

I'd shift the class_id value to the right in case the returned value is 4 digit, just to verify the result would be "c(2)s(2) 0" and not "0 s(2)p(2)", however when testing your change I couldn't find a case where only 4 digits matched.

Either than that LGTM.
> 
> Thanks,
> Nick

[dpdk-dev] [PATCH v2] bus/pci: nvme on Windows requires class id and bus

[Nick Connolly]

Attaching to an NVMe disk on Windows using SPDK requires the
PCI class ID and device.bus fields. Decode the class ID from the PCI
device info strings if it is present and set device.bus.

Signed-off-by: Nick Connolly <nick.connolly@mayadata.io>
---
 drivers/bus/pci/windows/pci.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/drivers/bus/pci/windows/pci.c b/drivers/bus/pci/windows/pci.c
index f66258452..d380fc1ae 100644
--- a/drivers/bus/pci/windows/pci.c
+++ b/drivers/bus/pci/windows/pci.c
@@ -280,17 +280,29 @@ parse_pci_hardware_id(const char *buf, struct rte_pci_id *pci_id)
 {
 	int ids = 0;
 	uint16_t vendor_id, device_id;
-	uint32_t subvendor_id = 0;
+	uint32_t subvendor_id = 0, class_id = 0;
+	const char *cp;
 
 	ids = sscanf_s(buf, "PCI\\VEN_%" PRIx16 "&DEV_%" PRIx16 "&SUBSYS_%"
 		PRIx32, &vendor_id, &device_id, &subvendor_id);
 	if (ids != 3)
 		return -1;
 
+	/* Try and find PCI class ID */
+	for (cp = buf; !(cp[0] == 0 && cp[1] == 0); cp++)
+		if (*cp == '&' && sscanf_s(cp,
+				"&CC_%" PRIx32, &class_id) == 1) {
+			/* Convert 4-digit class IDs to 6-digit format */
+			if (strspn(cp + 4, "0123456789abcdefABCDEF") == 4)
+				class_id <<= 8;
+			break;
+		}
+
 	pci_id->vendor_id = vendor_id;
 	pci_id->device_id = device_id;
 	pci_id->subsystem_device_id = subvendor_id >> 16;
 	pci_id->subsystem_vendor_id = subvendor_id & 0xffff;
+	pci_id->class_id = class_id;
 	return 0;
 }
 
@@ -339,6 +351,7 @@ pci_scan_one(HDEVINFO dev_info, PSP_DEVINFO_DATA device_info_data)
 	if (ret != 0)
 		goto end;
 
+	dev->device.bus = &rte_pci_bus.bus;
 	dev->addr = addr;
 	dev->id = pci_id;
 	dev->max_vfs = 0; /* TODO: get max_vfs */
-- 
2.25.1

v2:
* If only a 4-digit class ID is available, convert it to 6-digit format

[dpdk-dev] [PATCH v3] bus/pci: nvme on Windows requires class id and bus

[Nick Connolly]

Attaching to an NVMe disk on Windows using SPDK requires the
PCI class ID and device.bus fields. Decode the class ID from the PCI
device info strings if it is present and set device.bus.

Signed-off-by: Nick Connolly <nick.connolly@mayadata.io>
---
v3:
* Put version history at top - v2 mistakenly had it after the diffs

v2:
* If only a 4-digit class ID is available, convert it to 6-digit format

 drivers/bus/pci/windows/pci.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/drivers/bus/pci/windows/pci.c b/drivers/bus/pci/windows/pci.c
index f66258452..d380fc1ae 100644
--- a/drivers/bus/pci/windows/pci.c
+++ b/drivers/bus/pci/windows/pci.c
@@ -280,17 +280,29 @@ parse_pci_hardware_id(const char *buf, struct rte_pci_id *pci_id)
 {
 	int ids = 0;
 	uint16_t vendor_id, device_id;
-	uint32_t subvendor_id = 0;
+	uint32_t subvendor_id = 0, class_id = 0;
+	const char *cp;
 
 	ids = sscanf_s(buf, "PCI\\VEN_%" PRIx16 "&DEV_%" PRIx16 "&SUBSYS_%"
 		PRIx32, &vendor_id, &device_id, &subvendor_id);
 	if (ids != 3)
 		return -1;
 
+	/* Try and find PCI class ID */
+	for (cp = buf; !(cp[0] == 0 && cp[1] == 0); cp++)
+		if (*cp == '&' && sscanf_s(cp,
+				"&CC_%" PRIx32, &class_id) == 1) {
+			/* Convert 4-digit class IDs to 6-digit format */
+			if (strspn(cp + 4, "0123456789abcdefABCDEF") == 4)
+				class_id <<= 8;
+			break;
+		}
+
 	pci_id->vendor_id = vendor_id;
 	pci_id->device_id = device_id;
 	pci_id->subsystem_device_id = subvendor_id >> 16;
 	pci_id->subsystem_vendor_id = subvendor_id & 0xffff;
+	pci_id->class_id = class_id;
 	return 0;
 }
 
@@ -339,6 +351,7 @@ pci_scan_one(HDEVINFO dev_info, PSP_DEVINFO_DATA device_info_data)
 	if (ret != 0)
 		goto end;
 
+	dev->device.bus = &rte_pci_bus.bus;
 	dev->addr = addr;
 	dev->id = pci_id;
 	dev->max_vfs = 0; /* TODO: get max_vfs */
-- 
2.25.1

Re: [dpdk-dev] [PATCH v3] bus/pci: nvme on Windows requires class id and bus

[Tal Shnaiderman]

> Subject: [PATCH v3] bus/pci: nvme on Windows requires class id and bus
> 
> External email: Use caution opening links or attachments
> 
> 
> Attaching to an NVMe disk on Windows using SPDK requires the PCI class ID
> and device.bus fields. Decode the class ID from the PCI device info strings if it
> is present and set device.bus.
> 
> Signed-off-by: Nick Connolly <nick.connolly@mayadata.io>
> ---
> v3:
> * Put version history at top - v2 mistakenly had it after the diffs
> 
> v2:
> * If only a 4-digit class ID is available, convert it to 6-digit format
> 
>  drivers/bus/pci/windows/pci.c | 15 ++++++++++++++-
>  1 file changed, 14 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/bus/pci/windows/pci.c b/drivers/bus/pci/windows/pci.c
> index f66258452..d380fc1ae 100644
> --- a/drivers/bus/pci/windows/pci.c
> +++ b/drivers/bus/pci/windows/pci.c
> @@ -280,17 +280,29 @@ parse_pci_hardware_id(const char *buf, struct
> rte_pci_id *pci_id)  {
>         int ids = 0;
>         uint16_t vendor_id, device_id;
> -       uint32_t subvendor_id = 0;
> +       uint32_t subvendor_id = 0, class_id = 0;
> +       const char *cp;
> 
>         ids = sscanf_s(buf, "PCI\\VEN_%" PRIx16 "&DEV_%" PRIx16
> "&SUBSYS_%"
>                 PRIx32, &vendor_id, &device_id, &subvendor_id);
>         if (ids != 3)
>                 return -1;
> 
> +       /* Try and find PCI class ID */
> +       for (cp = buf; !(cp[0] == 0 && cp[1] == 0); cp++)
> +               if (*cp == '&' && sscanf_s(cp,
> +                               "&CC_%" PRIx32, &class_id) == 1) {
> +                       /* Convert 4-digit class IDs to 6-digit format */
> +                       if (strspn(cp + 4, "0123456789abcdefABCDEF") == 4)

Maybe we can move this format string to a define in the header, something like RTE_PCI_DRV_CLASSID_FMT?

> +                               class_id <<= 8;
> +                       break;
> +               }
> +
>         pci_id->vendor_id = vendor_id;
>         pci_id->device_id = device_id;
>         pci_id->subsystem_device_id = subvendor_id >> 16;
>         pci_id->subsystem_vendor_id = subvendor_id & 0xffff;
> +       pci_id->class_id = class_id;
>         return 0;
>  }
> 
> @@ -339,6 +351,7 @@ pci_scan_one(HDEVINFO dev_info,
> PSP_DEVINFO_DATA device_info_data)
>         if (ret != 0)
>                 goto end;
> 
> +       dev->device.bus = &rte_pci_bus.bus;
>         dev->addr = addr;
>         dev->id = pci_id;
>         dev->max_vfs = 0; /* TODO: get max_vfs */
> --
> 2.25.1

Re: [dpdk-dev] [PATCH v3] bus/pci: nvme on Windows requires class id and bus

[Nick Connolly]

Hi Tal,

>> +                       /* Convert 4-digit class IDs to 6-digit format */
>> +                       if (strspn(cp + 4, "0123456789abcdefABCDEF") == 4)
> Maybe we can move this format string to a define in the header, something like RTE_PCI_DRV_CLASSID_FMT?
>

I'll send out a v4 with a definition at the top of pci.c

Regards,
Nick

[dpdk-dev] [PATCH v4] bus/pci: nvme on Windows requires class id and bus

[Nick Connolly]

Attaching to an NVMe disk on Windows using SPDK requires the
PCI class ID and device.bus fields. Decode the class ID from the PCI
device info strings if it is present and set device.bus.

Signed-off-by: Nick Connolly <nick.connolly@mayadata.io>
---
 drivers/bus/pci/windows/pci.c | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

diff --git a/drivers/bus/pci/windows/pci.c b/drivers/bus/pci/windows/pci.c
index f66258452..dceb0f4b2 100644
--- a/drivers/bus/pci/windows/pci.c
+++ b/drivers/bus/pci/windows/pci.c
@@ -23,6 +23,9 @@ DEFINE_DEVPROPKEY(DEVPKEY_Device_Numa_Node, 0x540b947e, 0x8b40, 0x45bc,
  * the registry hive for PCI devices.
  */
 
+/* Class ID consists of hexadecimal digits */
+#define RTE_PCI_DRV_CLASSID_DIGIT	"0123456789abcdefABCDEF"
+
 /* The functions below are not implemented on Windows,
  * but need to be defined for compilation purposes
  */
@@ -280,17 +283,29 @@ parse_pci_hardware_id(const char *buf, struct rte_pci_id *pci_id)
 {
 	int ids = 0;
 	uint16_t vendor_id, device_id;
-	uint32_t subvendor_id = 0;
+	uint32_t subvendor_id = 0, class_id = 0;
+	const char *cp;
 
 	ids = sscanf_s(buf, "PCI\\VEN_%" PRIx16 "&DEV_%" PRIx16 "&SUBSYS_%"
 		PRIx32, &vendor_id, &device_id, &subvendor_id);
 	if (ids != 3)
 		return -1;
 
+	/* Try and find PCI class ID */
+	for (cp = buf; !(cp[0] == 0 && cp[1] == 0); cp++)
+		if (*cp == '&' && sscanf_s(cp,
+				"&CC_%" PRIx32, &class_id) == 1) {
+			/* Convert 4-digit class IDs to 6-digit format */
+			if (strspn(cp + 4, RTE_PCI_DRV_CLASSID_DIGIT) == 4)
+				class_id <<= 8;
+			break;
+		}
+
 	pci_id->vendor_id = vendor_id;
 	pci_id->device_id = device_id;
 	pci_id->subsystem_device_id = subvendor_id >> 16;
 	pci_id->subsystem_vendor_id = subvendor_id & 0xffff;
+	pci_id->class_id = class_id;
 	return 0;
 }
 
@@ -339,6 +354,7 @@ pci_scan_one(HDEVINFO dev_info, PSP_DEVINFO_DATA device_info_data)
 	if (ret != 0)
 		goto end;
 
+	dev->device.bus = &rte_pci_bus.bus;
 	dev->addr = addr;
 	dev->id = pci_id;
 	dev->max_vfs = 0; /* TODO: get max_vfs */
-- 
2.25.1

[dpdk-dev] [PATCH v5] bus/pci: nvme on Windows requires class id and bus

[Nick Connolly]

Attaching to an NVMe disk on Windows using SPDK requires the
PCI class ID and device.bus fields. Decode the class ID from the PCI
device info strings if it is present and set device.bus.

Signed-off-by: Nick Connolly <nick.connolly@mayadata.io>
---
v5:
* Add missing version history

v4:
* Use #define to determine length of Class ID

v3:
* Put version history at top - v2 mistakenly had it after the diffs

v2:
* If only a 4-digit class ID is available, convert it to 6-digit format

 drivers/bus/pci/windows/pci.c | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

diff --git a/drivers/bus/pci/windows/pci.c b/drivers/bus/pci/windows/pci.c
index f66258452..dceb0f4b2 100644
--- a/drivers/bus/pci/windows/pci.c
+++ b/drivers/bus/pci/windows/pci.c
@@ -23,6 +23,9 @@ DEFINE_DEVPROPKEY(DEVPKEY_Device_Numa_Node, 0x540b947e, 0x8b40, 0x45bc,
  * the registry hive for PCI devices.
  */
 
+/* Class ID consists of hexadecimal digits */
+#define RTE_PCI_DRV_CLASSID_DIGIT	"0123456789abcdefABCDEF"
+
 /* The functions below are not implemented on Windows,
  * but need to be defined for compilation purposes
  */
@@ -280,17 +283,29 @@ parse_pci_hardware_id(const char *buf, struct rte_pci_id *pci_id)
 {
 	int ids = 0;
 	uint16_t vendor_id, device_id;
-	uint32_t subvendor_id = 0;
+	uint32_t subvendor_id = 0, class_id = 0;
+	const char *cp;
 
 	ids = sscanf_s(buf, "PCI\\VEN_%" PRIx16 "&DEV_%" PRIx16 "&SUBSYS_%"
 		PRIx32, &vendor_id, &device_id, &subvendor_id);
 	if (ids != 3)
 		return -1;
 
+	/* Try and find PCI class ID */
+	for (cp = buf; !(cp[0] == 0 && cp[1] == 0); cp++)
+		if (*cp == '&' && sscanf_s(cp,
+				"&CC_%" PRIx32, &class_id) == 1) {
+			/* Convert 4-digit class IDs to 6-digit format */
+			if (strspn(cp + 4, RTE_PCI_DRV_CLASSID_DIGIT) == 4)
+				class_id <<= 8;
+			break;
+		}
+
 	pci_id->vendor_id = vendor_id;
 	pci_id->device_id = device_id;
 	pci_id->subsystem_device_id = subvendor_id >> 16;
 	pci_id->subsystem_vendor_id = subvendor_id & 0xffff;
+	pci_id->class_id = class_id;
 	return 0;
 }
 
@@ -339,6 +354,7 @@ pci_scan_one(HDEVINFO dev_info, PSP_DEVINFO_DATA device_info_data)
 	if (ret != 0)
 		goto end;
 
+	dev->device.bus = &rte_pci_bus.bus;
 	dev->addr = addr;
 	dev->id = pci_id;
 	dev->max_vfs = 0; /* TODO: get max_vfs */
-- 
2.25.1

Re: [dpdk-dev] [PATCH v5] bus/pci: nvme on Windows requires class id and bus

[Tal Shnaiderman]

> Subject: [PATCH v5] bus/pci: nvme on Windows requires class id and bus
> 
> External email: Use caution opening links or attachments
> 
> 
> Attaching to an NVMe disk on Windows using SPDK requires the PCI class ID
> and device.bus fields. Decode the class ID from the PCI device info strings if it
> is present and set device.bus.
> 
> Signed-off-by: Nick Connolly <nick.connolly@mayadata.io>
> ---
> v5:
> * Add missing version history
> 
> v4:
> * Use #define to determine length of Class ID
> 
> v3:
> * Put version history at top - v2 mistakenly had it after the diffs
> 
> v2:
> * If only a 4-digit class ID is available, convert it to 6-digit format
> 
>  drivers/bus/pci/windows/pci.c | 18 +++++++++++++++++-
>  1 file changed, 17 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/bus/pci/windows/pci.c b/drivers/bus/pci/windows/pci.c
> index f66258452..dceb0f4b2 100644
> --- a/drivers/bus/pci/windows/pci.c
> +++ b/drivers/bus/pci/windows/pci.c
> @@ -23,6 +23,9 @@ DEFINE_DEVPROPKEY(DEVPKEY_Device_Numa_Node,
> 0x540b947e, 0x8b40, 0x45bc,
>   * the registry hive for PCI devices.
>   */
> 
> +/* Class ID consists of hexadecimal digits */
> +#define RTE_PCI_DRV_CLASSID_DIGIT      "0123456789abcdefABCDEF"
> +
>  /* The functions below are not implemented on Windows,
>   * but need to be defined for compilation purposes
>   */
> @@ -280,17 +283,29 @@ parse_pci_hardware_id(const char *buf, struct
> rte_pci_id *pci_id)  {
>         int ids = 0;
>         uint16_t vendor_id, device_id;
> -       uint32_t subvendor_id = 0;
> +       uint32_t subvendor_id = 0, class_id = 0;
> +       const char *cp;
> 
>         ids = sscanf_s(buf, "PCI\\VEN_%" PRIx16 "&DEV_%" PRIx16
> "&SUBSYS_%"
>                 PRIx32, &vendor_id, &device_id, &subvendor_id);
>         if (ids != 3)
>                 return -1;
> 
> +       /* Try and find PCI class ID */
> +       for (cp = buf; !(cp[0] == 0 && cp[1] == 0); cp++)
> +               if (*cp == '&' && sscanf_s(cp,
> +                               "&CC_%" PRIx32, &class_id) == 1) {
> +                       /* Convert 4-digit class IDs to 6-digit format */
> +                       if (strspn(cp + 4, RTE_PCI_DRV_CLASSID_DIGIT) == 4)
> +                               class_id <<= 8;
> +                       break;
> +               }
> +
>         pci_id->vendor_id = vendor_id;
>         pci_id->device_id = device_id;
>         pci_id->subsystem_device_id = subvendor_id >> 16;
>         pci_id->subsystem_vendor_id = subvendor_id & 0xffff;
> +       pci_id->class_id = class_id;
>         return 0;
>  }
> 
> @@ -339,6 +354,7 @@ pci_scan_one(HDEVINFO dev_info,
> PSP_DEVINFO_DATA device_info_data)
>         if (ret != 0)
>                 goto end;
> 
> +       dev->device.bus = &rte_pci_bus.bus;
>         dev->addr = addr;
>         dev->id = pci_id;
>         dev->max_vfs = 0; /* TODO: get max_vfs */
> --
> 2.25.1

Acked-by: Tal Shnaiderman <talshn@nvidia.com>

[dpdk-dev] [PATCH v6] bus/pci: nvme on Windows requires class id and bus

[Nick Connolly]

Attaching to an NVMe disk on Windows using SPDK requires the
PCI class ID and device.bus fields. Decode the class ID from the PCI
device info strings if it is present and set device.bus.

Signed-off-by: Nick Connolly <nick.connolly@mayadata.io>
Acked-by: Tal Shnaiderman <talshn@nvidia.com>
---
v6:
* no changes - resending to resolve spurious iol-testing failure

v5:
* Add missing version history

v4:
* Use #define to determine length of Class ID

v3:
* Put version history at top - v2 mistakenly had it after the diffs

v2:
* If only a 4-digit class ID is available, convert it to 6-digit format

 drivers/bus/pci/windows/pci.c | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

diff --git a/drivers/bus/pci/windows/pci.c b/drivers/bus/pci/windows/pci.c
index f66258452..dceb0f4b2 100644
--- a/drivers/bus/pci/windows/pci.c
+++ b/drivers/bus/pci/windows/pci.c
@@ -23,6 +23,9 @@ DEFINE_DEVPROPKEY(DEVPKEY_Device_Numa_Node, 0x540b947e, 0x8b40, 0x45bc,
  * the registry hive for PCI devices.
  */
 
+/* Class ID consists of hexadecimal digits */
+#define RTE_PCI_DRV_CLASSID_DIGIT	"0123456789abcdefABCDEF"
+
 /* The functions below are not implemented on Windows,
  * but need to be defined for compilation purposes
  */
@@ -280,17 +283,29 @@ parse_pci_hardware_id(const char *buf, struct rte_pci_id *pci_id)
 {
 	int ids = 0;
 	uint16_t vendor_id, device_id;
-	uint32_t subvendor_id = 0;
+	uint32_t subvendor_id = 0, class_id = 0;
+	const char *cp;
 
 	ids = sscanf_s(buf, "PCI\\VEN_%" PRIx16 "&DEV_%" PRIx16 "&SUBSYS_%"
 		PRIx32, &vendor_id, &device_id, &subvendor_id);
 	if (ids != 3)
 		return -1;
 
+	/* Try and find PCI class ID */
+	for (cp = buf; !(cp[0] == 0 && cp[1] == 0); cp++)
+		if (*cp == '&' && sscanf_s(cp,
+				"&CC_%" PRIx32, &class_id) == 1) {
+			/* Convert 4-digit class IDs to 6-digit format */
+			if (strspn(cp + 4, RTE_PCI_DRV_CLASSID_DIGIT) == 4)
+				class_id <<= 8;
+			break;
+		}
+
 	pci_id->vendor_id = vendor_id;
 	pci_id->device_id = device_id;
 	pci_id->subsystem_device_id = subvendor_id >> 16;
 	pci_id->subsystem_vendor_id = subvendor_id & 0xffff;
+	pci_id->class_id = class_id;
 	return 0;
 }
 
@@ -339,6 +354,7 @@ pci_scan_one(HDEVINFO dev_info, PSP_DEVINFO_DATA device_info_data)
 	if (ret != 0)
 		goto end;
 
+	dev->device.bus = &rte_pci_bus.bus;
 	dev->addr = addr;
 	dev->id = pci_id;
 	dev->max_vfs = 0; /* TODO: get max_vfs */
-- 
2.25.1

Re: [dpdk-dev] [PATCH v6] bus/pci: nvme on Windows requires class id and bus

[Dmitry Kozlyuk]

2021-02-23 18:18, Nick Connolly:
> Attaching to an NVMe disk on Windows using SPDK requires the
> PCI class ID and device.bus fields. Decode the class ID from the PCI
> device info strings if it is present and set device.bus.
> 
> Signed-off-by: Nick Connolly <nick.connolly@mayadata.io>
> Acked-by: Tal Shnaiderman <talshn@nvidia.com>
> ---
> v6:
> * no changes - resending to resolve spurious iol-testing failure
> 
> v5:
> * Add missing version history
> 
> v4:
> * Use #define to determine length of Class ID
> 
> v3:
> * Put version history at top - v2 mistakenly had it after the diffs
> 
> v2:
> * If only a 4-digit class ID is available, convert it to 6-digit format
> 
>  drivers/bus/pci/windows/pci.c | 18 +++++++++++++++++-
>  1 file changed, 17 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/bus/pci/windows/pci.c b/drivers/bus/pci/windows/pci.c
> index f66258452..dceb0f4b2 100644
> --- a/drivers/bus/pci/windows/pci.c
> +++ b/drivers/bus/pci/windows/pci.c
> @@ -23,6 +23,9 @@ DEFINE_DEVPROPKEY(DEVPKEY_Device_Numa_Node, 0x540b947e, 0x8b40, 0x45bc,
>   * the registry hive for PCI devices.
>   */
>  
> +/* Class ID consists of hexadecimal digits */
> +#define RTE_PCI_DRV_CLASSID_DIGIT	"0123456789abcdefABCDEF"
> +
>  /* The functions below are not implemented on Windows,
>   * but need to be defined for compilation purposes
>   */
> @@ -280,17 +283,29 @@ parse_pci_hardware_id(const char *buf, struct rte_pci_id *pci_id)
>  {
>  	int ids = 0;
>  	uint16_t vendor_id, device_id;
> -	uint32_t subvendor_id = 0;
> +	uint32_t subvendor_id = 0, class_id = 0;
> +	const char *cp;
>  
>  	ids = sscanf_s(buf, "PCI\\VEN_%" PRIx16 "&DEV_%" PRIx16 "&SUBSYS_%"
>  		PRIx32, &vendor_id, &device_id, &subvendor_id);
>  	if (ids != 3)
>  		return -1;
>  
> +	/* Try and find PCI class ID */
> +	for (cp = buf; !(cp[0] == 0 && cp[1] == 0); cp++)
> +		if (*cp == '&' && sscanf_s(cp,
> +				"&CC_%" PRIx32, &class_id) == 1) {
> +			/* Convert 4-digit class IDs to 6-digit format */
> +			if (strspn(cp + 4, RTE_PCI_DRV_CLASSID_DIGIT) == 4)
> +				class_id <<= 8;
> +			break;
> +		}
> +

Is "4/6-digit format" used commonly for class ID, subclass ID, and optional
programming interface code? If not, I suggest sticking to official
terminology, something like "Assume zero programming interface code if
unspecified".

In general, a link to format reference would be useful in commit message or
function comment, for readers to understand what's being parsed:

https://docs.microsoft.com/en-us/windows-hardware/drivers/install/identifiers-for-pci-devices

With above nits,
Acked-by: Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>

Re: [dpdk-dev] [PATCH v6] bus/pci: nvme on Windows requires class id and bus

[Nick Connolly]

Thanks Dmitry - will address and send v7.

On 28/02/2021 14:38, Dmitry Kozlyuk wrote:
> 2021-02-23 18:18, Nick Connolly:
>> Attaching to an NVMe disk on Windows using SPDK requires the
>> PCI class ID and device.bus fields. Decode the class ID from the PCI
>> device info strings if it is present and set device.bus.
>>
>> Signed-off-by: Nick Connolly <nick.connolly@mayadata.io>
>> Acked-by: Tal Shnaiderman <talshn@nvidia.com>
>> ---
>> v6:
>> * no changes - resending to resolve spurious iol-testing failure
>>
>> v5:
>> * Add missing version history
>>
>> v4:
>> * Use #define to determine length of Class ID
>>
>> v3:
>> * Put version history at top - v2 mistakenly had it after the diffs
>>
>> v2:
>> * If only a 4-digit class ID is available, convert it to 6-digit format
>>
>>   drivers/bus/pci/windows/pci.c | 18 +++++++++++++++++-
>>   1 file changed, 17 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/bus/pci/windows/pci.c b/drivers/bus/pci/windows/pci.c
>> index f66258452..dceb0f4b2 100644
>> --- a/drivers/bus/pci/windows/pci.c
>> +++ b/drivers/bus/pci/windows/pci.c
>> @@ -23,6 +23,9 @@ DEFINE_DEVPROPKEY(DEVPKEY_Device_Numa_Node, 0x540b947e, 0x8b40, 0x45bc,
>>    * the registry hive for PCI devices.
>>    */
>>   
>> +/* Class ID consists of hexadecimal digits */
>> +#define RTE_PCI_DRV_CLASSID_DIGIT	"0123456789abcdefABCDEF"
>> +
>>   /* The functions below are not implemented on Windows,
>>    * but need to be defined for compilation purposes
>>    */
>> @@ -280,17 +283,29 @@ parse_pci_hardware_id(const char *buf, struct rte_pci_id *pci_id)
>>   {
>>   	int ids = 0;
>>   	uint16_t vendor_id, device_id;
>> -	uint32_t subvendor_id = 0;
>> +	uint32_t subvendor_id = 0, class_id = 0;
>> +	const char *cp;
>>   
>>   	ids = sscanf_s(buf, "PCI\\VEN_%" PRIx16 "&DEV_%" PRIx16 "&SUBSYS_%"
>>   		PRIx32, &vendor_id, &device_id, &subvendor_id);
>>   	if (ids != 3)
>>   		return -1;
>>   
>> +	/* Try and find PCI class ID */
>> +	for (cp = buf; !(cp[0] == 0 && cp[1] == 0); cp++)
>> +		if (*cp == '&' && sscanf_s(cp,
>> +				"&CC_%" PRIx32, &class_id) == 1) {
>> +			/* Convert 4-digit class IDs to 6-digit format */
>> +			if (strspn(cp + 4, RTE_PCI_DRV_CLASSID_DIGIT) == 4)
>> +				class_id <<= 8;
>> +			break;
>> +		}
>> +
> Is "4/6-digit format" used commonly for class ID, subclass ID, and optional
> programming interface code? If not, I suggest sticking to official
> terminology, something like "Assume zero programming interface code if
> unspecified".
>
> In general, a link to format reference would be useful in commit message or
> function comment, for readers to understand what's being parsed:
>
> https://docs.microsoft.com/en-us/windows-hardware/drivers/install/identifiers-for-pci-devices
>
> With above nits,
> Acked-by: Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>

[dpdk-dev] [PATCH v7] bus/pci: nvme on Windows requires class id and bus

[Nick Connolly]

Attaching to an NVMe disk on Windows using SPDK requires the
PCI class ID and device.bus fields. Decode the class ID from the PCI
device info strings if it is present and set device.bus.

Signed-off-by: Nick Connolly <nick.connolly@mayadata.io>
Acked-by: Tal Shnaiderman <talshn@nvidia.com>
Acked-by: Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>
---
v7:
* Improve comments as per review by Dmitry Kozlyuk

v6:
* no changes - resending to resolve spurious iol-testing failure

v5:
* Add missing version history

v4:
* Use #define to determine length of Class ID

v3:
* Put version history at top - v2 mistakenly had it after the diffs

v2:
* If only a 4-digit class ID is available, convert it to 6-digit format

 drivers/bus/pci/windows/pci.c | 24 +++++++++++++++++++++++-
 1 file changed, 23 insertions(+), 1 deletion(-)

diff --git a/drivers/bus/pci/windows/pci.c b/drivers/bus/pci/windows/pci.c
index f66258452..bc173af2d 100644
--- a/drivers/bus/pci/windows/pci.c
+++ b/drivers/bus/pci/windows/pci.c
@@ -23,6 +23,9 @@ DEFINE_DEVPROPKEY(DEVPKEY_Device_Numa_Node, 0x540b947e, 0x8b40, 0x45bc,
  * the registry hive for PCI devices.
  */
 
+/* Class ID consists of hexadecimal digits */
+#define RTE_PCI_DRV_CLASSID_DIGIT	"0123456789abcdefABCDEF"
+
 /* The functions below are not implemented on Windows,
  * but need to be defined for compilation purposes
  */
@@ -274,23 +277,41 @@ get_pci_hardware_id(HDEVINFO dev_info, PSP_DEVINFO_DATA device_info_data,
 
 /*
  * parse the SPDRP_HARDWAREID output and assign to rte_pci_id
+ *
+ * A list of the device identification string formats can be found at:
+ * https://docs.microsoft.com/en-us/windows-hardware/drivers/install/identifiers-for-pci-devices
  */
 static int
 parse_pci_hardware_id(const char *buf, struct rte_pci_id *pci_id)
 {
 	int ids = 0;
 	uint16_t vendor_id, device_id;
-	uint32_t subvendor_id = 0;
+	uint32_t subvendor_id = 0, class_id = 0;
+	const char *cp;
 
 	ids = sscanf_s(buf, "PCI\\VEN_%" PRIx16 "&DEV_%" PRIx16 "&SUBSYS_%"
 		PRIx32, &vendor_id, &device_id, &subvendor_id);
 	if (ids != 3)
 		return -1;
 
+	/* Try and find PCI class ID */
+	for (cp = buf; !(cp[0] == 0 && cp[1] == 0); cp++)
+		if (*cp == '&' && sscanf_s(cp,
+				"&CC_%" PRIx32, &class_id) == 1) {
+			/*
+			 * If the Programming Interface code is not specified,
+			 * assume that it is zero.
+			 */
+			if (strspn(cp + 4, RTE_PCI_DRV_CLASSID_DIGIT) == 4)
+				class_id <<= 8;
+			break;
+		}
+
 	pci_id->vendor_id = vendor_id;
 	pci_id->device_id = device_id;
 	pci_id->subsystem_device_id = subvendor_id >> 16;
 	pci_id->subsystem_vendor_id = subvendor_id & 0xffff;
+	pci_id->class_id = class_id;
 	return 0;
 }
 
@@ -339,6 +360,7 @@ pci_scan_one(HDEVINFO dev_info, PSP_DEVINFO_DATA device_info_data)
 	if (ret != 0)
 		goto end;
 
+	dev->device.bus = &rte_pci_bus.bus;
 	dev->addr = addr;
 	dev->id = pci_id;
 	dev->max_vfs = 0; /* TODO: get max_vfs */
-- 
2.25.1

Re: [dpdk-dev] [PATCH v7] bus/pci: nvme on Windows requires class id and bus

[Thomas Monjalon]

01/03/2021 10:56, Nick Connolly:
> Attaching to an NVMe disk on Windows using SPDK requires the
> PCI class ID and device.bus fields. Decode the class ID from the PCI
> device info strings if it is present and set device.bus.
> 
> Signed-off-by: Nick Connolly <nick.connolly@mayadata.io>
> Acked-by: Tal Shnaiderman <talshn@nvidia.com>
> Acked-by: Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>

Applied with the title "bus/pci: set Windows device class and bus",
thanks