Re: [PATCH v3] pci: correct pci config size default for cap version 2 endpoints

[Chris Wright <chrisw@sous-sol.org>]

* Don Dutile (ddutile@redhat.com) wrote:
> On 07/24/2011 06:58 AM, Michael S. Tsirkin wrote:
> >On Sun, Jul 24, 2011 at 11:41:10AM +0300, Michael S. Tsirkin wrote:
> >>On Sun, Jul 24, 2011 at 11:12:44AM +0300, Michael S. Tsirkin wrote:
> >>>On Fri, Jul 22, 2011 at 02:35:47PM -0700, Chris Wright wrote:
> >>>>* Alex Williamson (alex.williamson@redhat.com) wrote:
> >>>>>On Fri, 2011-07-22 at 14:24 -0700, Chris Wright wrote:
> >>>>>>* Donald Dutile (ddutile@redhat.com) wrote:
> >>>>>>>+        } else if (version == 2) {
> >>>>>>>+            /* don't include slot cap/stat/ctrl 2 regs; only support endpoints */
> >>>>>>>+            size = 0x34;
> >>>>>>
> >>>>>>That doesn't look correct to me.  The size is fixed, just that some
> >>>>>>registers are Reserved Zero when they do not apply (e.g. endpoint only).
> >>>>>
> >>>>>Apparently it can be interpreted differently.  In this case, we've seen
> >>>>>a tg3 device expose a v2 PCI express capability at offset 0xcc.  Using
> >>>>>0x3c bytes, we extend 8 bytes past the legacy config space area :(
> >>>>
> >>>>Wow, that device sounds broken to me.  The spec is pretty clear.
> >>>
> >>>Yes, I agree it's broken. Looks like something that
> >>>happens when a device is designed in parallel with the spec.
> >>>
> >>>What bothers me is this patch seems to make devices that do behave
> >>>correctly out of spec (registers will be writeable by default) -
> >>>correct?
> >>>
> >>>How about we check for overflow and only do the hacks
> >>>if it happens?
> >>>
> >>>Also, the code to initialize slot and root control registers is still
> >>>there: it would seem that running it will corrupt memmory beyond the
> >>>config array?
> >>
> >>I take this last bit back: registers we touch are at offset<  0x34.
> >>Sorry about the noise. But the question about read-only registers
> >>still stands.
> >
> >Also, where does the magic 0x34 come from? I'm guessing this is
> >simply what's left till the end of the config space.
> >So let's be conservative specific as possible with
> >this hack:
> 
> I believe the spec leaves room for interpretation, and thus the
> resulting 'broken' device.  As I read the spec, the size of the struct can be:

Yeah, I can see how it might be misinterpreted, however, it's made
really clear in the config space test spec.  This strucuture is meant to
be full size.  Perhaps something like Michael suggested (and if really
paranoid + pci vendor/device id to quirk it).  I haven't come across many
devices have this wrong.

thanks,
-chris