[Qemu-devel] Re: [PATCH v9 1/8] pci: revise pci command register initialization

[Michael S. Tsirkin]

On Thu, Nov 18, 2010 at 11:08:40AM +0900, Isaku Yamahata wrote:
> On Wed, Nov 17, 2010 at 02:02:00PM +0200, Michael S. Tsirkin wrote:
> > > > Another bug is that migrating from qemu where a bit is writeable to one
> > > > where it's RO creates a situation where a RW bit becomes RO, or the
> > > > reverse, which might confuse guests.  So we will need a compatibility
> > > > flag and set it for old machine types.
> > > 
> > > We needs to keep compatibility. Which way do you prefer?
> > > 
> > > - don't care: no way
> > > 
> > > - introduce global property to indicate compat qemu version or flags
> > >   something like if (compat version <= 0.13) old behaviour...
> > >   or if (flags & ...)
> > > 
> > > - introduce global-pci property
> > > 
> > > - introduce pci bus property
> > >   Users needs to specify this property for all pci devices.
> > > 
> > > - Don't change common code(pci.c), and provide a helper function.
> > >   Each device which needs new behavior like pcie calls it.
> > >   Probably each device may provide property to specify compat behavior
> > > 
> > > - any other?
> > 
> > - Don't change behaviour at all.
> > 
> >     What is the motivation for the change?  Why do we bother? What we have
> >     is spec compliant, I think, so it's hard for me to believe pcie *needs*
> >     the new behaviour.
> 
> AER wants SERR bit to be writable and you requested it as below.
> I thought, you wanted me to revise PCI_COMMAND and PCI_STATUS initialization.
> If I misunderstood, can you please elaborate on it?
> If you accept the following PCI_COMMAND line,
> I'm fine with dropping this clean up patch.
> 
> http://lists.nongnu.org/archive/html/qemu-devel/2010-11/msg00131.html
> > > +void pcie_aer_init(PCIDevice *dev, uint16_t offset)
> > > +{
> > > +    PCIExpressDevice *exp;
> > > +
> > > +    pci_word_test_and_set_mask(dev->wmask + PCI_COMMAND, 
> > > PCI_COMMAND_SERR);
> > > +    pci_word_test_and_set_mask(dev->w1cmask + PCI_STATUS,
> > > +                               PCI_STATUS_SIG_SYSTEM_ERROR);
> > > +
> > 
> > I would say we should just set these for all devices.
> > But if we do my concern is that guest might write 1 to this register,
> > then we migrate to an old guest and that one can not clear this bit.
> > Thoughts? Let's add a flag so old machine types can disable this?
> > 
> > 
> > Also - what about other bits in the status register?

I think what you did for PCI_STATUS addressed this comment. Thanks!

Yes, for AER we need to enable SERR, and just for SERR, I think
a global property or a bus property will be the simplest (I think
properties are inherited from bus to device, right?)
Something like pci_command_serr_enable, should be a bit property.
Default to on and disable for 0.13 and back. Hmm?

Also, need to check this and fail aer initialization if not
there. Maybe simply don't add aer capability if aer_init fails?
Or make it yet another property ...

-- 
MST