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

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

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.
> 
>  hw/xen/xen_pt.h          |   4 ++
>  hw/xen/xen_pt_graphics.c | 159 
> +++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 163 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 51b174f..72e30ac 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)
>  {
> @@ -293,3 +294,161 @@ static int create_pch_isa_bridge(PCIBus *bus, 
> XenHostPCIDevice *hdev)
>      XEN_PT_LOG(dev, "Intel PCH ISA bridge created.\n");
>      return 0;
>  }
> +
> +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;


> +    }
> +
> +    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?
In that case, shouldn't it just be:

if (pci_dev) {


> +        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;
> +
> +    /* 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.
> +     */
> +
> +    assert(pci_dev->devfn == 0x00);
> +
> +    if (!is_igd_passthrough(pci_dev)) {
> +        goto write_default;
> +    }
> +
> +    switch (config_addr) {
> +    case 0x58:      /* PAVPC Offset */
> +        break;
> +    default:
> +        /* Just sets the emulated values. */
> +        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;
> +
> +    /* 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.
> +     */
> +    assert(pci_dev->devfn == 0x00);
> +
> +    if (!is_igd_passthrough(pci_dev)) {
> +        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:
> +        /* Just gets the emulated values. */
> +        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;
> +}
> -- 
> 1.9.1
> 

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