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

[Alex Williamson]

On Thu, 2012-04-12 at 18:27 +0300, Michael S. Tsirkin wrote:
> On Thu, Apr 12, 2012 at 08:57:18AM -0600, Alex Williamson wrote:
> > On Wed, 2012-04-11 at 10:40 +0300, Michael S. Tsirkin wrote:
> > > On Tue, Apr 10, 2012 at 04:07:41PM -0600, Alex Williamson wrote:
> > > > > 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:
> > > 
> > > Ah, this was off-list, I see. Thanks very much for the summary.
> > > 
> > > > 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.
> > > 
> > > Very briefly and not really correct (see real spec for details
> > > if you are curious), in the hotplug spec you give a slot minimal power
> > > to be able to detect device presence, but you must enable it to be able
> > > to enumerate devices behind it.
> > > 
> > > I thought using the hardware terms makes it clearer what we emulate
> > > and makes it possible to check the hotplug spec to verify behaviour,
> > > but this is not really critical I can translate in my head.
> > 
> > I think that _PSx marks the difference between just having standby power
> > enabled for config access and full power for address decoders.
> 
> Really? Where do you see this in spec? I thought config cycles
> and address decoders use the same amount of power.

Looking at the msft article:

6. Notify(,0)
7. Execute _STA
8. _STA returns 0xa
9. PCI driver enumerates bus
10. PCI driver reads config space
11. PCI/PNP loads and starts drivers for all functions
12. Driver requests functions turned on
13. ACPI driver executes _PS0
14. PCI power on via PCI power management

So I guess we're in a D3hot state.  It's entirely possible the msft
paper is a red herring, the ACPI spec certainly doesn't link _PSx as
strongly.

> >  Is there
> > a need to model standby power?  I imagine this would be toggled by the
> > _Lxx method at insertion and the _EJ0 at removal.  That would allow us
> > to know that the OSPM has been notified of an add event, but I'm not
> > sure how useful that is (it almost seems preferable to let qemu trigger
> > a re-notify).
> > > > 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).
> > > 
> > > OK, probably a good idea to list it for completeness.
> > > 
> > > > 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 ACPI spec does recommend that device is powered off by _EJ0.
> > 
> > It seems like a trivial robustness feature to test power in _EJ0 and
> > disable it.  I wonder if this is really referring more to standby power
> > though.
> > 
> > > >  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.
> > > 
> > > My testing showed _PS3 not called before _EJ0. Probably easiest to
> > > implement and try.
> > 
> > Yep.
> > 
> > > >  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.
> > > 
> > > Yes but will _STA return "present" and will it reappear if you
> > > do a manual scan?
> > 
> > I expect so... hmm, maybe there is a need to model standby power to
> > toggle whether config space is accessible.  On the other hand, if a
> > guest ejects a device on it's own and qemu doesn't free it, should there
> > be a way for the guest to re-enable it.  That would be a problem if
> > standby power enable is hidden in _Lxx & _EJ0.
> > 
> > > > 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?
> > > 
> > > Unfortunately windows doesn't seem to support SHPC, only native
> > > express hotplug. If true we are stuck with supporting ACPI as well.
> > > 
> > > >  Just say "no" to non-root
> > > > ACPI PCI hotplug ;)
> > > 
> > > Not sure what non-root has to do with it.
> > 
> > I guess I was hoping we would not end up with bridge slots described in
> > an SSDT, but if this is the only form of hotplug windows supports then I
> > guess we need to factor that in.  Is this why you were suggesting
> > implementing the registers in vendor specific config space, so they
> > could be replicated on each bridge?  Thanks,
> > 
> > Alex
> 
> Not really - ACPI spec explicitly forbids accessing
> config space in devices not on bus 0 since child buses
> can be reconfigured by the OS at any time.

Yes, we couldn't count on getting to config space on a 2nd level bridge.

> The idea of using config space was to save on io port space,
> we'll need to use the piix device for all acpi needs though.

Right, I thought maybe you had a secondary motivation.  This proposal
doesn't account for non-bus0, so maybe we need something better.
Thanks,

Alex