Xen devel - Feb 2013 - Patch series for IGD passthrough

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

"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

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

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

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

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

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

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

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

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

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

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.

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

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.

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

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

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

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

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.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

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
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

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.

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)

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

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

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)

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

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
>

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

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

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?

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.

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.

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?

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.

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.

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

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

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

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

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

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
>

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