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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 07.02.13 at 17:12, Rui Guo <firemeteor@xxxxxxxxxxxxxxxxxxxxx> 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


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