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
