[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] seabios: acpi: Add _STA for PCI hotplug slots
From: |
Alex Williamson |
Subject: |
Re: [Qemu-devel] [PATCH] seabios: acpi: Add _STA for PCI hotplug slots |
Date: |
Sun, 04 Mar 2012 20:30:00 -0700 |
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