Re: [dpdk-dev] [PATCH v3 7/7] bus/pci: support Windows with bifurcated drivers

[Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>]

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;
 };

-- 
Dmitry Kozlyuk