[v3,7/7] bus/pci: support Windows with bifurcated drivers
Checks
Commit Message
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.
Uses SPDRP_BUSNUMBER and SPDRP_BUSNUMBER to get the BDF.
scanning currently supports types RTE_KDRV_NONE.
Signed-off-by: Tal Shnaiderman <talshn@mellanox.com>
---
drivers/bus/pci/windows/pci.c | 225 ++++++++++++++++++++++++++-
lib/librte_eal/windows/include/rte_windows.h | 1 +
2 files changed, 224 insertions(+), 2 deletions(-)
Comments
On 2020-05-07 15:16 GMT+0300 talshn@mellanox.com wrote:
> 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.
> Uses SPDRP_BUSNUMBER and SPDRP_BUSNUMBER to get the BDF.
> scanning currently supports types RTE_KDRV_NONE.
>
> Signed-off-by: Tal Shnaiderman <talshn@mellanox.com>
> ---
> drivers/bus/pci/windows/pci.c | 225 ++++++++++++++++++++++++++-
> lib/librte_eal/windows/include/rte_windows.h | 1 +
> 2 files changed, 224 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/bus/pci/windows/pci.c b/drivers/bus/pci/windows/pci.c
> index b1d34ae11..e8eff4f6f 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>
Not used.
> #include <rte_eal_memconfig.h>
>
> #include "private.h"
>
> +#include <devpkey.h>
> +
> +#define MAX_STR_TOKENS 8
> +#define DEC 10
> +#define HEX 16
Not used.
> +/*
> + * 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
> */
> @@ -158,6 +168,184 @@ pci_uio_remap_resource(struct rte_pci_device *dev __rte_unused)
> */
> return -1;
> }
> +
> +static
> +int get_device_pci_address(HDEVINFO hDevInfo,
> + PSP_DEVINFO_DATA pDeviceInfoData, struct rte_pci_addr *addr)
Result type should be on a separate line (that is, "static int <line break>").
This is also the first from numerous violations of DPDK naming conventions.
IMO, Windows EAL should strictly follow them to become "a first-class citizen"
in DPDK and to attract existing developers. Even if not, style should be at
least consistent across each file (now we have snake_case, Hungarian
notation, camelCase, and PascalCase mixed).
> +{
> + BOOL bResult;
> + ULONG bus_num, dev_and_func;
> +
> + bResult = SetupDiGetDeviceRegistryProperty(hDevInfo, pDeviceInfoData,
> + SPDRP_BUSNUMBER, NULL, (PBYTE)&bus_num, sizeof(bus_num), NULL);
> + if (!bResult)
> + goto end;
RTE_LOG_WIN32_ERROR("SetupDiGetDeviceRegistryProperty(SPDRP_BUSNUMBER)"), etc.
would be more helpful in finding exact error cause here and in other places.
Since you'll have to save GetLastError() before logging, you can also get rid
of goto and just use plain return.
> +
> + bResult = SetupDiGetDeviceRegistryProperty(hDevInfo, pDeviceInfoData,
> + SPDRP_ADDRESS, NULL, (PBYTE)&dev_and_func, sizeof(dev_and_func),
> + NULL);
> + if (!bResult)
> + goto end;
> +
> + addr->domain = 0;
> + addr->bus = bus_num;
> + addr->devid = dev_and_func >> 16;
> + addr->function = dev_and_func & 0xffff;
> + return 0;
> +end:
> + return GetLastError();
> +
> +}
> +
[snip]
> +
> +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));
Nitpicking, but sizeof(*dev) would be more concise and robust.
> + if (dev == NULL) {
> + ret = -1;
> + goto end;
> + }
> +
> + memset(dev, 0, sizeof(*dev));
> +
> + char strPCIDeviceInfo[PATH_MAX];
> + BOOL bResult;
[1/6] Compiling C object
'drivers/a715181@@rte_bus_pci@sta/bus_pci_windows_pci.c.obj'.
../../../drivers/bus/pci/windows/pci.c: In function ‘pci_scan_one’:
../../../drivers/bus/pci/windows/pci.c:284:8: warning: variable ‘bResult’ set but not used [-Wunused-but-set-variable]
> + struct rte_pci_addr addr;
> + struct rte_pci_id pci_id;
> +
[snip]
> int
> rte_pci_scan(void)
> {
> - return 0;
> + DWORD DeviceIndex = 0, FoundDevice = 0;
> + HDEVINFO hDevInfo = NULL;
> + SP_DEVINFO_DATA DeviceInfoData = { 0 };
No need to initialize hDevInfo and DeviceInfoData: first, there's a memset()
below and NULL is meaningless for hDevInfo, second, this way you lose
compiler warnings in case you really forget to initialize variables.
> + int ret = -1;
> +
> + hDevInfo = SetupDiGetClassDevs(&GUID_DEVCLASS_NET, NULL, NULL,
> + DIGCF_PRESENT);
Shouldn't devices be limited to PCI here?
> + if (hDevInfo == INVALID_HANDLE_VALUE) {
Would be helpful to log error code here.
> + RTE_LOG(ERR, EAL, "Unable to enumerate PCI devices.\n");
> + 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 %lu devices\n", FoundDevice);
Why ERR?
> + ret = (FoundDevice != 0) ? 0 : -1;
Zero PCI network devices is not necessarily an error: consider testing with
--vdev only, but without explicit --no-pci.
Some issues I found when cross-compiling with MinGW-w64
(gcc 9.2.1 "cc (Arch Linux 9.2.1+20200130-2) 9.2.1 20200130").
1. Missing DEVPKEY_Device_Numa_Node (MinGW-w64 defect):
../../../drivers/bus/pci/windows/pci.c: In function ‘get_device_resource_info’:
../../../drivers/bus/pci/windows/pci.c:213:5: error: ‘DEVPKEY_Device_Numa_Node’ undeclared (first use in this function)
2. undefined reference to `__emutls_v.per_lcore__rte_errno'
(related to TLS issues on Windows).
3. LINK.EXE specific options:
FAILED: drivers/librte_bus_pci-20.0.dll
/usr/bin/x86_64-w64-mingw32-gcc -o drivers/librte_bus_pci-20.0.dll 'drivers/a715181@@rte_bus_pci@sha/bus_pci_pci_common.c.obj' 'drivers/a715181@@rte_bus_pci@sha/bus_pci_pci_params.c.obj' 'drivers/a715181@@rte_bus_pci@sha/bus_pci_windows_pci.c.obj' -Wl,--allow-shlib-undefined -Wl,-O1 -shared -Wl,--start-group -Wl,--out-implib=drivers/librte_bus_pci.dll.a -pthread -lm -ladvapi32 -lsetupapi lib/librte_eal.dll.a lib/librte_kvargs.dll.a lib/librte_pci.dll.a -Wl,/def:/home/dmitry/src/dpdk/public/build/cross/gcc/drivers/rte_bus_pci_exports.def '-Wl,/implib:drivers\\librte_pci.dll.a' -lkernel32 -luser32 -lgdi32 -lwinspool -lshell32 -lole32 -loleaut32 -luuid -lcomdlg32 -Wl,--end-group
/usr/lib/gcc/x86_64-w64-mingw32/9.2.0/../../../../x86_64-w64-mingw32/bin/ld: cannot find /def:/home/dmitry/src/dpdk/public/build/cross/gcc/drivers/rte_bus_pci_exports.def: No such file or directory
/usr/lib/gcc/x86_64-w64-mingw32/9.2.0/../../../../x86_64-w64-mingw32/bin/ld: cannot find /implib:drivers\\librte_pci.dll.a: No such file or directory
Cumulative fixes diff, feel free to distribute it among your patches:
diff --git a/drivers/bus/pci/windows/pci.c b/drivers/bus/pci/windows/pci.c
index e8eff4f6f..9568a090a 100644
--- a/drivers/bus/pci/windows/pci.c
+++ b/drivers/bus/pci/windows/pci.c
@@ -11,6 +11,10 @@
#include <devpkey.h>
+#ifndef DEVPKEY_Device_Numa_Node
+DEFINE_DEVPROPKEY(DEVPKEY_Device_Numa_Node, 0x540b947e, 0x8b40, 0x45bc, 0xa8, 0xa2, 0x6a, 0x0b, 0x89, 0x4c, 0xbd, 0xa2, 3);
+#endif
+
#define MAX_STR_TOKENS 8
#define DEC 10
#define HEX 16
diff --git a/drivers/meson.build b/drivers/meson.build
index e11a1cd39..96af28b9c 100644
--- a/drivers/meson.build
+++ b/drivers/meson.build
@@ -159,7 +159,7 @@ foreach class:dpdk_driver_classes
input: version_map,
output: '@0@_exports.def'.format(lib_name))
lk_deps = [version_map, def_file]
- if is_windows
+ if is_ms_linker
lk_args = ['-Wl,/def:' + def_file.full_path(),
'-Wl,/implib:drivers\\' + implib]
else
diff --git a/lib/librte_eal/rte_eal_version.map b/lib/librte_eal/rte_eal_version.map
index 5d6d3a8c6..74665145b 100644
--- a/lib/librte_eal/rte_eal_version.map
+++ b/lib/librte_eal/rte_eal_version.map
@@ -391,4 +391,6 @@ EXPERIMENTAL {
rte_trace_point_lookup;
rte_trace_regexp;
rte_trace_save;
+
+ __emutls_v.per_lcore__rte_errno;
};
On 2020-05-08 01:50 GMT+0300 Dmitry Kozlyuk wrote:
[snip]
> Cumulative fixes diff, feel free to distribute it among your patches:
>
> diff --git a/drivers/bus/pci/windows/pci.c b/drivers/bus/pci/windows/pci.c
> index e8eff4f6f..9568a090a 100644
> --- a/drivers/bus/pci/windows/pci.c
> +++ b/drivers/bus/pci/windows/pci.c
> @@ -11,6 +11,10 @@
>
> #include <devpkey.h>
>
> +#ifndef DEVPKEY_Device_Numa_Node
> +DEFINE_DEVPROPKEY(DEVPKEY_Device_Numa_Node, 0x540b947e, 0x8b40, 0x45bc, 0xa8, 0xa2, 0x6a, 0x0b, 0x89, 0x4c, 0xbd, 0xa2, 3);
> +#endif
> +
> #define MAX_STR_TOKENS 8
> #define DEC 10
> #define HEX 16
Correction:
#ifdef RTE_TOOLCHAIN_GCC
DEFINE_DEVPROPKEY(DEVPKEY_Device_Numa_Node, 0x540b947e, 0x8b40, 0x45bc, 0xa8, 0xa2, 0x6a, 0x0b, 0x89, 0x4c, 0xbd, 0xa2, 3);
#endif
since DEVPKEY_Device_Numa_Node is not a define.
08/05/2020 00:50, Dmitry Kozlyuk:
> On 2020-05-07 15:16 GMT+0300 talshn@mellanox.com wrote:
> > +static
> > +int get_device_pci_address(HDEVINFO hDevInfo,
> > + PSP_DEVINFO_DATA pDeviceInfoData, struct rte_pci_addr *addr)
>
> Result type should be on a separate line (that is, "static int <line break>").
>
> This is also the first from numerous violations of DPDK naming conventions.
> IMO, Windows EAL should strictly follow them to become "a first-class citizen"
> in DPDK and to attract existing developers. Even if not, style should be at
> least consistent across each file (now we have snake_case, Hungarian
> notation, camelCase, and PascalCase mixed).
I agree, snake_case must be used for all variables.
@@ -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
*/
@@ -158,6 +168,184 @@ pci_uio_remap_resource(struct rte_pci_device *dev __rte_unused)
*/
return -1;
}
+
+static
+int get_device_pci_address(HDEVINFO hDevInfo,
+ PSP_DEVINFO_DATA pDeviceInfoData, struct rte_pci_addr *addr)
+{
+ BOOL bResult;
+ ULONG bus_num, dev_and_func;
+
+ bResult = SetupDiGetDeviceRegistryProperty(hDevInfo, pDeviceInfoData,
+ SPDRP_BUSNUMBER, NULL, (PBYTE)&bus_num, sizeof(bus_num), NULL);
+ if (!bResult)
+ goto end;
+
+ bResult = SetupDiGetDeviceRegistryProperty(hDevInfo, pDeviceInfoData,
+ SPDRP_ADDRESS, NULL, (PBYTE)&dev_and_func, sizeof(dev_and_func),
+ NULL);
+ if (!bResult)
+ goto end;
+
+ addr->domain = 0;
+ addr->bus = bus_num;
+ addr->devid = dev_and_func >> 16;
+ addr->function = dev_and_func & 0xffff;
+ return 0;
+end:
+ return GetLastError();
+
+}
+
+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;
+}
+/*
+ * get_pci_hardware_info from the SPDRP_HARDWAREID output
+ */
+static int
+get_pci_hardware_info(const char *buf, struct rte_pci_id *pci_id)
+{
+ int ids = 0;
+ unsigned int vendorID, deviceID, subvendorID = 0;
+
+ ids = sscanf_s(buf, "PCI\\VEN_%x&DEV_%x&SUBSYS_%x", &vendorID,
+ &deviceID, &subvendorID);
+ if (ids != 3)
+ return -1;
+
+ pci_id->vendor_id = vendorID;
+ pci_id->device_id = deviceID;
+ pci_id->subsystem_vendor_id = subvendorID >> 16;
+ pci_id->subsystem_device_id = subvendorID & 0xffff;
+ return 0;
+}
+
+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;
+ struct rte_pci_addr addr;
+ struct rte_pci_id pci_id;
+
+ /* Retrieve PCI device IDs */
+ bResult = SetupDiGetDeviceRegistryPropertyA(hDevInfo, pDeviceInfoData,
+ SPDRP_HARDWAREID, NULL, (BYTE *)&strPCIDeviceInfo,
+ sizeof(strPCIDeviceInfo), NULL);
+
+ ret = get_pci_hardware_info((const char *)&strPCIDeviceInfo, &pci_id);
+ if (ret != 0) {
+ /*
+ * We won't add this device, but we want to continue
+ * looking for supported devices
+ */
+ ret = ERROR_CONTINUE;
+ goto end;
+ }
+
+ ret = get_device_pci_address(hDevInfo, pDeviceInfoData, &addr);
+ if (ret != 0)
+ goto end;
+
+ dev->addr = addr;
+ dev->id = pci_id;
+ dev->max_vfs = 0; /* TODO: get max_vfs */
+
+ pci_name_set(dev);
+
+ get_kernel_driver_type(dev);
+
+ /* get resources */
+ if (get_device_resource_info(hDevInfo, pDeviceInfoData, dev)
+ != ERROR_SUCCESS) {
+ 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);
+ }
+
+ return 0;
+end:
+ if (dev)
+ free(dev);
+ return ret;
+}
+
/*
* Scan the contents of the PCI bus
* and add all network class devices into the devices list.
@@ -165,5 +353,38 @@ 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");
+ 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 %lu devices\n", FoundDevice);
+ ret = (FoundDevice != 0) ? 0 : -1;
+end:
+ if (hDevInfo != INVALID_HANDLE_VALUE)
+ SetupDiDestroyDeviceInfoList(hDevInfo);
+
+ return ret;
}
@@ -25,6 +25,7 @@
#include <psapi.h>
#include <setupapi.h>
#include <winioctl.h>
+#include <devguid.h>
/* Have GUIDs defined. */
#ifndef INITGUID