Re: [Xen-devel] [v2][PATCH 6/8] xen, gfx passthrough: support Intel IGD passthrough with VT-D

["Zhang, Yang Z" <yang.z.zhang@xxxxxxxxx>]

Konrad Rzeszutek Wilk wrote on 2014-05-16:
> On Fri, May 16, 2014 at 06:53:42PM +0800, 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.

Thanks for your review for the whole series patch.

> 
> When you say mapped - you mean they are aliased - so if I change the value in
> the IGD they will change in the host bridge as well, right?

I guess you mean the physical host bridge. For it, only PAVPC will write to 
physical host bridge directly, see:

+    switch (config_addr) {
+    case 0x58:      /* PAVPC Offset */
+        break;
+    default:
+        goto write_default;
+    }


> 
> >
> > 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>
> > ---
> > v2:
> >
> > * To introduce is_igd_passthrough() to make sure we touch physical host
> bridge
> >   only in IGD case.
> >
> >  hw/xen/xen_pt.h          |   4 ++
> >  hw/xen/xen_pt_graphics.c | 140
> > +++++++++++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 144 insertions(+)
> >
> > diff --git a/hw/xen/xen_pt.h b/hw/xen/xen_pt.h index 4d3a18d..507165c
> > 100644
> > --- a/hw/xen/xen_pt.h
> > +++ b/hw/xen/xen_pt.h
> > @@ -302,5 +302,9 @@ extern int xen_has_gfx_passthru;  int
> > xen_pt_register_vga_regions(XenHostPCIDevice *dev);  int
> > xen_pt_unregister_vga_regions(XenHostPCIDevice *dev);  int
> > xen_pt_setup_vga(XenHostPCIDevice *dev);
> > +int pci_create_pch(PCIBus *bus);
> > +void igd_pci_write(PCIDevice *pci_dev, uint32_t config_addr,
> > +                   uint32_t val, int len); uint32_t
> > +igd_pci_read(PCIDevice *pci_dev, uint32_t config_addr, int len);
> >
> >  #endif /* !XEN_PT_H */
> > diff --git a/hw/xen/xen_pt_graphics.c b/hw/xen/xen_pt_graphics.c index
> > 6b86293..066bc4d 100644
> > --- a/hw/xen/xen_pt_graphics.c
> > +++ b/hw/xen/xen_pt_graphics.c
> > @@ -4,6 +4,7 @@
> >  #include "xen_pt.h"
> >  #include "xen-host-pci-device.h"
> >  #include "hw/xen/xen_backend.h"
> > +#include "hw/pci/pci_bus.h"
> >
> >  static int is_vga_passthrough(XenHostPCIDevice *dev)  { @@ -246,3
> > +247,142 @@ static int create_pch_isa_bridge(PCIBus *bus,
> XenHostPCIDevice *hdev)
> >      XEN_PT_LOG(dev, "Intel PCH ISA bridge is created.\n");
> >      return 0;
> >  }
> > +
> > +int pci_create_pch(PCIBus *bus)
> > +{
> > +    XenHostPCIDevice hdev;
> > +    int r = 0;
> > +
> > +    if (!xen_has_gfx_passthru) {
> > +        return -1;
> > +    }
> > +
> > +    r = xen_host_pci_device_get(&hdev, 0, 0, 0x1f, 0);
> > +    if (r) {
> > +        XEN_PT_ERR(NULL, "Fail to find intel PCH in host\n");
> 
> Failed to find Intel PCH on host!
> 
> > +        goto err;
> > +    }
> > +
> > +    if (hdev.vendor_id == PCI_VENDOR_ID_INTEL) {
> > +        r = create_pch_isa_bridge(bus, &hdev);
> > +        if (r) {
> > +            XEN_PT_ERR(NULL, "Fail to create PCH ISA bridge.\n");
> 
> Failed
> > +            goto err;
> > +        }
> > +    }
> > +
> > +    xen_host_pci_device_put(&hdev);
> > +
> > +    return  r;
> 
> Remove this return and let it go through.
> > +
> > +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)]) {
> > +        XenPCIPassthroughState *s =
> DO_UPCAST(XenPCIPassthroughState, dev, f);
> > +        return (is_vga_passthrough(&s->real_device)
> > +                    && (s->real_device.vendor_id ==
> PCI_VENDOR_ID_INTEL));
> > +    } else {
> > +        return 0;
> > +    }
> > +}
> > +
> > +void igd_pci_write(PCIDevice *pci_dev, uint32_t config_addr,
> > +                   uint32_t val, int len) {
> > +    XenHostPCIDevice dev;
> > +    int r;
> > +
> > +    assert(pci_dev->devfn == 0x00);
> 
> Now I am confused. That would mean the host bridge, while earlier
> (pci_create_pch) you were using the ISA bridge.

IGD read/write is through the host bridge.
ISA bridge is only for detect purpose. In i915 driver it will probe ISA bridge 
to discover the IGD, see comment in i915_drv.c:
intel_detect_pch():
     * The reason to probe ISA bridge instead of Dev31:Fun0 is to
     * make graphics device passthrough work easy for VMM, that only
     * need to expose ISA bridge to let driver know the real hardware
     * underneath. This is a requirement from virtualization team.



> 
> Could you add a comment please?

Sure. 

> > +
> > +    if (!is_igd_passthrough(pci_dev)) {
> > +        goto write_default;
> > +    }
> > +
> > +    switch (config_addr) {
> > +    case 0x58:      /* PAVPC Offset */
> > +        break;
> > +    default:
> > +        goto write_default;
> 
> As I understand this function will be called when the guest tries to touch the
> configuration of the VGA and write in the configuration entries.
> 
> > +    }
> > +
> > +    /* Host write */
> > +    r = xen_host_pci_device_get(&dev, 0, 0, 0, 0);
> 
> > +    if (r) {
> > +        XEN_PT_ERR(pci_dev, "Can't get pci_dev_host_bridge\n");
> > +        abort();
> > +    }
> > +
> > +    r = xen_host_pci_set_block(&dev, config_addr, (uint8_t *)&val, len);
> > +    if (r) {
> > +        XEN_PT_ERR(pci_dev, "Can't get pci_dev_host_bridge\n");
> > +        abort();
> > +    }
> > +
> > +    xen_host_pci_device_put(&dev);
> > +
> > +    return;
> > +
> > +write_default:
> > +    pci_default_write_config(pci_dev, config_addr, val, len);
> 
> 
> and we just allow it through. But what happens if the guest decides to change
> the BAR sizes?  Or fiddle with the GTT?
> 
> Ouch. That really looks dangerous - or maybe I am too paranoid?
> 

I do not quite understand your concern. We only pass through PAVPC to physical 
host bridge. The others are handled by current logic. We don't change any of 
it. So what problem will be exposed by this patch?

Best regards,
Yang



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