[PATCH v2] qemu-xen:Correctly expose PCH ISA bridge for IGD passthrough

[PATCH v2] qemu-xen:Correctly expose PCH ISA bridge for IGD passthrough

[G.R.]

Fix IGD passthrough logic to properly expose PCH ISA bridge (instead
of exposing as pci-pci bridge). The i915 driver require this to
correctly detect the PCH version and enable version specific code
path.

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>,
Timothy Guo <firemeteor@users.sourceforge.net>

diff --git a/hw/pci.c b/hw/pci.c
index f051de1..d371bd7 100644
--- a/hw/pci.c
+++ b/hw/pci.c
@@ -871,11 +871,6 @@ void pci_unplug_netifs(void)
     }
 }

-typedef struct {
-    PCIDevice dev;
-    PCIBus *bus;
-} PCIBridge;
-
 void pci_bridge_write_config(
PCIDevice *d,
                              uint32_t address, uint32_t val, int len)
 {
diff --git a/hw/pci.h b/hw/pci.h
index edc58b6..c2acab9 100644
--- a/hw/pci.h
+++ b/hw/pci.h
@@ -222,6 +222,11 @@ struct PCIDevice {
     int irq_state[4];
 };

+typedef struct {
+    PCIDevice dev;
+    PCIBus *bus;
+} PCIBridge;
+
 extern char direct_pci_str[];
 extern int direct_pci_msitranslate;
 extern int direct_pci_power_mgmt;
diff --git a/hw/pt-graphics.c b/hw/pt-graphics.c
index c6f8869..de21f90 100644
--- a/hw/pt-graphics.c
+++ b/hw/pt-graphics.c
@@ -3,6 +3,7 @@
  */

 #include "pass-through.h"
+#include "pci.h"
 #include "pci/header.h"
 #include "pci/pci.h"

@@ -40,9 +41,26 @@ void intel_pch_init(PCIBus *bus)
     did = pt_pci_host_read(pci_dev_1f, PCI_DEVICE_ID, 2);
     rid = pt_pci_host_read(pci_dev_1f, PCI_REVISION, 1);

-    if ( vid == PCI_VENDOR_ID_INTEL )
-        pci_bridge_init(bus, PCI_DEVFN(0x1f, 0), vid, did, rid,
-                        pch_map_irq, "intel_bridge_1f");
+    if (vid == PCI_VENDOR_ID_INTEL) {
+        PCIBridge *s = (PCIBridge *)pci_register_device(bus, "intel_bridge_1f",
+                sizeof(PCIBridge), PCI_DEVFN(0x1f, 0), NULL,
pci_bridge_write_config);
+
+        pci_config_set_vendor_id(s->dev.config, vid);
+        pci_config_set_device_id(s->dev.config, did);
+
+        s->dev.config[PCI_COMMAND] = 0x06; // command = bus master, pci mem
+        s->dev.config[PCI_COMMAND + 1] = 0x00;
+        s->dev.config[PCI_STATUS] = 0xa0; // status = fast
back-to-back, 66MHz, no error
+        s->dev.config[PCI_STATUS + 1] = 0x00; // status = fast devsel
+        s->dev.config[PCI_REVISION] = rid;
+        s->dev.config[PCI_CLASS_PROG] = 0x00; // programming i/f
+        pci_config_set_class(s->dev.config, PCI_CLASS_BRIDGE_ISA);
+        s->dev.config[PCI_LATENCY_TIMER] = 0x10;
+        s->dev.config[PCI_HEADER_TYPE] = 0x80;
+        s->dev.config[PCI_SEC_STATUS] = 0xa0;
+
+        s->bus = pci_register_secondary_bus(&s->dev, pch_map_irq);
+    }
 }

 uint32_t igd_read_opregion(struct pt_dev *pci_dev)

Re: [PATCH v2] qemu-xen:Correctly expose PCH ISA bridge for IGD passthrough

[Stefano Stabellini]

On Wed, 19 Dec 2012, G.R. wrote:
> Fix IGD passthrough logic to properly expose PCH ISA bridge (instead
> of exposing as pci-pci bridge). The i915 driver require this to
> correctly detect the PCH version and enable version specific code
> path.
> 
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>,
> Timothy Guo <firemeteor@users.sourceforge.net>

The patch is fine by me

> diff --git a/hw/pci.c b/hw/pci.c
> index f051de1..d371bd7 100644
> --- a/hw/pci.c
> +++ b/hw/pci.c
> @@ -871,11 +871,6 @@ void pci_unplug_netifs(void)
>      }
>  }
> 
> -typedef struct {
> -    PCIDevice dev;
> -    PCIBus *bus;
> -} PCIBridge;
> -
>  void pci_bridge_write_config(
> PCIDevice *d,
>                               uint32_t address, uint32_t val, int len)
>  {
> diff --git a/hw/pci.h b/hw/pci.h
> index edc58b6..c2acab9 100644
> --- a/hw/pci.h
> +++ b/hw/pci.h
> @@ -222,6 +222,11 @@ struct PCIDevice {
>      int irq_state[4];
>  };
> 
> +typedef struct {
> +    PCIDevice dev;
> +    PCIBus *bus;
> +} PCIBridge;
> +
>  extern char direct_pci_str[];
>  extern int direct_pci_msitranslate;
>  extern int direct_pci_power_mgmt;
> diff --git a/hw/pt-graphics.c b/hw/pt-graphics.c
> index c6f8869..de21f90 100644
> --- a/hw/pt-graphics.c
> +++ b/hw/pt-graphics.c
> @@ -3,6 +3,7 @@
>   */
> 
>  #include "pass-through.h"
> +#include "pci.h"
>  #include "pci/header.h"
>  #include "pci/pci.h"
> 
> @@ -40,9 +41,26 @@ void intel_pch_init(PCIBus *bus)
>      did = pt_pci_host_read(pci_dev_1f, PCI_DEVICE_ID, 2);
>      rid = pt_pci_host_read(pci_dev_1f, PCI_REVISION, 1);
> 
> -    if ( vid == PCI_VENDOR_ID_INTEL )
> -        pci_bridge_init(bus, PCI_DEVFN(0x1f, 0), vid, did, rid,
> -                        pch_map_irq, "intel_bridge_1f");
> +    if (vid == PCI_VENDOR_ID_INTEL) {
> +        PCIBridge *s = (PCIBridge *)pci_register_device(bus, "intel_bridge_1f",
> +                sizeof(PCIBridge), PCI_DEVFN(0x1f, 0), NULL,
> pci_bridge_write_config);
> +
> +        pci_config_set_vendor_id(s->dev.config, vid);
> +        pci_config_set_device_id(s->dev.config, did);
> +
> +        s->dev.config[PCI_COMMAND] = 0x06; // command = bus master, pci mem
> +        s->dev.config[PCI_COMMAND + 1] = 0x00;
> +        s->dev.config[PCI_STATUS] = 0xa0; // status = fast
> back-to-back, 66MHz, no error
> +        s->dev.config[PCI_STATUS + 1] = 0x00; // status = fast devsel
> +        s->dev.config[PCI_REVISION] = rid;
> +        s->dev.config[PCI_CLASS_PROG] = 0x00; // programming i/f
> +        pci_config_set_class(s->dev.config, PCI_CLASS_BRIDGE_ISA);
> +        s->dev.config[PCI_LATENCY_TIMER] = 0x10;
> +        s->dev.config[PCI_HEADER_TYPE] = 0x80;
> +        s->dev.config[PCI_SEC_STATUS] = 0xa0;
> +
> +        s->bus = pci_register_secondary_bus(&s->dev, pch_map_irq);
> +    }
>  }
> 
>  uint32_t igd_read_opregion(struct pt_dev *pci_dev)
> 

Patch series for IGD passthrough

[Rui Guo]

This series contains all the fixes required to produce a working IGD
passthrough box. All the changes are previously seen in the dev list but
not yet accepted. Some of them are out-dated and need some reshape.

Detailed description can be found later in each patch.

. [PATCH 1/3] qemu-xen-trad/pt_msi_disable: do not clear all MSI flags
. [PATCH 2/3] qemu-xen-trad: Correctly expose PCH ISA bridge for IGD
. [PATCH 3/3] qemu-xen-trad: IGD passthrough: Expose vendor specific

Thanks,
Timothy

[PATCH 1/3] qemu-xen-trad/pt_msi_disable: do not clear all MSI flags

[Rui Guo]

"qemu-xen-trad: fix msi_translate with PV event delivery" added a
pt_msi_disable() call into pt_msgctrl_reg_write, clearing the MSI flags as a
consequence. MSIs get enabled again soon after by calling pt_msi_setup.

However the MSI flags are only setup once in the pt_msgctrl_reg_init function,
so from the QEMU point of view the device has lost some important properties,
like for example PCI_MSI_FLAGS_64BIT.

This patch fixes the bug by clearing only the MSI enabled/mapped/initialized
flags in pt_msi_disable.

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Tested-by: G.R. <firemeteor@users.sourceforge.net>
Xen-devel: http://marc.info/?l=xen-devel&m=135489879503075
---
 hw/pt-msi.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/pt-msi.c b/hw/pt-msi.c
index 73f737d..b03b989 100644
--- a/hw/pt-msi.c
+++ b/hw/pt-msi.c
@@ -213,7 +213,7 @@ void pt_msi_disable(struct pt_dev *dev)
 
 out:
     /* clear msi info */
-    dev->msi->flags = 0;
+    dev->msi->flags &= ~(MSI_FLAG_UNINIT | PT_MSI_MAPPED | PCI_MSI_FLAGS_ENABLE);
     dev->msi->pirq = -1;
     dev->msi_trans_en = 0;
 }
-- 
1.7.10.4

[PATCH 2/3] qemu-xen-trad: Correctly expose PCH ISA bridge for IGD passthrough

[Rui Guo]

The i915 driver probes chip version through PCH ISA bridge device / vendor ID.
Previously, the PCH ISA bridge is exposed as PCI-PCI bridge in qemu-xen-trad,
which breaks the assumption of the driver. This change fixes the issue by
correctly exposing the ISA bridge to domU.

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>,
               Rui Guo <firemeteor@users.sourceforge.net>
Tested-by: Rui Guo <firemeteor@users.sourceforge.net>
Xen-devel: http://marc.info/?l=xen-devel&m=135548433715750
---
 hw/pci.c         |    5 -----
 hw/pci.h         |    5 +++++
 hw/pt-graphics.c |   24 +++++++++++++++++++++---
 3 files changed, 26 insertions(+), 8 deletions(-)

diff --git a/hw/pci.c b/hw/pci.c
index f051de1..d371bd7 100644
--- a/hw/pci.c
+++ b/hw/pci.c
@@ -871,11 +871,6 @@ void pci_unplug_netifs(void)
     }
 }
 
-typedef struct {
-    PCIDevice dev;
-    PCIBus *bus;
-} PCIBridge;
-
 void pci_bridge_write_config(PCIDevice *d,
                              uint32_t address, uint32_t val, int len)
 {
diff --git a/hw/pci.h b/hw/pci.h
index edc58b6..c2acab9 100644
--- a/hw/pci.h
+++ b/hw/pci.h
@@ -222,6 +222,11 @@ struct PCIDevice {
     int irq_state[4];
 };
 
+typedef struct {
+    PCIDevice dev;
+    PCIBus *bus;
+} PCIBridge;
+
 extern char direct_pci_str[];
 extern int direct_pci_msitranslate;
 extern int direct_pci_power_mgmt;
diff --git a/hw/pt-graphics.c b/hw/pt-graphics.c
index c6f8869..5d4cf4a 100644
--- a/hw/pt-graphics.c
+++ b/hw/pt-graphics.c
@@ -3,6 +3,7 @@
  */
 
 #include "pass-through.h"
+#include "pci.h"
 #include "pci/header.h"
 #include "pci/pci.h"
 
@@ -40,9 +41,26 @@ void intel_pch_init(PCIBus *bus)
     did = pt_pci_host_read(pci_dev_1f, PCI_DEVICE_ID, 2);
     rid = pt_pci_host_read(pci_dev_1f, PCI_REVISION, 1);
 
-    if ( vid == PCI_VENDOR_ID_INTEL )
-        pci_bridge_init(bus, PCI_DEVFN(0x1f, 0), vid, did, rid,
-                        pch_map_irq, "intel_bridge_1f");
+    if (vid == PCI_VENDOR_ID_INTEL) {
+        PCIBridge *s = (PCIBridge *)pci_register_device(bus, "intel_bridge_1f",
+                sizeof(PCIBridge), PCI_DEVFN(0x1f, 0), NULL, pci_bridge_write_config);
+
+        pci_config_set_vendor_id(s->dev.config, vid);
+        pci_config_set_device_id(s->dev.config, did);
+
+        s->dev.config[PCI_COMMAND] = 0x06; // command = bus master, pci mem
+        s->dev.config[PCI_COMMAND + 1] = 0x00;
+        s->dev.config[PCI_STATUS] = 0xa0; // status = fast back-to-back, 66MHz, no error
+        s->dev.config[PCI_STATUS + 1] = 0x00; // status = fast devsel
+        s->dev.config[PCI_REVISION] = rid;
+        s->dev.config[PCI_CLASS_PROG] = 0x00; // programming i/f
+        pci_config_set_class(s->dev.config, PCI_CLASS_BRIDGE_ISA);
+        s->dev.config[PCI_LATENCY_TIMER] = 0x10;
+        s->dev.config[PCI_HEADER_TYPE] = 0x80;
+        s->dev.config[PCI_SEC_STATUS] = 0xa0;
+
+        s->bus = pci_register_secondary_bus(&s->dev, pch_map_irq);
+    }
 }
 
 uint32_t igd_read_opregion(struct pt_dev *pci_dev)
-- 
1.7.10.4

[PATCH 3/3] qemu-xen-trad: IGD passthrough: Expose vendor specific pci cap on host bridge.

[Rui Guo]

Some versions of the Windows Intel GPU driver expect the vendor
PCI capability to be there on the host bridge config space when
passing through a Intel GPU.

As part of the change, the init for pt_pci_host() return value
has to be modified. With an init of -1 all the return value
smaller than a double word will be prefixed with "f"s.

Signed-off-by: Jean Guyader <jean.guyader@gmail.com>,
               Rui Guo <firemeteor@users.sourceforge.net>
Tested-by: Rui Guo <firemeteor@users.sourceforge.net>
Xen-devel: http://marc.info/?l=xen-devel&m=135748187808766
---
 hw/pass-through.c |    2 +-
 hw/pt-graphics.c  |   69 ++++++++++++++++++++++++++++++++++++++++++++++++-----
 2 files changed, 64 insertions(+), 7 deletions(-)

diff --git a/hw/pass-through.c b/hw/pass-through.c
index 304c438..2e795e1 100644
--- a/hw/pass-through.c
+++ b/hw/pass-through.c
@@ -2101,7 +2101,7 @@ struct pci_dev *pt_pci_get_dev(int bus, int dev, int fn)
 
 u32 pt_pci_host_read(struct pci_dev *pci_dev, u32 addr, int len)
 {
-    u32 val = -1;
+    u32 val = 0;
 
     pci_access_init();
     pci_read_block(pci_dev, addr, (u8 *) &val, len);
diff --git a/hw/pt-graphics.c b/hw/pt-graphics.c
index 5d4cf4a..269aade 100644
--- a/hw/pt-graphics.c
+++ b/hw/pt-graphics.c
@@ -144,6 +144,53 @@ write_default:
     pci_default_write_config(pci_dev, config_addr, val, len);
 }
 
+#define PCI_INTEL_VENDOR_CAP            0x34
+#define PCI_INTEL_VENDOR_CAP_TYPE       0x09
+/*
+ * This function returns 0 is the value hasn't been
+ * updated. That mean the offset doesn't anything to
+ * do with the vendor capability.
+ */
+static uint32_t igd_pci_read_vendor_cap(PCIDevice *pci_dev, uint32_t config_addr, int len,
+                                        uint32_t *val)
+{
+    struct pci_dev *pci_dev_host_bridge = pt_pci_get_dev(0, 0, 0);
+    uint32_t vendor_cap = 0;
+    uint32_t cap_type = 0;
+    uint32_t cap_size = 0;
+
+    vendor_cap = pt_pci_host_read(pci_dev_host_bridge, PCI_INTEL_VENDOR_CAP, 1);
+    if (!vendor_cap)
+        return 0;
+
+    cap_type = pt_pci_host_read(pci_dev_host_bridge, vendor_cap, 1);
+    if (cap_type != PCI_INTEL_VENDOR_CAP_TYPE)
+        return 0;
+
+    if (config_addr == PCI_INTEL_VENDOR_CAP)
+    {
+        *val = vendor_cap;
+        return 1;
+    }
+
+    /* Remove the next capability link */
+    if (config_addr == vendor_cap + 1)
+    {
+        *val = 0;
+        return 1;
+    }
+
+    cap_size = pt_pci_host_read(pci_dev_host_bridge, vendor_cap + 2, 1);
+    if (config_addr >= vendor_cap &&
+            config_addr + len <= vendor_cap + cap_size)
+    {
+        *val = pt_pci_host_read(pci_dev_host_bridge, config_addr, len);
+        return 1;
+    }
+
+    return 0;
+}
+
 uint32_t igd_pci_read(PCIDevice *pci_dev, uint32_t config_addr, int len)
 {
     struct pci_dev *pci_dev_host_bridge;
@@ -151,12 +198,22 @@ uint32_t igd_pci_read(PCIDevice *pci_dev, uint32_t config_addr, int len)
 
     assert(pci_dev->devfn == 0x00);
     if ( !igd_passthru )
-        goto read_default;
+    {
+        val = pci_default_read_config(pci_dev, config_addr, len);
+        goto read_return;
+    }
 
+    /* Exposing writable register does not lead to security risk since this
+       only apply to read. This may confuse the guest, but it works good so far.
+       Will switch to mask & merge style only if this is proved broken.
+       Note: Always expose aligned address if any byte of the dword is to be
+       exposed. This provides a consistent view, at least for read. */
     switch (config_addr)
     {
         case 0x00:        /* vendor id */
         case 0x02:        /* device id */
+        case 0x04:        /* command */
+        case 0x06:        /* status, needed for the cap list bit*/
         case 0x08:        /* revision id */
         case 0x2c:        /* sybsystem vendor id */
         case 0x2e:        /* sybsystem id */
@@ -169,7 +226,9 @@ uint32_t igd_pci_read(PCIDevice *pci_dev, uint32_t config_addr, int len)
         case 0xa8:        /* SNB: base of GTT stolen memory */
             break;
         default:
-            goto read_default;
+            if (!(igd_passthru && igd_pci_read_vendor_cap(pci_dev, config_addr, len, &val)))
+                val = pci_default_read_config(pci_dev, config_addr, len);
+            goto read_return;
     }
 
     /* Host read */
@@ -180,15 +239,13 @@ uint32_t igd_pci_read(PCIDevice *pci_dev, uint32_t config_addr, int len)
     }
 
     val = pt_pci_host_read(pci_dev_host_bridge, config_addr, len);
+
+read_return:
 #ifdef PT_DEBUG_PCI_CONFIG_ACCESS
     PT_LOG_DEV(pci_dev, "addr=%x len=%x val=%x\n",
                config_addr, len, val);
 #endif
     return val;
-   
-read_default:
-   
-   return pci_default_read_config(pci_dev, config_addr, len);
 }
 
 /*
-- 
1.7.10.4

Re: [PATCH 2/3] qemu-xen-trad: Correctly expose PCH ISA bridge for IGD passthrough

[Jan Beulich]

>>> On 07.02.13 at 17:12, Rui Guo <firemeteor@users.sourceforge.net> wrote:
> @@ -40,9 +41,26 @@ void intel_pch_init(PCIBus *bus)
>      did = pt_pci_host_read(pci_dev_1f, PCI_DEVICE_ID, 2);
>      rid = pt_pci_host_read(pci_dev_1f, PCI_REVISION, 1);
>  
> -    if ( vid == PCI_VENDOR_ID_INTEL )
> -        pci_bridge_init(bus, PCI_DEVFN(0x1f, 0), vid, did, rid,
> -                        pch_map_irq, "intel_bridge_1f");
> +    if (vid == PCI_VENDOR_ID_INTEL) {
> +        PCIBridge *s = (PCIBridge *)pci_register_device(bus, "intel_bridge_1f",
> +                sizeof(PCIBridge), PCI_DEVFN(0x1f, 0), NULL, pci_bridge_write_config);
> +
> +        pci_config_set_vendor_id(s->dev.config, vid);
> +        pci_config_set_device_id(s->dev.config, did);
> +
> +        s->dev.config[PCI_COMMAND] = 0x06; // command = bus master, pci mem
> +        s->dev.config[PCI_COMMAND + 1] = 0x00;
> +        s->dev.config[PCI_STATUS] = 0xa0; // status = fast back-to-back, 66MHz, no error
> +        s->dev.config[PCI_STATUS + 1] = 0x00; // status = fast devsel
> +        s->dev.config[PCI_REVISION] = rid;
> +        s->dev.config[PCI_CLASS_PROG] = 0x00; // programming i/f
> +        pci_config_set_class(s->dev.config, PCI_CLASS_BRIDGE_ISA);
> +        s->dev.config[PCI_LATENCY_TIMER] = 0x10;
> +        s->dev.config[PCI_HEADER_TYPE] = 0x80;
> +        s->dev.config[PCI_SEC_STATUS] = 0xa0;
> +
> +        s->bus = pci_register_secondary_bus(&s->dev, pch_map_irq);
> +    }
>  }
>  
>  uint32_t igd_read_opregion(struct pt_dev *pci_dev)

For one I wonder whether this is really _always_ correct. I.e.
the device at 00:1f.0 always being an ISA bridge. Wouldn't it
be better to mirror whatever the host device says?

And then I don't see why you need to open code all of
pci_bridge_init() instead of just overriding the class value after
you called that function (or, if the order of events for some
reason matters, adding another parameter to the function).

Jan

Re: [PATCH 3/3] qemu-xen-trad: IGD passthrough: Expose vendor specific pci cap on host bridge.

[Jan Beulich]

>>> On 07.02.13 at 17:12, Rui Guo <firemeteor@users.sourceforge.net> wrote:
> +static uint32_t igd_pci_read_vendor_cap(PCIDevice *pci_dev, uint32_t config_addr, int len,
> +                                        uint32_t *val)
> +{
> +    struct pci_dev *pci_dev_host_bridge = pt_pci_get_dev(0, 0, 0);
> +    uint32_t vendor_cap = 0;
> +    uint32_t cap_type = 0;
> +    uint32_t cap_size = 0;
> +
> +    vendor_cap = pt_pci_host_read(pci_dev_host_bridge, PCI_INTEL_VENDOR_CAP, 1);
> +    if (!vendor_cap)
> +        return 0;
> +
> +    cap_type = pt_pci_host_read(pci_dev_host_bridge, vendor_cap, 1);
> +    if (cap_type != PCI_INTEL_VENDOR_CAP_TYPE)
> +        return 0;
> +
> +    if (config_addr == PCI_INTEL_VENDOR_CAP)
> +    {
> +        *val = vendor_cap;
> +        return 1;
> +    }
> +
> +    /* Remove the next capability link */
> +    if (config_addr == vendor_cap + 1)
> +    {
> +        *val = 0;
> +        return 1;
> +    }

It doesn't look right to ignore "len" up to here? What if this is a
word read at "vendor_cap"?

Also, why would removing the next capability be correct here,
when you're not removing _all_ other capabilities?

> +
> +    cap_size = pt_pci_host_read(pci_dev_host_bridge, vendor_cap + 2, 1);
> +    if (config_addr >= vendor_cap &&
> +            config_addr + len <= vendor_cap + cap_size)
> +    {
> +        *val = pt_pci_host_read(pci_dev_host_bridge, config_addr, len);
> +        return 1;
> +    }

Similarly such a read wouldn't get handled consistently (with the
above intention) here: You're not masking off the byte at
"vendor_cap + 1".

> +    /* Exposing writable register does not lead to security risk since this
> +       only apply to read. This may confuse the guest, but it works good so far.
> +       Will switch to mask & merge style only if this is proved broken.
> +       Note: Always expose aligned address if any byte of the dword is to be
> +       exposed. This provides a consistent view, at least for read. */

Such a comment is certainly not helping being confident in the
changes you're trying to get merged in.

Jan

Re: [PATCH 3/3] qemu-xen-trad: IGD passthrough: Expose vendor specific pci cap on host bridge.

[G.R.]

>> +    if (config_addr == PCI_INTEL_VENDOR_CAP)
>> +    {
>> +        *val = vendor_cap;
>> +        return 1;
>> +    }
>> +
>> +    /* Remove the next capability link */
>> +    if (config_addr == vendor_cap + 1)
>> +    {
>> +        *val = 0;
>> +        return 1;
>> +    }
>
> It doesn't look right to ignore "len" up to here? What if this is a
> word read at "vendor_cap"?
The function is for intel IGD only and the first two returns are only
sanity checks.
The third is fine also since the PCI register is only one byte in
length and other bytes in that word are reserved.

>
> Also, why would removing the next capability be correct here,
> when you're not removing _all_ other capabilities?

I need help from the original author.
Jean, could you comment on this line in your original patch?

>
>> +
>> +    cap_size = pt_pci_host_read(pci_dev_host_bridge, vendor_cap + 2, 1);
>> +    if (config_addr >= vendor_cap &&
>> +            config_addr + len <= vendor_cap + cap_size)
>> +    {
>> +        *val = pt_pci_host_read(pci_dev_host_bridge, config_addr, len);
>> +        return 1;
>> +    }
>
> Similarly such a read wouldn't get handled consistently (with the
> above intention) here: You're not masking off the byte at
> "vendor_cap + 1".
>
Same as above. Jean, could you comment?

>> +    /* Exposing writable register does not lead to security risk since this
>> +       only apply to read. This may confuse the guest, but it works good so far.
>> +       Will switch to mask & merge style only if this is proved broken.
>> +       Note: Always expose aligned address if any byte of the dword is to be
>> +       exposed. This provides a consistent view, at least for read. */
>
> Such a comment is certainly not helping being confident in the
> changes you're trying to get merged in.

Could you be more specific on your suggestion?
I don't think you are recommending to remove them.
Some clarification: The 'Note' sentence just make it explicit the rule
that existing code follows.
And the first sentence about the writeable register are for the
'command' register, which is a side effect of the same rule due to the
'status' register.

Thanks,
Timothy (Rui)

Re: [PATCH 2/3] qemu-xen-trad: Correctly expose PCH ISA bridge for IGD passthrough

[G.R.]

>
> For one I wonder whether this is really _always_ correct. I.e.
> the device at 00:1f.0 always being an ISA bridge. Wouldn't it
> be better to mirror whatever the host device says?
>
>From the Intel driver code, it's looking for an intel ISA bridge.
So your question would be should it be always at 00:1f.0.
So far it seems that it is.

> And then I don't see why you need to open code all of
> pci_bridge_init() instead of just overriding the class value after
> you called that function (or, if the order of events for some
> reason matters, adding another parameter to the function).

Stefano, could you comment? It's your code after all.

Thanks,
Timothy (Rui)

Re: [PATCH 2/3] qemu-xen-trad: Correctly expose PCH ISA bridge for IGD passthrough

[Jan Beulich]

>>> On 07.02.13 at 18:43, "G.R." <firemeteor@users.sourceforge.net> wrote:
>> 
>> For one I wonder whether this is really _always_ correct. I.e.
>> the device at 00:1f.0 always being an ISA bridge. Wouldn't it
>> be better to mirror whatever the host device says?
>>
> From the Intel driver code, it's looking for an intel ISA bridge.

That doesn't mean that it always will be.

> So your question would be should it be always at 00:1f.0.
> So far it seems that it is.

Same thing here. We ought to be careful, or else we risk to
introduce issues that pretty hard to locate, debug, and fix.

Jan

Re: [PATCH 3/3] qemu-xen-trad: IGD passthrough: Expose vendor specific pci cap on host bridge.

[Jan Beulich]

>>> On 07.02.13 at 18:38, "G.R." <firemeteor@users.sourceforge.net> wrote:
>> > +    if (config_addr == PCI_INTEL_VENDOR_CAP)
>>> +    {
>>> +        *val = vendor_cap;
>>> +        return 1;
>>> +    }
>>> +
>>> +    /* Remove the next capability link */
>>> +    if (config_addr == vendor_cap + 1)
>>> +    {
>>> +        *val = 0;
>>> +        return 1;
>>> +    }
>>
>> It doesn't look right to ignore "len" up to here? What if this is a
>> word read at "vendor_cap"?
> The function is for intel IGD only and the first two returns are only
> sanity checks.
> The third is fine also since the PCI register is only one byte in
> length and other bytes in that word are reserved.

Just as for the other patch - this is way too lax an approach.
You mustn't introduce dependencies on current or observed
behavior, as this may break with future chipsets. You need to
follow what actual hardware does (i.e. allow multi-byte aligned
reads to properly return _all_ covered registers' values).

>>> +    /* Exposing writable register does not lead to security risk since this
>>> +       only apply to read. This may confuse the guest, but it works good so 
> far.
>>> +       Will switch to mask & merge style only if this is proved broken.
>>> +       Note: Always expose aligned address if any byte of the dword is to 
> be
>>> +       exposed. This provides a consistent view, at least for read. */
>>
>> Such a comment is certainly not helping being confident in the
>> changes you're trying to get merged in.
> 
> Could you be more specific on your suggestion?
> I don't think you are recommending to remove them.

Certainly not. I'm recommending to fix the code to make the
comment unnecessary.

Jan

Re: Patch series for IGD passthrough

[Stefano Stabellini]

On Thu, 7 Feb 2013, Rui Guo wrote:
> This series contains all the fixes required to produce a working IGD
> passthrough box. All the changes are previously seen in the dev list but
> not yet accepted. Some of them are out-dated and need some reshape.
> 
> Detailed description can be found later in each patch.
> 
> . [PATCH 1/3] qemu-xen-trad/pt_msi_disable: do not clear all MSI flags
> . [PATCH 2/3] qemu-xen-trad: Correctly expose PCH ISA bridge for IGD
> . [PATCH 3/3] qemu-xen-trad: IGD passthrough: Expose vendor specific

All patches look good

Re: [PATCH 2/3] qemu-xen-trad: Correctly expose PCH ISA bridge for IGD passthrough

[Stefano Stabellini]

On Thu, 7 Feb 2013, G.R. wrote:
> >
> > For one I wonder whether this is really _always_ correct. I.e.
> > the device at 00:1f.0 always being an ISA bridge. Wouldn't it
> > be better to mirror whatever the host device says?
> >
> >From the Intel driver code, it's looking for an intel ISA bridge.
> So your question would be should it be always at 00:1f.0.
> So far it seems that it is.
> 
> > And then I don't see why you need to open code all of
> > pci_bridge_init() instead of just overriding the class value after
> > you called that function (or, if the order of events for some
> > reason matters, adding another parameter to the function).
> 
> Stefano, could you comment? It's your code after all.

It's not trivial because PCIBus is defined in hw/pci.c, so you can't
dereference members of PCIBus from hw/pt-graphics.c.

However if you come up with a good patch that makes it work, even
better.

Re: [PATCH 2/3] qemu-xen-trad: Correctly expose PCH ISA bridge for IGD passthrough

[Jan Beulich]

>>> On 08.02.13 at 12:30, Stefano Stabellini <stefano.stabellini@eu.citrix.com>
wrote:
> On Thu, 7 Feb 2013, G.R. wrote:
>> >
>> > For one I wonder whether this is really _always_ correct. I.e.
>> > the device at 00:1f.0 always being an ISA bridge. Wouldn't it
>> > be better to mirror whatever the host device says?
>> >
>> >From the Intel driver code, it's looking for an intel ISA bridge.
>> So your question would be should it be always at 00:1f.0.
>> So far it seems that it is.
>> 
>> > And then I don't see why you need to open code all of
>> > pci_bridge_init() instead of just overriding the class value after
>> > you called that function (or, if the order of events for some
>> > reason matters, adding another parameter to the function).
>> 
>> Stefano, could you comment? It's your code after all.
> 
> It's not trivial because PCIBus is defined in hw/pci.c, so you can't
> dereference members of PCIBus from hw/pt-graphics.c.

But the patch makes it so the structure itself becomes public?
What problem do you see?

Jan

Re: [PATCH 2/3] qemu-xen-trad: Correctly expose PCH ISA bridge for IGD passthrough

[Stefano Stabellini]

On Fri, 8 Feb 2013, Jan Beulich wrote:
> >>> On 08.02.13 at 12:30, Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> wrote:
> > On Thu, 7 Feb 2013, G.R. wrote:
> >> >
> >> > For one I wonder whether this is really _always_ correct. I.e.
> >> > the device at 00:1f.0 always being an ISA bridge. Wouldn't it
> >> > be better to mirror whatever the host device says?
> >> >
> >> >From the Intel driver code, it's looking for an intel ISA bridge.
> >> So your question would be should it be always at 00:1f.0.
> >> So far it seems that it is.
> >> 
> >> > And then I don't see why you need to open code all of
> >> > pci_bridge_init() instead of just overriding the class value after
> >> > you called that function (or, if the order of events for some
> >> > reason matters, adding another parameter to the function).
> >> 
> >> Stefano, could you comment? It's your code after all.
> > 
> > It's not trivial because PCIBus is defined in hw/pci.c, so you can't
> > dereference members of PCIBus from hw/pt-graphics.c.
> 
> But the patch makes it so the structure itself becomes public?
> What problem do you see?

QEMU's include chain can be devious at times but I have no objections in
making PCIBus public.

Re: [PATCH v2] qemu-xen:Correctly expose PCH ISA bridge for IGD passthrough

[Ian Jackson]

G.R. writes ("[Xen-devel] [PATCH v2] qemu-xen:Correctly expose PCH ISA bridge for IGD passthrough"):
> Fix IGD passthrough logic to properly expose PCH ISA bridge (instead
> of exposing as pci-pci bridge). The i915 driver require this to
> correctly detect the PCH version and enable version specific code
> path.

Applied, thanks.  I fixed up some wrap damage.

Ian.

Re: [PATCH v2] qemu-xen:Correctly expose PCH ISA bridge for IGD passthrough

[Jan Beulich]

>>> On 22.02.13 at 19:05, Ian Jackson <Ian.Jackson@eu.citrix.com> wrote:
> G.R. writes ("[Xen-devel] [PATCH v2] qemu-xen:Correctly expose PCH ISA bridge 
> for IGD passthrough"):
>> Fix IGD passthrough logic to properly expose PCH ISA bridge (instead
>> of exposing as pci-pci bridge). The i915 driver require this to
>> correctly detect the PCH version and enable version specific code
>> path.
> 
> Applied, thanks.  I fixed up some wrap damage.

Which means that my pointing out of shortcomings in this patch
got completely ignored - we can only hope that this won't bite us
later...

Jan

Re: [PATCH v2] qemu-xen:Correctly expose PCH ISA bridge for IGD passthrough

[Ian Jackson]

Jan Beulich writes ("Re: [Xen-devel] [PATCH v2] qemu-xen:Correctly expose PCH ISA bridge for IGD passthrough"):
> > Applied, thanks.  I fixed up some wrap damage.
> 
> Which means that my pointing out of shortcomings in this patch
> got completely ignored - we can only hope that this won't bite us
> later...

Sorry about that, perhaps I didn't spot the thread.  Do you think it
should be reverted ?

Ian.

Re: [PATCH v2] qemu-xen:Correctly expose PCH ISA bridge for IGD passthrough

[Jan Beulich]

>>> On 25.02.13 at 12:24, Ian Jackson <Ian.Jackson@eu.citrix.com> wrote:
> Jan Beulich writes ("Re: [Xen-devel] [PATCH v2] qemu-xen:Correctly expose PCH 
> ISA bridge for IGD passthrough"):
>> > Applied, thanks.  I fixed up some wrap damage.
>> 
>> Which means that my pointing out of shortcomings in this patch
>> got completely ignored - we can only hope that this won't bite us
>> later...
> 
> Sorry about that, perhaps I didn't spot the thread.  Do you think it
> should be reverted ?

I'd prefer if you did so, and wait for a cleaned up version of the
patch to be submitted. If you leave in what's there right now, I
would b afraid that we'd never get to see a fixup patch on top.

Jan

Re: [PATCH v2] qemu-xen:Correctly expose PCH ISA bridge for IGD passthrough

[Stefano Stabellini]

On Mon, 25 Feb 2013, Jan Beulich wrote:
> >>> On 25.02.13 at 12:24, Ian Jackson <Ian.Jackson@eu.citrix.com> wrote:
> > Jan Beulich writes ("Re: [Xen-devel] [PATCH v2] qemu-xen:Correctly expose PCH 
> > ISA bridge for IGD passthrough"):
> >> > Applied, thanks.  I fixed up some wrap damage.
> >> 
> >> Which means that my pointing out of shortcomings in this patch
> >> got completely ignored - we can only hope that this won't bite us
> >> later...
> > 
> > Sorry about that, perhaps I didn't spot the thread.  Do you think it
> > should be reverted ?
> 
> I'd prefer if you did so, and wait for a cleaned up version of the
> patch to be submitted. If you leave in what's there right now, I
> would b afraid that we'd never get to see a fixup patch on top.

You are talking about 5114F15002000078000BD2B2@nat28.tlf.novell.com?
G.R., can you work on this patch a bit more and address Jan's comments?
It shouldn't take long..

Re: [PATCH v2] qemu-xen:Correctly expose PCH ISA bridge for IGD passthrough

[Ian Jackson]

Jan Beulich writes ("Re: [Xen-devel] [PATCH v2] qemu-xen:Correctly
expose PCH ISA bridge for IGD passthrough"):
> [Ian Jackson:]
> > Sorry about that, perhaps I didn't spot the thread.  Do you think it
> > should be reverted ?
> 
> I'd prefer if you did so, and wait for a cleaned up version of the
> patch to be submitted. If you leave in what's there right now, I
> would b afraid that we'd never get to see a fixup patch on top.

Done.

Ian.

Re: Patch series for IGD passthrough

[Pasi Kärkkäinen]

On Fri, Feb 08, 2013 at 12:12:05AM +0800, Rui Guo wrote:
> This series contains all the fixes required to produce a working IGD
> passthrough box. All the changes are previously seen in the dev list but
> not yet accepted. Some of them are out-dated and need some reshape.
> 
> Detailed description can be found later in each patch.
> 
> . [PATCH 1/3] qemu-xen-trad/pt_msi_disable: do not clear all MSI flags
> . [PATCH 2/3] qemu-xen-trad: Correctly expose PCH ISA bridge for IGD
> . [PATCH 3/3] qemu-xen-trad: IGD passthrough: Expose vendor specific
> 

Looking at qemu-xen-unstable I think patches 2 and 3 are not yet applied.
(patch 2 was applied earlier but it got reverted).

Is it likely that we'll get these merged in for Xen 4.3 ? 

Thanks,

-- Pasi

Re: Patch series for IGD passthrough

[Pasi Kärkkäinen]

On Wed, Mar 20, 2013 at 07:17:14PM +0200, Pasi Kärkkäinen wrote:
> On Fri, Feb 08, 2013 at 12:12:05AM +0800, Rui Guo wrote:
> > This series contains all the fixes required to produce a working IGD
> > passthrough box. All the changes are previously seen in the dev list but
> > not yet accepted. Some of them are out-dated and need some reshape.
> > 
> > Detailed description can be found later in each patch.
> > 
> > . [PATCH 1/3] qemu-xen-trad/pt_msi_disable: do not clear all MSI flags
> > . [PATCH 2/3] qemu-xen-trad: Correctly expose PCH ISA bridge for IGD
> > . [PATCH 3/3] qemu-xen-trad: IGD passthrough: Expose vendor specific
> > 
> 
> Looking at qemu-xen-unstable I think patches 2 and 3 are not yet applied.
> (patch 2 was applied earlier but it got reverted).
> 
> Is it likely that we'll get these merged in for Xen 4.3 ? 
> 

Ping?


-- Pasi

Re: Patch series for IGD passthrough

[George Dunlap]

On Mon, Apr 15, 2013 at 9:48 PM, Pasi Kärkkäinen <pasik@iki.fi> wrote:
> On Wed, Mar 20, 2013 at 07:17:14PM +0200, Pasi Kärkkäinen wrote:
>> On Fri, Feb 08, 2013 at 12:12:05AM +0800, Rui Guo wrote:
>> > This series contains all the fixes required to produce a working IGD
>> > passthrough box. All the changes are previously seen in the dev list but
>> > not yet accepted. Some of them are out-dated and need some reshape.
>> >
>> > Detailed description can be found later in each patch.
>> >
>> > . [PATCH 1/3] qemu-xen-trad/pt_msi_disable: do not clear all MSI flags
>> > . [PATCH 2/3] qemu-xen-trad: Correctly expose PCH ISA bridge for IGD
>> > . [PATCH 3/3] qemu-xen-trad: IGD passthrough: Expose vendor specific
>> >
>>
>> Looking at qemu-xen-unstable I think patches 2 and 3 are not yet applied.
>> (patch 2 was applied earlier but it got reverted).
>>
>> Is it likely that we'll get these merged in for Xen 4.3 ?
>>
>
> Ping?

Whose decision is this?

I thought we were actually not going to accept new functionality into qemu-trad?

 -George

Re: Patch series for IGD passthrough

[Pasi Kärkkäinen]

On Tue, Apr 16, 2013 at 11:56:19AM +0100, George Dunlap wrote:
> On Mon, Apr 15, 2013 at 9:48 PM, Pasi Kärkkäinen <pasik@iki.fi> wrote:
> > On Wed, Mar 20, 2013 at 07:17:14PM +0200, Pasi Kärkkäinen wrote:
> >> On Fri, Feb 08, 2013 at 12:12:05AM +0800, Rui Guo wrote:
> >> > This series contains all the fixes required to produce a working IGD
> >> > passthrough box. All the changes are previously seen in the dev list but
> >> > not yet accepted. Some of them are out-dated and need some reshape.
> >> >
> >> > Detailed description can be found later in each patch.
> >> >
> >> > . [PATCH 1/3] qemu-xen-trad/pt_msi_disable: do not clear all MSI flags
> >> > . [PATCH 2/3] qemu-xen-trad: Correctly expose PCH ISA bridge for IGD
> >> > . [PATCH 3/3] qemu-xen-trad: IGD passthrough: Expose vendor specific
> >> >
> >>
> >> Looking at qemu-xen-unstable I think patches 2 and 3 are not yet applied.
> >> (patch 2 was applied earlier but it got reverted).
> >>
> >> Is it likely that we'll get these merged in for Xen 4.3 ?
> >>
> >
> > Ping?
> 
> Whose decision is this?
>

I think at least one of the patches needs fixing and re-send..
 
> I thought we were actually not going to accept new functionality into qemu-trad?
>

I don't think this is a new feature really.. it's just fixing the existing Intel VGA passthru functionality.

-- Pasi
 

Re: Patch series for IGD passthrough

[Stefano Stabellini]

[-- Attachment #1: Type: text/plain, Size: 1000 bytes --]

On Mon, 15 Apr 2013, Pasi Kärkkäinen wrote:
> On Wed, Mar 20, 2013 at 07:17:14PM +0200, Pasi Kärkkäinen wrote:
> > On Fri, Feb 08, 2013 at 12:12:05AM +0800, Rui Guo wrote:
> > > This series contains all the fixes required to produce a working IGD
> > > passthrough box. All the changes are previously seen in the dev list but
> > > not yet accepted. Some of them are out-dated and need some reshape.
> > > 
> > > Detailed description can be found later in each patch.
> > > 
> > > . [PATCH 1/3] qemu-xen-trad/pt_msi_disable: do not clear all MSI flags
> > > . [PATCH 2/3] qemu-xen-trad: Correctly expose PCH ISA bridge for IGD
> > > . [PATCH 3/3] qemu-xen-trad: IGD passthrough: Expose vendor specific
> > > 
> > 
> > Looking at qemu-xen-unstable I think patches 2 and 3 are not yet applied.
> > (patch 2 was applied earlier but it got reverted).
> > 
> > Is it likely that we'll get these merged in for Xen 4.3 ? 
> > 
> 
> Ping?

That's a call for Ian Jackson, CC'ing him.

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

Re: Patch series for IGD passthrough

[Stefano Stabellini]

[-- Attachment #1: Type: text/plain, Size: 1191 bytes --]

On Tue, 16 Apr 2013, George Dunlap wrote:
> On Mon, Apr 15, 2013 at 9:48 PM, Pasi Kärkkäinen <pasik@iki.fi> wrote:
> > On Wed, Mar 20, 2013 at 07:17:14PM +0200, Pasi Kärkkäinen wrote:
> >> On Fri, Feb 08, 2013 at 12:12:05AM +0800, Rui Guo wrote:
> >> > This series contains all the fixes required to produce a working IGD
> >> > passthrough box. All the changes are previously seen in the dev list but
> >> > not yet accepted. Some of them are out-dated and need some reshape.
> >> >
> >> > Detailed description can be found later in each patch.
> >> >
> >> > . [PATCH 1/3] qemu-xen-trad/pt_msi_disable: do not clear all MSI flags
> >> > . [PATCH 2/3] qemu-xen-trad: Correctly expose PCH ISA bridge for IGD
> >> > . [PATCH 3/3] qemu-xen-trad: IGD passthrough: Expose vendor specific
> >> >
> >>
> >> Looking at qemu-xen-unstable I think patches 2 and 3 are not yet applied.
> >> (patch 2 was applied earlier but it got reverted).
> >>
> >> Is it likely that we'll get these merged in for Xen 4.3 ?
> >>
> >
> > Ping?
> 
> Whose decision is this?

Ian Jackson

> I thought we were actually not going to accept new functionality into qemu-trad?

They are fixes

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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

Re: Patch series for IGD passthrough

[Pasi Kärkkäinen]

IanJ: ping again.. 

-- Pasi

On Thu, Apr 18, 2013 at 12:47:51PM +0100, Stefano Stabellini wrote:
> On Mon, 15 Apr 2013, Pasi Kärkkäinen wrote:
> > On Wed, Mar 20, 2013 at 07:17:14PM +0200, Pasi Kärkkäinen wrote:
> > > On Fri, Feb 08, 2013 at 12:12:05AM +0800, Rui Guo wrote:
> > > > This series contains all the fixes required to produce a working IGD
> > > > passthrough box. All the changes are previously seen in the dev list but
> > > > not yet accepted. Some of them are out-dated and need some reshape.
> > > > 
> > > > Detailed description can be found later in each patch.
> > > > 
> > > > . [PATCH 1/3] qemu-xen-trad/pt_msi_disable: do not clear all MSI flags
> > > > . [PATCH 2/3] qemu-xen-trad: Correctly expose PCH ISA bridge for IGD
> > > > . [PATCH 3/3] qemu-xen-trad: IGD passthrough: Expose vendor specific
> > > > 
> > > 
> > > Looking at qemu-xen-unstable I think patches 2 and 3 are not yet applied.
> > > (patch 2 was applied earlier but it got reverted).
> > > 
> > > Is it likely that we'll get these merged in for Xen 4.3 ? 
> > > 
> > 
> > Ping?
> 
> That's a call for Ian Jackson, CC'ing him.

Re: [PATCH v2] qemu-xen:Correctly expose PCH ISA bridge for IGD passthrough [and 2 more messages]

[Ian Jackson]

Stefano Stabellini writes ("Re: [Xen-devel] [PATCH v2] qemu-xen:Correctly expose PCH ISA bridge for IGD passthrough"):
> On Mon, 25 Feb 2013, Jan Beulich wrote:
> > >>> On 25.02.13 at 12:24, Ian Jackson <Ian.Jackson@eu.citrix.com> wrote:
> > > Jan Beulich writes ("Re: [Xen-devel] [PATCH v2] qemu-xen:Correctly expose PCH 
> > >> Which means that my pointing out of shortcomings in this patch
> > >> got completely ignored - we can only hope that this won't bite us
> > >> later...
> > > 
> > > Sorry about that, perhaps I didn't spot the thread.  Do you think it
> > > should be reverted ?
> > 
> > I'd prefer if you did so, and wait for a cleaned up version of the
> > patch to be submitted. If you leave in what's there right now, I
> > would b afraid that we'd never get to see a fixup patch on top.
> 
> You are talking about 5114F15002000078000BD2B2@nat28.tlf.novell.com?
> G.R., can you work on this patch a bit more and address Jan's comments?
> It shouldn't take long..

Stefano Stabellini writes ("Re: [Xen-devel] Patch series for IGD passthrough"):
> On Tue, 16 Apr 2013, George Dunlap wrote:
> > On Mon, Apr 15, 2013 at 9:48 PM, Pasi Kärkkäinen <pasik@iki.fi> wrote:
> > > On Wed, Mar 20, 2013 at 07:17:14PM +0200, Pasi Kärkkäinen wrote:
> > >> On Fri, Feb 08, 2013 at 12:12:05AM +0800, Rui Guo wrote:
> > >> > This series contains all the fixes required to produce a working IGD
> > >> > passthrough box. All the changes are previously seen in the dev list but
> > >> > not yet accepted. Some of them are out-dated and need some reshape.
> > >> >
> > >> > Detailed description can be found later in each patch.
> > >> >
> > >> > . [PATCH 1/3] qemu-xen-trad/pt_msi_disable: do not clear all MSI flags
> > >> > . [PATCH 2/3] qemu-xen-trad: Correctly expose PCH ISA bridge for IGD
> > >> > . [PATCH 3/3] qemu-xen-trad: IGD passthrough: Expose vendor specific
> > >> >
> > >>
> > >> Looking at qemu-xen-unstable I think patches 2 and 3 are not yet applied.
> > >> (patch 2 was applied earlier but it got reverted).
> > >>
> > >> Is it likely that we'll get these merged in for Xen 4.3 ?

Have you addressed Jan's comments from February ?  I don't see
anything saying you have done so.

Also please could you CC patches to the maintainer (ie, me, in this
case) and also to people who have commented on them in the past (Jan,
in this case).

Simply constantly pinging is not appropriate.

Ian.

Re: [PATCH v2] qemu-xen:Correctly expose PCH ISA bridge for IGD passthrough [and 2 more messages]

[Pasi Kärkkäinen]

On Tue, May 07, 2013 at 06:12:30PM +0100, Ian Jackson wrote:
> Stefano Stabellini writes ("Re: [Xen-devel] [PATCH v2] qemu-xen:Correctly expose PCH ISA bridge for IGD passthrough"):
> > On Mon, 25 Feb 2013, Jan Beulich wrote:
> > > >>> On 25.02.13 at 12:24, Ian Jackson <Ian.Jackson@eu.citrix.com> wrote:
> > > > Jan Beulich writes ("Re: [Xen-devel] [PATCH v2] qemu-xen:Correctly expose PCH 
> > > >> Which means that my pointing out of shortcomings in this patch
> > > >> got completely ignored - we can only hope that this won't bite us
> > > >> later...
> > > > 
> > > > Sorry about that, perhaps I didn't spot the thread.  Do you think it
> > > > should be reverted ?
> > > 
> > > I'd prefer if you did so, and wait for a cleaned up version of the
> > > patch to be submitted. If you leave in what's there right now, I
> > > would b afraid that we'd never get to see a fixup patch on top.
> > 
> > You are talking about 5114F15002000078000BD2B2@nat28.tlf.novell.com?
> > G.R., can you work on this patch a bit more and address Jan's comments?
> > It shouldn't take long..
> 

So this "PATCH v2" here was posted on 19 Dec 2012. It got applied, 
and then reverted due to Jan's comments/concerns.

"Patch series for IGD passthrough" has a newer version of this patch,
posted on 08 Feb 2013.


> Stefano Stabellini writes ("Re: [Xen-devel] Patch series for IGD passthrough"):
> > On Tue, 16 Apr 2013, George Dunlap wrote:
> > > On Mon, Apr 15, 2013 at 9:48 PM, Pasi Kärkkäinen <pasik@iki.fi> wrote:
> > > > On Wed, Mar 20, 2013 at 07:17:14PM +0200, Pasi Kärkkäinen wrote:
> > > >> On Fri, Feb 08, 2013 at 12:12:05AM +0800, Rui Guo wrote:
> > > >> > This series contains all the fixes required to produce a working IGD
> > > >> > passthrough box. All the changes are previously seen in the dev list but
> > > >> > not yet accepted. Some of them are out-dated and need some reshape.
> > > >> >
> > > >> > Detailed description can be found later in each patch.
> > > >> >
> > > >> > . [PATCH 1/3] qemu-xen-trad/pt_msi_disable: do not clear all MSI flags
> > > >> > . [PATCH 2/3] qemu-xen-trad: Correctly expose PCH ISA bridge for IGD
> > > >> > . [PATCH 3/3] qemu-xen-trad: IGD passthrough: Expose vendor specific
> > > >> >
> > > >>
> > > >> Looking at qemu-xen-unstable I think patches 2 and 3 are not yet applied.
> > > >> (patch 2 was applied earlier but it got reverted).
> > > >>
> > > >> Is it likely that we'll get these merged in for Xen 4.3 ?
> 
> Have you addressed Jan's comments from February ?  I don't see
> anything saying you have done so.
> 

"Patch series for IGD passthrough" has a later version of the patch in question.
I'm not sure if Jan's comments are addressed properly, there's no changelog on the patch,
but the patch seems different from the one that got applied+reverted.


> Also please could you CC patches to the maintainer (ie, me, in this
> case) and also to people who have commented on them in the past (Jan,
> in this case).
> 
> Simply constantly pinging is not appropriate.
> 

Sorry for that, but I just wanted to get some attention to these patches,
because it wasn't clear what still needs to be done to get them merged.

All three patches on the "Patch series for IGD passthrough" are ACKed by Stefano:
http://lists.xen.org/archives/html/xen-devel/2013-02/msg00593.html


patch 1/3: This one seems to have been merged already to qemu-xen-unstable.
patch 2/3: this one got some comments, but to me it looks like it didn't get a NACK? (and it has Stefano's ACK).
patch 3/3: This one might still have un-addressed comments?


Jan: Can you please re-check 2/3 and 3/3 ? 

"[PATCH 2/3] qemu-xen-trad: Correctly expose PCH ISA bridge for IGD passthrough":
http://lists.xen.org/archives/html/xen-devel/2013-02/msg00536.html

"[PATCH 3/3] qemu-xen-trad: IGD passthrough: Expose vendor specific pci cap on host bridge":
http://lists.xen.org/archives/html/xen-devel/2013-02/msg00538.html


Thanks,

-- Pasi

Re: [PATCH v2] qemu-xen:Correctly expose PCH ISA bridge for IGD passthrough [and 2 more messages]

[Jan Beulich]

>>> On 09.05.13 at 15:17, Pasi Kärkkäinen<pasik@iki.fi> wrote:
> Jan: Can you please re-check 2/3 and 3/3 ? 
> 
> "[PATCH 2/3] qemu-xen-trad: Correctly expose PCH ISA bridge for IGD 
> passthrough":
> http://lists.xen.org/archives/html/xen-devel/2013-02/msg00536.html 

I'm not sure what you're asking for - my comments there weren't
followed up with by the submitter, so I also don't have anything
new to add. I don't think I've seen a cleaned up patch so far.

> "[PATCH 3/3] qemu-xen-trad: IGD passthrough: Expose vendor specific pci cap on 
> host bridge":
> http://lists.xen.org/archives/html/xen-devel/2013-02/msg00538.html 

Quite similar here, except that Jean was also asked to comment
on why some previously existing code is the way it is, and I don't
think any explanation was ever given.

I can only second what Ian already said - pinging on a patch that
has responses missing from the originator is bad practice. If you
wanted to ping anyone, you'd have to ping those who failed to
answer questions raised to them, not give the appearance of
requesting a patch to be taken that is only half baked.

Jan

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

Re: [PATCH v2] qemu-xen:Correctly expose PCH ISA bridge for IGD passthrough [and 2 more messages]

[Pasi Kärkkäinen]

On Fri, May 10, 2013 at 10:12:31AM +0100, Jan Beulich wrote:
> >>> On 09.05.13 at 15:17, Pasi Kärkkäinen<pasik@iki.fi> wrote:
> > Jan: Can you please re-check 2/3 and 3/3 ? 
> > 
> > "[PATCH 2/3] qemu-xen-trad: Correctly expose PCH ISA bridge for IGD 
> > passthrough":
> > http://lists.xen.org/archives/html/xen-devel/2013-02/msg00536.html 
> 
> I'm not sure what you're asking for - my comments there weren't
> followed up with by the submitter, so I also don't have anything
> new to add. I don't think I've seen a cleaned up patch so far.
> 

Ok. Thanks for clarifying that (I wasn't sure if there was still unresolved issues or not).


> > "[PATCH 3/3] qemu-xen-trad: IGD passthrough: Expose vendor specific pci cap on 
> > host bridge":
> > http://lists.xen.org/archives/html/xen-devel/2013-02/msg00538.html 
> 
> Quite similar here, except that Jean was also asked to comment
> on why some previously existing code is the way it is, and I don't
> think any explanation was ever given.
>

Ok. 
 
> I can only second what Ian already said - pinging on a patch that
> has responses missing from the originator is bad practice. If you
> wanted to ping anyone, you'd have to ping those who failed to
> answer questions raised to them, not give the appearance of
> requesting a patch to be taken that is only half baked.
> 

I mostly wanted to clarify the situation with these patches,
I'm not suggesting to merge half baked patches. 

So thanks for the comments!

-- Pasi

Re: [PATCH v2] qemu-xen:Correctly expose PCH ISA bridge for IGD passthrough [and 2 more messages]

[Ian Campbell]

On Fri, 2013-05-10 at 10:40 +0100, Pasi Kärkkäinen wrote:
> > I can only second what Ian already said - pinging on a patch that
> > has responses missing from the originator is bad practice. If you
> > wanted to ping anyone, you'd have to ping those who failed to
> > answer questions raised to them, not give the appearance of
> > requesting a patch to be taken that is only half baked.
> > 
> 
> I mostly wanted to clarify the situation with these patches,

Instead of adding more work to our already overburdened maintainers
please start in these cases by working with the submitter.

It is the submitters responsibility to respond to queries and to keep
pushing the patch as necessary. If the submitter isn't engaged any more
then you need to either pick it up yourself or find someone else who is
willing to carry the patch forward, there's no point asking the
maintainers to clarify the situation if no one is around who is going to
act on their responses.

Ian.


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

Re: [PATCH v2] qemu-xen:Correctly expose PCH ISA bridge for IGD passthrough [and 2 more messages]

[George Dunlap]

On 10/05/13 11:00, Ian Campbell wrote:
> On Fri, 2013-05-10 at 10:40 +0100, Pasi Kärkkäinen wrote:
>>> I can only second what Ian already said - pinging on a patch that
>>> has responses missing from the originator is bad practice. If you
>>> wanted to ping anyone, you'd have to ping those who failed to
>>> answer questions raised to them, not give the appearance of
>>> requesting a patch to be taken that is only half baked.
>>>
>> I mostly wanted to clarify the situation with these patches,
> Instead of adding more work to our already overburdened maintainers
> please start in these cases by working with the submitter.
>
> It is the submitters responsibility to respond to queries and to keep
> pushing the patch as necessary. If the submitter isn't engaged any more
> then you need to either pick it up yourself or find someone else who is
> willing to carry the patch forward, there's no point asking the
> maintainers to clarify the situation if no one is around who is going to
> act on their responses.

But if he were going to pick it up, or if he wants to go work with the 
submitter, who may not have a clear understanding what the delay was 
about or how the development process works, then he'd first need to 
figure out what the status was, right?

This sort of "facilitating" role seems like a useful thing to do to 
allow contributions which would otherwise flounder to make it above the 
bar into acceptance.

  -George

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

Re: [PATCH v2] qemu-xen:Correctly expose PCH ISA bridge for IGD passthrough [and 2 more messages]

[Ian Campbell]

On Fri, 2013-05-10 at 11:09 +0100, George Dunlap wrote:
> On 10/05/13 11:00, Ian Campbell wrote:
> > On Fri, 2013-05-10 at 10:40 +0100, Pasi Kärkkäinen wrote:
> >>> I can only second what Ian already said - pinging on a patch that
> >>> has responses missing from the originator is bad practice. If you
> >>> wanted to ping anyone, you'd have to ping those who failed to
> >>> answer questions raised to them, not give the appearance of
> >>> requesting a patch to be taken that is only half baked.
> >>>
> >> I mostly wanted to clarify the situation with these patches,
> > Instead of adding more work to our already overburdened maintainers
> > please start in these cases by working with the submitter.
> >
> > It is the submitters responsibility to respond to queries and to keep
> > pushing the patch as necessary. If the submitter isn't engaged any more
> > then you need to either pick it up yourself or find someone else who is
> > willing to carry the patch forward, there's no point asking the
> > maintainers to clarify the situation if no one is around who is going to
> > act on their responses.
> 
> But if he were going to pick it up, or if he wants to go work with the 
> submitter, who may not have a clear understanding what the delay was 
> about or how the development process works, then he'd first need to 
> figure out what the status was, right?
>
> This sort of "facilitating" role seems like a useful thing to do to 
> allow contributions which would otherwise flounder to make it above the 
> bar into acceptance.

A facilitator should first ask the submitter what they think is going
on, and try and offer them guidance, which could be fairly generic e.g.
remind them that the need to resubmit if a patch is dropped, or it could
be more specific, i.e. reminding them of the need to respond to
particular queries made by the maintainers last time.

Yes, this means they would have needed to have read the background and
make an attempt to understand the current status first. They may not be
able to figure it out (or might get it wrong) but at least the obvious
cases can then be covered.

Facilitating can surely be useful and appreciated. But if "facilitate"
just means pinging the maintainers without having spoken to the
submitter, read up on the background and followed up on outstanding
queries then it is just making more work for maintainers for no gain and
isn't really facilitating anything.

Remember, Submitters (and facilitators) scale while maintainers do not.

Ian.



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

Re: [PATCH v2] qemu-xen:Correctly expose PCH ISA bridge for IGD passthrough [and 2 more messages]

[Jan Beulich]

>>> On 10.05.13 at 12:09, George Dunlap <george.dunlap@eu.citrix.com> wrote:
> But if he were going to pick it up, or if he wants to go work with the 
> submitter, who may not have a clear understanding what the delay was 
> about or how the development process works, then he'd first need to 
> figure out what the status was, right?

But that's what is available in the list archives - in the case at hand,
Pasi pointed at patches that had trees of subsequent communication
attached to them, and in all cases the leaves where unanswered
questions. So figuring out the status was quite strait forward here.

I admit that this may not always be as simple (or otherwise I likely
wouldn't even have bothered looking at the old conversation again).

Bottom line is that I think we should be allowed to expect homework
to be done before pinging maintainers.

Jan

Re: [PATCH v2] qemu-xen:Correctly expose PCH ISA bridge for IGD passthrough [and 2 more messages]

[Pasi Kärkkäinen]

On Fri, May 10, 2013 at 11:33:09AM +0100, Ian Campbell wrote:
> > >> I mostly wanted to clarify the situation with these patches,
> > > Instead of adding more work to our already overburdened maintainers
> > > please start in these cases by working with the submitter.
> > >
> > > It is the submitters responsibility to respond to queries and to keep
> > > pushing the patch as necessary. If the submitter isn't engaged any more
> > > then you need to either pick it up yourself or find someone else who is
> > > willing to carry the patch forward, there's no point asking the
> > > maintainers to clarify the situation if no one is around who is going to
> > > act on their responses.
> > 
> > But if he were going to pick it up, or if he wants to go work with the 
> > submitter, who may not have a clear understanding what the delay was 
> > about or how the development process works, then he'd first need to 
> > figure out what the status was, right?
> >
> > This sort of "facilitating" role seems like a useful thing to do to 
> > allow contributions which would otherwise flounder to make it above the 
> > bar into acceptance.
> 
> A facilitator should first ask the submitter what they think is going
> on, and try and offer them guidance, which could be fairly generic e.g.
> remind them that the need to resubmit if a patch is dropped, or it could
> be more specific, i.e. reminding them of the need to respond to
> particular queries made by the maintainers last time.
> 

Yep, I should have talked to G.R. first. Sorry about that.

> Yes, this means they would have needed to have read the background and
> make an attempt to understand the current status first. They may not be
> able to figure it out (or might get it wrong) but at least the obvious
> cases can then be covered.
> 

I did read all the emails in various threads related to these patches,
and I tried understanding where we are.

It was a bit confusing because for example one of the patches got applied,
then reverted, re-submitted after that, and I didn't 100% understand from
the comments if there are still unresolved issues or not. 

Now it's clear that not all of the issues are sorted out yet.

(Btw right now we're discussing in a thread that doesn't have the latest version
of the patches..)

> Facilitating can surely be useful and appreciated. But if "facilitate"
> just means pinging the maintainers without having spoken to the
> submitter, read up on the background and followed up on outstanding
> queries then it is just making more work for maintainers for no gain and
> isn't really facilitating anything.
> 
> Remember, Submitters (and facilitators) scale while maintainers do not.
> 

I do understand that. Thanks for your patience :)

-- Pasi

Re: [PATCH 2/3] qemu-xen-trad: Correctly expose PCH ISA bridge for IGD passthrough

[G.R.]

It has been a long time, but Pasi reminded me to follow this up.
Here is my feedback to your concern:

On Fri, Feb 8, 2013 at 3:51 PM, Jan Beulich <JBeulich@suse.com> wrote:
>>>> On 07.02.13 at 18:43, "G.R." <firemeteor@users.sourceforge.net> wrote:
>>>
>>> For one I wonder whether this is really _always_ correct. I.e.
>>> the device at 00:1f.0 always being an ISA bridge. Wouldn't it
>>> be better to mirror whatever the host device says?
>>>
>> From the Intel driver code, it's looking for an intel ISA bridge.
>
> That doesn't mean that it always will be.
Unless you can 100% simulate the HW, you have to rely on the known
protocol between driver && HW.
I agree that they may switch to different protocol some day, but I
don't think we have any better choice.

>
>> So your question would be should it be always at 00:1f.0.
>> So far it seems that it is.
>
> Same thing here. We ought to be careful, or else we risk to
> introduce issues that pretty hard to locate, debug, and fix.
Since most (if not all) recent intel chipsets in the market have ISA
bridge at address 00:1f.0, simulate one at the same address to the
guest won't be so bad, given the current protocol between driver &&
HW.
But I guess your concern is about the hard-coded '00:1f.0' address.
Yes, I agree that this is not beautiful at all.
I don't mind changing it to probe the ISA bridge from host. But I'm
not familiar with qemu at all, could you show me the API to achieve
this purpose?

Also, if we really care about doing the 'correct' thing, I think we
should get rid of the default ISA bridge provided by qemu -- currently
it requires extra patches to linux i915 driver to work around. Anyway
to achieve this purpose?

Thanks,
Timothy (Rui)

Re: [PATCH 2/3] qemu-xen-trad: Correctly expose PCH ISA bridge for IGD passthrough

[G.R.]

On Fri, Feb 8, 2013 at 7:48 PM, Stefano Stabellini
<stefano.stabellini@eu.citrix.com> wrote:
> On Fri, 8 Feb 2013, Jan Beulich wrote:
>> >>> On 08.02.13 at 12:30, Stefano Stabellini <stefano.stabellini@eu.citrix.com>
>> wrote:
>> > On Thu, 7 Feb 2013, G.R. wrote:
>> >> >
>> >> > For one I wonder whether this is really _always_ correct. I.e.
>> >> > the device at 00:1f.0 always being an ISA bridge. Wouldn't it
>> >> > be better to mirror whatever the host device says?
>> >> >
>> >> >From the Intel driver code, it's looking for an intel ISA bridge.
>> >> So your question would be should it be always at 00:1f.0.
>> >> So far it seems that it is.
>> >>
>> >> > And then I don't see why you need to open code all of
>> >> > pci_bridge_init() instead of just overriding the class value after
>> >> > you called that function (or, if the order of events for some
>> >> > reason matters, adding another parameter to the function).
>> >>
>> >> Stefano, could you comment? It's your code after all.
>> >
>> > It's not trivial because PCIBus is defined in hw/pci.c, so you can't
>> > dereference members of PCIBus from hw/pt-graphics.c.
>>
>> But the patch makes it so the structure itself becomes public?
>> What problem do you see?
>
> QEMU's include chain can be devious at times but I have no objections in
> making PCIBus public.

Are you talking about making PCIBus public (to hw/pci.h) and change
the code to something like this:

    if (vid == PCI_VENDOR_ID_INTEL) {
        PCIBus *s = pci_bridge_init(bus, PCI_DEVFN(0x1f, 0), vid, did, rid,
                                       pch_map_irq, "intel_bridge_1f");

        pci_config_set_class(s->parent_dev.config, PCI_CLASS_BRIDGE_ISA);
        s->parent_dev.config[PCI_HEADER_TYPE] = 0x80;
    }

Re: [PATCH 2/3] qemu-xen-trad: Correctly expose PCH ISA bridge for IGD passthrough

[Jan Beulich]

>>> On 21.05.13 at 17:52, "G.R." <firemeteor@users.sourceforge.net> wrote:
> On Fri, Feb 8, 2013 at 7:48 PM, Stefano Stabellini
> <stefano.stabellini@eu.citrix.com> wrote:
>> On Fri, 8 Feb 2013, Jan Beulich wrote:
>>> >>> On 08.02.13 at 12:30, Stefano Stabellini <stefano.stabellini@eu.citrix.com>
>>> wrote:
>>> > On Thu, 7 Feb 2013, G.R. wrote:
>>> >> >
>>> >> > For one I wonder whether this is really _always_ correct. I.e.
>>> >> > the device at 00:1f.0 always being an ISA bridge. Wouldn't it
>>> >> > be better to mirror whatever the host device says?
>>> >> >
>>> >> >From the Intel driver code, it's looking for an intel ISA bridge.
>>> >> So your question would be should it be always at 00:1f.0.
>>> >> So far it seems that it is.
>>> >>
>>> >> > And then I don't see why you need to open code all of
>>> >> > pci_bridge_init() instead of just overriding the class value after
>>> >> > you called that function (or, if the order of events for some
>>> >> > reason matters, adding another parameter to the function).
>>> >>
>>> >> Stefano, could you comment? It's your code after all.
>>> >
>>> > It's not trivial because PCIBus is defined in hw/pci.c, so you can't
>>> > dereference members of PCIBus from hw/pt-graphics.c.
>>>
>>> But the patch makes it so the structure itself becomes public?
>>> What problem do you see?
>>
>> QEMU's include chain can be devious at times but I have no objections in
>> making PCIBus public.
> 
> Are you talking about making PCIBus public (to hw/pci.h) and change
> the code to something like this:
> 
>     if (vid == PCI_VENDOR_ID_INTEL) {
>         PCIBus *s = pci_bridge_init(bus, PCI_DEVFN(0x1f, 0), vid, did, rid,
>                                        pch_map_irq, "intel_bridge_1f");
> 
>         pci_config_set_class(s->parent_dev.config, PCI_CLASS_BRIDGE_ISA);
>         s->parent_dev.config[PCI_HEADER_TYPE] = 0x80;
>     }

Along those lines, yes.

Jan

Re: [PATCH 2/3] qemu-xen-trad: Correctly expose PCH ISA bridge for IGD passthrough

[G.R.]

On Tue, May 21, 2013 at 11:39 PM, G.R. <firemeteor@users.sourceforge.net> wrote:
> It has been a long time, but Pasi reminded me to follow this up.
> Here is my feedback to your concern:
>
> On Fri, Feb 8, 2013 at 3:51 PM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>> On 07.02.13 at 18:43, "G.R." <firemeteor@users.sourceforge.net> wrote:
>>>>
>>>> For one I wonder whether this is really _always_ correct. I.e.
>>>> the device at 00:1f.0 always being an ISA bridge. Wouldn't it
>>>> be better to mirror whatever the host device says?
>>>>
>>> From the Intel driver code, it's looking for an intel ISA bridge.
>>
>> That doesn't mean that it always will be.
> Unless you can 100% simulate the HW, you have to rely on the known
> protocol between driver && HW.
> I agree that they may switch to different protocol some day, but I
> don't think we have any better choice.
>
>>
>>> So your question would be should it be always at 00:1f.0.
>>> So far it seems that it is.
>>
>> Same thing here. We ought to be careful, or else we risk to
>> introduce issues that pretty hard to locate, debug, and fix.
> Since most (if not all) recent intel chipsets in the market have ISA
> bridge at address 00:1f.0, simulate one at the same address to the
> guest won't be so bad, given the current protocol between driver &&
> HW.
> But I guess your concern is about the hard-coded '00:1f.0' address.
> Yes, I agree that this is not beautiful at all.
> I don't mind changing it to probe the ISA bridge from host. But I'm
> not familiar with qemu at all, could you show me the API to achieve
> this purpose?
Hi,

if I got no response on the API that I queried for, I would just send
out a patch version that does not touch this part.

> Also, if we really care about doing the 'correct' thing, I think we
> should get rid of the default ISA bridge provided by qemu -- currently
> it requires extra patches to linux i915 driver to work around. Anyway
> to achieve this purpose?

I found one API pci_hide_device() may be helpful for this purpose.
I would like to try out, but still lack of the API to locate specified
device in the bus.
>
> Thanks,
> Timothy (Rui)

Re: [PATCH 3/3] qemu-xen-trad: IGD passthrough: Expose vendor specific pci cap on host bridge.

[G.R.]

I'm going to rework this patch to address Jan's concern.
Here is my proposal, please review and comment before I begin:

The proposal is to read a shadow copy of the exposed host register into
the config space of the emulated host bridge and relies on the
pci_default_read_config() function
to provide proper access.

This methodology only works for constant values, which is our case here.
The exposed value is either read-only or write-locked (for BIOS).

The only exception is that the PAVPC (0x58) register is write-locked
but not for BIOS.
This is exposed for RW and my proposal is to perform write-through in
the register write-support.

>
> Also, why would removing the next capability be correct here,
> when you're not removing _all_ other capabilities?
I have no answer about this question. *Jean*, could you help comment
since this is from your code?

Thanks,
Timothy

Re: [PATCH 3/3] qemu-xen-trad: IGD passthrough: Expose vendor specific pci cap on host bridge.

[Konrad Rzeszutek Wilk]

On Wed, Jun 19, 2013 at 06:37:06PM +0800, G.R. wrote:
> I'm going to rework this patch to address Jan's concern.
> Here is my proposal, please review and comment before I begin:
> 
> The proposal is to read a shadow copy of the exposed host register into
> the config space of the emulated host bridge and relies on the
> pci_default_read_config() function
> to provide proper access.
> 
> This methodology only works for constant values, which is our case here.
> The exposed value is either read-only or write-locked (for BIOS).
> 
> The only exception is that the PAVPC (0x58) register is write-locked
> but not for BIOS.

So only SeaBIOS or hvmloader should touch it?

> This is exposed for RW and my proposal is to perform write-through in
> the register write-support.

What does PAVPC do? As in if the driver wrote 0xdeadbeef in there what
would happen? Is there a list of the appropiate values it should
accept?

> 
> >
> > Also, why would removing the next capability be correct here,
> > when you're not removing _all_ other capabilities?
> I have no answer about this question. *Jean*, could you help comment
> since this is from your code?


If he doesn't answer - if you don't remove the capability does it
still work?
> 
> Thanks,
> Timothy
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
> 

Re: [PATCH 3/3] qemu-xen-trad: IGD passthrough: Expose vendor specific pci cap on host bridge.

[Ross Philipson]

On 06/21/2013 02:03 PM, Konrad Rzeszutek Wilk wrote:
> On Wed, Jun 19, 2013 at 06:37:06PM +0800, G.R. wrote:
>> I'm going to rework this patch to address Jan's concern.
>> Here is my proposal, please review and comment before I begin:
>>
>> The proposal is to read a shadow copy of the exposed host register into
>> the config space of the emulated host bridge and relies on the
>> pci_default_read_config() function
>> to provide proper access.
>>
>> This methodology only works for constant values, which is our case here.
>> The exposed value is either read-only or write-locked (for BIOS).
>>
>> The only exception is that the PAVPC (0x58) register is write-locked
>> but not for BIOS.
>
> So only SeaBIOS or hvmloader should touch it?
>
>> This is exposed for RW and my proposal is to perform write-through in
>> the register write-support.
>
> What does PAVPC do? As in if the driver wrote 0xdeadbeef in there what
> would happen? Is there a list of the appropiate values it should
> accept?
>
>>
>>>
>>> Also, why would removing the next capability be correct here,
>>> when you're not removing _all_ other capabilities?
>> I have no answer about this question. *Jean*, could you help comment
>> since this is from your code?
>
>
> If he doesn't answer - if you don't remove the capability does it
> still work?

So I actually originally found this issue with the vendor capabilities 
and created the original patch for it. This was quite some time ago so I 
had to go back and look. IIRC the vendor specific capabilities were 
always the first one in the chain and the unchaining code would drop all 
further capabilities (which we did not want to pass directly to the guest).

We never saw a configuration where the vendor capabilities were not the 
first. I guess the suggestion is that to make the patch consistent, 
preceding capabilities should be detected and handled. I am not sure 
what the best way to do it would be. Perhaps scanning through the chain 
until 0x09 is found and reporting its offset through 0x34 instead of 
what is there would be the way to go. Then unchain anything past the 
0x09 caps too as is currently done.

>>
>> Thanks,
>> Timothy
>>
>> _______________________________________________
>> Xen-devel mailing list
>> Xen-devel@lists.xen.org
>> http://lists.xen.org/xen-devel
>>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Re: [PATCH 3/3] qemu-xen-trad: IGD passthrough: Expose vendor specific pci cap on host bridge.

[G.R.]

On Sat, Jun 22, 2013 at 2:03 AM, Konrad Rzeszutek Wilk
<konrad.wilk@oracle.com> wrote:
> On Wed, Jun 19, 2013 at 06:37:06PM +0800, G.R. wrote:
>> I'm going to rework this patch to address Jan's concern.
>> Here is my proposal, please review and comment before I begin:
>>
>> The proposal is to read a shadow copy of the exposed host register into
>> the config space of the emulated host bridge and relies on the
>> pci_default_read_config() function
>> to provide proper access.
>>
>> This methodology only works for constant values, which is our case here.
>> The exposed value is either read-only or write-locked (for BIOS).
>>
>> The only exception is that the PAVPC (0x58) register is write-locked
>> but not for BIOS.
>
> So only SeaBIOS or hvmloader should touch it?
>

No, here I mean the host BIOS.
Those write-locked registers should have been locked by host BIOS and
be read-only after boot.
Most of these are for graphics memory. I'm not sure how the value
specified by host BIOS works in the VM, but it just works.

You can find a list of these registers in this document (section 2.5, page 47):
http://www.intel.com/content/dam/www/public/us/en/documents/datasheets/3rd-gen-core-desktop-vol-2-datasheet.pdf

Just for your reference, here are the list of registers that exposed
RO (with the exception of 0x58 as RW):

198         case 0x00:        /* vendor id */
199         case 0x02:        /* device id */
201         case 0x06:        /* status, needed for the cap list bit*/
202         case 0x08:        /* revision id */
203         case 0x2c:        /* sybsystem vendor id */
204         case 0x2e:        /* sybsystem id */
205         case 0x50:        /* SNB: processor graphics control register */
206         case 0x52:        /* processor graphics control register
*/ <= actually RSVD from datasheet
207         case 0xa0:        /* top of memory */
208         case 0xb0:        /* ILK: BSM: should read from dev 2 offset 0x5c */
209         case 0x58:        /* SNB: PAVPC Offset */
210         case 0xa4:        /* SNB: graphics base of stolen memory */
211         case 0xa8:        /* SNB: base of GTT stolen memory */


>> This is exposed for RW and my proposal is to perform write-through in
>> the register write-support.
>
> What does PAVPC do? As in if the driver wrote 0xdeadbeef in there what
> would happen? Is there a list of the appropiate values it should
> accept?
>
I have no idea about this.
I can't get meaningful data from the datasheet.
And the value returned from lspci can't decode well according to the datasheet.
The datasheet above shows only one write-lock bit at bit 2, while all
others are RO and reset to zero.
But here is the lspci value from my system (The four bytes starting from 0x58):
50: 41 02 00 00 11 00 00 00 07 00 90 df 01 00 00 cf

Anyway, I don't think we should dig into the detailed register spec.
The good news is that none of the registers in device 0:00.0 have side
effect for read.
We should be able to perform write to host and read-back to shadow for
RW support.

>>
>> >
>> > Also, why would removing the next capability be correct here,
>> > when you're not removing _all_ other capabilities?
>> I have no answer about this question. *Jean*, could you help comment
>> since this is from your code?
>
>
> If he doesn't answer - if you don't remove the capability does it
> still work?

Should work at least for IVB -- since this is the only cap.
Not sure if there will be other concerns.

>>
>> Thanks,
>> Timothy
>>
>> _______________________________________________
>> Xen-devel mailing list
>> Xen-devel@lists.xen.org
>> http://lists.xen.org/xen-devel
>>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Re: [PATCH 3/3] qemu-xen-trad: IGD passthrough: Expose vendor specific pci cap on host bridge.

[Konrad Rzeszutek Wilk]

On Tue, Jun 25, 2013 at 10:08:14AM -0400, Ross Philipson wrote:
> On 06/21/2013 02:03 PM, Konrad Rzeszutek Wilk wrote:
> >On Wed, Jun 19, 2013 at 06:37:06PM +0800, G.R. wrote:
> >>I'm going to rework this patch to address Jan's concern.
> >>Here is my proposal, please review and comment before I begin:
> >>
> >>The proposal is to read a shadow copy of the exposed host register into
> >>the config space of the emulated host bridge and relies on the
> >>pci_default_read_config() function
> >>to provide proper access.
> >>
> >>This methodology only works for constant values, which is our case here.
> >>The exposed value is either read-only or write-locked (for BIOS).
> >>
> >>The only exception is that the PAVPC (0x58) register is write-locked
> >>but not for BIOS.
> >
> >So only SeaBIOS or hvmloader should touch it?
> >
> >>This is exposed for RW and my proposal is to perform write-through in
> >>the register write-support.
> >
> >What does PAVPC do? As in if the driver wrote 0xdeadbeef in there what
> >would happen? Is there a list of the appropiate values it should
> >accept?
> >
> >>
> >>>
> >>>Also, why would removing the next capability be correct here,
> >>>when you're not removing _all_ other capabilities?
> >>I have no answer about this question. *Jean*, could you help comment
> >>since this is from your code?
> >
> >
> >If he doesn't answer - if you don't remove the capability does it
> >still work?
> 
> So I actually originally found this issue with the vendor
> capabilities and created the original patch for it. This was quite
> some time ago so I had to go back and look. IIRC the vendor specific
> capabilities were always the first one in the chain and the
> unchaining code would drop all further capabilities (which we did
> not want to pass directly to the guest).

OK, so blacklisting.
> 
> We never saw a configuration where the vendor capabilities were not
> the first. I guess the suggestion is that to make the patch
> consistent, preceding capabilities should be detected and handled. I
> am not sure what the best way to do it would be. Perhaps scanning
> through the chain until 0x09 is found and reporting its offset
> through 0x34 instead of what is there would be the way to go. Then
> unchain anything past the 0x09 caps too as is currently done.

Or just scan through the capabilities, and chain only the ones
that we want to "Whitelist" and the rest are to be blacklisted.
The rest can also have its values set to some bogus value (0xdeadbeef?)
Perhaps only when built with 'debug=y'.

Anyhow, if we retain the location (aka offset) of the capabilities
then if there is some silly code that expects those to be at
specific locations it should still work?

Re: [PATCH 3/3] qemu-xen-trad: IGD passthrough: Expose vendor specific pci cap on host bridge.

[Ross Philipson]

On 06/25/2013 10:54 AM, Konrad Rzeszutek Wilk wrote:
> On Tue, Jun 25, 2013 at 10:08:14AM -0400, Ross Philipson wrote:
>> On 06/21/2013 02:03 PM, Konrad Rzeszutek Wilk wrote:
>>> On Wed, Jun 19, 2013 at 06:37:06PM +0800, G.R. wrote:
>>>> I'm going to rework this patch to address Jan's concern.
>>>> Here is my proposal, please review and comment before I begin:
>>>>
>>>> The proposal is to read a shadow copy of the exposed host register into
>>>> the config space of the emulated host bridge and relies on the
>>>> pci_default_read_config() function
>>>> to provide proper access.
>>>>
>>>> This methodology only works for constant values, which is our case here.
>>>> The exposed value is either read-only or write-locked (for BIOS).
>>>>
>>>> The only exception is that the PAVPC (0x58) register is write-locked
>>>> but not for BIOS.
>>>
>>> So only SeaBIOS or hvmloader should touch it?
>>>
>>>> This is exposed for RW and my proposal is to perform write-through in
>>>> the register write-support.
>>>
>>> What does PAVPC do? As in if the driver wrote 0xdeadbeef in there what
>>> would happen? Is there a list of the appropiate values it should
>>> accept?
>>>
>>>>
>>>>>
>>>>> Also, why would removing the next capability be correct here,
>>>>> when you're not removing _all_ other capabilities?
>>>> I have no answer about this question. *Jean*, could you help comment
>>>> since this is from your code?
>>>
>>>
>>> If he doesn't answer - if you don't remove the capability does it
>>> still work?
>>
>> So I actually originally found this issue with the vendor
>> capabilities and created the original patch for it. This was quite
>> some time ago so I had to go back and look. IIRC the vendor specific
>> capabilities were always the first one in the chain and the
>> unchaining code would drop all further capabilities (which we did
>> not want to pass directly to the guest).
>
> OK, so blacklisting.
>>
>> We never saw a configuration where the vendor capabilities were not
>> the first. I guess the suggestion is that to make the patch
>> consistent, preceding capabilities should be detected and handled. I
>> am not sure what the best way to do it would be. Perhaps scanning
>> through the chain until 0x09 is found and reporting its offset
>> through 0x34 instead of what is there would be the way to go. Then
>> unchain anything past the 0x09 caps too as is currently done.
>
> Or just scan through the capabilities, and chain only the ones
> that we want to "Whitelist" and the rest are to be blacklisted.
> The rest can also have its values set to some bogus value (0xdeadbeef?)
> Perhaps only when built with 'debug=y'.

That sounds about right. Back when I first did the patch (in an old 
qemu) there were no other capabilities on the piix4 host bridge so it 
was simple. Not sure if other capabilities will be an issue now.

>
> Anyhow, if we retain the location (aka offset) of the capabilities
> then if there is some silly code that expects those to be at
> specific locations it should still work?

Yes, that could be a definite concern. When I was dredging up memories 
of finding the issue, I had a notion that this might actually be the 
case. That is why I suggest leaving them at their offset.

Re: [PATCH 3/3] qemu-xen-trad: IGD passthrough: Expose vendor specific pci cap on host bridge.

[G.R.]

On Tue, Jun 25, 2013 at 11:01 PM, Ross Philipson
<ross.philipson@citrix.com> wrote:
> On 06/25/2013 10:54 AM, Konrad Rzeszutek Wilk wrote:
>>
>> On Tue, Jun 25, 2013 at 10:08:14AM -0400, Ross Philipson wrote:
>>>
>>> On 06/21/2013 02:03 PM, Konrad Rzeszutek Wilk wrote:
>>>>
>>>> On Wed, Jun 19, 2013 at 06:37:06PM +0800, G.R. wrote:
>>>>>
>>>>> I'm going to rework this patch to address Jan's concern.
>>>>> Here is my proposal, please review and comment before I begin:
>>>>>
>>>>> The proposal is to read a shadow copy of the exposed host register into
>>>>> the config space of the emulated host bridge and relies on the
>>>>> pci_default_read_config() function
>>>>> to provide proper access.
>>>>>
>>>>> This methodology only works for constant values, which is our case
>>>>> here.
>>>>> The exposed value is either read-only or write-locked (for BIOS).
>>>>>
>>>>> The only exception is that the PAVPC (0x58) register is write-locked
>>>>> but not for BIOS.
>>>>
>>>>
>>>> So only SeaBIOS or hvmloader should touch it?
>>>>
>>>>> This is exposed for RW and my proposal is to perform write-through in
>>>>> the register write-support.
>>>>
>>>>
>>>> What does PAVPC do? As in if the driver wrote 0xdeadbeef in there what
>>>> would happen? Is there a list of the appropiate values it should
>>>> accept?
>>>>
>>>>>
>>>>>>
>>>>>> Also, why would removing the next capability be correct here,
>>>>>> when you're not removing _all_ other capabilities?
>>>>>
>>>>> I have no answer about this question. *Jean*, could you help comment
>>>>> since this is from your code?
>>>>
>>>>
>>>>
>>>> If he doesn't answer - if you don't remove the capability does it
>>>> still work?
>>>
>>>
>>> So I actually originally found this issue with the vendor
>>> capabilities and created the original patch for it. This was quite
>>> some time ago so I had to go back and look. IIRC the vendor specific
>>> capabilities were always the first one in the chain and the
>>> unchaining code would drop all further capabilities (which we did
>>> not want to pass directly to the guest).
>>
>>
>> OK, so blacklisting.
>>>
>>>
>>> We never saw a configuration where the vendor capabilities were not
>>> the first. I guess the suggestion is that to make the patch
>>> consistent, preceding capabilities should be detected and handled. I
>>> am not sure what the best way to do it would be. Perhaps scanning
>>> through the chain until 0x09 is found and reporting its offset
>>> through 0x34 instead of what is there would be the way to go. Then
>>> unchain anything past the 0x09 caps too as is currently done.
>>
>>
>> Or just scan through the capabilities, and chain only the ones
>> that we want to "Whitelist" and the rest are to be blacklisted.
>> The rest can also have its values set to some bogus value (0xdeadbeef?)
>> Perhaps only when built with 'debug=y'.
>
>
> That sounds about right. Back when I first did the patch (in an old qemu)
> there were no other capabilities on the piix4 host bridge so it was simple.
> Not sure if other capabilities will be an issue now.

It's still the case as for the IVB host bridge.
And from what I can find from the datasheet for the Haswell, it's
still the case.

Note that the datasheet explicitly documents the offset of the
CAPABILITY registers.
I guess there will be code that rely on this offset that been publicly
documented.

Btw. Ross, now that you appears to be the original author (sorry for
mess you up with Jean),
could you also comment on my rework proposal? Jan believe the current
form is not clean enough.

Currently we use a whitelist of registers to pass-through.How do you
come up with the current list?
The shadow copy way appears to work for the current list.
But what if we are going to need some special registers that cannot be
handled well? (e.g. has side effect for reading and cannot perform
read-back?)

Thanks,
Timothy

>
>
>>
>> Anyhow, if we retain the location (aka offset) of the capabilities
>> then if there is some silly code that expects those to be at
>> specific locations it should still work?
>
>
> Yes, that could be a definite concern. When I was dredging up memories of
> finding the issue, I had a notion that this might actually be the case. That
> is why I suggest leaving them at their offset.

Re: [PATCH 3/3] qemu-xen-trad: IGD passthrough: Expose vendor specific pci cap on host bridge.

[Konrad Rzeszutek Wilk]

On Wed, Jun 26, 2013 at 12:18:02PM +0800, G.R. wrote:
> On Tue, Jun 25, 2013 at 11:01 PM, Ross Philipson
> <ross.philipson@citrix.com> wrote:
> > On 06/25/2013 10:54 AM, Konrad Rzeszutek Wilk wrote:
> >>
> >> On Tue, Jun 25, 2013 at 10:08:14AM -0400, Ross Philipson wrote:
> >>>
> >>> On 06/21/2013 02:03 PM, Konrad Rzeszutek Wilk wrote:
> >>>>
> >>>> On Wed, Jun 19, 2013 at 06:37:06PM +0800, G.R. wrote:
> >>>>>
> >>>>> I'm going to rework this patch to address Jan's concern.
> >>>>> Here is my proposal, please review and comment before I begin:
> >>>>>
> >>>>> The proposal is to read a shadow copy of the exposed host register into
> >>>>> the config space of the emulated host bridge and relies on the
> >>>>> pci_default_read_config() function
> >>>>> to provide proper access.
> >>>>>
> >>>>> This methodology only works for constant values, which is our case
> >>>>> here.
> >>>>> The exposed value is either read-only or write-locked (for BIOS).
> >>>>>
> >>>>> The only exception is that the PAVPC (0x58) register is write-locked
> >>>>> but not for BIOS.
> >>>>
> >>>>
> >>>> So only SeaBIOS or hvmloader should touch it?
> >>>>
> >>>>> This is exposed for RW and my proposal is to perform write-through in
> >>>>> the register write-support.
> >>>>
> >>>>
> >>>> What does PAVPC do? As in if the driver wrote 0xdeadbeef in there what
> >>>> would happen? Is there a list of the appropiate values it should
> >>>> accept?
> >>>>
> >>>>>
> >>>>>>
> >>>>>> Also, why would removing the next capability be correct here,
> >>>>>> when you're not removing _all_ other capabilities?
> >>>>>
> >>>>> I have no answer about this question. *Jean*, could you help comment
> >>>>> since this is from your code?
> >>>>
> >>>>
> >>>>
> >>>> If he doesn't answer - if you don't remove the capability does it
> >>>> still work?
> >>>
> >>>
> >>> So I actually originally found this issue with the vendor
> >>> capabilities and created the original patch for it. This was quite
> >>> some time ago so I had to go back and look. IIRC the vendor specific
> >>> capabilities were always the first one in the chain and the
> >>> unchaining code would drop all further capabilities (which we did
> >>> not want to pass directly to the guest).
> >>
> >>
> >> OK, so blacklisting.
> >>>
> >>>
> >>> We never saw a configuration where the vendor capabilities were not
> >>> the first. I guess the suggestion is that to make the patch
> >>> consistent, preceding capabilities should be detected and handled. I
> >>> am not sure what the best way to do it would be. Perhaps scanning
> >>> through the chain until 0x09 is found and reporting its offset
> >>> through 0x34 instead of what is there would be the way to go. Then
> >>> unchain anything past the 0x09 caps too as is currently done.
> >>
> >>
> >> Or just scan through the capabilities, and chain only the ones
> >> that we want to "Whitelist" and the rest are to be blacklisted.
> >> The rest can also have its values set to some bogus value (0xdeadbeef?)
> >> Perhaps only when built with 'debug=y'.
> >
> >
> > That sounds about right. Back when I first did the patch (in an old qemu)
> > there were no other capabilities on the piix4 host bridge so it was simple.
> > Not sure if other capabilities will be an issue now.
> 
> It's still the case as for the IVB host bridge.
> And from what I can find from the datasheet for the Haswell, it's
> still the case.
> 
> Note that the datasheet explicitly documents the offset of the
> CAPABILITY registers.
> I guess there will be code that rely on this offset that been publicly
> documented.
> 
> Btw. Ross, now that you appears to be the original author (sorry for
> mess you up with Jean),
> could you also comment on my rework proposal? Jan believe the current
> form is not clean enough.
> 
> Currently we use a whitelist of registers to pass-through.How do you
> come up with the current list?
> The shadow copy way appears to work for the current list.

OK.
> But what if we are going to need some special registers that cannot be
> handled well? (e.g. has side effect for reading and cannot perform
> read-back?)

Hopefully the i915 driver in Linux will help in figuring out which
ones of those are needed?

Re: [PATCH 3/3] qemu-xen-trad: IGD passthrough: Expose vendor specific pci cap on host bridge.

[Ross Philipson]

On 06/26/2013 08:53 AM, Konrad Rzeszutek Wilk wrote:
> On Wed, Jun 26, 2013 at 12:18:02PM +0800, G.R. wrote:
>> On Tue, Jun 25, 2013 at 11:01 PM, Ross Philipson
>> <ross.philipson@citrix.com>  wrote:
>>> On 06/25/2013 10:54 AM, Konrad Rzeszutek Wilk wrote:
>>>>
>>>> On Tue, Jun 25, 2013 at 10:08:14AM -0400, Ross Philipson wrote:
>>>>>
>>>>> On 06/21/2013 02:03 PM, Konrad Rzeszutek Wilk wrote:
>>>>>>
>>>>>> On Wed, Jun 19, 2013 at 06:37:06PM +0800, G.R. wrote:
>>>>>>>
>>>>>>> I'm going to rework this patch to address Jan's concern.
>>>>>>> Here is my proposal, please review and comment before I begin:
>>>>>>>
>>>>>>> The proposal is to read a shadow copy of the exposed host register into
>>>>>>> the config space of the emulated host bridge and relies on the
>>>>>>> pci_default_read_config() function
>>>>>>> to provide proper access.
>>>>>>>
>>>>>>> This methodology only works for constant values, which is our case
>>>>>>> here.
>>>>>>> The exposed value is either read-only or write-locked (for BIOS).
>>>>>>>
>>>>>>> The only exception is that the PAVPC (0x58) register is write-locked
>>>>>>> but not for BIOS.
>>>>>>
>>>>>>
>>>>>> So only SeaBIOS or hvmloader should touch it?
>>>>>>
>>>>>>> This is exposed for RW and my proposal is to perform write-through in
>>>>>>> the register write-support.
>>>>>>
>>>>>>
>>>>>> What does PAVPC do? As in if the driver wrote 0xdeadbeef in there what
>>>>>> would happen? Is there a list of the appropiate values it should
>>>>>> accept?
>>>>>>
>>>>>>>
>>>>>>>>
>>>>>>>> Also, why would removing the next capability be correct here,
>>>>>>>> when you're not removing _all_ other capabilities?
>>>>>>>
>>>>>>> I have no answer about this question. *Jean*, could you help comment
>>>>>>> since this is from your code?
>>>>>>
>>>>>>
>>>>>>
>>>>>> If he doesn't answer - if you don't remove the capability does it
>>>>>> still work?
>>>>>
>>>>>
>>>>> So I actually originally found this issue with the vendor
>>>>> capabilities and created the original patch for it. This was quite
>>>>> some time ago so I had to go back and look. IIRC the vendor specific
>>>>> capabilities were always the first one in the chain and the
>>>>> unchaining code would drop all further capabilities (which we did
>>>>> not want to pass directly to the guest).
>>>>
>>>>
>>>> OK, so blacklisting.
>>>>>
>>>>>
>>>>> We never saw a configuration where the vendor capabilities were not
>>>>> the first. I guess the suggestion is that to make the patch
>>>>> consistent, preceding capabilities should be detected and handled. I
>>>>> am not sure what the best way to do it would be. Perhaps scanning
>>>>> through the chain until 0x09 is found and reporting its offset
>>>>> through 0x34 instead of what is there would be the way to go. Then
>>>>> unchain anything past the 0x09 caps too as is currently done.
>>>>
>>>>
>>>> Or just scan through the capabilities, and chain only the ones
>>>> that we want to "Whitelist" and the rest are to be blacklisted.
>>>> The rest can also have its values set to some bogus value (0xdeadbeef?)
>>>> Perhaps only when built with 'debug=y'.
>>>
>>>
>>> That sounds about right. Back when I first did the patch (in an old qemu)
>>> there were no other capabilities on the piix4 host bridge so it was simple.
>>> Not sure if other capabilities will be an issue now.
>>
>> It's still the case as for the IVB host bridge.
>> And from what I can find from the datasheet for the Haswell, it's
>> still the case.
>>
>> Note that the datasheet explicitly documents the offset of the
>> CAPABILITY registers.
>> I guess there will be code that rely on this offset that been publicly
>> documented.

IIRC I think that may be the case; probably should proceed with that 
assumption.

>>
>> Btw. Ross, now that you appears to be the original author (sorry for
>> mess you up with Jean),

No worries - Jean and I used to work together. He took the patch, 
cleaned it up and upstreamed it.

>> could you also comment on my rework proposal? Jan believe the current
>> form is not clean enough.

Using the shadow registers? That sounds fine to me.

>>
>> Currently we use a whitelist of registers to pass-through.How do you
>> come up with the current list?
>> The shadow copy way appears to work for the current list.

Originally the only registers being dealt with were the vendor 
capabilities. The other registers came along later and I am not sure who 
identified them or why.

For example, I looked up the PAVPC register and it is pretty much 
undocumented in the spec I have. It says it is disabled by TXT which I 
assume means if you don't have TXT (or it is disabled), this register 
would not be disabled for write. I am not sure the best way to handle 
all that. Maybe set the PAVPLCK bit at the appropriate time and just 
disable it.

>
> OK.
>> But what if we are going to need some special registers that cannot be
>> handled well? (e.g. has side effect for reading and cannot perform
>> read-back?)
>
> Hopefully the i915 driver in Linux will help in figuring out which
> ones of those are needed?

That sounds like a good suggestion since a lot of those registers are 
minimally documented.

Re: [PATCH 3/3] qemu-xen-trad: IGD passthrough: Expose vendor specific pci cap on host bridge.

[G.R.]

On Wed, Jun 26, 2013 at 8:53 PM, Konrad Rzeszutek Wilk
<konrad.wilk@oracle.com> wrote:
> On Wed, Jun 26, 2013 at 12:18:02PM +0800, G.R. wrote:
>> On Tue, Jun 25, 2013 at 11:01 PM, Ross Philipson
>> <ross.philipson@citrix.com> wrote:
>> > On 06/25/2013 10:54 AM, Konrad Rzeszutek Wilk wrote:
>> >>
>> >> On Tue, Jun 25, 2013 at 10:08:14AM -0400, Ross Philipson wrote:
>> >>>
>> >>> On 06/21/2013 02:03 PM, Konrad Rzeszutek Wilk wrote:
>> >>>>
>> >>>> On Wed, Jun 19, 2013 at 06:37:06PM +0800, G.R. wrote:
>> >>>>>
>> >>>>> I'm going to rework this patch to address Jan's concern.
>> >>>>> Here is my proposal, please review and comment before I begin:
>> >>>>>
>> >>>>> The proposal is to read a shadow copy of the exposed host register into
>> >>>>> the config space of the emulated host bridge and relies on the
>> >>>>> pci_default_read_config() function
>> >>>>> to provide proper access.
>> >>>>>
>> >>>>> This methodology only works for constant values, which is our case
>> >>>>> here.
>> >>>>> The exposed value is either read-only or write-locked (for BIOS).
>> >>>>>
>> >>>>> The only exception is that the PAVPC (0x58) register is write-locked
>> >>>>> but not for BIOS.
>> >>>>
>> >>>>
>> >>>> So only SeaBIOS or hvmloader should touch it?
>> >>>>
>> >>>>> This is exposed for RW and my proposal is to perform write-through in
>> >>>>> the register write-support.
>> >>>>
>> >>>>
>> >>>> What does PAVPC do? As in if the driver wrote 0xdeadbeef in there what
>> >>>> would happen? Is there a list of the appropiate values it should
>> >>>> accept?
>> >>>>
>> >>>>>
>> >>>>>>
>> >>>>>> Also, why would removing the next capability be correct here,
>> >>>>>> when you're not removing _all_ other capabilities?
>> >>>>>
>> >>>>> I have no answer about this question. *Jean*, could you help comment
>> >>>>> since this is from your code?
>> >>>>
>> >>>>
>> >>>>
>> >>>> If he doesn't answer - if you don't remove the capability does it
>> >>>> still work?
>> >>>
>> >>>
>> >>> So I actually originally found this issue with the vendor
>> >>> capabilities and created the original patch for it. This was quite
>> >>> some time ago so I had to go back and look. IIRC the vendor specific
>> >>> capabilities were always the first one in the chain and the
>> >>> unchaining code would drop all further capabilities (which we did
>> >>> not want to pass directly to the guest).
>> >>
>> >>
>> >> OK, so blacklisting.
>> >>>
>> >>>
>> >>> We never saw a configuration where the vendor capabilities were not
>> >>> the first. I guess the suggestion is that to make the patch
>> >>> consistent, preceding capabilities should be detected and handled. I
>> >>> am not sure what the best way to do it would be. Perhaps scanning
>> >>> through the chain until 0x09 is found and reporting its offset
>> >>> through 0x34 instead of what is there would be the way to go. Then
>> >>> unchain anything past the 0x09 caps too as is currently done.
>> >>
>> >>
>> >> Or just scan through the capabilities, and chain only the ones
>> >> that we want to "Whitelist" and the rest are to be blacklisted.
>> >> The rest can also have its values set to some bogus value (0xdeadbeef?)
>> >> Perhaps only when built with 'debug=y'.
>> >
>> >
>> > That sounds about right. Back when I first did the patch (in an old qemu)
>> > there were no other capabilities on the piix4 host bridge so it was simple.
>> > Not sure if other capabilities will be an issue now.
>>
>> It's still the case as for the IVB host bridge.
>> And from what I can find from the datasheet for the Haswell, it's
>> still the case.
>>
>> Note that the datasheet explicitly documents the offset of the
>> CAPABILITY registers.
>> I guess there will be code that rely on this offset that been publicly
>> documented.
>>
>> Btw. Ross, now that you appears to be the original author (sorry for
>> mess you up with Jean),
>> could you also comment on my rework proposal? Jan believe the current
>> form is not clean enough.
>>
>> Currently we use a whitelist of registers to pass-through.How do you
>> come up with the current list?
>> The shadow copy way appears to work for the current list.
>
> OK.
>> But what if we are going to need some special registers that cannot be
>> handled well? (e.g. has side effect for reading and cannot perform
>> read-back?)
>
> Hopefully the i915 driver in Linux will help in figuring out which
> ones of those are needed?
I remember the vendor cap fix only helps windows guest.
Linux guest just run happily without this.

Re: [PATCH 3/3] qemu-xen-trad: IGD passthrough: Expose vendor specific pci cap on host bridge.

[Konrad Rzeszutek Wilk]

> >> >> Or just scan through the capabilities, and chain only the ones
> >> >> that we want to "Whitelist" and the rest are to be blacklisted.
> >> >> The rest can also have its values set to some bogus value (0xdeadbeef?)
> >> >> Perhaps only when built with 'debug=y'.
> >> >
> >> >
> >> > That sounds about right. Back when I first did the patch (in an old qemu)
> >> > there were no other capabilities on the piix4 host bridge so it was simple.
> >> > Not sure if other capabilities will be an issue now.
> >>
> >> It's still the case as for the IVB host bridge.
> >> And from what I can find from the datasheet for the Haswell, it's
> >> still the case.
> >>
> >> Note that the datasheet explicitly documents the offset of the
> >> CAPABILITY registers.
> >> I guess there will be code that rely on this offset that been publicly
> >> documented.
> >>
> >> Btw. Ross, now that you appears to be the original author (sorry for
> >> mess you up with Jean),
> >> could you also comment on my rework proposal? Jan believe the current
> >> form is not clean enough.
> >>
> >> Currently we use a whitelist of registers to pass-through.How do you
> >> come up with the current list?
> >> The shadow copy way appears to work for the current list.
> >
> > OK.
> >> But what if we are going to need some special registers that cannot be
> >> handled well? (e.g. has side effect for reading and cannot perform
> >> read-back?)
> >
> > Hopefully the i915 driver in Linux will help in figuring out which
> > ones of those are needed?
> I remember the vendor cap fix only helps windows guest.

How was that diagnosed? Perhaps that information can be part of the source
code to help in the future with diagnosiing which caps are needed and
which ones can be blacklisted?

> Linux guest just run happily without this.
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
> 

Re: [PATCH 3/3] qemu-xen-trad: IGD passthrough: Expose vendor specific pci cap on host bridge.

[Pasi Kärkkäinen]

On Mon, Jul 01, 2013 at 09:06:37AM -0400, Konrad Rzeszutek Wilk wrote:
> > >> >> Or just scan through the capabilities, and chain only the ones
> > >> >> that we want to "Whitelist" and the rest are to be blacklisted.
> > >> >> The rest can also have its values set to some bogus value (0xdeadbeef?)
> > >> >> Perhaps only when built with 'debug=y'.
> > >> >
> > >> >
> > >> > That sounds about right. Back when I first did the patch (in an old qemu)
> > >> > there were no other capabilities on the piix4 host bridge so it was simple.
> > >> > Not sure if other capabilities will be an issue now.
> > >>
> > >> It's still the case as for the IVB host bridge.
> > >> And from what I can find from the datasheet for the Haswell, it's
> > >> still the case.
> > >>
> > >> Note that the datasheet explicitly documents the offset of the
> > >> CAPABILITY registers.
> > >> I guess there will be code that rely on this offset that been publicly
> > >> documented.
> > >>
> > >> Btw. Ross, now that you appears to be the original author (sorry for
> > >> mess you up with Jean),
> > >> could you also comment on my rework proposal? Jan believe the current
> > >> form is not clean enough.
> > >>
> > >> Currently we use a whitelist of registers to pass-through.How do you
> > >> come up with the current list?
> > >> The shadow copy way appears to work for the current list.
> > >
> > > OK.
> > >> But what if we are going to need some special registers that cannot be
> > >> handled well? (e.g. has side effect for reading and cannot perform
> > >> read-back?)
> > >
> > > Hopefully the i915 driver in Linux will help in figuring out which
> > > ones of those are needed?
> > I remember the vendor cap fix only helps windows guest.
> 
> How was that diagnosed? Perhaps that information can be part of the source
> code to help in the future with diagnosiing which caps are needed and
> which ones can be blacklisted?
> 

I guess that's a question mostly for Ross/Jean as they're the original authors of the patch? 

-- Pasi

Re: [PATCH 3/3] qemu-xen-trad: IGD passthrough: Expose vendor specific pci cap on host bridge.

[Ross Philipson]

On 07/15/2013 12:06 PM, Pasi Kärkkäinen wrote:
> On Mon, Jul 01, 2013 at 09:06:37AM -0400, Konrad Rzeszutek Wilk wrote:
>>>>>>> Or just scan through the capabilities, and chain only the ones
>>>>>>> that we want to "Whitelist" and the rest are to be blacklisted.
>>>>>>> The rest can also have its values set to some bogus value (0xdeadbeef?)
>>>>>>> Perhaps only when built with 'debug=y'.
>>>>>>
>>>>>>
>>>>>> That sounds about right. Back when I first did the patch (in an old qemu)
>>>>>> there were no other capabilities on the piix4 host bridge so it was simple.
>>>>>> Not sure if other capabilities will be an issue now.
>>>>>
>>>>> It's still the case as for the IVB host bridge.
>>>>> And from what I can find from the datasheet for the Haswell, it's
>>>>> still the case.
>>>>>
>>>>> Note that the datasheet explicitly documents the offset of the
>>>>> CAPABILITY registers.
>>>>> I guess there will be code that rely on this offset that been publicly
>>>>> documented.
>>>>>
>>>>> Btw. Ross, now that you appears to be the original author (sorry for
>>>>> mess you up with Jean),
>>>>> could you also comment on my rework proposal? Jan believe the current
>>>>> form is not clean enough.
>>>>>
>>>>> Currently we use a whitelist of registers to pass-through.How do you
>>>>> come up with the current list?
>>>>> The shadow copy way appears to work for the current list.
>>>>
>>>> OK.
>>>>> But what if we are going to need some special registers that cannot be
>>>>> handled well? (e.g. has side effect for reading and cannot perform
>>>>> read-back?)
>>>>
>>>> Hopefully the i915 driver in Linux will help in figuring out which
>>>> ones of those are needed?
>>> I remember the vendor cap fix only helps windows guest.
>>
>> How was that diagnosed? Perhaps that information can be part of the source
>> code to help in the future with diagnosiing which caps are needed and
>> which ones can be blacklisted?
>>
>
> I guess that's a question mostly for Ross/Jean as they're the original authors of the patch?

We discovered the issue with Windows guests running the vendor drivers 
for the passed in IGD graphics device. Under certain circumstances 
(resuming from S3/S4 IIRC), the guest would BSOD. I finally tracked it 
down to a bad state in the resuming driver because it was not coded to 
handle the vendor capabilities not being present on the host bridge. 
BTW, those capabilities are flags indicating what features the IGD card 
has - their exact meaning is of course proprietary.

I cannot say it was only a problem on Windows but rather that that is 
the only place we ever saw it.

I never saw any other capabilities on the hosts bridges at that time, 
just vendor ones so the patch just handled that. If there were other 
capabilities, I would think it would have to be determined on a case by 
case basis whether they were included. Inclusion of each new type would 
have different ramifications it seems.

>
> -- Pasi
>


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

Re: [PATCH 3/3] qemu-xen-trad: IGD passthrough: Expose vendor specific pci cap on host bridge.

[Pasi Kärkkäinen]

On Mon, Jul 15, 2013 at 01:47:43PM -0400, Ross Philipson wrote:
> On 07/15/2013 12:06 PM, Pasi Kärkkäinen wrote:
> >On Mon, Jul 01, 2013 at 09:06:37AM -0400, Konrad Rzeszutek Wilk wrote:
> >>>>>>>Or just scan through the capabilities, and chain only the ones
> >>>>>>>that we want to "Whitelist" and the rest are to be blacklisted.
> >>>>>>>The rest can also have its values set to some bogus value (0xdeadbeef?)
> >>>>>>>Perhaps only when built with 'debug=y'.
> >>>>>>
> >>>>>>
> >>>>>>That sounds about right. Back when I first did the patch (in an old qemu)
> >>>>>>there were no other capabilities on the piix4 host bridge so it was simple.
> >>>>>>Not sure if other capabilities will be an issue now.
> >>>>>
> >>>>>It's still the case as for the IVB host bridge.
> >>>>>And from what I can find from the datasheet for the Haswell, it's
> >>>>>still the case.
> >>>>>
> >>>>>Note that the datasheet explicitly documents the offset of the
> >>>>>CAPABILITY registers.
> >>>>>I guess there will be code that rely on this offset that been publicly
> >>>>>documented.
> >>>>>
> >>>>>Btw. Ross, now that you appears to be the original author (sorry for
> >>>>>mess you up with Jean),
> >>>>>could you also comment on my rework proposal? Jan believe the current
> >>>>>form is not clean enough.
> >>>>>
> >>>>>Currently we use a whitelist of registers to pass-through.How do you
> >>>>>come up with the current list?
> >>>>>The shadow copy way appears to work for the current list.
> >>>>
> >>>>OK.
> >>>>>But what if we are going to need some special registers that cannot be
> >>>>>handled well? (e.g. has side effect for reading and cannot perform
> >>>>>read-back?)
> >>>>
> >>>>Hopefully the i915 driver in Linux will help in figuring out which
> >>>>ones of those are needed?
> >>>I remember the vendor cap fix only helps windows guest.
> >>
> >>How was that diagnosed? Perhaps that information can be part of the source
> >>code to help in the future with diagnosiing which caps are needed and
> >>which ones can be blacklisted?
> >>
> >
> >I guess that's a question mostly for Ross/Jean as they're the original authors of the patch?
> 
> We discovered the issue with Windows guests running the vendor
> drivers for the passed in IGD graphics device. Under certain
> circumstances (resuming from S3/S4 IIRC), the guest would BSOD. I
> finally tracked it down to a bad state in the resuming driver
> because it was not coded to handle the vendor capabilities not being
> present on the host bridge. BTW, those capabilities are flags
> indicating what features the IGD card has - their exact meaning is
> of course proprietary.
> 
> I cannot say it was only a problem on Windows but rather that that
> is the only place we ever saw it.
> 
> I never saw any other capabilities on the hosts bridges at that
> time, just vendor ones so the patch just handled that. If there were
> other capabilities, I would think it would have to be determined on
> a case by case basis whether they were included. Inclusion of each
> new type would have different ramifications it seems.
> 

Thanks for the explanation. I guess parts of that should go to the patch description aswell..

-- Pasi

Re: [PATCH 3/3] qemu-xen-trad: IGD passthrough: Expose vendor specific pci cap on host bridge.

[Pasi Kärkkäinen]

On Tue, Jul 16, 2013 at 01:55:20AM +0300, Pasi Kärkkäinen wrote:
> > >>How was that diagnosed? Perhaps that information can be part of the source
> > >>code to help in the future with diagnosiing which caps are needed and
> > >>which ones can be blacklisted?
> > >>
> > >
> > >I guess that's a question mostly for Ross/Jean as they're the original authors of the patch?
> > 
> > We discovered the issue with Windows guests running the vendor
> > drivers for the passed in IGD graphics device. Under certain
> > circumstances (resuming from S3/S4 IIRC), the guest would BSOD. I
> > finally tracked it down to a bad state in the resuming driver
> > because it was not coded to handle the vendor capabilities not being
> > present on the host bridge. BTW, those capabilities are flags
> > indicating what features the IGD card has - their exact meaning is
> > of course proprietary.
> > 
> > I cannot say it was only a problem on Windows but rather that that
> > is the only place we ever saw it.
> > 
> > I never saw any other capabilities on the hosts bridges at that
> > time, just vendor ones so the patch just handled that. If there were
> > other capabilities, I would think it would have to be determined on
> > a case by case basis whether they were included. Inclusion of each
> > new type would have different ramifications it seems.
> > 
> 
> Thanks for the explanation. I guess parts of that should go to the patch description aswell..
> 

Now patch 2/3 has been applied to qemu-traditional, so only this patch 3/3 is missing from qemu-traditional from this series.

Any other changes outstanding? or only to add some of Ross's comments to the patch description (and/or sources) ? 

Thanks,

-- Pasi

Re: [PATCH 3/3] qemu-xen-trad: IGD passthrough: Expose vendor specific pci cap on host bridge.

[G.R.]

On Tue, Aug 6, 2013 at 12:15 AM, Pasi Kärkkäinen <pasik@iki.fi> wrote:
> On Tue, Jul 16, 2013 at 01:55:20AM +0300, Pasi Kärkkäinen wrote:
>> > >>How was that diagnosed? Perhaps that information can be part of the source
>> > >>code to help in the future with diagnosiing which caps are needed and
>> > >>which ones can be blacklisted?
>> > >>
>> > >
>> > >I guess that's a question mostly for Ross/Jean as they're the original authors of the patch?
>> >
>> > We discovered the issue with Windows guests running the vendor
>> > drivers for the passed in IGD graphics device. Under certain
>> > circumstances (resuming from S3/S4 IIRC), the guest would BSOD. I
>> > finally tracked it down to a bad state in the resuming driver
>> > because it was not coded to handle the vendor capabilities not being
>> > present on the host bridge. BTW, those capabilities are flags
>> > indicating what features the IGD card has - their exact meaning is
>> > of course proprietary.
>> >
>> > I cannot say it was only a problem on Windows but rather that that
>> > is the only place we ever saw it.
>> >
>> > I never saw any other capabilities on the hosts bridges at that
>> > time, just vendor ones so the patch just handled that. If there were
>> > other capabilities, I would think it would have to be determined on
>> > a case by case basis whether they were included. Inclusion of each
>> > new type would have different ramifications it seems.
>> >
>>
>> Thanks for the explanation. I guess parts of that should go to the patch description aswell..
>>
>
> Now patch 2/3 has been applied to qemu-traditional, so only this patch 3/3 is missing from qemu-traditional from this series.
>
> Any other changes outstanding? or only to add some of Ross's comments to the patch description (and/or sources) ?
>
I'll have to rework this patch -- Jan believe the code is not very
clean with regard to offset / size handling.

I proposed an alternative to shadow the registers into the PCI config
space of the emulated host controller.
There seems no objection on this proposal. I'll do it when I got some
spare time.

> Thanks,
>
> -- Pasi
>

Re: [PATCH 3/3] qemu-xen-trad: IGD passthrough: Expose vendor specific pci cap on host bridge.

[Pasi Kärkkäinen]

Hello,

On Tue, Aug 06, 2013 at 11:44:52AM +0800, G.R. wrote:
> >
> > Now patch 2/3 has been applied to qemu-traditional, so only this patch 3/3 is missing from qemu-traditional from this series.
> >
> > Any other changes outstanding? or only to add some of Ross's comments to the patch description (and/or sources) ?
> >
> I'll have to rework this patch -- Jan believe the code is not very
> clean with regard to offset / size handling.
> 
> I proposed an alternative to shadow the registers into the PCI config
> space of the emulated host controller.
> There seems no objection on this proposal. I'll do it when I got some
> spare time.
> 

Do you think you'll find time to rework patch 3/3 before Xen 4.4 feature freeze? 
It'd be nice to get the last patch from this series finally applied!

Thanks,

-- Pasi

Re: [PATCH 3/3] qemu-xen-trad: IGD passthrough: Expose vendor specific pci cap on host bridge.

[Pasi Kärkkäinen]

Hello Tiejun,

Since you've been working on upstreaming the Xen IGD passthru patches to qemu-upstream,
you might be able to comment on this patch aswell.

This patch is for qemu-traditional, but this one hasn't been acked / applied yet. 
It has been floating around on xen-devel for a couple of years now.
It would be nice to get this finally merged to qemu-traditional.. 

Can you comment about best approach to implement this patch?
There was some comments/concerns about the earlier versions.

"[PATCH 3/3] qemu-xen-trad: IGD passthrough: Expose vendor specific pci cap on host bridge.":
http://lists.xenproject.org/archives/html/xen-devel/2013-02/msg00538.html

And some discussion about the patch/justification one year ago:
http://lists.xen.org/archives/html/xen-devel/2013-07/msg01376.html


Also G.R. wrote earlier on this thread:

> I proposed an alternative to shadow the registers into the PCI config
> space of the emulated host controller.
> There seems no objection on this proposal. I'll do it when I got some spare time.


Thanks,

-- Pasi


On Tue, Aug 06, 2013 at 11:44:52AM +0800, G.R. wrote:
> On Tue, Aug 6, 2013 at 12:15 AM, Pasi Kärkkäinen <pasik@iki.fi> wrote:
> > On Tue, Jul 16, 2013 at 01:55:20AM +0300, Pasi Kärkkäinen wrote:
> >> > >>How was that diagnosed? Perhaps that information can be part of the source
> >> > >>code to help in the future with diagnosiing which caps are needed and
> >> > >>which ones can be blacklisted?
> >> > >>
> >> > >
> >> > >I guess that's a question mostly for Ross/Jean as they're the original authors of the patch?
> >> >
> >> > We discovered the issue with Windows guests running the vendor
> >> > drivers for the passed in IGD graphics device. Under certain
> >> > circumstances (resuming from S3/S4 IIRC), the guest would BSOD. I
> >> > finally tracked it down to a bad state in the resuming driver
> >> > because it was not coded to handle the vendor capabilities not being
> >> > present on the host bridge. BTW, those capabilities are flags
> >> > indicating what features the IGD card has - their exact meaning is
> >> > of course proprietary.
> >> >
> >> > I cannot say it was only a problem on Windows but rather that that
> >> > is the only place we ever saw it.
> >> >
> >> > I never saw any other capabilities on the hosts bridges at that
> >> > time, just vendor ones so the patch just handled that. If there were
> >> > other capabilities, I would think it would have to be determined on
> >> > a case by case basis whether they were included. Inclusion of each
> >> > new type would have different ramifications it seems.
> >> >
> >>
> >> Thanks for the explanation. I guess parts of that should go to the patch description aswell..
> >>
> >
> > Now patch 2/3 has been applied to qemu-traditional, so only this patch 3/3 is missing from qemu-traditional from this series.
> >
> > Any other changes outstanding? or only to add some of Ross's comments to the patch description (and/or sources) ?
> >
> I'll have to rework this patch -- Jan believe the code is not very
> clean with regard to offset / size handling.
> 
> I proposed an alternative to shadow the registers into the PCI config
> space of the emulated host controller.
> There seems no objection on this proposal. I'll do it when I got some
> spare time.
> 
> > Thanks,
> >
> > -- Pasi
> >