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

[Michael S. Tsirkin]

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.

>  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.

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.

-- 
MST