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.
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. It doesn't hurt anything if _STA is called spuriously.
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.
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. Implementing both caused interrupts to stop working on Linux guests. Note that _PS0/3 is even less closely associated with device removal in 1.0b than _STA even though the MSFT document references it.
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.
} 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.
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. I'm just following the lead of previous patches in this space.
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