Re: [Qemu-devel] [v3][PATCH 3/5] xen, gfx passthrough: support Intel IGD passthrough with VT-D

[Chen, Tiejun]

> -----Original Message-----
> From: Stefano Stabellini [mailto:address@hidden
> Sent: Wednesday, May 28, 2014 2:02 AM
> To: Chen, Tiejun
> Cc: address@hidden; address@hidden;
> address@hidden; address@hidden; address@hidden;
> address@hidden; address@hidden;
> address@hidden; Kay, Allen M; Zhang, Yang Z
> Subject: Re: [v3][PATCH 3/5] xen, gfx passthrough: support Intel IGD
> passthrough with VT-D
> 
> On Mon, 26 May 2014, Tiejun Chen wrote:
> > Some registers of Intel IGD are mapped in host bridge, so it needs to
> > passthrough these registers of physical host bridge to guest because
> > emulated host bridge in guest doesn't have these mappings.
> >
> > The original patch is from Weidong Han < weidong.han @ intel.com >
> >
> > Signed-off-by: Yang Zhang <address@hidden>
> > Signed-off-by: Tiejun Chen <address@hidden> Cc:Weidong Han
> > <address@hidden>
> > ---
> > v3:
> >
> > * Improve comments to make that readable.
> >
> > v2:
> >
> > * To introduce is_igd_passthrough() to make sure we touch physical host
> bridge
> >   only in IGD case.
> >

[snip]

> > +int pci_create_pch(PCIBus *bus)
> > +{
> > +    XenHostPCIDevice hdev;
> > +    int r = 0;
> > +
> > +    if (!xen_has_gfx_passthru) {
> > +        return -1;
> 
> If this function ends up being called unconditionally, then this should 
> return 0;
> 

Fixed.

> 
> > +    }
> > +
> > +    r = xen_host_pci_device_get(&hdev, 0, 0, 0x1f, 0);
> > +    if (r) {
> > +        XEN_PT_ERR(NULL, "Failed to find Intel PCH on host\n");
> > +        goto err;
> > +    }
> > +
> > +    if (hdev.vendor_id == PCI_VENDOR_ID_INTEL) {
> > +        r = create_pch_isa_bridge(bus, &hdev);
> > +        if (r) {
> > +            XEN_PT_ERR(NULL, "Failed to create PCH ISA bridge.\n");
> > +            goto err;
> > +        }
> > +    }
> > +
> > +    xen_host_pci_device_put(&hdev);
> > +
> > +err:
> > +    return r;
> > +}
> > +
> > +/*
> > + * Currently we just pass this physical host bridge for IGD, 00:02.0.
> > + */
> > +static int is_igd_passthrough(PCIDevice *pci_dev) {
> > +    PCIDevice *f = pci_dev->bus->devices[PCI_DEVFN(2, 0)];
> > +    if (pci_dev->bus->devices[PCI_DEVFN(2, 0)]) {
> 
> Isn't the purpose of this function to check that the *current* device is the
> graphic passthrough device?

No.

> In that case, shouldn't it just be:
> 
> if (pci_dev) {
> 

Here pci_dev is just that host bridge, so here we have to get that real 
passthrough device by that given devfn to further confirm. 

Thanks
Tiejun