[v2,7/7] bus/pci: support Windows with bifurcated drivers

Message ID 20200428091111.13416-8-talshn@mellanox.com (mailing list archive)
State Superseded, archived
Delegated to: Thomas Monjalon
Headers
Series Windows bus/pci support |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation fail apply issues

Commit Message

Tal Shnaiderman April 28, 2020, 9:11 a.m. UTC
  From: Tal Shnaiderman <talshn@mellanox.com>

Uses SetupAPI.h functions to scan PCI tree.
Uses DEVPKEY_Device_Numa_Node to get the PCI Numa node.
scanning currently supports types RTE_KDRV_NONE.

Signed-off-by: Tal Shnaiderman <talshn@mellanox.com>
---
 drivers/bus/pci/windows/pci.c                | 342 ++++++++++++++++++++++++++-
 lib/librte_eal/rte_eal_exports.def           |   1 +
 lib/librte_eal/windows/include/rte_windows.h |   1 +
 3 files changed, 342 insertions(+), 2 deletions(-)
  

Comments

Dmitry Kozlyuk April 29, 2020, 12:01 a.m. UTC | #1
On 2020-04-28 12:11 GMT+0300 talshn@mellanox.com wrote:
[snip]
> +	switch (dev->kdrv) {
> +	case RTE_KDRV_NONE:
> +		/* Get NUMA node using DEVPKEY_Device_Numa_Node */
> +		bResult = SetupDiGetDevicePropertyW(hDevInfo, pDeviceInfoData,
> +			&DEVPKEY_Device_Numa_Node, &uPropertyType,
> +			(BYTE *)&uNumaNode, sizeof(uNumaNode), NULL, 0);
> +		if (!bResult) {
> +			ret = GetLastError();
> +			goto end;
> +		}
> +		dev->device.numa_node = uNumaNode;

Note: NUMA node != socket ID, but this field is used as socket ID by PMDs.
I suggest adding Windows-only EAL API to do the translation.

[snip]
> +	/* We now have the various hw IDs in tokens in the str[] array */
> +	/* Isolate the numerical IDs - '_' as the delimiter */
> +	if (parse_hardware_ids(str[0], strlen(str[0]), vendorID, NULL))
> +		goto end;
> +
> +	if (parse_hardware_ids(str[1], strlen(str[1]), deviceID, NULL))
> +		goto end;
> +
> +	if (parse_hardware_ids(str[2], strlen(str[2]), subdeviceID,
> +		subvendorID)) {
> +		goto end;
> +	}

You could avoid hand-written string parsing by using sscanf(), because
hardware ID formats are fixed, limited, and documented:

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

[snip]
> +
> +	bResult = SetupDiGetDeviceRegistryPropertyA(hDevInfo, pDeviceInfoData,
> +		SPDRP_LOCATION_INFORMATION, NULL, (BYTE *)&strPCIDeviceInfo,
> +		sizeof(strPCIDeviceInfo), NULL);

> +
> +	/* Some devices don't have location information - simply return 0 */
> +	/* Also, if we don't find 'PCI' in the description, we'll skip it */
> +	if (!bResult) {
> +		ret = (GetLastError() == ERROR_INVALID_DATA) ? ERROR_CONTINUE
> +			: -1;
> +		goto end;
> +	} else if (!strstr(strPCIDeviceInfo, "PCI")) {
> +		ret = ERROR_CONTINUE;
> +		goto end;
> +	}

You could get PCI address without parsing strings with SPDRP_BUSNUMBER and
SPDRP_ADDRESS.

> +
> +	struct rte_pci_addr addr;
> +
> +	if (parse_pci_addr_format((const char *)&strPCIDeviceInfo,
> +			sizeof(strPCIDeviceInfo), &addr) != 0) {
> +		ret = -1;
> +		goto end;
> +	}
> +
> +	dev->addr.domain = addr.domain;
> +	dev->addr.bus = addr.bus;
> +	dev->addr.devid = addr.devid;
> +	dev->addr.function = addr.function;

Why not dev->addr = addr?

[snip]
> diff --git a/lib/librte_eal/rte_eal_exports.def b/lib/librte_eal/rte_eal_exports.def
> index edbb6b277..7a0468a02 100644
> --- a/lib/librte_eal/rte_eal_exports.def
> +++ b/lib/librte_eal/rte_eal_exports.def
> @@ -18,6 +18,7 @@ EXPORTS
>  	rte_eal_tailq_lookup
>  	rte_eal_tailq_register
>  	rte_eal_using_phys_addrs
> +	rte_find_numerical_value
>  	rte_free
>  	rte_log
>  	rte_malloc

This belongs to the patch that added the API, doesn't it?
  
Tal Shnaiderman May 3, 2020, 11:53 a.m. UTC | #2
> Subject: Re: [PATCH v2 7/7] bus/pci: support Windows with bifurcated
> drivers
> 
> On 2020-04-28 12:11 GMT+0300 talshn@mellanox.com wrote:
> [snip]
> > +	switch (dev->kdrv) {
> > +	case RTE_KDRV_NONE:
> > +		/* Get NUMA node using DEVPKEY_Device_Numa_Node */
> > +		bResult = SetupDiGetDevicePropertyW(hDevInfo,
> pDeviceInfoData,
> > +			&DEVPKEY_Device_Numa_Node, &uPropertyType,
> > +			(BYTE *)&uNumaNode, sizeof(uNumaNode), NULL,
> 0);
> > +		if (!bResult) {
> > +			ret = GetLastError();
> > +			goto end;
> > +		}
> > +		dev->device.numa_node = uNumaNode;
> 
> Note: NUMA node != socket ID, but this field is used as socket ID by PMDs.
> I suggest adding Windows-only EAL API to do the translation.

Thank you for the review and notes Dmitry, will apply in v3.

Regarding numa node and socket id, AFAIK in x86 processors numa node == socket id
In other processors that is not the case but of the current Windows support of DPDK the
Implementation above should be sufficient.

> [snip]
  

Patch

diff --git a/drivers/bus/pci/windows/pci.c b/drivers/bus/pci/windows/pci.c
index d02bfce36..238312637 100644
--- a/drivers/bus/pci/windows/pci.c
+++ b/drivers/bus/pci/windows/pci.c
@@ -1,14 +1,24 @@ 
 /* SPDX-License-Identifier: BSD-3-Clause
  * Copyright 2020 Mellanox Technologies, Ltd
  */
+#include <rte_windows.h>
 #include <rte_errno.h>
 #include <rte_log.h>
-
 #include <rte_string_fns.h>
 #include <rte_eal_memconfig.h>
 
 #include "private.h"
 
+#include <devpkey.h>
+
+#define  MAX_STR_TOKENS		8
+#define  DEC			10
+#define  HEX			16
+/*
+ * This code is used to simulate a PCI probe by parsing information in
+ * the registry hive for PCI devices.
+ */
+
 /* The functions below are not implemented on Windows,
  * but need to be defined for compilation purposes
  */
@@ -167,6 +177,300 @@  pci_uio_remap_resource(struct rte_pci_device *dev __rte_unused)
 	 */
 	return -1;
 }
+
+static
+int get_device_resource_info(HDEVINFO hDevInfo,
+	PSP_DEVINFO_DATA pDeviceInfoData, struct rte_pci_device *dev)
+{
+	int  ret = -1;
+	DEVPROPTYPE uPropertyType;
+	DWORD uNumaNode;
+	BOOL  bResult;
+
+	switch (dev->kdrv) {
+	case RTE_KDRV_NONE:
+		/* Get NUMA node using DEVPKEY_Device_Numa_Node */
+		bResult = SetupDiGetDevicePropertyW(hDevInfo, pDeviceInfoData,
+			&DEVPKEY_Device_Numa_Node, &uPropertyType,
+			(BYTE *)&uNumaNode, sizeof(uNumaNode), NULL, 0);
+		if (!bResult) {
+			ret = GetLastError();
+			goto end;
+		}
+		dev->device.numa_node = uNumaNode;
+		/* mem_resource - Unneeded for RTE_KDRV_NONE */
+		dev->mem_resource[0].phys_addr = 0;
+		dev->mem_resource[0].len = 0;
+		dev->mem_resource[0].addr = NULL;
+		break;
+	default:
+		ret = ERROR_NOT_SUPPORTED;
+		goto end;
+	}
+
+	ret = ERROR_SUCCESS;
+end:
+	return ret;
+}
+
+
+/*
+ * split up a pci address into its constituent parts.
+ */
+static int
+parse_pci_addr_format(const char *buf, int bufsize, struct rte_pci_addr *addr)
+{
+	int ret = -1;
+
+	char *str[MAX_STR_TOKENS] = { 0 };
+
+	char *buf_copy = _strdup(buf);
+	if (buf_copy == NULL)
+		goto end;
+
+	/* PCI Addr format string is delimited by ',' */
+	/* eg: "PCI bus 5, device 35, function 0" */
+	if (rte_strsplit(buf_copy, bufsize, str, MAX_STR_TOKENS, ',')
+			 > MAX_STR_TOKENS - 1) {
+		goto end;
+	}
+
+	/* Bus, device and function values tokens in the str[] */
+	/* Convert to numerical values */
+	addr->domain = 0;
+	addr->bus = (uint8_t)rte_find_numerical_value(str[0], DEC);
+	addr->devid = (uint8_t)rte_find_numerical_value(str[1], DEC);
+	addr->function = (uint8_t)rte_find_numerical_value(str[2], DEC);
+
+	if (rte_errno != 0)
+		goto end;
+
+	ret = 0;
+end:
+	/* free the copy made (if any) with _tcsdup */
+	if (buf_copy)
+		free(buf_copy);
+	return ret;
+}
+
+static int
+parse_hardware_ids(char *str, unsigned int strlength, uint16_t *val1,
+		uint16_t *val2)
+{
+	int ret = -1;
+	char *strID[MAX_STR_TOKENS] = { 0 };
+
+	if (rte_strsplit(str, strlength, strID, MAX_STR_TOKENS, '_')
+		 > MAX_STR_TOKENS - 1) {
+		goto end;
+	}
+
+	/* check if string combined ID value (eg: subdeviceID+subvendorID) */
+	if (strlen(strID[1]) == 8) {
+		char strval1[8];
+		char strval2[8];
+		memset(strval1, 0, sizeof(strval1));
+		memset(strval2, 0, sizeof(strval2));
+
+		memcpy_s(strval1, sizeof(strval1), strID[1], 4);
+		memcpy_s(strval2, sizeof(strval2), strID[1]+4, 4);
+
+		if (val1)
+			*val1 = (uint16_t)rte_find_numerical_value(
+					strval1, HEX);
+		if (val2)
+			*val2 = (uint16_t)rte_find_numerical_value(
+					strval2, HEX);
+	} else {
+		if (val1)
+			*val1 = (uint16_t)rte_find_numerical_value(
+					strID[1], HEX);
+	}
+
+	if (rte_errno != 0)
+		goto end;
+
+	ret = 0;
+end:
+	return ret;
+}
+
+/*
+ * split up hardware ID into its constituent parts.
+ */
+static int
+parse_pci_hardware_id_format(const char *buf, int bufsize, uint16_t *vendorID,
+	uint16_t *deviceID, uint16_t *subvendorID, uint16_t *subdeviceID)
+{
+	int ret = -1;
+
+	char *str[MAX_STR_TOKENS] = { 0 };
+
+	char *buf_copy = _strdup(buf);
+	if (buf_copy == NULL)
+		goto end;
+
+	/* PCI Hardware ID format string is first delimited by '\\' */
+	/* eg: "PCI\VEN_8086&DEV_153A&SUBSYS_00008086&REV_04" */
+	if (rte_strsplit(buf_copy, bufsize, str, MAX_STR_TOKENS, '\\') >
+			MAX_STR_TOKENS - 1) {
+		goto end;
+	}
+	char *buf_copy_stripped = _strdup(str[1]);
+	if (buf_copy_stripped == NULL)
+		goto end;
+	/* The remaining string is delimited by '&' */
+	if (rte_strsplit(buf_copy_stripped, bufsize, str, MAX_STR_TOKENS, '&')
+			> MAX_STR_TOKENS - 1) {
+		goto end;
+	}
+	/* We now have the various hw IDs in tokens in the str[] array */
+	/* Isolate the numerical IDs - '_' as the delimiter */
+	if (parse_hardware_ids(str[0], strlen(str[0]), vendorID, NULL))
+		goto end;
+
+	if (parse_hardware_ids(str[1], strlen(str[1]), deviceID, NULL))
+		goto end;
+
+	if (parse_hardware_ids(str[2], strlen(str[2]), subdeviceID,
+		subvendorID)) {
+		goto end;
+	}
+
+	ret = 0;
+end:
+	/* free the copy made (if any) with _tcsdup */
+	if (buf_copy)
+		free(buf_copy);
+	if (buf_copy_stripped)
+		free(buf_copy_stripped);
+	return ret;
+}
+
+static void
+get_kernel_driver_type(struct rte_pci_device *dev __rte_unused)
+{
+	/*
+	 * If another kernel driver is supported the relevant checking
+	 * functions should be here
+	 */
+	dev->kdrv = RTE_KDRV_NONE;
+}
+
+static int
+pci_scan_one(HDEVINFO hDevInfo, PSP_DEVINFO_DATA pDeviceInfoData)
+{
+	struct rte_pci_device *dev;
+	int    ret = -1;
+
+	dev = malloc(sizeof(struct rte_pci_device));
+	if (dev == NULL) {
+		ret = -1;
+		goto end;
+	}
+
+	memset(dev, 0, sizeof(*dev));
+
+	char  strPCIDeviceInfo[PATH_MAX];
+	BOOL  bResult;
+
+	bResult = SetupDiGetDeviceRegistryPropertyA(hDevInfo, pDeviceInfoData,
+		SPDRP_LOCATION_INFORMATION, NULL, (BYTE *)&strPCIDeviceInfo,
+		sizeof(strPCIDeviceInfo), NULL);
+
+	/* Some devices don't have location information - simply return 0 */
+	/* Also, if we don't find 'PCI' in the description, we'll skip it */
+	if (!bResult) {
+		ret = (GetLastError() == ERROR_INVALID_DATA) ? ERROR_CONTINUE
+			: -1;
+		goto end;
+	} else if (!strstr(strPCIDeviceInfo, "PCI")) {
+		ret = ERROR_CONTINUE;
+		goto end;
+	}
+
+	struct rte_pci_addr addr;
+
+	if (parse_pci_addr_format((const char *)&strPCIDeviceInfo,
+			sizeof(strPCIDeviceInfo), &addr) != 0) {
+		ret = -1;
+		goto end;
+	}
+
+	dev->addr.domain = addr.domain;
+	dev->addr.bus = addr.bus;
+	dev->addr.devid = addr.devid;
+	dev->addr.function = addr.function;
+
+	/* Retrieve PCI device IDs */
+	bResult = SetupDiGetDeviceRegistryPropertyA(hDevInfo, pDeviceInfoData,
+			SPDRP_HARDWAREID, NULL, (BYTE *)&strPCIDeviceInfo,
+			sizeof(strPCIDeviceInfo), NULL);
+
+	/* parse hardware ID string */
+	uint16_t vendorID, deviceID, subvendorID, subdeviceID = 0;
+	if (parse_pci_hardware_id_format((const char *)&strPCIDeviceInfo,
+		sizeof(strPCIDeviceInfo), &vendorID, &deviceID, &subvendorID,
+		&subdeviceID) != 0) {
+		ret = -1;
+		goto end;
+	}
+
+	dev->id.vendor_id = vendorID;
+	dev->id.device_id = deviceID;
+	dev->id.subsystem_vendor_id = subvendorID;
+	dev->id.subsystem_device_id = subdeviceID;
+
+	dev->max_vfs = 0; /* TODO: get max_vfs */
+
+	pci_name_set(dev);
+
+	get_kernel_driver_type(dev);
+
+	/* parse resources */
+	if (get_device_resource_info(hDevInfo, pDeviceInfoData, dev)
+			!= ERROR_SUCCESS) {
+		/*
+		 * We won't add this device, but we want to continue
+		 * looking forsupported devices
+		 */
+		ret = ERROR_CONTINUE;
+		goto end;
+	}
+
+	/* device is valid, add in list (sorted) */
+	if (TAILQ_EMPTY(&rte_pci_bus.device_list)) {
+		rte_pci_add_device(dev);
+	} else {
+		struct rte_pci_device *dev2 = NULL;
+		int ret;
+
+		TAILQ_FOREACH(dev2, &rte_pci_bus.device_list, next) {
+			ret = rte_pci_addr_cmp(&dev->addr, &dev2->addr);
+			if (ret > 0) {
+				continue;
+			} else if (ret < 0) {
+				rte_pci_insert_device(dev2, dev);
+			} else { /* already registered */
+				dev2->kdrv = dev->kdrv;
+				dev2->max_vfs = dev->max_vfs;
+				memmove(dev2->mem_resource, dev->mem_resource,
+					sizeof(dev->mem_resource));
+				free(dev);
+			}
+			return 0;
+		}
+		rte_pci_add_device(dev);
+	}
+
+	ret = 0;
+	return ret;
+end:
+	if (dev)
+		free(dev);
+	return ret;
+}
+
 /*
  * Scan the contents of the PCI bus
  * and add all network class devices into the devices list.
@@ -174,5 +478,39 @@  pci_uio_remap_resource(struct rte_pci_device *dev __rte_unused)
 int
 rte_pci_scan(void)
 {
-	return 0;
+	DWORD			DeviceIndex = 0, FoundDevice = 0;
+	HDEVINFO		hDevInfo = NULL;
+	SP_DEVINFO_DATA	DeviceInfoData = { 0 };
+	int				ret = -1;
+
+	hDevInfo = SetupDiGetClassDevs(&GUID_DEVCLASS_NET, NULL, NULL,
+				DIGCF_PRESENT);
+	if (hDevInfo == INVALID_HANDLE_VALUE) {
+		RTE_LOG(ERR, EAL,
+			"Unable to enumerate PCI devices.\n", __func__);
+		goto end;
+	}
+
+	DeviceInfoData.cbSize = sizeof(SP_DEVINFO_DATA);
+	DeviceIndex = 0;
+
+	while (SetupDiEnumDeviceInfo(hDevInfo, DeviceIndex, &DeviceInfoData)) {
+		DeviceIndex++;
+		ret = pci_scan_one(hDevInfo, &DeviceInfoData);
+		if (ret == ERROR_SUCCESS)
+			FoundDevice++;
+		else if (ret != ERROR_CONTINUE)
+			goto end;
+
+		memset(&DeviceInfoData, 0, sizeof(SP_DEVINFO_DATA));
+		DeviceInfoData.cbSize = sizeof(SP_DEVINFO_DATA);
+	}
+
+	RTE_LOG(ERR, EAL, "PCI scan found %u devices\n", FoundDevice);
+	ret = (FoundDevice != 0) ? 0 : -1;
+end:
+	if (hDevInfo != INVALID_HANDLE_VALUE)
+		SetupDiDestroyDeviceInfoList(hDevInfo);
+
+	return ret;
 }
diff --git a/lib/librte_eal/rte_eal_exports.def b/lib/librte_eal/rte_eal_exports.def
index edbb6b277..7a0468a02 100644
--- a/lib/librte_eal/rte_eal_exports.def
+++ b/lib/librte_eal/rte_eal_exports.def
@@ -18,6 +18,7 @@  EXPORTS
 	rte_eal_tailq_lookup
 	rte_eal_tailq_register
 	rte_eal_using_phys_addrs
+	rte_find_numerical_value
 	rte_free
 	rte_log
 	rte_malloc
diff --git a/lib/librte_eal/windows/include/rte_windows.h b/lib/librte_eal/windows/include/rte_windows.h
index 899ed7d87..725ac4f9b 100644
--- a/lib/librte_eal/windows/include/rte_windows.h
+++ b/lib/librte_eal/windows/include/rte_windows.h
@@ -25,6 +25,7 @@ 
 #include <psapi.h>
 #include <setupapi.h>
 #include <winioctl.h>
+#include <devguid.h>
 
 /* Have GUIDs defined. */
 #ifndef INITGUID