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

[Alex Williamson]

On Tue, 2012-04-10 at 23:45 +0300, Michael S. Tsirkin wrote:
> On Tue, Apr 10, 2012 at 02:03:05PM -0600, Alex Williamson wrote:
> > On Tue, 2012-04-10 at 22:36 +0300, Michael S. Tsirkin wrote:
> > > On Tue, Apr 10, 2012 at 12:29:09PM -0600, Alex Williamson wrote:
> > > > 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
> > > 
> > > You already said exactly this and both me and Marcelo responded, no?
> > 
> > Sorry, difficult to try to summarize a reply to a weekend+ of
> > conversation, is this better?
> > 
> > On Sun, 2012-04-08 at 02:13 -0300, Marcelo Tosatti wrote:
> > On Wed, Apr 04, 2012 at 11:51:00PM -0600, Alex Williamson wrote:
> > > 
> > > What problem are you trying to address by doing so?
> > 
> > I'm trying to come up with a model that provides better event life
> > cycle, clear register ownership, and facilitates features like the event
> > flow in the msft link above.  We can get away with asking the guest to
> > do extra device checks on slots, but it's a workaround for a broken
> > model.
> > 
> > > What is the need for power control? Clearly there is no advantage in    
> > > powering down a virtual device.
> > 
> > In combination with the "present" register it allows emulating more _STA
> > states.
> > 
> > On Sun, 2012-04-08 at 10:15 +0300, Michael S. Tsirkin wrote:
> > On Sun, Apr 08, 2012 at 02:13:00AM -0300, Marcelo Tosatti wrote:
> > > > 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?
> > 
> > That was not my intention
> > 
> > [Skipping the rest since it's all headed in the direction of using the
> > power register to do some kind of chipset/device power control]
> > 
> > If there was some decision or direction provided by the rest of the
> > thread, I missed it, please restate.  Thanks,
> > 
> > Alex
> 
> Some points I remember
> - power on is better called slot enabled
> - guests dont actually call _PSX like you want them to
>   (PS3 for sure, in my testing PS0 too), and _EJ0 must
>   remove power
> - populated slots after reset must be auto-enabled

Ok, none of that was in this thread or even on-list though.  I figured
we'd start from the ground floor here and re-derive your comments
instead of having them materialize from thin air.  I apologize for
splitting the discussion, so let me paraphrase what you previously
stated off-list and add my responses so we can continue here:

MST> Can the new registers be in vendor specific config space instead of
burning ioports?
AW> Good idea, I'll look into it

MST> Is "power" really write-only?
AW> No, the power state is read to construct the _STA value and
differentiates 0xa from 0xf.

MST> Suggest changing "power" to "enabled".
AW> When is a slot "enabled" by the OSPM?  Does that just mean the _Lxx
method sent a Notify?  As noted above, we could change it to
"actual" (pretty much the same as "enabled"), but that just introduces a
translation if it's controlled via _PSx.  I figure call it what it is,
but I'm open for debate if we're going to drop _PSx and trigger the
state change some other way.

MST> We still need hotpluggable mask, right?
AW> Yes, this serves the same purpose as it does today, allowing seabios
init code to dynamically remove _EJ0 from the SSDT for non-hotplug
slots.  IIRC, there is no additional usage of it for this hotplug scheme
(we could also use it to remove additional methods from non-hotplug
slots).

MST> Tried implementing _PSx, wasn't invoked as expected, but also
didn't implement _STA.
AW> We'll obviously need to validate that the proposed scheme works, but
the OSPM has no reason to gratuitously call _PS0 on a slot if there's no
_STA pr _PSC to tell it the device is powered off.  This register scheme
could also support _PSC.

MST> What does _EJ0 do vs _PS3?  Doesn't eject also clear power?  Since
these are effectively the same, how do we know what the guest wants to
do?  Perhaps we need an "ok to remove" flag set by eject.  Suggest this
is actually the "slot power" flag.
AW> Qemu does nothing on _PSx changes.  This could be an internal
variable for AML, but I think we want access to it for debugging and to
pre-populate it for reset, thus it's a register.  At run time, the OSPM
owns it.  If a guest tries to eject a powered slot, that seems more like
a compatibility question, where we can have the _EJ0 method set the
power state before eject.  As Gleb noted (also internally), the ACPI
spec doesn't seem to link _PSx to the eject process, but the above
document clearly does.  The Linux kernel had long ago decided that in
the case of Windows implementation vs ACPI spec, implementation wins.  I
assume we'd take the same tactic.  So depending on how we write the AML,
eject called on a powered slot can be an unreachable state that we don't
need to worry about.  Any time eject is called on a slot, that
translates to "ok to remove", but also gives us the option to not remove
it if not requested by qemu.

MST> Also need register to enable/disable native hotplug.
AW> Ok, we can define that it exists via a feature bit once we have a
bridge that includes native hotplug.  Isn't there already an ACPI way to
do this, so this is just some AML checks to make sure we're in the right
mode before interacting with these registers?

MST> Need to define reset state for registers.  Think we should enable
all existing devices on reset.
AW> Yes, my thought was that any cold plugged device will be "present",
"powered", and "requested" on reset.

MST> [Some thoughts on handling buses behind bridges]
AW> I would assume we'd emulate a hotplug capable bridge, can't we drop
all this ACPI hotplug support in that case?  Just say "no" to non-root
ACPI PCI hotplug ;)  Let's discuss.

Thanks,
Alex