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

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

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?

-- 
Dmitry Kozlyuk