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

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>
Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
Cc: Weidong Han <weidong.han@intel.com>
---
v5:

* Don't pass vendor/device ids in igd_pci_read().
* Add to support offset 0x44/0x48.

v4:

* Given that pci_create_pch() is called unconditionally, so we just return 0
  even if its failed to check xen_has_gfx_passthru.
* Remove one spurious change. 

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 | 156 +++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 160 insertions(+)

Il 25/06/2014 04:17, Tiejun Chen ha scritto:
> +int pci_create_pch(PCIBus *bus)
> +{
> +    XenHostPCIDevice hdev;
> +    int r = 0;
> +
> +    if (!xen_has_gfx_passthru) {
> +        return r;
> +    }
> +

You could make this an assertion, since the function is never called 
with xen_has_gfx_passthru == 0.  Or just drop the check completely, so 
the function can be moved in patch 2.

Paolo

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
> 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>
> Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
> Cc: Weidong Han <weidong.han@intel.com>
> ---
> v5:
> 
> * Don't pass vendor/device ids in igd_pci_read().
> * Add to support offset 0x44/0x48.
> 
> v4:
> 
> * Given that pci_create_pch() is called unconditionally, so we just return 0
>   even if its failed to check xen_has_gfx_passthru.
> * Remove one spurious change. 
> 
> 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 | 156 +++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 160 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 974b7e9..f3fbfed 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)
>  {
> @@ -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.
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?
It seems to return success.
Maybe you should just verify that vendor and device
ID have the expected values on the host, and
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?
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.

> +{
> +    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?

> +     */
> +
> +    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) {
	     /* 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.
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.

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

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

On 2014/6/25 14:25, Paolo Bonzini wrote:
> Il 25/06/2014 04:17, Tiejun Chen ha scritto:
>> +int pci_create_pch(PCIBus *bus)
>> +{
>> +    XenHostPCIDevice hdev;
>> +    int r = 0;
>> +
>> +    if (!xen_has_gfx_passthru) {
>> +        return r;
>> +    }
>> +
>
> You could make this an assertion, since the function is never called
> with xen_has_gfx_passthru == 0.  Or just drop the check completely, so

I will drop this completely.

Thanks
Tiejun

> the function can be moved in patch 2.
>
> Paolo
>
>

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
> 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>
> Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
> Cc: Weidong Han <weidong.han@intel.com>
> ---
> v5:
> 
> * Don't pass vendor/device ids in igd_pci_read().
> * Add to support offset 0x44/0x48.
> 
> v4:
> 
> * Given that pci_create_pch() is called unconditionally, so we just return 0
>   even if its failed to check xen_has_gfx_passthru.
> * Remove one spurious change. 
> 
> 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 | 156 +++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 160 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 974b7e9..f3fbfed 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)
>  {
> @@ -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)
> +{
> +    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;
> +        }
> +    }
> +
> +    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.
> + */
> +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;
> +
> +    /* 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 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();
> +    }
> +
> +    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 */
> +    case 0x44:        /* MCHBAR I915 */
> +    case 0x48:        /* MCHBAR I965 */

In fact, this is returning host addresses to guest, right?
Don't see how this can work except by luck.

> +    case 0x50:        /* SNB: processor graphics control register */
> +    case 0x52:        /* processor graphics control register */
> +    case 0xa0:        /* top of memory */

which memory? Could be similar.

> +    case 0xb0:        /* ILK: BSM: should read from dev 2 offset 0x5c */
> +    case 0x58:        /* SNB: PAVPC Offset */
> +    case 0xa4:        /* SNB: graphics base of stolen memory */

Same thing.

> +    case 0xa8:        /* SNB: base of GTT stolen memory */

Here too.

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

On 2014/6/25 22:05, 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
>> 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>
>> Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
>> Cc: Weidong Han <weidong.han@intel.com>
>> ---

[snip]

>> +
>> +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 */
>> +    case 0x44:        /* MCHBAR I915 */
>> +    case 0x48:        /* MCHBAR I965 */
>
> In fact, this is returning host addresses to guest, right?

Yes. These two registers includes the address and some bits to control 
something. And the i915 driver just need to *read* address to reserve 
for PHP, but actually we don't support PHP now. So these address space 
are just be reserved.

And then it will *write* them with the PCI resources allocated in guest. 
And update those bits to enable or disable something in different occasions.

Actually we didn't introduce 0x44/0x48 originally but when Paolo saw the 
i915 driver will access them, so ask me what will be occurred. So I just 
add them simply to test again. But now looks they should be emulated, so 
I think we can get them out now. Then this should be enough to make sense.

> Don't see how this can work except by luck.
>
>> +    case 0x50:        /* SNB: processor graphics control register */
>> +    case 0x52:        /* processor graphics control register */
>> +    case 0xa0:        /* top of memory */
>
> which memory? Could be similar.

This is just a total memory used to the video device so we call top of 
memory. And this is used to limit that range, but as you know we never 
go over this so we can always live here.

>
>> +    case 0xb0:        /* ILK: BSM: should read from dev 2 offset 0x5c */
>> +    case 0x58:        /* SNB: PAVPC Offset */
>> +    case 0xa4:        /* SNB: graphics base of stolen memory */
>
> Same thing.
>
>> +    case 0xa8:        /* SNB: base of GTT stolen memory */
>
> Here too.
>

We have 1:1 mapping for these two ranges so its fine.

These info are known to Linux. For Windows, it is opaque to us so we 
can't know what will be happened to Windows exactly, but I think this 
should be similar with Linux i915 driver mostly. And especially we also 
validate this on Windows 7 to confirm everything can work under Windows.

Thanks
Tiejun

On Thu, Jun 26, 2014 at 01:34:14PM +0800, Chen, Tiejun wrote:
> On 2014/6/25 22:05, 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
> >>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>
> >>Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
> >>Cc: Weidong Han <weidong.han@intel.com>
> >>---
> 
> [snip]
> 
> >>+
> >>+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 */
> >>+    case 0x44:        /* MCHBAR I915 */
> >>+    case 0x48:        /* MCHBAR I965 */
> >
> >In fact, this is returning host addresses to guest, right?
> 
> Yes. These two registers includes the address and some bits to control
> something. And the i915 driver just need to *read* address to reserve for
> PHP, but actually we don't support PHP now. So these address space are just
> be reserved.
> 
> And then it will *write* them with the PCI resources allocated in guest. And
> update those bits to enable or disable something in different occasions.
> 
> Actually we didn't introduce 0x44/0x48 originally but when Paolo saw the
> i915 driver will access them, so ask me what will be occurred. So I just add
> them simply to test again. But now looks they should be emulated, so I think
> we can get them out now. Then this should be enough to make sense.

Yes.

> >Don't see how this can work except by luck.
> >
> >>+    case 0x50:        /* SNB: processor graphics control register */
> >>+    case 0x52:        /* processor graphics control register */
> >>+    case 0xa0:        /* top of memory */
> >
> >which memory? Could be similar.
> 
> This is just a total memory used to the video device so we call top of
> memory. And this is used to limit that range, but as you know we never go
> over this so we can always live here.

Sorry I don't understand what you are saying here.

> >
> >>+    case 0xb0:        /* ILK: BSM: should read from dev 2 offset 0x5c */
> >>+    case 0x58:        /* SNB: PAVPC Offset */
> >>+    case 0xa4:        /* SNB: graphics base of stolen memory */
> >
> >Same thing.
> >
> >>+    case 0xa8:        /* SNB: base of GTT stolen memory */
> >
> >Here too.
> >
> 
> We have 1:1 mapping for these two ranges so its fine.

Could you clarify? My question, again, is whether this is
returning host addresses to guest.

> These info are known to Linux. For Windows, it is opaque to us so we can't
> know what will be happened to Windows exactly, but I think this should be
> similar with Linux i915 driver mostly. And especially we also validate this
> on Windows 7 to confirm everything can work under Windows.
> 
> Thanks
> Tiejun

On 2014/6/26 14:04, Michael S. Tsirkin wrote:
> On Thu, Jun 26, 2014 at 01:34:14PM +0800, Chen, Tiejun wrote:
>> On 2014/6/25 22:05, 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
>>>> 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>
>>>> Signed-off-by: Tiejun Chen <tiejun.chen@intel.com>
>>>> Cc: Weidong Han <weidong.han@intel.com>
>>>> ---
>>
>> [snip]
>>
>>>> +
>>>> +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 */
>>>> +    case 0x44:        /* MCHBAR I915 */
>>>> +    case 0x48:        /* MCHBAR I965 */
>>>
>>> In fact, this is returning host addresses to guest, right?
>>
>> Yes. These two registers includes the address and some bits to control
>> something. And the i915 driver just need to *read* address to reserve for
>> PHP, but actually we don't support PHP now. So these address space are just
>> be reserved.
>>
>> And then it will *write* them with the PCI resources allocated in guest. And
>> update those bits to enable or disable something in different occasions.
>>
>> Actually we didn't introduce 0x44/0x48 originally but when Paolo saw the
>> i915 driver will access them, so ask me what will be occurred. So I just add
>> them simply to test again. But now looks they should be emulated, so I think
>> we can get them out now. Then this should be enough to make sense.
>
> Yes.
>
>>> Don't see how this can work except by luck.
>>>
>>>> +    case 0x50:        /* SNB: processor graphics control register */
>>>> +    case 0x52:        /* processor graphics control register */
>>>> +    case 0xa0:        /* top of memory */
>>>
>>> which memory? Could be similar.
>>
>> This is just a total memory used to the video device so we call top of
>> memory. And this is used to limit that range, but as you know we never go
>> over this so we can always live here.
>
> Sorry I don't understand what you are saying here.

Gfx guy told me the memory here is indicating the total amount of 
populated physical memory that can be allocated to VGA, so this is fine 
to guest as a amount.

>
>>>
>>>> +    case 0xb0:        /* ILK: BSM: should read from dev 2 offset 0x5c */
>>>> +    case 0x58:        /* SNB: PAVPC Offset */
>>>> +    case 0xa4:        /* SNB: graphics base of stolen memory */
>>>
>>> Same thing.
>>>
>>>> +    case 0xa8:        /* SNB: base of GTT stolen memory */
>>>
>>> Here too.
>>>
>>
>> We have 1:1 mapping for these two ranges so its fine.
>
> Could you clarify? My question, again, is whether this is
> returning host addresses to guest.

I means the host address is equal to the guest address and then 1:1 map 
between them.

Thanks
Tiejun

>
>> These info are known to Linux. For Windows, it is opaque to us so we can't
>> know what will be happened to Windows exactly, but I think this should be
>> similar with Linux i915 driver mostly. And especially we also validate this
>> on Windows 7 to confirm everything can work under Windows.
>>
>> Thanks
>> Tiejun
>
>

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