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

["G.R." <firemeteor@xxxxxxxxxxxxxxxxxxxxx>]

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

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