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

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

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@xxxxxxxxxx>
> Signed-off-by: Jean Guyader <jean.guyader@xxxxxxxxxxxxx>
> Acked-by: Ross Philipson <Ross.Philipson@xxxxxxxxxx>
> 
> ---
>  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;
>  }
>  

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