| Message ID | 20120224231735.17761.31411.stgit@bling.home |
|---|---|
| State | New |
| Headers | show |
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. > > The _STA method also writes the slot identifier to I/O port > register 0xae00 as an acknowledgment of the hotplug request. Thanks. The patch looks fine to me. However, since I'm not all that familiar with the details of the qemu/kvm support for this, I'd like to see an Ack from one of the qemu or kvm maintainers. -Kevin
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? 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 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. 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. > --- > > 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. > } > > 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. I also think a request for the OS to rescan the bus will trigger _STA calls. Same race can get triggered. > + 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? > 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. > 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.
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/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
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/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 > >
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. ... > > > The _STA method also writes the slot identifier to I/O port > > > register 0xae00 as an acknowledgment of the hotplug request. To summarize my previous messages, my notes are - not clear that we want to implement _STA: yes we can tell hypervisor what did _STA report to OSPM but this won't be needed without _STA - assuming we do, it seems clear that we want hypervisor to know what it is that we told OSPM about slot status - the specific interface used for the above is fairly tricky so it needs documentation explaining how both sides cooperate
On Mon, Mar 05, 2012 at 08:26:23AM +0200, Michael S. Tsirkin wrote: > 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. > > ... > > > > > The _STA method also writes the slot identifier to I/O port > > > > register 0xae00 as an acknowledgment of the hotplug request. > > > To summarize my previous messages, my notes are > - not clear that we want to implement _STA: yes we can tell hypervisor > what did _STA report to OSPM but this won't be needed without _STA > - assuming we do, it seems clear that we want hypervisor > to know what it is that we told OSPM about slot status > - the specific interface used for the above is fairly tricky > so it needs documentation explaining how both sides cooperate > Why have this up, down things at all? What's wrong with how CPU hotplug works. It has only one HW register that returns a single bitmask that has 1 for available cpu and 0 for non available. AML can figure out what changed by having local copy of the old register's value to compare new value with. -- Gleb.
On Mon, 2012-03-05 at 07:15 +0200, Michael S. Tsirkin wrote: > 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 current design up UP/DOWN is that they're transient. A bit in UP is set to cause a hot-add notification, a bit in DOWN is set to cause a hot remove. > 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. The trouble with that, for my purposes, is that any OSPM will trigger the Notify, that's just part of GPE processing. Only hotplug capable OSPMs will respond to the Notify with a _STA check. > > > 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. That's a possibility. > > 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. It seems to work well though. In practice, the OSPM doesn't go around randomly calling _STA on devices to poll if the status has changed. It does it in response to events, device check being one of those events. However, only an OSPM that intends to do something about a device check is going to call _STA in response to it. So the behavior I see is that a non-hotplug capable guest sits quietly and does nothing after the SCI is injected and GPE Notify is triggered, while hotplug capable guests respond by checking _STA. > > > 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? Linux: modprobe -r acpiphp (or just don't load it automatically) > 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. Well, there's the one above, not all guests support PCI hotplug. If you hot add a device, then remember acpiphp isn't loaded, the device is held hostage, unusable by the guest, un-removable by qemu. The only solution is to reboot the guest. There are clearly some bugs in the hotplug code blindly overwriting UP/DOWN bits that break hotplugging multiple devices or hotplugging at such a rate that we stomp on ourselves. That can result in trying to add a device, immediately removing it, clearing UP, setting DOWN, confusing the guest and again ending up with a device held hostage. That's also fixed by these patches. I also thought that since this adds a virtual "end" to a hot add, and we already have a beginning and end to a hot remove, we could think about adding parameters to devie_add/del to wait for a user specified time and report success/failure. Failure to add can then immediately remove the device. > > > 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? Does the OSPM know to call _PS0/3 unless _STA reports the current state? The other serious problem that I remembered here is that Windows only powers up a slot if it has a driver. So if you hotplug a virio-net-pci device but don't have the drivers, Windows has ownership of the device, but doesn't power it up. _STA detects this, using _PS0 as an ack would not. > > Implementing both caused > > interrupts to stop working on Linux guests. > > So there's some bug then? Let's fix? Given the above, I didn't see any reason to pursue it. > > 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. See problem above. > There's also _OST - linux doesn't implement it but it seems > modern windows versions do. If we us _OST then we have a solution that only works on very, very new guests... that's not much of a solution. > > > > --- > > > > > > > > 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 :) Hmm, hard to imagine how a 32bit register can signal anything more than a single bus. > > > > } > > > > > > > > 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. Hmm, I wonder if we should clear DOWN from the qemu side of _STA too instead of from _EJ0. > > > > > > > > + 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. Then should we also commit binaries to git so people without gcc 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. Noted. [adding in follow-up] ... > > > > > The _STA method also writes the slot identifier to I/O port > > > > register 0xae00 as an acknowledgment of the hotplug request. > > > To summarize my previous messages, my notes are > - not clear that we want to implement _STA: yes we can tell hypervisor > what did _STA report to OSPM but this won't be needed without _STA /me tries to find the beginning of the circular logic. > - assuming we do, it seems clear that we want hypervisor > to know what it is that we told OSPM about slot status _STA always reports which slots have devices at the time it's called. > - the specific interface used for the above is fairly tricky > so it needs documentation explaining how both sides cooperate The field written from _STA is documented as an "ACK" and it's pretty clear that a write to that field clears the pending UP. There's also a pretty good comment in the commit log. It seems better documented than most of our ACPI interface. I can add more documentation, but this doesn't seem like your primary concern. Perhaps Gleb is right that we should redesign the whole thing, removing UP/DOWN. Maybe we can come up with better synchronization logic that way. I'm not sure if a change like that can be made with any kind of backwards compatibility like this has though. I think we'd still depend on _STA for some kind of coordination too, so in the end, I'm not sure it's any better. Thanks, Alex
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, } 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) + 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) } } 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, 0x0, 0x1, -0x2d, +0x10, 0x42, 0x58, 0x50, @@ -110,16 +110,16 @@ static unsigned char AmlCode[] = { 0x47, 0x42, 0x10, -0x44, -0x81, +0x40, +0x80, 0x5f, 0x53, 0x42, 0x5f, 0x5b, 0x82, -0x4c, -0x80, +0x48, +0x7f, 0x50, 0x43, 0x49, @@ -2014,47 +2014,27 @@ static unsigned char AmlCode[] = { 0x0, 0xae, 0xa, -0x8, +0xc, 0x5b, 0x81, -0x10, +0x15, 0x50, 0x43, 0x53, 0x54, 0x43, 0x50, -0x43, -0x49, 0x55, +0x50, +0x41, 0x20, 0x50, -0x43, -0x49, 0x44, +0x57, +0x4e, 0x20, -0x5b, -0x80, -0x53, -0x45, -0x4a, -0x5f, -0x1, -0xb, -0x8, -0xae, -0xa, -0x4, -0x5b, -0x81, -0xb, -0x53, -0x45, -0x4a, -0x5f, -0x43, -0x42, -0x30, +0x50, +0x50, 0x45, 0x4a, 0x20, @@ -3039,8 +3019,8 @@ static unsigned char AmlCode[] = { 0x4a, 0x20, 0x10, -0x46, -0x5, +0x42, +0x8, 0x2e, 0x5f, 0x53, @@ -3062,14 +3042,52 @@ static unsigned char AmlCode[] = { 0x1, 0x68, 0x0, -0x42, -0x30, +0x50, +0x50, 0x45, 0x4a, 0xa4, 0x0, 0x14, -0x38, +0x25, +0x50, +0x53, +0x54, +0x41, +0x1, +0x70, +0x79, +0x1, +0x68, +0x0, +0x50, +0x55, +0x50, +0x41, +0x70, +0x50, +0x50, +0x45, +0x4a, +0x60, +0xa0, +0xb, +0x7b, +0x60, +0x79, +0x1, +0x68, +0x0, +0x0, +0xa4, +0xa, +0xf, +0xa1, +0x3, +0xa4, +0x0, +0x14, +0x3e, 0x50, 0x43, 0x4e, @@ -3078,8 +3096,20 @@ static unsigned char AmlCode[] = { 0x70, 0x0, 0x60, +0x70, +0x50, +0x55, +0x50, +0x41, +0x61, +0x70, +0x50, +0x44, +0x57, +0x4e, +0x62, 0xa2, -0x2c, +0x26, 0x95, 0x60, 0xa, @@ -3087,12 +3117,9 @@ static unsigned char AmlCode[] = { 0x75, 0x60, 0xa0, -0x11, +0xe, 0x7b, -0x50, -0x43, -0x49, -0x55, +0x61, 0x79, 0x1, 0x60, @@ -3105,12 +3132,9 @@ static unsigned char AmlCode[] = { 0x60, 0x1, 0xa0, -0x12, +0xf, 0x7b, -0x50, -0x43, -0x49, -0x44, +0x62, 0x79, 0x1, 0x60, 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, -0x120, -0x146, -0x16c, -0x192, -0x1b8, -0x1de, -0x204, -0x22a, -0x250, -0x276, -0x29c, -0x2c2, -0x2e8, -0x30e, -0x334, -0x35a, -0x380, -0x3a6, -0x3cc, -0x3f2, -0x418, -0x43e, -0x464, -0x48a, -0x4b0 +0x6f, +0xa3, +0xd7, +0x10b, +0x13f, +0x173, +0x1a7, +0x1db, +0x20f, +0x243, +0x277, +0x2ab, +0x2df, +0x313, +0x347, +0x37b, +0x3af, +0x3e3, +0x417, +0x44b, +0x47f, +0x4b3, +0x4e7, +0x51b, +0x54f, +0x583, +0x5b7, +0x5eb, +0x61f, +0x653 +}; +static unsigned short aml_sta_name[] = { +0x51, +0x83, +0xb7, +0xeb, +0x11f, +0x153, +0x187, +0x1bb, +0x1ef, +0x223, +0x257, +0x28b, +0x2bf, +0x2f3, +0x327, +0x35b, +0x38f, +0x3c3, +0x3f7, +0x42b, +0x45f, +0x493, +0x4c7, +0x4fb, +0x52f, +0x563, +0x597, +0x5cb, +0x5ff, +0x633, +0x667 }; static unsigned short aml_ej0_name[] = { 0x44, -0x68, -0x8e, -0xb4, -0xda, -0x100, -0x126, -0x14c, -0x172, -0x198, -0x1be, -0x1e4, -0x20a, -0x230, -0x256, -0x27c, -0x2a2, -0x2c8, -0x2ee, -0x314, -0x33a, -0x360, -0x386, -0x3ac, -0x3d2, -0x3f8, -0x41e, -0x444, -0x46a, -0x490, -0x4b6 +0x75, +0xa9, +0xdd, +0x111, +0x145, +0x179, +0x1ad, +0x1e1, +0x215, +0x249, +0x27d, +0x2b1, +0x2e5, +0x319, +0x34d, +0x381, +0x3b5, +0x3e9, +0x41d, +0x451, +0x485, +0x4b9, +0x4ed, +0x521, +0x555, +0x589, +0x5bd, +0x5f1, +0x625, +0x659 }; static unsigned char ssdp_pcihp_aml[] = { 0x53, 0x53, 0x44, 0x54, -0x44, -0x6, +0xf5, +0x7, 0x0, 0x0, 0x1, -0x94, +0xcd, 0x42, 0x58, 0x50, @@ -102,8 +135,8 @@ static unsigned char ssdp_pcihp_aml[] = { 0x10, 0x20, 0x10, -0x4f, -0x61, +0x40, +0x7d, 0x5c, 0x2e, 0x5f, @@ -116,7 +149,7 @@ static unsigned char ssdp_pcihp_aml[] = { 0x30, 0x5b, 0x82, -0x22, +0x2f, 0x53, 0x30, 0x31, @@ -144,6 +177,19 @@ static unsigned char ssdp_pcihp_aml[] = { 0x45, 0x4a, 0x1, +0x14, +0xc, +0x5f, +0x53, +0x54, +0x41, +0x0, +0xa4, +0x50, +0x53, +0x54, +0x41, +0x1, 0x8, 0x5f, 0x53, @@ -152,7 +198,7 @@ static unsigned char ssdp_pcihp_aml[] = { 0x1, 0x5b, 0x82, -0x24, +0x32, 0x53, 0x30, 0x32, @@ -181,6 +227,20 @@ static unsigned char ssdp_pcihp_aml[] = { 0x4a, 0xa, 0x2, +0x14, +0xd, +0x5f, +0x53, +0x54, +0x41, +0x0, +0xa4, +0x50, +0x53, +0x54, +0x41, +0xa, +0x2, 0x8, 0x5f, 0x53, @@ -190,7 +250,7 @@ static unsigned char ssdp_pcihp_aml[] = { 0x2, 0x5b, 0x82, -0x24, +0x32, 0x53, 0x30, 0x33, @@ -219,6 +279,20 @@ static unsigned char ssdp_pcihp_aml[] = { 0x4a, 0xa, 0x3, +0x14, +0xd, +0x5f, +0x53, +0x54, +0x41, +0x0, +0xa4, +0x50, +0x53, +0x54, +0x41, +0xa, +0x3, 0x8, 0x5f, 0x53, @@ -228,7 +302,7 @@ static unsigned char ssdp_pcihp_aml[] = { 0x3, 0x5b, 0x82, -0x24, +0x32, 0x53, 0x30, 0x34, @@ -257,6 +331,20 @@ static unsigned char ssdp_pcihp_aml[] = { 0x4a, 0xa, 0x4, +0x14, +0xd, +0x5f, +0x53, +0x54, +0x41, +0x0, +0xa4, +0x50, +0x53, +0x54, +0x41, +0xa, +0x4, 0x8, 0x5f, 0x53, @@ -266,7 +354,7 @@ static unsigned char ssdp_pcihp_aml[] = { 0x4, 0x5b, 0x82, -0x24, +0x32, 0x53, 0x30, 0x35, @@ -295,6 +383,20 @@ static unsigned char ssdp_pcihp_aml[] = { 0x4a, 0xa, 0x5, +0x14, +0xd, +0x5f, +0x53, +0x54, +0x41, +0x0, +0xa4, +0x50, +0x53, +0x54, +0x41, +0xa, +0x5, 0x8, 0x5f, 0x53, @@ -304,7 +406,7 @@ static unsigned char ssdp_pcihp_aml[] = { 0x5, 0x5b, 0x82, -0x24, +0x32, 0x53, 0x30, 0x36, @@ -333,6 +435,20 @@ static unsigned char ssdp_pcihp_aml[] = { 0x4a, 0xa, 0x6, +0x14, +0xd, +0x5f, +0x53, +0x54, +0x41, +0x0, +0xa4, +0x50, +0x53, +0x54, +0x41, +0xa, +0x6, 0x8, 0x5f, 0x53, @@ -342,7 +458,7 @@ static unsigned char ssdp_pcihp_aml[] = { 0x6, 0x5b, 0x82, -0x24, +0x32, 0x53, 0x30, 0x37, @@ -371,6 +487,20 @@ static unsigned char ssdp_pcihp_aml[] = { 0x4a, 0xa, 0x7, +0x14, +0xd, +0x5f, +0x53, +0x54, +0x41, +0x0, +0xa4, +0x50, +0x53, +0x54, +0x41, +0xa, +0x7, 0x8, 0x5f, 0x53, @@ -380,7 +510,7 @@ static unsigned char ssdp_pcihp_aml[] = { 0x7, 0x5b, 0x82, -0x24, +0x32, 0x53, 0x30, 0x38, @@ -409,6 +539,20 @@ static unsigned char ssdp_pcihp_aml[] = { 0x4a, 0xa, 0x8, +0x14, +0xd, +0x5f, +0x53, +0x54, +0x41, +0x0, +0xa4, +0x50, +0x53, +0x54, +0x41, +0xa, +0x8, 0x8, 0x5f, 0x53, @@ -418,7 +562,7 @@ static unsigned char ssdp_pcihp_aml[] = { 0x8, 0x5b, 0x82, -0x24, +0x32, 0x53, 0x30, 0x39, @@ -447,6 +591,20 @@ static unsigned char ssdp_pcihp_aml[] = { 0x4a, 0xa, 0x9, +0x14, +0xd, +0x5f, +0x53, +0x54, +0x41, +0x0, +0xa4, +0x50, +0x53, +0x54, +0x41, +0xa, +0x9, 0x8, 0x5f, 0x53, @@ -456,7 +614,7 @@ static unsigned char ssdp_pcihp_aml[] = { 0x9, 0x5b, 0x82, -0x24, +0x32, 0x53, 0x30, 0x41, @@ -485,6 +643,20 @@ static unsigned char ssdp_pcihp_aml[] = { 0x4a, 0xa, 0xa, +0x14, +0xd, +0x5f, +0x53, +0x54, +0x41, +0x0, +0xa4, +0x50, +0x53, +0x54, +0x41, +0xa, +0xa, 0x8, 0x5f, 0x53, @@ -494,7 +666,7 @@ static unsigned char ssdp_pcihp_aml[] = { 0xa, 0x5b, 0x82, -0x24, +0x32, 0x53, 0x30, 0x42, @@ -523,6 +695,20 @@ static unsigned char ssdp_pcihp_aml[] = { 0x4a, 0xa, 0xb, +0x14, +0xd, +0x5f, +0x53, +0x54, +0x41, +0x0, +0xa4, +0x50, +0x53, +0x54, +0x41, +0xa, +0xb, 0x8, 0x5f, 0x53, @@ -532,7 +718,7 @@ static unsigned char ssdp_pcihp_aml[] = { 0xb, 0x5b, 0x82, -0x24, +0x32, 0x53, 0x30, 0x43, @@ -561,6 +747,20 @@ static unsigned char ssdp_pcihp_aml[] = { 0x4a, 0xa, 0xc, +0x14, +0xd, +0x5f, +0x53, +0x54, +0x41, +0x0, +0xa4, +0x50, +0x53, +0x54, +0x41, +0xa, +0xc, 0x8, 0x5f, 0x53, @@ -570,7 +770,7 @@ static unsigned char ssdp_pcihp_aml[] = { 0xc, 0x5b, 0x82, -0x24, +0x32, 0x53, 0x30, 0x44, @@ -599,6 +799,20 @@ static unsigned char ssdp_pcihp_aml[] = { 0x4a, 0xa, 0xd, +0x14, +0xd, +0x5f, +0x53, +0x54, +0x41, +0x0, +0xa4, +0x50, +0x53, +0x54, +0x41, +0xa, +0xd, 0x8, 0x5f, 0x53, @@ -608,7 +822,7 @@ static unsigned char ssdp_pcihp_aml[] = { 0xd, 0x5b, 0x82, -0x24, +0x32, 0x53, 0x30, 0x45, @@ -637,6 +851,20 @@ static unsigned char ssdp_pcihp_aml[] = { 0x4a, 0xa, 0xe, +0x14, +0xd, +0x5f, +0x53, +0x54, +0x41, +0x0, +0xa4, +0x50, +0x53, +0x54, +0x41, +0xa, +0xe, 0x8, 0x5f, 0x53, @@ -646,7 +874,7 @@ static unsigned char ssdp_pcihp_aml[] = { 0xe, 0x5b, 0x82, -0x24, +0x32, 0x53, 0x30, 0x46, @@ -675,6 +903,20 @@ static unsigned char ssdp_pcihp_aml[] = { 0x4a, 0xa, 0xf, +0x14, +0xd, +0x5f, +0x53, +0x54, +0x41, +0x0, +0xa4, +0x50, +0x53, +0x54, +0x41, +0xa, +0xf, 0x8, 0x5f, 0x53, @@ -684,7 +926,7 @@ static unsigned char ssdp_pcihp_aml[] = { 0xf, 0x5b, 0x82, -0x24, +0x32, 0x53, 0x31, 0x30, @@ -713,6 +955,20 @@ static unsigned char ssdp_pcihp_aml[] = { 0x4a, 0xa, 0x10, +0x14, +0xd, +0x5f, +0x53, +0x54, +0x41, +0x0, +0xa4, +0x50, +0x53, +0x54, +0x41, +0xa, +0x10, 0x8, 0x5f, 0x53, @@ -722,7 +978,7 @@ static unsigned char ssdp_pcihp_aml[] = { 0x10, 0x5b, 0x82, -0x24, +0x32, 0x53, 0x31, 0x31, @@ -751,6 +1007,20 @@ static unsigned char ssdp_pcihp_aml[] = { 0x4a, 0xa, 0x11, +0x14, +0xd, +0x5f, +0x53, +0x54, +0x41, +0x0, +0xa4, +0x50, +0x53, +0x54, +0x41, +0xa, +0x11, 0x8, 0x5f, 0x53, @@ -760,7 +1030,7 @@ static unsigned char ssdp_pcihp_aml[] = { 0x11, 0x5b, 0x82, -0x24, +0x32, 0x53, 0x31, 0x32, @@ -789,6 +1059,20 @@ static unsigned char ssdp_pcihp_aml[] = { 0x4a, 0xa, 0x12, +0x14, +0xd, +0x5f, +0x53, +0x54, +0x41, +0x0, +0xa4, +0x50, +0x53, +0x54, +0x41, +0xa, +0x12, 0x8, 0x5f, 0x53, @@ -798,7 +1082,7 @@ static unsigned char ssdp_pcihp_aml[] = { 0x12, 0x5b, 0x82, -0x24, +0x32, 0x53, 0x31, 0x33, @@ -827,6 +1111,20 @@ static unsigned char ssdp_pcihp_aml[] = { 0x4a, 0xa, 0x13, +0x14, +0xd, +0x5f, +0x53, +0x54, +0x41, +0x0, +0xa4, +0x50, +0x53, +0x54, +0x41, +0xa, +0x13, 0x8, 0x5f, 0x53, @@ -836,7 +1134,7 @@ static unsigned char ssdp_pcihp_aml[] = { 0x13, 0x5b, 0x82, -0x24, +0x32, 0x53, 0x31, 0x34, @@ -865,6 +1163,20 @@ static unsigned char ssdp_pcihp_aml[] = { 0x4a, 0xa, 0x14, +0x14, +0xd, +0x5f, +0x53, +0x54, +0x41, +0x0, +0xa4, +0x50, +0x53, +0x54, +0x41, +0xa, +0x14, 0x8, 0x5f, 0x53, @@ -874,7 +1186,7 @@ static unsigned char ssdp_pcihp_aml[] = { 0x14, 0x5b, 0x82, -0x24, +0x32, 0x53, 0x31, 0x35, @@ -903,6 +1215,20 @@ static unsigned char ssdp_pcihp_aml[] = { 0x4a, 0xa, 0x15, +0x14, +0xd, +0x5f, +0x53, +0x54, +0x41, +0x0, +0xa4, +0x50, +0x53, +0x54, +0x41, +0xa, +0x15, 0x8, 0x5f, 0x53, @@ -912,7 +1238,7 @@ static unsigned char ssdp_pcihp_aml[] = { 0x15, 0x5b, 0x82, -0x24, +0x32, 0x53, 0x31, 0x36, @@ -941,6 +1267,20 @@ static unsigned char ssdp_pcihp_aml[] = { 0x4a, 0xa, 0x16, +0x14, +0xd, +0x5f, +0x53, +0x54, +0x41, +0x0, +0xa4, +0x50, +0x53, +0x54, +0x41, +0xa, +0x16, 0x8, 0x5f, 0x53, @@ -950,7 +1290,7 @@ static unsigned char ssdp_pcihp_aml[] = { 0x16, 0x5b, 0x82, -0x24, +0x32, 0x53, 0x31, 0x37, @@ -979,6 +1319,20 @@ static unsigned char ssdp_pcihp_aml[] = { 0x4a, 0xa, 0x17, +0x14, +0xd, +0x5f, +0x53, +0x54, +0x41, +0x0, +0xa4, +0x50, +0x53, +0x54, +0x41, +0xa, +0x17, 0x8, 0x5f, 0x53, @@ -988,7 +1342,7 @@ static unsigned char ssdp_pcihp_aml[] = { 0x17, 0x5b, 0x82, -0x24, +0x32, 0x53, 0x31, 0x38, @@ -1017,6 +1371,20 @@ static unsigned char ssdp_pcihp_aml[] = { 0x4a, 0xa, 0x18, +0x14, +0xd, +0x5f, +0x53, +0x54, +0x41, +0x0, +0xa4, +0x50, +0x53, +0x54, +0x41, +0xa, +0x18, 0x8, 0x5f, 0x53, @@ -1026,7 +1394,7 @@ static unsigned char ssdp_pcihp_aml[] = { 0x18, 0x5b, 0x82, -0x24, +0x32, 0x53, 0x31, 0x39, @@ -1055,6 +1423,20 @@ static unsigned char ssdp_pcihp_aml[] = { 0x4a, 0xa, 0x19, +0x14, +0xd, +0x5f, +0x53, +0x54, +0x41, +0x0, +0xa4, +0x50, +0x53, +0x54, +0x41, +0xa, +0x19, 0x8, 0x5f, 0x53, @@ -1064,7 +1446,7 @@ static unsigned char ssdp_pcihp_aml[] = { 0x19, 0x5b, 0x82, -0x24, +0x32, 0x53, 0x31, 0x41, @@ -1093,6 +1475,20 @@ static unsigned char ssdp_pcihp_aml[] = { 0x4a, 0xa, 0x1a, +0x14, +0xd, +0x5f, +0x53, +0x54, +0x41, +0x0, +0xa4, +0x50, +0x53, +0x54, +0x41, +0xa, +0x1a, 0x8, 0x5f, 0x53, @@ -1102,7 +1498,7 @@ static unsigned char ssdp_pcihp_aml[] = { 0x1a, 0x5b, 0x82, -0x24, +0x32, 0x53, 0x31, 0x42, @@ -1131,6 +1527,20 @@ static unsigned char ssdp_pcihp_aml[] = { 0x4a, 0xa, 0x1b, +0x14, +0xd, +0x5f, +0x53, +0x54, +0x41, +0x0, +0xa4, +0x50, +0x53, +0x54, +0x41, +0xa, +0x1b, 0x8, 0x5f, 0x53, @@ -1140,7 +1550,7 @@ static unsigned char ssdp_pcihp_aml[] = { 0x1b, 0x5b, 0x82, -0x24, +0x32, 0x53, 0x31, 0x43, @@ -1169,6 +1579,20 @@ static unsigned char ssdp_pcihp_aml[] = { 0x4a, 0xa, 0x1c, +0x14, +0xd, +0x5f, +0x53, +0x54, +0x41, +0x0, +0xa4, +0x50, +0x53, +0x54, +0x41, +0xa, +0x1c, 0x8, 0x5f, 0x53, @@ -1178,7 +1602,7 @@ static unsigned char ssdp_pcihp_aml[] = { 0x1c, 0x5b, 0x82, -0x24, +0x32, 0x53, 0x31, 0x44, @@ -1207,6 +1631,20 @@ static unsigned char ssdp_pcihp_aml[] = { 0x4a, 0xa, 0x1d, +0x14, +0xd, +0x5f, +0x53, +0x54, +0x41, +0x0, +0xa4, +0x50, +0x53, +0x54, +0x41, +0xa, +0x1d, 0x8, 0x5f, 0x53, @@ -1216,7 +1654,7 @@ static unsigned char ssdp_pcihp_aml[] = { 0x1d, 0x5b, 0x82, -0x24, +0x32, 0x53, 0x31, 0x45, @@ -1245,6 +1683,20 @@ static unsigned char ssdp_pcihp_aml[] = { 0x4a, 0xa, 0x1e, +0x14, +0xd, +0x5f, +0x53, +0x54, +0x41, +0x0, +0xa4, +0x50, +0x53, +0x54, +0x41, +0xa, +0x1e, 0x8, 0x5f, 0x53, @@ -1254,7 +1706,7 @@ static unsigned char ssdp_pcihp_aml[] = { 0x1e, 0x5b, 0x82, -0x24, +0x32, 0x53, 0x31, 0x46, @@ -1283,6 +1735,20 @@ static unsigned char ssdp_pcihp_aml[] = { 0x4a, 0xa, 0x1f, +0x14, +0xd, +0x5f, +0x53, +0x54, +0x41, +0x0, +0xa4, +0x50, +0x53, +0x54, +0x41, +0xa, +0x1f, 0x8, 0x5f, 0x53,
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. The _STA method also writes the slot identifier to I/O port register 0xae00 as an acknowledgment of the hotplug request. Signed-off-by: Alex Williamson <alex.williamson@redhat.com> --- 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(-)