Re: [Qemu-devel] [PATCH 0/5] PCI hotplug fixes/cleanup

[Alex Williamson]

On Mon, 2012-04-09 at 11:24 +0300, Michael S. Tsirkin wrote:
> On Mon, Apr 09, 2012 at 11:07:03AM +0300, Gleb Natapov wrote:
> > On Sun, Apr 08, 2012 at 10:48:02PM +0300, Michael S. Tsirkin wrote:
> > > On Sun, Apr 08, 2012 at 03:45:38PM -0300, Marcelo Tosatti wrote:
> > > > On Sun, Apr 08, 2012 at 10:15:26AM +0300, Michael S. Tsirkin wrote:
> > > > > On Sun, Apr 08, 2012 at 02:13:00AM -0300, Marcelo Tosatti wrote:
> > > > > > On Wed, Apr 04, 2012 at 11:51:00PM -0600, Alex Williamson wrote:
> > > > > > > We've been batting this one back and forth.  This series includes
> > > > > > > several of the cleanups and specification clarifications from my
> > > > > > > series awhile back.  Patch 5 is my proposed alternative to
> > > > > > > Michael's PCI hotplug race fix.  Since that version I added slot
> > > > > > > present tracking so we can be a little more strategic about which
> > > > > > > slots we ask the guest to check.  The approach for that path is
> > > > > > > described in the commit log.  I tested hotplug on both Linux an
> > > > > > > Windows guests (XP + 2k8), intentionally trying to do back to back
> > > > > > > device_add and device_del to get a race, but couldn't (I did
> > > > > > > however get a glibc double free that seems unrelated to this
> > > > > > > series).
> > > > > > > 
> > > > > > > Long term I'd like to deprecate the up/down PCI hotplug interface
> > > > > > > and move to a new model.  I think perhaps we should define 3 new
> > > > > > > registers:
> > > > > > > 
> > > > > > > 1) Device present in slot bitmap (foundation already in 5/5 here)
> > > > > > > 2) Virtual slot power state bitmap
> > > > > > > 3) Requested slot date bitmap
> > > > > > > 
> > > > > > > With these we should be able to do proper _STA, _PS0, and _PS3.
> > > > > > > We'd maintain the eject register for _EJ0, but deprecate up/down.
> > > > > > 
> > > > > > What problem are you trying to address by doing so?
> > > > > > 
> > > > > > What is the need for power control? Clearly there is no advantage 
> > > > > > in    
> > > > > > powering down a virtual device.
> > > > > 
> > > > > There probably are for an assigned device?
> > > > 
> > > > Can't these use native PCI PM?
> > > 
> > > I haven't looked at PM closely but I think to support D3-cold
> > > you remove power from the host component. I think this means that
> > > we need host PCI bridge support.
> > > 
> > And since host PCI bridge we emulate does not have it, we invent our own
> > mechanism. When we will move to a chipset that supports PM we will do it
> > according to its spec.
> 
> Yes.
> Playing devil's advocate here, for PM we probably need PME#
> support and that probably means another register.

I never really intended the power state bitmap to be for power
management of any sort, I was trying to support this kind of event flow:

http://www.microsoft.com/china/whdc/system/pnppwr/hotadd/hotplugpci.mspx#EYH

Hot-add:
 - Generate SCI
 - _Lxx/_Exx method determines which slots to Notify
 - _STA method for slots indicates device present
 - _PS0 requests slot turned on
 - _STA method reports device present & enabled

Hot-remove:
 - Generate SCI
 - _Lxx/_Exx method determines which slots to Notify
 - _PS3 powers off slot
 - _EJ0 ejects device
 - _STA verifies success

_STA is generated using the "present" and "power" registers and _PSx
methods directly manipulate "power".  We could then have some kind of
"info pcislot" command that allows querying slot state.  Maybe there's
even an opportunity for unpowered slot surprise removal.  In effect, the
"power" register tracks the "actual" state vs the "requested" state, but
if it's controlled by _PSx, it seems better to not try to do a
translation.

Are there better models or event flows we should be aiming for?  Thanks,

Alex