Re: [PATCH 4/5] xen, gfx passthrough: support Intel IGD passthrough with VT-D

["Michael S. Tsirkin" <mst@redhat.com>]

On Thu, Mar 27, 2014 at 06:21:08PM +0000, Stefano Stabellini wrote:
> CC'ing Michael.
> 
> On Fri, 21 Feb 2014, Yang Zhang wrote:
> > From: Yang Zhang <yang.z.zhang@Intel.com>
> > 
> > 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@Intel.com>
> > Cc:Weidong Han <weidong.han@intel.com>
> > ---
> >  hw/pci-host/piix.c       |   15 ++++++
> >  hw/pci/pci.c             |   13 +++++
> >  hw/xen/xen_pt.h          |    5 ++
> >  hw/xen/xen_pt_graphics.c |  127 ++++++++++++++++++++++++++++++++++++++++++++++
> >  4 files changed, 160 insertions(+), 0 deletions(-)
> > 
> > diff --git a/hw/pci-host/piix.c b/hw/pci-host/piix.c
> > index ffdc853..68cf756 100644
> > --- a/hw/pci-host/piix.c
> > +++ b/hw/pci-host/piix.c
> > @@ -34,6 +34,9 @@
> >  #include "sysemu/sysemu.h"
> >  #include "hw/i386/ioapic.h"
> >  #include "qapi/visitor.h"
> > +#if defined(CONFIG_XEN_PCI_PASSTHROUGH)
> > +#include "hw/xen/xen_pt.h"
> > +#endif
> >  
> >  /*
> >   * I440FX chipset data sheet.
> > @@ -389,6 +392,18 @@ PCIBus *i440fx_init(PCII440FXState **pi440fx_state,
> >  
> >      i440fx_update_memory_mappings(f);
> >  
> > +#if defined(CONFIG_XEN_PCI_PASSTHROUGH)
> > +    /*
> > +     * 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.
> > +     */
> > +    if (intel_pch_init(b) == 0) {
> > +        d->config_read = igd_pci_read;
> > +        d->config_write = igd_pci_write;
> > +    }
> > +#endif
> 
> I can't see the other QEMU maintainers being happy with these changes.
> Michael, do you have a suggestion on how to do this in a better way?

Implement your own pci host bridge, inherit the standard one.


> 
> >      return b;
> >  }
> >  
> > diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> > index e81816e..7016b71 100644
> > --- a/hw/pci/pci.c
> > +++ b/hw/pci/pci.c
> > @@ -36,6 +36,9 @@
> >  #include "hw/pci/msix.h"
> >  #include "exec/address-spaces.h"
> >  #include "hw/hotplug.h"
> > +#if defined(CONFIG_XEN_PCI_PASSTHROUGH)
> > +#include "hw/xen/xen_pt.h"
> > +#endif
> >  
> >  //#define DEBUG_PCI
> >  #ifdef DEBUG_PCI
> > @@ -805,6 +808,16 @@ static PCIDevice *do_pci_register_device(PCIDevice *pci_dev, PCIBus *bus,
> >      PCIConfigWriteFunc *config_write = pc->config_write;
> >      AddressSpace *dma_as;
> >  
> > +#if defined(CONFIG_XEN_PCI_PASSTHROUGH)
> > +    /*
> > +     * Some video bioses and gfx drivers will assume the bdf of IGD is 00:02.0.
> > +     * So user need to set it to 00:02.0 in Xen configure file explicitly,
> > +     * otherwise IGD will fail to work.
> > +     */
> > +    if (gfx_passthru && devfn == 0x10)
> > +        igd_passthru = 1;
> > +    else
> > +#endif
> >      if (devfn < 0) {
> >          for(devfn = bus->devfn_min ; devfn < ARRAY_SIZE(bus->devices);
> >              devfn += PCI_FUNC_MAX) {
> 
> Same here
> 

So just set the address property to 00:02.0.


> > diff --git a/hw/xen/xen_pt.h b/hw/xen/xen_pt.h
> > index c04bbfd..92e4d51 100644
> > --- a/hw/xen/xen_pt.h
> > +++ b/hw/xen/xen_pt.h
> > @@ -299,8 +299,13 @@ static inline bool xen_pt_has_msix_mapping(XenPCIPassthroughState *s, int bar)
> >  }
> >  
> >  extern int gfx_passthru;
> > +extern int igd_passthru;
> >  int register_vga_regions(XenHostPCIDevice *dev);
> >  int unregister_vga_regions(XenHostPCIDevice *dev);
> >  int setup_vga_pt(XenHostPCIDevice *dev);
> > +int intel_pch_init(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 54f16cf..2a01406 100644
> > --- a/hw/xen/xen_pt_graphics.c
> > +++ b/hw/xen/xen_pt_graphics.c
> > @@ -5,6 +5,8 @@
> >  #include "xen-host-pci-device.h"
> >  #include "hw/xen/xen_backend.h"
> >  
> > +int igd_passthru;
> > +
> >  /*
> >   * register VGA resources for the domain with assigned gfx
> >   */
> > @@ -233,3 +235,128 @@ static int create_pch_isa_bridge(PCIBus *bus, XenHostPCIDevice *hdev)
> >      XEN_PT_LOG(dev, "Intel PCH ISA bridge is created.\n");
> >      return 0;
> >  }
> > +
> > +int intel_pch_init(PCIBus *bus)
> > +{
> > +    XenHostPCIDevice hdev;
> > +    int r = 0;
> > +
> > +    if (!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");
> > +        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");
> > +            goto err;
> > +        }
> > +    }
> > +
> > +    xen_host_pci_device_put(&hdev);
> > +
> > +    return  r;
> > +
> > +err:
> > +    return r;
> > +}
> > +
> > +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);
> > +
> > +    if (!igd_passthru) {
> > +        goto write_default;
> > +    }
> > +
> > +    switch (config_addr) {
> > +    case 0x58:      /* PAVPC Offset */
> > +        break;
> > +    default:
> > +        goto write_default;
> > +    }
> > +
> > +    /* 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);
> > +}
> > +
> > +uint32_t igd_pci_read(PCIDevice *pci_dev, uint32_t config_addr, int len)
> > +{
> > +    XenHostPCIDevice dev;
> > +    uint32_t val;
> > +    int r;
> > +
> > +    assert(pci_dev->devfn == 0x00);
> > +
> > +    if (!igd_passthru) {
> > +        goto read_default;
> > +    }
> > +
> > +    switch (config_addr) {
> > +    case 0x00:        /* vendor id */
> > +    case 0x02:        /* device id */
> > +    case 0x08:        /* revision id */
> > +    case 0x2c:        /* sybsystem vendor id */
> > +    case 0x2e:        /* sybsystem id */
> > +    case 0x50:        /* SNB: processor graphics control register */
> > +    case 0x52:        /* processor graphics control register */
> > +    case 0xa0:        /* top of memory */
> > +    case 0xb0:        /* ILK: BSM: should read from dev 2 offset 0x5c */
> > +    case 0x58:        /* SNB: PAVPC Offset */
> > +    case 0xa4:        /* SNB: graphics base of stolen memory */
> > +    case 0xa8:        /* SNB: base of GTT stolen memory */
> > +        break;
> > +    default:
> > +        goto read_default;
> > +    }
> > +
> > +    /* Host read */
> > +    r = xen_host_pci_device_get(&dev, 0, 0, 0, 0);
> > +    if (r) {
> > +        goto err_out;
> > +    }
> > +
> > +    r = xen_host_pci_get_block(&dev, config_addr, (uint8_t *)&val, len);
> > +    if (r) {
> > +        goto err_out;
> > +    }
> > +
> > +    xen_host_pci_device_put(&dev);
> > +
> > +    return val;
> > +
> > +read_default:
> > +    return pci_default_read_config(pci_dev, config_addr, len);
> > +
> > +err_out:
> > +    XEN_PT_ERR(pci_dev, "Can't get pci_dev_host_bridge\n");
> > +    return -1;
> > +}
> 
> The Xen specific part is OK.