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

[Jean Guyader <jean.guyader@xxxxxxxxxxxxx>]

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

Thanks for the review.

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

Jean

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