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

["Chen, Tiejun" <tiejun.chen@xxxxxxxxx>]

On 2014/6/25 15:04, Michael S. Tsirkin wrote:
> On Wed, Jun 25, 2014 at 10:17:19AM +0800, Tiejun Chen wrote:
> > Some registers of Intel IGD are mapped in host bridge, so it needs to
[snip]

> >   static int is_vga_passthrough(XenHostPCIDevice *dev)
> >   {
> > @@ -291,3 +292,158 @@ static int create_pseudo_pch_isa_bridge(PCIBus *bus, 
> > XenHostPCIDevice *hdev)
> >       XEN_PT_LOG(dev, "The pseudo Intel PCH ISA bridge created.\n");
> >       return 0;
> >   }
> > +
> > +int pci_create_pch(PCIBus *bus)
>
> Please prefix all xen specific non static functions
> with xen_ or something like this.
Okay.

> pci_ is for pci core.
>
> In fact it's a good idea to do this for static functions
> as well, in case we add a conflicting function in
> some header.
>
> > +{
> > +    XenHostPCIDevice hdev;
> > +    int r = 0;
> > +
> > +    if (!xen_has_gfx_passthru) {
> > +        return r;
> > +    }
> > +
> > +    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_pseudo_pch_isa_bridge(bus, &hdev);
> > +        if (r) {
> > +            XEN_PT_ERR(NULL, "Failed to create PCH ISA bridge.\n");
> > +            goto err;
> > +        }
> > +    }
> Does it work on non intel?
IGD means this should work on Intel platform.

> It seems to return success.
Okay, I'd like to change this a void.

> Maybe you should just verify that vendor and device
> ID have the expected values on the host, and
Vendor id is enough.

> fail otherwise.
>
> > +
> > +    xen_host_pci_device_put(&hdev);
> > +
> > +err:
> > +    return r;
> > +}
> > +
> > +/*
> > + * Currently we just pass this physical host bridge for IGD, 00:02.0.
> > + *
> > + * Here pci_dev is just that host bridge, so we have to get that real
> > + * passthrough device by that given devfn to further confirm.
> > + */
>
> confirm what?
So change like:

* passthrough device by that given devfn to avoid other devices access.

> Comments like this need to document what function does.
>
> Maybe
>
> /* Can we support IGD passthrough for this device?
>   * We require ... <XYZ - fill in here>
>   */
>
> > +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)
> Same here, xen_ everywhere please.
Okay.

> > +{
> > +    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().
> You mean in linux kernel I guess?
So change like,

* probe ISA bridge to discover the IGD, see comment in Linux:i915_drv.c:

> > +     */
> > +
> > +    assert(pci_dev->devfn == 0x00);
> > +
> > +    if (!is_igd_passthrough(pci_dev)) {
> > +        goto write_default;
> > +    }
> > +
> > +    /* Just work for the i915 driver. */
> > +    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();
> > +    }
>
> Cleaner:
>
>         if (config_addr == 0x58) {
Maybe we add other offset in the future, so we'd better keep in them in 
switch().
>              /* 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();
>              }
>         }
>
> Note this does not work on e.g. BE.
Why do we need take BE into consideration here? Shouldn't PCI already be LE?

> The best way is really to make the register writeable in wmask.
> Then
>      pci_default_write_config(pci_dev, config_addr, val, len);
>      if (range_covers_byte(addr, len, 0x58)) {
>                 ....
>
>              r = xen_host_pci_set_block(&dev, config_addr,
>                  pci_dev->config + config_addr, len);
>      }
>
>
>
>
>
> > +
> > +    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().
> > +     */
> > +    assert(pci_dev->devfn == 0x00);
> > +
> > +    if (!is_igd_passthrough(pci_dev)) {
> > +        goto read_default;
> > +    }
> > +
> > +    /* Just work for the i915 driver. */
> > +    switch (config_addr) {
> > +    case 0x08:        /* revision id */
> > +    case 0x2c:        /* sybsystem vendor id */
> > +    case 0x2e:        /* sybsystem id */
> Since you set them to match host previously,
> should be no need to override now.
Just *read* the host bridge, no *write* here.

> > +    case 0x44:        /* MCHBAR I915 */
> > +    case 0x48:        /* MCHBAR I965 */
> > +    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 */
> Move the actual code here.
Sorry, whats the actual code? Are you saying we can read all values from 
the host bridge in advance then restore them here?
Thanks
Tiejun

> > +        break;
> > +    default:
> > +        /* Just gets the emulated values. */
> and here.
>
> > +        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