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

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

On Wed, 28 May 2014, Chen, Tiejun wrote:
> > -----Original Message-----
> > From: Stefano Stabellini [mailto:stefano.stabellini@xxxxxxxxxxxxx]
> > Sent: Wednesday, May 28, 2014 2:02 AM
> > To: Chen, Tiejun
> > Cc: anthony.perard@xxxxxxxxxx; stefano.stabellini@xxxxxxxxxxxxx;
> > mst@xxxxxxxxxx; Kelly.Zytaruk@xxxxxxx; qemu-devel@xxxxxxxxxx;
> > xen-devel@xxxxxxxxxxxxxxxxxxx; peter.maydell@xxxxxxxxxx;
> > anthony@xxxxxxxxxxxxx; 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 <yang.z.zhang@xxxxxxxxx>
> > > Signed-off-by: Tiejun Chen <tiejun.chen@xxxxxxxxx> Cc:Weidong Han
> > > <weidong.han@xxxxxxxxx>
> > > ---
> > > 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. 

I understand now, thanks for the explanation. Maybe you want to expand
the comment on top of is_igd_passthrough.
Cheers,

Stefano

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel