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