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

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

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
>
>