[SeaBIOS] [PATCH] seabios: acpi: Add _STA for PCI hotplug slots
Michael S. Tsirkin
mst at redhat.com
Mon Mar 5 06:15:33 CET 2012
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 at 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
>
>
More information about the SeaBIOS
mailing list