> -----Original Message-----
> From: Stefano Stabellini [mailto:stefano.stabell...@eu.citrix.com]
> Sent: Wednesday, May 28, 2014 2:02 AM
> To: Chen, Tiejun
> Cc: anthony.per...@citrix.com; stefano.stabell...@eu.citrix.com;
> m...@redhat.com; kelly.zyta...@amd.com; qemu-devel@nongnu.org;
> xen-de...@lists.xensource.com; peter.mayd...@linaro.org;
> anth...@codemonkey.ws; 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.zh...@intel.com>
> > Signed-off-by: Tiejun Chen <tiejun.c...@intel.com> Cc:Weidong Han
> > <weidong....@intel.com>
> > ---
> > 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
