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