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

[Chen, Tiejun]

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 <address@hidden>
> > ---
> >   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