Re: [dpdk-dev] [PATCH v5] bus/pci: netuio interface for windows

["Kadam, Pallavi" <pallavi.kadam@intel.com>]

On 9/29/2020 1:28 AM, Tal Shnaiderman wrote:
>> Subject: [PATCH v5] bus/pci: netuio interface for windows
>>
>> This patch adds implementations to probe PCI devices bound to netuio with
>> the help of "netuio" class device changes.
>> Now Windows will support both "netuio" and "net" device class and can set
>> kernel driver type based on the device class selection.
>>
>> Note: Few definitions and structures have been copied from
>> netuio_interface.h file from ("[v4] windows/netuio: add Windows NetUIO
>> kernel driver") series and this will be fixed once the exact path for netuio
>> source code is known.
>>
>> v5 changes:
>>          Changed when netuio driver handle is closed
>>
>> v4 changes:
>>          Removed 'reserved' member as it is not used
>>
>> v3 changes:
>>          Removed the casts
>>
>> v2 changes:
>>          - Moved all netuio specific definitions and functions to
>>            pci_netuio.c and pci_netuio.h files
>>          - Added a single function call to scan all the devices
>>
>> Signed-off-by: John Alexander <John.Alexander@datapath.co.uk>
>> Signed-off-by: Pallavi Kadam <pallavi.kadam@intel.com>
>> Reviewed-by: Ranjit Menon <ranjit.menon@intel.com>
>> ---
> [snip]
>
>> +/*
>> + * get device resource information by sending ioctl to netuio driver
>> +*/ int get_netuio_device_info(HDEVINFO dev_info, PSP_DEVINFO_DATA
>> +dev_info_data,
>> +       struct rte_pci_device *dev)
>> +{
> [snip]
>
>> +       /* get NUMA node using DEVPKEY_Device_Numa_Node */
>> +       res = SetupDiGetDevicePropertyW(dev_info, dev_info_data,
>> +               &DEVPKEY_Device_Numa_Node, &property_type,
>> +               (BYTE *)&numa_node, sizeof(numa_node), NULL, 0);
>> +       if (!res) {
>> +               RTE_LOG_WIN32_ERR(
>> +
>> "SetupDiGetDevicePropertyW(DEVPKEY_Device_Numa_Node)");
>> +               goto end;
>> +       }
>> +       dev->device.numa_node = numa_node;
> The is the same code RTE_PCI_KDRV_NONE devices are using to get numa node id, I suggest removing it and changing the original code in get_device_resource_info to work on both RTE_PCI_KDRV_NONE and RTE_PCI_KDRV_NIC_UIO

Thanks, Tal. We have modified get numa node id code to support both

RTE_PCI_KDRV_NONE and RTE_PCI_KDRV_NIC_UIO and split up the

get_netuio_device_info function into multiple functions in v6.

>
> In general:
>
> Regarding the issue Ranjit mentioned in the last community call on duplicated detection of netuio devices both as GUID_DEVCLASS_NETUIO and GUID_DEVCLASS_NET, in pci_scan_one there is actually code that take cares of duplicated detection by leaving only one in the device list, up until nor I saw it in action only in virtualization where BDF of several devices was equal.
>
> Assuming this is resolved with the addition of segment id as domain id the change we need is to give precedence to GUID_DEVCLASS_NETUIO, something like:
>
> @@ -372,11 +372,15 @@ pci_scan_one(HDEVINFO dev_info, PSP_DEVINFO_DATA device_info_data)
>   	} else if (ret < 0) {
>   		rte_pci_insert_device(dev2, dev);
>   	} else { /* already registered */
> +		if (IsEqualGUID(&(device_info_data.ClassGuid),
> +			&GUID_DEVCLASS_NETUIO) {
>   		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;
>   }
>
> The other pci.c changes of the patch LGTM, I'd prefer someone from MSFT to review in depth the pci_netuio.c/.h, the only other comment I have on it is a suggestion to break down the get_netuio_device_info function into several helper functions (get_netuio_device_information_set, allocate_netuio_interface_detail, etc)
>