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

[Alex Williamson]

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 <address@hidden>
> 
> 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