Patchwork seabios: acpi: Add _STA for PCI hotplug slots

login
register
mail settings
Submitter Alex Williamson
Date Feb. 24, 2012, 11:21 p.m.
Message ID <20120224231735.17761.31411.stgit@bling.home>
Download mbox | patch
Permalink /patch/142998/
State New
Headers show

Comments

Alex Williamson - Feb. 24, 2012, 11:21 p.m.
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(-)
Kevin O'Connor - March 4, 2012, 5:06 p.m.
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
Michael S. Tsirkin - March 4, 2012, 6:53 p.m.
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.
Alex Williamson - March 5, 2012, 3:30 a.m.
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
Michael S. Tsirkin - March 5, 2012, 5:15 a.m.
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
> 
>
Michael S. Tsirkin - March 5, 2012, 6:26 a.m.
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
Gleb Natapov - March 5, 2012, 10:39 a.m.
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.
Alex Williamson - March 5, 2012, 3:38 p.m.
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

Patch

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,