Re: [Qemu-devel] [PATCH] seabios: acpi: Add _STA for PCI hotplug slots

["Michael S. Tsirkin" <mst@redhat.com>]

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.