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

[Michael S. Tsirkin]

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.


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

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

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

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

>  Let's discuss.
> 
> Thanks,
> Alex