Re: [SeaBIOS] [PATCH] seabios: acpi: Add _STA for PCI hotplug slots
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. ___ SeaBIOS mailing list SeaBIOS@seabios.org http://www.seabios.org/mailman/listinfo/seabios
Re: [SeaBIOS] [PATCH] seabios: acpi: Add _STA for PCI hotplug slots
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.william...@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)
}
Re: [SeaBIOS] [PATCH] seabios: acpi: Add _STA for PCI hotplug slots
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.william...@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
Re: [SeaBIOS] [PATCH] seabios: acpi: Add _STA for PCI hotplug slots
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.william...@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) +
Re: [SeaBIOS] [PATCH] seabios: acpi: Add _STA for PCI hotplug slots
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 -- MST ___ SeaBIOS mailing list SeaBIOS@seabios.org http://www.seabios.org/mailman/listinfo/seabios
[SeaBIOS] [PATCH] seabios: acpi: Add _STA for PCI hotplug slots
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.william...@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(-)
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
