On 2020-05-07 15:16 GMT+0300 tal...@mellanox.com wrote:
> From: Tal Shnaiderman <tal...@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 <tal...@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;
};
--
Dmitry Kozlyuk
