[PATCH RFC] acpi: add ssdt for cpi hotplug

[Michael S. Tsirkin]

So here's the plan: move all hotplug handling out
to ssdt, this way it'll keep working even with a
user-supplied dsdt. Next step we can patch
this ssdt at runtime.

There's little point in this change alone, so posting as RFC,
will repost with the patching part when it's ready,
posting now to present opportunity for early feedback.

Compiled only.

Signed-off-by: Michael S. Tsirkin <mst(a)redhat.com>
---
 Makefile           |    2 +-
 src/acpi-dsdt.dsl  |  179 -------------------------------------------------
 src/acpi.c         |    4 +
 src/ssdt-pcihp.dsl |  188 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 193 insertions(+), 180 deletions(-)
 create mode 100644 src/ssdt-pcihp.dsl

diff --git a/Makefile b/Makefile
index 540f1ea..ef8e15d 100644
--- a/Makefile
+++ b/Makefile
@@ -200,7 +200,7 @@ src/%.hex: src/%.dsl ./tools/acpi_extract_preprocess.py
./tools/acpi_extract.py
 	$(Q)./tools/acpi_extract.py $(OUT)$*.lst > $(OUT)$*.off
 	$(Q)cat $(OUT)$*.off > $@
 
-$(OUT)ccode32flat.o: src/acpi-dsdt.hex src/ssdt-proc.hex
+$(OUT)ccode32flat.o: src/acpi-dsdt.hex src/ssdt-proc.hex src/ssdt-pcihp.hex
 
 ####### Kconfig rules
 export HOSTCC             := $(CC)
diff --git a/src/acpi-dsdt.dsl b/src/acpi-dsdt.dsl
index b9b06f2..32eaeee 100644
--- a/src/acpi-dsdt.dsl
+++ b/src/acpi-dsdt.dsl
@@ -119,25 +119,6 @@ DefinitionBlock (
                prt_slot3(0x001f),
             })
 
-            OperationRegion(PCST, SystemIO, 0xae00, 0x08)
-            Field (PCST, DWordAcc, NoLock, WriteAsZeros)
-            {
-                PCIU, 32,
-                PCID, 32,
-            }
-
-            OperationRegion(SEJ, SystemIO, 0xae08, 0x04)
-            Field (SEJ, DWordAcc, NoLock, WriteAsZeros)
-            {
-                B0EJ, 32,
-            }
-
-            OperationRegion(RMVC, SystemIO, 0xae0c, 0x04)
-            Field(RMVC, DWordAcc, NoLock, WriteAsZeros)
-            {
-                PCRM, 32,
-            }
-
             Name (_CRS, ResourceTemplate ()
             {
                 WordBusNumber (ResourceProducer, MinFixed, MaxFixed, PosDecode,
@@ -458,162 +439,6 @@ DefinitionBlock (
 	    }
 	}
     }
-
-
-/****************************************************************
- * PCI hotplug
- ****************************************************************/
-
-    Scope(\_SB.PCI0) {
-        /* Methods called by bulk generated PCI devices below */
-        Method (PRMV, 1, NotSerialized) {
-            // _RMV method - check if device can be removed
-            If (And(\_SB.PCI0.PCRM, ShiftLeft(1, Arg0))) {
-                Return (0x1)
-            }
-            Return (0x0)
-        }
-
-#define gen_pci_device(slot)                                    \
-        Device(SL##slot) {                                      \
-            Name (_ADR, 0x##slot##0000)                         \
-            Method (_RMV) { Return (PRMV(0x##slot)) }           \
-            Name (_SUN, 0x##slot)                               \
-        }
-
-        /* VGA (slot 1) and ISA bus (slot 2) defined above */
-        gen_pci_device(03)
-        gen_pci_device(04)
-        gen_pci_device(05)
-        gen_pci_device(06)
-        gen_pci_device(07)
-        gen_pci_device(08)
-        gen_pci_device(09)
-        gen_pci_device(0a)
-        gen_pci_device(0b)
-        gen_pci_device(0c)
-        gen_pci_device(0d)
-        gen_pci_device(0e)
-        gen_pci_device(0f)
-        gen_pci_device(10)
-        gen_pci_device(11)
-        gen_pci_device(12)
-        gen_pci_device(13)
-        gen_pci_device(14)
-        gen_pci_device(15)
-        gen_pci_device(16)
-        gen_pci_device(17)
-        gen_pci_device(18)
-        gen_pci_device(19)
-        gen_pci_device(1a)
-        gen_pci_device(1b)
-        gen_pci_device(1c)
-        gen_pci_device(1d)
-        gen_pci_device(1e)
-        gen_pci_device(1f)
-
-        /* Methods called by bulk generated hotplug devices below */
-        Method (PCEJ, 1, NotSerialized) {
-            // _EJ0 method - eject callback
-            Store(ShiftLeft(1, Arg0), B0EJ)
-            Return (0x0)
-        }
-
-        /* Bulk generated PCI hotplug devices */
-#define hotplug_slot(slot)                              \
-        Device (S##slot) {                              \
-           Name (_ADR, 0x##slot##0000)                  \
-           Method (_EJ0, 1) { Return(PCEJ(0x##slot)) }  \
-           Name (_SUN, 0x##slot)                        \
-        }
-
-        hotplug_slot(01)
-        hotplug_slot(02)
-        hotplug_slot(03)
-        hotplug_slot(04)
-        hotplug_slot(05)
-        hotplug_slot(06)
-        hotplug_slot(07)
-        hotplug_slot(08)
-        hotplug_slot(09)
-        hotplug_slot(0a)
-        hotplug_slot(0b)
-        hotplug_slot(0c)
-        hotplug_slot(0d)
-        hotplug_slot(0e)
-        hotplug_slot(0f)
-        hotplug_slot(10)
-        hotplug_slot(11)
-        hotplug_slot(12)
-        hotplug_slot(13)
-        hotplug_slot(14)
-        hotplug_slot(15)
-        hotplug_slot(16)
-        hotplug_slot(17)
-        hotplug_slot(18)
-        hotplug_slot(19)
-        hotplug_slot(1a)
-        hotplug_slot(1b)
-        hotplug_slot(1c)
-        hotplug_slot(1d)
-        hotplug_slot(1e)
-        hotplug_slot(1f)
-
-        /* PCI hotplug notify method */
-        Method(PCNF, 0) {
-            // Local0 = iterator
-            Store (Zero, Local0)
-            While (LLess(Local0, 31)) {
-                Increment(Local0)
-                If (And(PCIU, ShiftLeft(1, Local0))) {
-                    PCNT(Local0, 1)
-                }
-                If (And(PCID, ShiftLeft(1, Local0))) {
-                    PCNT(Local0, 3)
-                }
-            }
-            Return(One)
-        }
-
-#define gen_pci_hotplug(slot)   \
-            If (LEqual(Arg0, 0x##slot)) { Notify(S##slot, Arg1) }
-
-        Method(PCNT, 2) {
-            gen_pci_hotplug(01)
-            gen_pci_hotplug(02)
-            gen_pci_hotplug(03)
-            gen_pci_hotplug(04)
-            gen_pci_hotplug(05)
-            gen_pci_hotplug(06)
-            gen_pci_hotplug(07)
-            gen_pci_hotplug(08)
-            gen_pci_hotplug(09)
-            gen_pci_hotplug(0a)
-            gen_pci_hotplug(0b)
-            gen_pci_hotplug(0c)
-            gen_pci_hotplug(0d)
-            gen_pci_hotplug(0e)
-            gen_pci_hotplug(0f)
-            gen_pci_hotplug(10)
-            gen_pci_hotplug(11)
-            gen_pci_hotplug(12)
-            gen_pci_hotplug(13)
-            gen_pci_hotplug(14)
-            gen_pci_hotplug(15)
-            gen_pci_hotplug(16)
-            gen_pci_hotplug(17)
-            gen_pci_hotplug(18)
-            gen_pci_hotplug(19)
-            gen_pci_hotplug(1a)
-            gen_pci_hotplug(1b)
-            gen_pci_hotplug(1c)
-            gen_pci_hotplug(1d)
-            gen_pci_hotplug(1e)
-            gen_pci_hotplug(1f)
-        }
-    }
-
-
 /****************************************************************
  * PCI IRQs
  ****************************************************************/
@@ -837,10 +662,6 @@ DefinitionBlock (
         Method(_L00) {
             Return(0x01)
         }
-        Method(_L01) {
-            // PCI hotplug event
-            Return(\_SB.PCI0.PCNF())
-        }
         Method(_L02) {
             // CPU hotplug event
             Return(\_SB.PRSC())
diff --git a/src/acpi.c b/src/acpi.c
index a3770ae..97cc6dd 100644
--- a/src/acpi.c
+++ b/src/acpi.c
@@ -406,6 +406,7 @@ encodeLen(u8 *ssdt_ptr, int length, int bytes)
 #define SD_PROC (ssdp_proc_aml + *ssdt_proc_start)
 
 #define SSDT_SIGNATURE 0x54445353 // SSDT
+
 static void*
 build_ssdt(void)
 {
@@ -633,6 +634,8 @@ static const struct pci_device_id acpi_find_tbl[] = {
 
 struct rsdp_descriptor *RsdpAddr;
 
+#include "ssdt-pcihp.hex"
+
 #define MAX_ACPI_TABLES 20
 void
 acpi_bios_init(void)
@@ -664,6 +667,7 @@ acpi_bios_init(void)
     ACPI_INIT_TABLE(build_madt());
     ACPI_INIT_TABLE(build_hpet());
     ACPI_INIT_TABLE(build_srat());
+    ACPI_INIT_TABLE(ssdp_pcihp_aml);
 
     u16 i, external_tables = qemu_cfg_acpi_additional_tables();
 
diff --git a/src/ssdt-pcihp.dsl b/src/ssdt-pcihp.dsl
new file mode 100644
index 0000000..7ed7eef
--- /dev/null
+++ b/src/ssdt-pcihp.dsl
@@ -0,0 +1,188 @@
+ACPI_EXTRACT_ALL_CODE ssdp_pcihp_aml
+
+DefinitionBlock ("ssdt-pcihp.aml", "SSDT", 0x01, "BXPC",
"BXSSDTPCIHP", 0x1)
+{
+
+/****************************************************************
+ * PCI hotplug
+ ****************************************************************/
+
+    External (\_SB.PCI0, DeviceObj)
+
+    Scope(\_SB.PCI0) {
+            OperationRegion(PCST, SystemIO, 0xae00, 0x08)
+            Field (PCST, DWordAcc, NoLock, WriteAsZeros)
+            {
+                PCIU, 32,
+                PCID, 32,
+            }
+
+            OperationRegion(SEJ, SystemIO, 0xae08, 0x04)
+            Field (SEJ, DWordAcc, NoLock, WriteAsZeros)
+            {
+                B0EJ, 32,
+            }
+
+            OperationRegion(RMVC, SystemIO, 0xae0c, 0x04)
+            Field(RMVC, DWordAcc, NoLock, WriteAsZeros)
+            {
+                PCRM, 32,
+            }
+
+        /* Methods called by bulk generated PCI devices below */
+        Method (PRMV, 1, NotSerialized) {
+            // _RMV method - check if device can be removed
+            If (And(\_SB.PCI0.PCRM, ShiftLeft(1, Arg0))) {
+                Return (0x1)
+            }
+            Return (0x0)
+        }
+
+#define gen_pci_device(slot)                                    \
+        Device(SL##slot) {                                      \
+            Name (_ADR, 0x##slot##0000)                         \
+            Method (_RMV) { Return (PRMV(0x##slot)) }           \
+            Name (_SUN, 0x##slot)                               \
+        }
+
+        /* VGA (slot 1) and ISA bus (slot 2) defined in DSDT */
+        gen_pci_device(03)
+        gen_pci_device(04)
+        gen_pci_device(05)
+        gen_pci_device(06)
+        gen_pci_device(07)
+        gen_pci_device(08)
+        gen_pci_device(09)
+        gen_pci_device(0a)
+        gen_pci_device(0b)
+        gen_pci_device(0c)
+        gen_pci_device(0d)
+        gen_pci_device(0e)
+        gen_pci_device(0f)
+        gen_pci_device(10)
+        gen_pci_device(11)
+        gen_pci_device(12)
+        gen_pci_device(13)
+        gen_pci_device(14)
+        gen_pci_device(15)
+        gen_pci_device(16)
+        gen_pci_device(17)
+        gen_pci_device(18)
+        gen_pci_device(19)
+        gen_pci_device(1a)
+        gen_pci_device(1b)
+        gen_pci_device(1c)
+        gen_pci_device(1d)
+        gen_pci_device(1e)
+        gen_pci_device(1f)
+
+        /* Methods called by bulk generated hotplug devices below */
+        Method (PCEJ, 1, NotSerialized) {
+            // _EJ0 method - eject callback
+            Store(ShiftLeft(1, Arg0), B0EJ)
+            Return (0x0)
+        }
+
+        /* Bulk generated PCI hotplug devices */
+#define hotplug_slot(slot)                              \
+        Device (S##slot) {                              \
+           Name (_ADR, 0x##slot##0000)                  \
+           Method (_EJ0, 1) { Return(PCEJ(0x##slot)) }  \
+           Name (_SUN, 0x##slot)                        \
+        }
+
+        hotplug_slot(01)
+        hotplug_slot(02)
+        hotplug_slot(03)
+        hotplug_slot(04)
+        hotplug_slot(05)
+        hotplug_slot(06)
+        hotplug_slot(07)
+        hotplug_slot(08)
+        hotplug_slot(09)
+        hotplug_slot(0a)
+        hotplug_slot(0b)
+        hotplug_slot(0c)
+        hotplug_slot(0d)
+        hotplug_slot(0e)
+        hotplug_slot(0f)
+        hotplug_slot(10)
+        hotplug_slot(11)
+        hotplug_slot(12)
+        hotplug_slot(13)
+        hotplug_slot(14)
+        hotplug_slot(15)
+        hotplug_slot(16)
+        hotplug_slot(17)
+        hotplug_slot(18)
+        hotplug_slot(19)
+        hotplug_slot(1a)
+        hotplug_slot(1b)
+        hotplug_slot(1c)
+        hotplug_slot(1d)
+        hotplug_slot(1e)
+        hotplug_slot(1f)
+
+        /* PCI hotplug notify method */
+        Method(PCNF, 0) {
+            // Local0 = iterator
+            Store (Zero, Local0)
+            While (LLess(Local0, 31)) {
+                Increment(Local0)
+                If (And(PCIU, ShiftLeft(1, Local0))) {
+                    PCNT(Local0, 1)
+                }
+                If (And(PCID, ShiftLeft(1, Local0))) {
+                    PCNT(Local0, 3)
+                }
+            }
+            Return(One)
+        }
+
+#define gen_pci_hotplug(slot)   \
+            If (LEqual(Arg0, 0x##slot)) { Notify(S##slot, Arg1) }
+
+        Method(PCNT, 2) {
+            gen_pci_hotplug(01)
+            gen_pci_hotplug(02)
+            gen_pci_hotplug(03)
+            gen_pci_hotplug(04)
+            gen_pci_hotplug(05)
+            gen_pci_hotplug(06)
+            gen_pci_hotplug(07)
+            gen_pci_hotplug(08)
+            gen_pci_hotplug(09)
+            gen_pci_hotplug(0a)
+            gen_pci_hotplug(0b)
+            gen_pci_hotplug(0c)
+            gen_pci_hotplug(0d)
+            gen_pci_hotplug(0e)
+            gen_pci_hotplug(0f)
+            gen_pci_hotplug(10)
+            gen_pci_hotplug(11)
+            gen_pci_hotplug(12)
+            gen_pci_hotplug(13)
+            gen_pci_hotplug(14)
+            gen_pci_hotplug(15)
+            gen_pci_hotplug(16)
+            gen_pci_hotplug(17)
+            gen_pci_hotplug(18)
+            gen_pci_hotplug(19)
+            gen_pci_hotplug(1a)
+            gen_pci_hotplug(1b)
+            gen_pci_hotplug(1c)
+            gen_pci_hotplug(1d)
+            gen_pci_hotplug(1e)
+            gen_pci_hotplug(1f)
+        }
+    }
+
+    Scope (\_GPE)
+    {
+        Method(_L01) {
+            // PCI hotplug event
+            Return(\_SB.PCI0.PCNF())
+        }
+    }
+
+}
-- 
1.7.5.53.gc233e

[Kevin O'Connor]

On Tue, Nov 01, 2011 at 09:11:40PM +0200, Michael S. Tsirkin wrote:
  So here's the plan: move all hotplug handling out
 to ssdt, this way it'll keep working even with a
 user-supplied dsdt. Next step we can patch
 this ssdt at runtime.
 
 There's little point in this change alone, so posting as RFC,
 will repost with the patching part when it's ready,
 posting now to present opportunity for early feedback.
[...]
  -            OperationRegion(PCST, SystemIO, 0xae00,
0x08)
 -            Field (PCST, DWordAcc, NoLock, WriteAsZeros)
 -            {
 -                PCIU, 32,
 -                PCID, 32,
 -            }
 -
 -            OperationRegion(SEJ, SystemIO, 0xae08, 0x04)
 -            Field (SEJ, DWordAcc, NoLock, WriteAsZeros)
 -            {
 -                B0EJ, 32,
 -            }
 -
 -            OperationRegion(RMVC, SystemIO, 0xae0c, 0x04)
 -            Field(RMVC, DWordAcc, NoLock, WriteAsZeros)
 -            {
 -                PCRM, 32,
 -            }
 -
[...]
  -    Scope(\_SB.PCI0) {
 -        /* Methods called by bulk generated PCI devices below */
 -        Method (PRMV, 1, NotSerialized) {
 -            // _RMV method - check if device can be removed
 -            If (And(\_SB.PCI0.PCRM, ShiftLeft(1, Arg0))) {
 -                Return (0x1)
 -            }
 -            Return (0x0)
 -        }
[...]
  -        /* Methods called by bulk generated hotplug
devices below */
 -        Method (PCEJ, 1, NotSerialized) {
 -            // _EJ0 method - eject callback
 -            Store(ShiftLeft(1, Arg0), B0EJ)
 -            Return (0x0)
 -        }
[...]
  -        /* PCI hotplug notify method */
 -        Method(PCNF, 0) {
 -            // Local0 = iterator
 -            Store (Zero, Local0)
 -            While (LLess(Local0, 31)) {
 -                Increment(Local0)
 -                If (And(PCIU, ShiftLeft(1, Local0))) {
 -                    PCNT(Local0, 1)
 -                }
 -                If (And(PCID, ShiftLeft(1, Local0))) {
 -                    PCNT(Local0, 3)
 -                }
 -            }
 -            Return(One)
 -        }
[...]
  -        Method(_L01) {
 -            // PCI hotplug event
 -            Return(\_SB.PCI0.PCNF())
 -        }
Can we leave these parts in the DSDT and only move the bulk generated
stuff to the SSDT?

-Kevin

[Michael S. Tsirkin]

On Tue, Nov 01, 2011 at 06:59:01PM -0400, Kevin O'Connor wrote:
  On Tue, Nov 01, 2011 at 09:11:40PM +0200, Michael S.
Tsirkin wrote:
  So here's the plan: move all hotplug handling
out
 to ssdt, this way it'll keep working even with a
 user-supplied dsdt. Next step we can patch
 this ssdt at runtime.
 
 There's little point in this change alone, so posting as RFC,
 will repost with the patching part when it's ready,
 posting now to present opportunity for early feedback.
 
 [...]
  -            OperationRegion(PCST, SystemIO,
0xae00, 0x08)
 -            Field (PCST, DWordAcc, NoLock, WriteAsZeros)
 -            {
 -                PCIU, 32,
 -                PCID, 32,
 -            }
 -
 -            OperationRegion(SEJ, SystemIO, 0xae08, 0x04)
 -            Field (SEJ, DWordAcc, NoLock, WriteAsZeros)
 -            {
 -                B0EJ, 32,
 -            }
 -
 -            OperationRegion(RMVC, SystemIO, 0xae0c, 0x04)
 -            Field(RMVC, DWordAcc, NoLock, WriteAsZeros)
 -            {
 -                PCRM, 32,
 -            }
 -
 [...]
  -    Scope(\_SB.PCI0) {
 -        /* Methods called by bulk generated PCI devices below */
 -        Method (PRMV, 1, NotSerialized) {
 -            // _RMV method - check if device can be removed
 -            If (And(\_SB.PCI0.PCRM, ShiftLeft(1, Arg0))) {
 -                Return (0x1)
 -            }
 -            Return (0x0)
 -        }
 [...]
  -        /* Methods called by bulk generated
hotplug devices below */
 -        Method (PCEJ, 1, NotSerialized) {
 -            // _EJ0 method - eject callback
 -            Store(ShiftLeft(1, Arg0), B0EJ)
 -            Return (0x0)
 -        }
 [...]
  -        /* PCI hotplug notify method */
 -        Method(PCNF, 0) {
 -            // Local0 = iterator
 -            Store (Zero, Local0)
 -            While (LLess(Local0, 31)) {
 -                Increment(Local0)
 -                If (And(PCIU, ShiftLeft(1, Local0))) {
 -                    PCNT(Local0, 1)
 -                }
 -                If (And(PCID, ShiftLeft(1, Local0))) {
 -                    PCNT(Local0, 3)
 -                }
 -            }
 -            Return(One)
 -        }
 [...]
  -        Method(_L01) {
 -            // PCI hotplug event
 -            Return(\_SB.PCI0.PCNF())
 -        }
 
 Can we leave these parts in the DSDT and only move the bulk generated
 stuff to the SSDT?
 
 -Kevin
They can, but I thought one of the reasons we do the split
is to make it possible for users to supply their own DSDT?
If so creating calls from SSDT into DSDT would make this very fragile.

Two other things to note:
1. I didn't find a way to avoid such a dependency completely:
   there is a requirement for the DSDT to call the root bus PCI0.
2. The RMV will be removed from asl and move into BIOS
   for runtime patching.

What do you think?

-- 
MST

[Kevin O'Connor]

On Wed, Nov 02, 2011 at 10:54:42AM +0200, Michael S. Tsirkin wrote:
  On Tue, Nov 01, 2011 at 06:59:01PM -0400, Kevin
O'Connor wrote:
  Can we leave these parts in the DSDT and only
move the bulk generated
 stuff to the SSDT?
 They can, but I thought one of the reasons we do the split
 is to make it possible for users to supply their own DSDT?
 If so creating calls from SSDT into DSDT would make this very fragile.
I think it's reasonable to require that a user supplied DSDT still
fill certain requirements.  Keep in mind that the reason for the "user
supplied" DSDT was for new platform (q35) development - not
necessarily so each individual user could set their own.

It's not great to have the DSDT and SSDT tied to each other.  However,
once RMV is removed, it's only two methods (PCNT supplied by the SSDT
and PCEJ supplied by the DSDT).

I do see the upside to moving it all to the SSDT - but that has the
disadvantage of taking away the ability of a user supplied DSDT to
tweak how the hotplug functions work.  Also, we'd then want to redo
CPU hotplug to be the same way.

-Kevin

[Wen Congyang]

At 11/03/2011 08:30 AM, Kevin O'Connor Write:
  On Wed, Nov 02, 2011 at 10:54:42AM +0200, Michael S.
Tsirkin wrote:
  On Tue, Nov 01, 2011 at 06:59:01PM -0400, Kevin
O'Connor wrote:
  Can we leave these parts in the DSDT and only
move the bulk generated
 stuff to the SSDT?
 They can, but I thought one of the reasons we do the split
 is to make it possible for users to supply their own DSDT?
 If so creating calls from SSDT into DSDT would make this very fragile.
 
 I think it's reasonable to require that a user supplied DSDT still
 fill certain requirements.  Keep in mind that the reason for the "user
 supplied" DSDT was for new platform (q35) development - not
 necessarily so each individual user could set their own.
I do not think it's reasonable to require that a user supplied DSDT still
fill certain requiements.

I think we should not use default SSDT if we use user supplied DSDT.
If we use user supplied DSDT, we should use user supplied SSDT too.

Thanks
Wen Congyang

  
 It's not great to have the DSDT and SSDT tied to each other.  However,
 once RMV is removed, it's only two methods (PCNT supplied by the SSDT
 and PCEJ supplied by the DSDT).
 
 I do see the upside to moving it all to the SSDT - but that has the
 disadvantage of taking away the ability of a user supplied DSDT to
 tweak how the hotplug functions work.  Also, we'd then want to redo
 CPU hotplug to be the same way.
 
 -Kevin
 
 _______________________________________________
 SeaBIOS mailing list
 SeaBIOS(a)seabios.org
 http://www.seabios.org/mailman/listinfo/seabios

[Kevin O'Connor]

On Thu, Nov 03, 2011 at 09:04:57AM +0800, Wen Congyang wrote:
  At 11/03/2011 08:30 AM, Kevin O'Connor Write:
  I think it's reasonable to require that a
user supplied DSDT still
 fill certain requirements.  Keep in mind that the reason for the "user
 supplied" DSDT was for new platform (q35) development - not
 necessarily so each individual user could set their own.
 
 I do not think it's reasonable to require that a user supplied DSDT still
 fill certain requiements.
Please elaborate on why.

The dependency on the DSDT is a few methods (PCEJ, CPEJ, CPST, CPMA).
Under what circumstances would it be difficult for these methods to be
provided?

  I think we should not use default SSDT if we use user
supplied DSDT.
 If we use user supplied DSDT, we should use user supplied SSDT too.
That's possible.  However, how do you plan to provide a DSDT that
accounts for the dynamic nature of the virtual system configuration.
A different qemu/kvm command line (which configures pci or cpu count)
will then require a different DSDT.

-Kevin

[Wen Congyang]

At 11/03/2011 09:36 AM, Kevin O'Connor Write:
  On Thu, Nov 03, 2011 at 09:04:57AM +0800, Wen Congyang
wrote:
  At 11/03/2011 08:30 AM, Kevin O'Connor
Write:
  I think it's reasonable to require that a
user supplied DSDT still
 fill certain requirements.  Keep in mind that the reason for the "user
 supplied" DSDT was for new platform (q35) development - not
 necessarily so each individual user could set their own.
 I do not think it's reasonable to require that a user supplied DSDT still
 fill certain requiements.
 
 Please elaborate on why.
If we require it, I think it is very bad designed, because the user should
read seabios before he writes DSDT. I think the user should only need to
know how to pass his DSDT and SSDT to seabios. He should be able to write
his DSDT and SSDT without knowing seabios's DSDT and SSDT.

Another problem is that: if we require it, but the user does not do it,
I do not know the behavior. According to ACPI, SSDT can only add data,
and can not overwrite the data. So the user must know the data in SSDT.
If we update SSDT, and add new data that may exist in user supplied
DSDT, the behavior is undefined.

So I think we should not use default SSDT when we use user supplied DSDT.
If so, the user can write his DSDT easier.


  
 The dependency on the DSDT is a few methods (PCEJ, CPEJ, CPST, CPMA).
 Under what circumstances would it be difficult for these methods to be
 provided?
 
  I think we should not use default SSDT if we use
user supplied DSDT.
 If we use user supplied DSDT, we should use user supplied SSDT too.
 
 That's possible.  However, how do you plan to provide a DSDT that
 accounts for the dynamic nature of the virtual system configuration.
 A different qemu/kvm command line (which configures pci or cpu count)
 will then require a different DSDT.
Hmm, I do not think it now. I think DSDT and SSDT can be generated in qemu,
but it is very complex.

I have another question: is seabios used only by qemu now? Or it is a common
bios, and is not only for qemu?

Thanks
Wen Congyang

  
 -Kevin

[Michael S. Tsirkin]

On Wed, Nov 02, 2011 at 09:36:58PM -0400, Kevin O'Connor wrote:
  On Thu, Nov 03, 2011 at 09:04:57AM +0800, Wen Congyang
wrote:
  At 11/03/2011 08:30 AM, Kevin O'Connor
Write:
  I think it's reasonable to require that a
user supplied DSDT still
 fill certain requirements.  Keep in mind that the reason for the "user
 supplied" DSDT was for new platform (q35) development - not
 necessarily so each individual user could set their own.
 
 I do not think it's reasonable to require that a user supplied DSDT still
 fill certain requiements.
 
 Please elaborate on why.
 
 The dependency on the DSDT is a few methods (PCEJ, CPEJ, CPST, CPMA).
The one that seems unavoidable is PCI0.

-- 
MST

[Michael S. Tsirkin]

On Wed, Nov 02, 2011 at 08:30:28PM -0400, Kevin O'Connor wrote:
  On Wed, Nov 02, 2011 at 10:54:42AM +0200, Michael S.
Tsirkin wrote:
  On Tue, Nov 01, 2011 at 06:59:01PM -0400, Kevin
O'Connor wrote:
  Can we leave these parts in the DSDT and only
move the bulk generated
 stuff to the SSDT?
 They can, but I thought one of the reasons we do the split
 is to make it possible for users to supply their own DSDT?
 If so creating calls from SSDT into DSDT would make this very fragile.
 
 I think it's reasonable to require that a user supplied DSDT still
 fill certain requirements.  Keep in mind that the reason for the "user
 supplied" DSDT was for new platform (q35) development - not
 necessarily so each individual user could set their own.
 
 It's not great to have the DSDT and SSDT tied to each other.  However,
 once RMV is removed, it's only two methods (PCNT supplied by the SSDT
 and PCEJ supplied by the DSDT).
 
 I do see the upside to moving it all to the SSDT - but that has the
 disadvantage of taking away the ability of a user supplied DSDT to
 tweak how the hotplug functions work.
Is this something q35 needs?

   Also, we'd then want to redo CPU hotplug to be
the same way.
OK, I'll do that if we decide this is a good direcction.

  -Kevin

[Isaku Yamahata]

On Thu, Nov 03, 2011 at 02:31:18PM +0200, Michael S. Tsirkin wrote:
  On Wed, Nov 02, 2011 at 08:30:28PM -0400, Kevin
O'Connor wrote:
  On Wed, Nov 02, 2011 at 10:54:42AM +0200, Michael
S. Tsirkin wrote:
  On Tue, Nov 01, 2011 at 06:59:01PM -0400, Kevin
O'Connor wrote:
  Can we leave these parts in the DSDT and only
move the bulk generated
 stuff to the SSDT?
 They can, but I thought one of the reasons we do the split
 is to make it possible for users to supply their own DSDT?
 If so creating calls from SSDT into DSDT would make this very fragile.
 
 I think it's reasonable to require that a user supplied DSDT still
 fill certain requirements.  Keep in mind that the reason for the "user
 supplied" DSDT was for new platform (q35) development - not
 necessarily so each individual user could set their own.
 
 It's not great to have the DSDT and SSDT tied to each other.  However,
 once RMV is removed, it's only two methods (PCNT supplied by the SSDT
 and PCEJ supplied by the DSDT).
 
 I do see the upside to moving it all to the SSDT - but that has the
 disadvantage of taking away the ability of a user supplied DSDT to
 tweak how the hotplug functions work.
 
 Is this something q35 needs?
q35 emulator will discard qemu-specific pci hotplug(hw/acpi_piix4.c).
Instead, pcie hotplug is introduced. (that's my plan at least.)
So pci hotplug code in acpi (DSDT or SSDT) for acpi_piix4 needs to be
suppressed somehow.

thanks,

   Also,
we'd then want to redo CPU hotplug to be the same way.
 
 OK, I'll do that if we decide this is a good direcction.
 
  -Kevin
 
 _______________________________________________
 SeaBIOS mailing list
 SeaBIOS(a)seabios.org
 http://www.seabios.org/mailman/listinfo/seabios
-- 
yamahata

[Wen Congyang]

At 11/02/2011 03:11 AM, Michael S. Tsirkin Write:
  So here's the plan: move all hotplug handling out
 to ssdt, this way it'll keep working even with a
 user-supplied dsdt. Next step we can patch
 this ssdt at runtime.
 
 There's little point in this change alone, so posting as RFC,
 will repost with the patching part when it's ready,
 posting now to present opportunity for early feedback.
 
 Compiled only.
Hot plug on PCI bus 0 can not work with this patch.
It can works without this patch.

I have met the following warning messages when hot removing
a PCI device:

WARNING: at
/builddir/build/BUILD/kernel-2.6.32-195.el6/linux-2.6.32-195.el6.i686/arch/x86/include/asm/dma-mapping.h:154
___free_dma_mem_cluster+0xe1/0xf0 [sym53c8xx]() (Tainted
: G        W  ----------------  )
Hardware name: Bochs
Modules linked in: sym53c8xx scsi_transport_spi autofs4 sunrpc ipt_REJECT
nf_conntrack_ipv4 nf_defrag_ipv4 iptable_filter ip_tables ip6t_REJECT nf_conntrack_ipv6
nf_defrag_ipv6 xt_state nf_conntrack ip6table_filter ip6_tables ipv6 dm_mirror
dm_region_hash dm_log virtio_net i6300esb virtio_balloon 8139too 8139cp mii i2c_piix4
i2c_core sg ext4 mbcache jbd2 virtio_blk sr_mod cdrom sd_mod crc_t10dif virtio_pci
virtio_ring virtio pata_acpi ata_generic ata_piix dm_mod [last unloaded: mperf]
Pid: 39, comm: kacpi_notify Tainted: G        W  ----------------   2.6.32-195.el6.i686
#1
Call Trace:
 [<c0454b11>] ? warn_slowpath_common+0x81/0xc0
 [<e13bb961>] ? ___free_dma_mem_cluster+0xe1/0xf0 [sym53c8xx]
 [<e13bb961>] ? ___free_dma_mem_cluster+0xe1/0xf0 [sym53c8xx]
 [<c0454b6b>] ? warn_slowpath_null+0x1b/0x20
 [<e13bb961>] ? ___free_dma_mem_cluster+0xe1/0xf0 [sym53c8xx]
 [<e13bb764>] ? __sym_mfree+0xa4/0xf0 [sym53c8xx]
 [<e13bb806>] ? __sym_mfree_dma+0x56/0xd0 [sym53c8xx]
 [<e13b288f>] ? sym_free_resources+0x4f/0x70 [sym53c8xx]
 [<e13b2947>] ? sym_detach+0x97/0xc0 [sym53c8xx]
 [<e13b2994>] ? sym2_remove+0x24/0x50 [sym53c8xx]
 [<c0610a36>] ? pci_device_remove+0x16/0x40
 [<c06b9c8d>] ? __device_release_driver+0x4d/0xb0
 [<c06b9d9d>] ? device_release_driver+0x1d/0x30
 [<c06b8e5c>] ? bus_remove_device+0x7c/0xe0
 [<c06b6f6f>] ? device_del+0xdf/0x160
 [<c06b7009>] ? device_unregister+0x19/0x60
 [<c06471f9>] ? acpi_os_execute_deferred+0x0/0x23
 [<c060afc6>] ? pci_stop_bus_device+0x66/0x80
 [<c061fee0>] ? acpiphp_disable_slot+0x80/0x1a0
 [<c06471f9>] ? acpi_os_execute_deferred+0x0/0x23
 [<c0620649>] ? handle_hotplug_event_func+0xb9/0x180
 [<c047b78b>] ? up+0xb/0x40
 [<c064711d>] ? acpi_os_signal_semaphore+0x1e/0x23
 [<c0665c14>] ? acpi_ut_release_mutex+0x50/0x53
 [<c065cd5e>] ? acpi_get_data+0x4a/0x58
 [<c06471f9>] ? acpi_os_execute_deferred+0x0/0x23
 [<c064931e>] ? acpi_bus_get_device+0x18/0x2c
 [<c065481b>] ? acpi_ev_notify_dispatch+0x4c/0x55
 [<c0647213>] ? acpi_os_execute_deferred+0x1a/0x23
 [<c047181b>] ? worker_thread+0x11b/0x230
 [<c0475f50>] ? autoremove_wake_function+0x0/0x40
 [<c0471700>] ? worker_thread+0x0/0x230
 [<c0475d14>] ? kthread+0x74/0x80
 [<c0475ca0>] ? kthread+0x0/0x80
 [<c0409fff>] ? kernel_thread_helper+0x7/0x10
---[ end trace 1261f0bed0518ca9 ]---


  
 Signed-off-by: Michael S. Tsirkin &lt;mst(a)redhat.com&gt;

[Wen Congyang]

At 11/02/2011 02:08 PM, Wen Congyang Write:
  At 11/02/2011 03:11 AM, Michael S. Tsirkin Write:
  So here's the plan: move all hotplug handling
out
 to ssdt, this way it'll keep working even with a
 user-supplied dsdt. Next step we can patch
 this ssdt at runtime.

 There's little point in this change alone, so posting as RFC,
 will repost with the patching part when it's ready,
 posting now to present opportunity for early feedback.

 Compiled only.
 
 Hot plug on PCI bus 0 can not work with this patch.
 It can works without this patch.
I think that I may find the reason:
================================
# ls -l /sys/firmware/acpi/tables/
total 0
-r--------. 1 root root 0 Nov  2 14:23 APIC
-r--------. 1 root root 0 Nov  2 14:23 DSDT
drwxr-xr-x. 2 root root 0 Nov  2 14:23 dynamic
-r--------. 1 root root 0 Nov  2 14:23 FACP
-r--------. 1 root root 0 Nov  2 14:23 FACS
-r--------. 1 root root 0 Nov  2 14:23 HPET
-r--------. 1 root root 0 Nov  2 14:23 SSDT
# ls -l /sys/firmware/acpi/tables/dynamic/
total 0
================================

The kernel only loads one SSDT. I use iasl to look at the
content of SSDT, and find that it is ssdt-proc.dsl.

The kernel does not load ssdt-pcihp.dsl.

Thanks
Wen Congyang