On Sun, Mar 04, 2012 at 08:30:00PM -0700, Alex Williamson wrote:
On Sun, 2012-03-04 at 20:53 +0200, Michael S. Tsirkin wrote:
On Fri, Feb 24, 2012 at 04:21:17PM -0700, Alex Williamson wrote:
When a Status method is provided on a slot, the OSPM evaluates _STA in response to the device check notify on the slot. This allows some degree of a handshake between the platform and the OSPM that the hotplug has been acknowledged.
In order to implement _STA, we need to know which slots have devices. A slot with device returns 0x0F, a slot without a device returns Zero. We get this information from Qemu using the 0xae08 I/O port register. This was previously the read-side of the register written to commit a device eject and always returned 0 on read. It now returns a bitmap of present slots, so we know that reading 0 means we have and old Qemu and dynamically modify our SSDT to rename the _STA methods. This is necessary to allow backwards compatibility.
Interesting. Isn't the UP register sufficient for _STA?
No, UP only reports the current slot being added, so we'd only be able to report a "present" value for that slot and not static or previously added slots.
It's probably a bug in qemu - for example, if two slots are added quickly we just lose the notification about the first one, right?
The fix might involve making e.g. the UP register write 1 to clear, but it needs to be cleared on Notify, not on _STA.
Assuming we want to implement _STA - for which the only motivation seems the handshake hack below.
The _STA method also writes the slot identifier to I/O port register 0xae00 as an acknowledgment of the hotplug request.
This part looks a bit like a hack. _STA is not intended as an acknowledgement - it's a query for state. ACPI spec 5.0 requires that _STA is called before _INI, but ACPI 1.0b doesn't. Did you try some 1.0 OSPMs (e.g. XP) to see what they do?
I did test with XP. Section 6.3 of ACPI spec 1.0b references the _STA method during hotplug. I also found this reference for Windows ACPI procedure for hotplug/unplug:
http://www.microsoft.com/china/whdc/system/pnppwr/hotadd/hotplugpci.mspx#EYH
I agree, _STA is not intended as an acknowledgment, but that doesn't mean we can't use it as one. The OSPM can call _STA at any point in time, however calling it after we've done a notify for device check is about the best indication we can get that the OSPM is processing it.
How about an actual access to the slot? The event that we send is just a change event. Guest accesses the slot to see whether any devices are present. No ACPI or interface changes are needed to detect this.
It doesn't hurt anything if _STA is called spuriously.
It makes its use as an acknowledgment unreliable: _STA called does *not* mean OSPM saw your hotplug event.
I also think I see how this can cause a race, see below.
Signed-off-by: Alex Williamson alex.williamson@redhat.com
Your description of the qemu patches made me think that all you really want is detect an OS without OSPM. If that is the case, I would suggest adding an _INI method at top level as a simpler and more robust procedure.
No, having OSPM is a prerequisite, but does not imply supporting hotplug.
It does not? What are the OSPMs that ignore hotplug? Hotplug has been defined since ACPI 1.0 so this seems strange. But it will me much easier to discuss whether a specific hack is efficient if you define the specific bug this handshake tries to work around.
Otherwise, how about implementing _PS0 (and probably _PS3) to manage slot power? Maybe this what you are really after, and it seems like a better interface than 'acknowledge' which does not seem to make sense for real hardware.
I tried this, _PS0/3 also requires _STA.
Interesting. Why does it require _STA?
Implementing both caused interrupts to stop working on Linux guests.
So there's some bug then? Let's fix?
Note that _PS0/3 is even less closely associated with device removal in 1.0b than _STA even though the MSFT document references it.
ACPI spec references it :) It seems clear that _PS0 must be called before device is used, otherwise the slot has no power.
There's also _OST - linux doesn't implement it but it seems modern windows versions do.
src/acpi-dsdt.dsl | 36 ++- src/acpi-dsdt.hex | 124 ++++++---- src/acpi.c | 27 ++ src/ssdt-pcihp.dsl | 3 src/ssdt-pcihp.hex | 658 ++++++++++++++++++++++++++++++++++++++++++++-------- 5 files changed, 686 insertions(+), 162 deletions(-)
diff --git a/src/acpi-dsdt.dsl b/src/acpi-dsdt.dsl index 7082b65..6b87086 100644 --- a/src/acpi-dsdt.dsl +++ b/src/acpi-dsdt.dsl @@ -119,17 +119,15 @@ DefinitionBlock ( prt_slot3(0x001f), })
OperationRegion(PCST, SystemIO, 0xae00, 0x08)
OperationRegion(PCST, SystemIO, 0xae00, 0x0c) Field (PCST, DWordAcc, NoLock, WriteAsZeros) {
PCIU, 32,PCID, 32,}OperationRegion(SEJ, SystemIO, 0xae08, 0x04)Field (SEJ, DWordAcc, NoLock, WriteAsZeros){B0EJ, 32,
// PCI Up/ACKPUPA, 32,// PCI DownPDWN, 32,// PCI Present/EjectPPEJ, 32,Note on the comment: this only affects bus0 not all of PCI.
As has always been the case.
But your comment implies otherwise :)
} Name (_CRS, ResourceTemplate ()@@ -462,10 +460,20 @@ DefinitionBlock ( /* Methods called by hotplug devices */ Method (PCEJ, 1, NotSerialized) { // _EJ0 method - eject callback
Store(ShiftLeft(1, Arg0), B0EJ)
Store(ShiftLeft(1, Arg0), PPEJ) Return (0x0) }Method (PSTA, 1, NotSerialized) {Store(ShiftLeft(1, Arg0), PUPA)So this looks wrong to me.
Specifically _STA is also called at the end after _EJ0. If the device is ejected then insterted, you get a window where _STA is called and hardware will think insertion was acknowledged, while in fact ejection was acknowledged.
The qemu patch doesn't allow an insertion while an eject is pending.
I also think a request for the OS to rescan the bus will trigger _STA calls. Same race can get triggered.
Spurious _STA calls don't matter, they'll clear a bit that wasn't set in the UP register anyway. If there's a race with the hotplug SCI, ie. we've set UP, but OSPM performs a rescan, they'll noticed _STA now reports the device is present and I think that should lead to the proper result.
To be more explicit:
_EJ0
host adds new device
_STA triggered to check _EJ0 success
You now think OSPM acknowledged new device with _STA but it didn't. OSPM thinks that _EJ0 failed but it didn't.
Store(PPEJ, Local0)If (And(Local0, ShiftLeft(1, Arg0))) {Return(0x0F)} Else {Return(Zero)}}- /* Hotplug notification method supplied by SSDT */ External (_SB.PCI0.PCNT, MethodObj)
@@ -473,12 +481,16 @@ DefinitionBlock ( Method(PCNF, 0) { // Local0 = iterator Store (Zero, Local0)
// Local1 = slots marked "up"Store (PUPA, Local1)// Local2 = slots marked "down"Store (PDWN, Local2) While (LLess(Local0, 31)) { Increment(Local0)
If (And(PCIU, ShiftLeft(1, Local0))) {
If (And(Local1, ShiftLeft(1, Local0))) { PCNT(Local0, 1) }
If (And(PCID, ShiftLeft(1, Local0))) {
If (And(Local2, ShiftLeft(1, Local0))) { PCNT(Local0, 3) } }Nothing wrong here but should be a separate patch?
It was pretty trivial, but I can split it if needed.
diff --git a/src/acpi-dsdt.hex b/src/acpi-dsdt.hex index 5dc7bb4..6d99f53 100644 --- a/src/acpi-dsdt.hex +++ b/src/acpi-dsdt.hex @@ -3,12 +3,12 @@ static unsigned char AmlCode[] = { 0x53, 0x44, 0x54, -0xd3, +0xeb, 0x10, 0x0,
...
I'd rather not see this part on list.
Then it should be .gitignore'd.
No, it's in git so people without iasl can build the bios.
I'm just following the lead of previous patches in this space.
People tend to forget these blobs (I do sometimes) but it makes review awkward.
diff --git a/src/acpi.c b/src/acpi.c index 107469f..056270b 100644 --- a/src/acpi.c +++ b/src/acpi.c @@ -486,13 +486,14 @@ build_ssdt(void)
#include "ssdt-pcihp.hex"
-#define PCI_RMV_BASE 0xae0c +#define PCI_HOTPLUG 0xae0c +#define PCI_PRES_EJ 0xae08
extern void link_time_assertion(void);
static void* build_pcihp(void) {
- u32 rmvc_pcrm;
u32 hotpluggable, present; int i;
u8 *ssdt = malloc_high(sizeof ssdp_pcihp_aml);
@@ -504,7 +505,7 @@ static void* build_pcihp(void) link_time_assertion(); }
- rmvc_pcrm = inl(PCI_RMV_BASE);
- hotpluggable = inl(PCI_HOTPLUG); for (i = 0; i < ARRAY_SIZE(aml_ej0_name); ++i) { /* Slot is in byte 2 in _ADR */ u8 slot = ssdp_pcihp_aml[aml_adr_dword[i] + 2] & 0x1F;
@@ -514,11 +515,29 @@ static void* build_pcihp(void) free(ssdt); return NULL; }
if (!(rmvc_pcrm & (0x1 << slot))) {
if (!(hotpluggable & (0x1 << slot))) { memcpy(ssdt + aml_ej0_name[i], "EJ0_", 4); }}
/* Runtime patching of STA. If running on system that
* doesn't support the Present/Eject field, replace _STA* with STA_ */if (ARRAY_SIZE(aml_sta_name) != ARRAY_SIZE(aml_adr_dword)) {
link_time_assertion();}
present = inl(PCI_PRES_EJ);
for (i = 0; present == 0 && i < ARRAY_SIZE(aml_sta_name); ++i) {
/* Sanity check */if (memcmp(ssdp_pcihp_aml + aml_sta_name[i], "_STA", 4)) {warn_internalerror();free(ssdt);return NULL;}memcpy(ssdt + aml_sta_name[i], "STA_", 4);}
return ssdt;
}
diff --git a/src/ssdt-pcihp.dsl b/src/ssdt-pcihp.dsl index 4b435b8..376476a 100644 --- a/src/ssdt-pcihp.dsl +++ b/src/ssdt-pcihp.dsl @@ -10,6 +10,7 @@ DefinitionBlock ("ssdt-pcihp.aml", "SSDT", 0x01, "BXPC", "BXSSDTPCIHP", 0x1) /* Objects supplied by DSDT */ External (_SB.PCI0, DeviceObj) External (_SB.PCI0.PCEJ, MethodObj)
External (_SB.PCI0.PSTA, MethodObj)
Scope(_SB.PCI0) { /* Bulk generated PCI hotplug devices */
@@ -23,6 +24,8 @@ DefinitionBlock ("ssdt-pcihp.aml", "SSDT", 0x01, "BXPC", "BXSSDTPCIHP", 0x1) Name (_ADR, 0x##slot##0000) \ ACPI_EXTRACT_METHOD_STRING aml_ej0_name \ Method (_EJ0, 1) { Return(PCEJ(0x##slot)) } \
ACPI_EXTRACT_METHOD_STRING aml_sta_name \Method (_STA, 0) { Return(PSTA(0x##slot)) } \ Name (_SUN, 0x##slot) \ }diff --git a/src/ssdt-pcihp.hex b/src/ssdt-pcihp.hex index b15ad5a..f060c94 100644 --- a/src/ssdt-pcihp.hex +++ b/src/ssdt-pcihp.hex @@ -1,80 +1,113 @@ static unsigned short aml_adr_dword[] = { 0x3e, -0x62, -0x88, -0xae, -0xd4, -0xfa,
....
I'd rather not see this part in the patches on list.
Same. Thanks,
Alex