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.william...@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/ACK > > + PUPA, 32, > > + // PCI Down > > + PDWN, 32, > > + // PCI Present/Eject > > + PPEJ, 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
