Re: [Qemu-devel] [PATCH 1/2] libxl: introduce libxl__is_igd_vga_passthru

["Chen, Tiejun" <tiejun.chen@intel.com>]

On 2015/3/6 20:40, Wei Liu wrote:
> On Fri, Mar 06, 2015 at 05:08:22PM +0800, Tiejun Chen wrote:
>> While working with qemu, IGD is a specific device in the case of pass through
>> so we need to identify that to handle more later. Here we define a table to
>> record all IGD types currently we can support. Also we need to introduce two
>> helper functions to get vendor and device ids to lookup that table.
>>
>> Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
>> ---
>>   tools/libxl/libxl_internal.h |   2 +
>>   tools/libxl/libxl_pci.c      | 124 +++++++++++++++++++++++++++++++++++++++++++
>>   2 files changed, 126 insertions(+)
>>
>> diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
>> index 934465a..8b952b8 100644
>> --- a/tools/libxl/libxl_internal.h
>> +++ b/tools/libxl/libxl_internal.h
>> @@ -1176,6 +1176,8 @@ _hidden int libxl__device_pci_add(libxl__gc *gc, uint32_t domid, libxl_device_pc
>>   _hidden int libxl__create_pci_backend(libxl__gc *gc, uint32_t domid,
>>                                         libxl_device_pci *pcidev, int num);
>>   _hidden int libxl__device_pci_destroy_all(libxl__gc *gc, uint32_t domid);
>> +_hidden int libxl__is_igd_vga_passthru(libxl__gc *gc,
>> +                                       const libxl_domain_config *d_config);
>>
>>   /*----- xswait: wait for a xenstore node to be suitable -----*/
>>
>> diff --git a/tools/libxl/libxl_pci.c b/tools/libxl/libxl_pci.c
>> index f3ae132..dc5a89e 100644
>> --- a/tools/libxl/libxl_pci.c
>> +++ b/tools/libxl/libxl_pci.c
>> @@ -491,6 +491,130 @@ static int sysfs_dev_unbind(libxl__gc *gc, libxl_device_pci *pcidev,
>>       return 0;
>>   }
>>
>> +static unsigned long sysfs_dev_get_vendor(libxl__gc *gc,
>> +                                          libxl_device_pci *pcidev)
>
> uint16_t?
>
>> +{
>> +    char *pci_device_vendor_path =
>> +            libxl__sprintf(gc, SYSFS_PCI_DEV"/"PCI_BDF"/vendor",
>> +                           pcidev->domain, pcidev->bus, pcidev->dev,
>> +                           pcidev->func);
>
> Please use GCSPRINTF macro.

Okay.

>
>> +    int read_items;
>> +    unsigned long pci_device_vendor;
>
> uint16_t?

Yes, I can but I don't see other similar helpers are doing this in this 
file :)

>
> Same comments apply to _get_device function.

And especially, if we really set that as uint16_t,

>
>> +
>> +    FILE *f = fopen(pci_device_vendor_path, "r");
>> +    if (!f) {
>> +        LOGE(ERROR,
>> +             "pci device "PCI_BDF" does not have vendor attribute",
>> +             pcidev->domain, pcidev->bus, pcidev->dev, pcidev->func);
>> +        return 0xffff;
>> +    }
>> +    read_items = fscanf(f, "0x%lx\n", &pci_device_vendor);

we have to refactor this as well,

read_items = fscanf(f, "0x%hx\n", &pci_device_vendor);

Right?

>> +    fclose(f);
>> +    if (read_items != 1) {
>> +        LOGE(ERROR,
>> +             "cannot read vendor of pci device "PCI_BDF,
>> +             pcidev->domain, pcidev->bus, pcidev->dev, pcidev->func);
>> +        return 0xffff;
>> +    }
>> +
>> +    return pci_device_vendor;
>> +}
>> +
>
> [...]
>
>> +/*
>> + * Some devices may need some ways to work well. Here like IGD,
>> + * we have to pass a specific option to qemu.
>> + */
>> +int libxl__is_igd_vga_passthru(libxl__gc *gc,
>
> bool.

Okay.

>
>> +                               const libxl_domain_config *d_config)
>> +{
>> +    unsigned int i, j, num = ARRAY_SIZE(fixup_ids);
>> +    uint16_t vendor, device;
>> +
>> +    for (i = 0 ; i < d_config->num_pcidevs ; i++) {
>> +        libxl_device_pci *pcidev = &d_config->pcidevs[i];
>> +
>> +        for (j = 0 ; j < num ; j++) {
>> +            vendor = fixup_ids[j].vendor;
>> +            device = fixup_ids[j].device;
>> +
>> +            if (sysfs_dev_get_vendor(gc, pcidev) == vendor &&
>> +                sysfs_dev_get_device(gc, pcidev) == device)
>> +                return 1;
>
> Get vendor and device in outer loop to avoid wasting cpu cycles. :-)
>

Yeah.

Thanks
Tiejun