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

["Michael S. Tsirkin" <mst@redhat.com>]

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:
> > > > > diff --git a/hw/device-assignment.c b/hw/device-assignment.c
> > > > > index 36ad6b0..34db52e 100644
> > > > > --- a/hw/device-assignment.c
> > > > > +++ b/hw/device-assignment.c
> > > > > @@ -1419,16 +1419,18 @@ static int assigned_device_pci_cap_init(PCIDevice *pci_dev)
> > > > >      }
> > > > >  
> > > > >      if ((pos = pci_find_cap_offset(pci_dev, PCI_CAP_ID_EXP, 0))) {
> > > > > -        uint8_t version;
> > > > > +        uint8_t version, size;
> > > > >          uint16_t type, devctl, lnkcap, lnksta;
> > > > >          uint32_t devcap;
> > > > > -        int size = 0x3c; /* version 2 size */
> > > > >  
> > > > >          version = pci_get_byte(pci_dev->config + pos + PCI_EXP_FLAGS);
> > > > >          version &= PCI_EXP_FLAGS_VERS;
> > > > >          if (version == 1) {
> > > > >              size = 0x14;
> > > > > -        } else if (version > 2) {
> > > > > +        } 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.

> 
> -- 
> MST