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

["Jan Beulich" <JBeulich@suse.com>]

>>> On 07.02.13 at 18:38, "G.R." <firemeteor@users.sourceforge.net> wrote:
>> > +    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.

Just as for the other patch - this is way too lax an approach.
You mustn't introduce dependencies on current or observed
behavior, as this may break with future chipsets. You need to
follow what actual hardware does (i.e. allow multi-byte aligned
reads to properly return _all_ covered registers' values).

>>> +    /* 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.

Certainly not. I'm recommending to fix the code to make the
comment unnecessary.

Jan