[PATCH 2/2] intel gpu passthrough: Expose vendor specific pci cap on host bridge.

[PATCH 2/2] intel gpu passthrough: Expose vendor specific pci cap on host bridge.

[Jean Guyader]

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


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.

From: Ross Philipson <Ross.Philipson@citrix.com>
Signed-off-by: Jean Guyader <jean.guyader@eu.citrix.com>
Acked-by: Ross Philipson <Ross.Philipson@citrix.com>

---
 hw/pt-graphics.c |   49 ++++++++++++++++++++++++++++++++++++++++++++-----
 1 files changed, 44 insertions(+), 5 deletions(-)


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0002-intel-gpu-passthrough-Expose-vendor-specific-pci-cap.patch --]
[-- Type: text/x-patch; name="0002-intel-gpu-passthrough-Expose-vendor-specific-pci-cap.patch", Size: 2478 bytes --]

diff --git a/hw/pt-graphics.c b/hw/pt-graphics.c
index fec7390..25e28ff 100644
--- a/hw/pt-graphics.c
+++ b/hw/pt-graphics.c
@@ -60,6 +60,42 @@ void igd_pci_write(PCIDevice *pci_dev, uint32_t config_addr, uint32_t val, int l
     }
 }
 
+#define PCI_INTEL_VENDOR_CAP            0x34
+#define PCI_INTEL_VENDOR_CAP_TYPE       0x09
+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 -1;
+
+    cap_type = pt_pci_host_read(pci_dev_host_bridge, vendor_cap, 1);
+    if (cap_type != PCI_INTEL_VENDOR_CAP_TYPE)
+        return -1;
+
+    if (config_addr == PCI_INTEL_VENDOR_CAP)
+        return vendor_cap;
+
+    /* Remove the next capability link */
+    if (config_addr == vendor_cap + 1)
+        return 0;
+
+    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)
+    {
+        return pt_pci_host_read(pci_dev_host_bridge, config_addr, len);
+    }
+
+    /* -1, this function doesn't deal with this config space offset */
+    return -1;
+}
+
 uint32_t igd_pci_read(PCIDevice *pci_dev, uint32_t config_addr, int len)
 {
     struct pci_dev *pci_dev_host_bridge = pt_pci_get_dev(0, 0, 0);
@@ -82,14 +118,17 @@ uint32_t igd_pci_read(PCIDevice *pci_dev, uint32_t config_addr, int len)
         case 0xa4:        /* SNB: graphics base of stolen memory */
         case 0xa8:        /* SNB: base of GTT stolen memory */
             val = pt_pci_host_read(pci_dev_host_bridge, config_addr, len);
-#ifdef PT_DEBUG_PCI_CONFIG_ACCESS
-            PT_LOG_DEV(pci_dev, "addr=%x len=%x val=%x\n",
-                    config_addr, len, val);
-#endif
             break;
         default:
-            val = pci_default_read_config(pci_dev, config_addr, len);
+            val = igd_pci_read_vendor_cap(pci_dev, config_addr, len, val);
+            if (val == -1)
+                val = pci_default_read_config(pci_dev, config_addr, len);
+
     }
+#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;
 }
 

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

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

Re: [PATCH 2/2] intel gpu passthrough: Expose vendor specific pci cap on host bridge.

[Stefano Stabellini]

On Tue, 17 Jan 2012, Jean Guyader wrote:
> 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.
> 
> From: Ross Philipson <Ross.Philipson@citrix.com>
> Signed-off-by: Jean Guyader <jean.guyader@eu.citrix.com>
> Acked-by: Ross Philipson <Ross.Philipson@citrix.com>
> 
> ---
>  hw/pt-graphics.c |   49 ++++++++++++++++++++++++++++++++++++++++++++-----
>  1 files changed, 44 insertions(+), 5 deletions(-)
> 
> 
> diff --git a/hw/pt-graphics.c b/hw/pt-graphics.c
> index fec7390..25e28ff 100644
> --- a/hw/pt-graphics.c
> +++ b/hw/pt-graphics.c
> @@ -60,6 +60,42 @@ void igd_pci_write(PCIDevice *pci_dev, uint32_t config_addr, uint32_t val, int l
>      }
>  }
>  
> +#define PCI_INTEL_VENDOR_CAP            0x34
> +#define PCI_INTEL_VENDOR_CAP_TYPE       0x09
> +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 -1;
> +
> +    cap_type = pt_pci_host_read(pci_dev_host_bridge, vendor_cap, 1);
> +    if (cap_type != PCI_INTEL_VENDOR_CAP_TYPE)
> +        return -1;
> +
> +    if (config_addr == PCI_INTEL_VENDOR_CAP)
> +        return vendor_cap;
> +
> +    /* Remove the next capability link */
> +    if (config_addr == vendor_cap + 1)
> +        return 0;
> +
> +    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)
> +    {
> +        return pt_pci_host_read(pci_dev_host_bridge, config_addr, len);
> +    }
> +
> +    /* -1, this function doesn't deal with this config space offset */
> +    return -1;
> +}

You are passing val to igd_pci_read_vendor_cap without actually using
it.
Also you are returning -1 from a function that returns uint32_t.

I would change the prototype of igd_pci_read_vendor_cap to:

static int igd_pci_read_vendor_cap(PCIDevice *pci_dev, uint32_t config_addr, int len,
                                        uint32_t *val)

return only 0 or error and set *val to the correct value.



>  uint32_t igd_pci_read(PCIDevice *pci_dev, uint32_t config_addr, int len)
>  {
>      struct pci_dev *pci_dev_host_bridge = pt_pci_get_dev(0, 0, 0);
> @@ -82,14 +118,17 @@ uint32_t igd_pci_read(PCIDevice *pci_dev, uint32_t config_addr, int len)
>          case 0xa4:        /* SNB: graphics base of stolen memory */
>          case 0xa8:        /* SNB: base of GTT stolen memory */
>              val = pt_pci_host_read(pci_dev_host_bridge, config_addr, len);
> -#ifdef PT_DEBUG_PCI_CONFIG_ACCESS
> -            PT_LOG_DEV(pci_dev, "addr=%x len=%x val=%x\n",
> -                    config_addr, len, val);
> -#endif
>              break;
>          default:
> -            val = pci_default_read_config(pci_dev, config_addr, len);
> +            val = igd_pci_read_vendor_cap(pci_dev, config_addr, len, val);
> +            if (val == -1)
> +                val = pci_default_read_config(pci_dev, config_addr, len);
> +
>      }
> +#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;
>  }
>  

Re: [PATCH 2/2] intel gpu passthrough: Expose vendor specific pci cap on host bridge.

[Jean Guyader]

On 17/01 03:16, Stefano Stabellini wrote:
> On Tue, 17 Jan 2012, Jean Guyader wrote:
> > 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.
> > 
> > From: Ross Philipson <Ross.Philipson@citrix.com>
> > Signed-off-by: Jean Guyader <jean.guyader@eu.citrix.com>
> > Acked-by: Ross Philipson <Ross.Philipson@citrix.com>
> > 
> > ---
> >  hw/pt-graphics.c |   49 ++++++++++++++++++++++++++++++++++++++++++++-----
> >  1 files changed, 44 insertions(+), 5 deletions(-)
> > 
> > 
> > diff --git a/hw/pt-graphics.c b/hw/pt-graphics.c
> > index fec7390..25e28ff 100644
> > --- a/hw/pt-graphics.c
> > +++ b/hw/pt-graphics.c
> > @@ -60,6 +60,42 @@ void igd_pci_write(PCIDevice *pci_dev, uint32_t config_addr, uint32_t val, int l
> >      }
> >  }
> >  
> > +#define PCI_INTEL_VENDOR_CAP            0x34
> > +#define PCI_INTEL_VENDOR_CAP_TYPE       0x09
> > +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 -1;
> > +
> > +    cap_type = pt_pci_host_read(pci_dev_host_bridge, vendor_cap, 1);
> > +    if (cap_type != PCI_INTEL_VENDOR_CAP_TYPE)
> > +        return -1;
> > +
> > +    if (config_addr == PCI_INTEL_VENDOR_CAP)
> > +        return vendor_cap;
> > +
> > +    /* Remove the next capability link */
> > +    if (config_addr == vendor_cap + 1)
> > +        return 0;
> > +
> > +    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)
> > +    {
> > +        return pt_pci_host_read(pci_dev_host_bridge, config_addr, len);
> > +    }
> > +
> > +    /* -1, this function doesn't deal with this config space offset */
> > +    return -1;
> > +}
> 
> You are passing val to igd_pci_read_vendor_cap without actually using
> it.
> Also you are returning -1 from a function that returns uint32_t.
> 
> I would change the prototype of igd_pci_read_vendor_cap to:
> 
> static int igd_pci_read_vendor_cap(PCIDevice *pci_dev, uint32_t config_addr, int len,
>                                         uint32_t *val)
> 
> return only 0 or error and set *val to the correct value.
> 
> 
> 

Thanks for the review.

I'll update the patch and send a new series.

Jean

[PATCH 1/2] [passthrough] Change init for pt_pci_host return value.

[Jean Guyader]

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


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@eu.citrix.com>
---
 hw/pass-through.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-passthrough-Change-init-for-pt_pci_host-return-value.patch --]
[-- Type: text/x-patch; name="0001-passthrough-Change-init-for-pt_pci_host-return-value.patch", Size: 396 bytes --]

diff --git a/hw/pass-through.c b/hw/pass-through.c
index dbe8804..6a77657 100644
--- a/hw/pass-through.c
+++ b/hw/pass-through.c
@@ -2085,7 +2085,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);

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

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

[PATCH 2/2] intel gpu passthrough: Expose vendor specific pci cap on host bridge.

[Jean Guyader]

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


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.

Signed-off-by: Jean Guyader <jean.guyader@eu.citrix.com>
---
 hw/pt-graphics.c |   59 +++++++++++++++++++++++++++++++++++++++++++++++++----
 1 files changed, 54 insertions(+), 5 deletions(-)


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0002-intel-gpu-passthrough-Expose-vendor-specific-pci-cap.patch --]
[-- Type: text/x-patch; name="0002-intel-gpu-passthrough-Expose-vendor-specific-pci-cap.patch", Size: 2612 bytes --]

diff --git a/hw/pt-graphics.c b/hw/pt-graphics.c
index fec7390..55cbf75 100644
--- a/hw/pt-graphics.c
+++ b/hw/pt-graphics.c
@@ -60,6 +60,53 @@ void igd_pci_write(PCIDevice *pci_dev, uint32_t config_addr, uint32_t val, int l
     }
 }
 
+#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 = pt_pci_get_dev(0, 0, 0);
@@ -82,14 +129,16 @@ uint32_t igd_pci_read(PCIDevice *pci_dev, uint32_t config_addr, int len)
         case 0xa4:        /* SNB: graphics base of stolen memory */
         case 0xa8:        /* SNB: base of GTT stolen memory */
             val = pt_pci_host_read(pci_dev_host_bridge, config_addr, len);
-#ifdef PT_DEBUG_PCI_CONFIG_ACCESS
-            PT_LOG_DEV(pci_dev, "addr=%x len=%x val=%x\n",
-                    config_addr, len, val);
-#endif
             break;
         default:
-            val = pci_default_read_config(pci_dev, config_addr, len);
+            if (!igd_pci_read_vendor_cap(pci_dev, config_addr, len, &val))
+                val = pci_default_read_config(pci_dev, config_addr, len);
+
     }
+#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;
 }
 

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

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