Skip to contentSkip to conversation

acpi: add ssdt for cpi hotplug

RFC PATCH In review Michael S. Tsirkin 4 participants 12 messages Nov 1, 2011 – Nov 4, 2011

Subsystems (2)
lore
Share & tools
lore
#0 lore raw

RFC PATCH 1/1 acpi: add ssdt for cpi hotplug

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.

---
 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
Makefile +1 -1
index 540f1ea..ef8e15d 100644
@@ -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)
src/acpi-dsdt.dsl +0 -179

197 lines not on this page · raw message

src/acpi.c +4 -0
index a3770ae..97cc6dd 100644
@@ -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();
 
src/ssdt-pcihp.dsl new file +188 -0

188 lines not on this page · raw message

Signature · 1 line
1.7.5.53.gc233e
#1 lore raw
Quoted 9 lines · 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.

[...]

Quoted 19 lines
src/acpi-dsdt.dsl +0 -19
-            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,
-            }
-

[...]

Quoted 9 lines
src/acpi-dsdt.dsl +0 -9
-    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)
-        }

[...]

Quoted 6 lines
src/acpi-dsdt.dsl +0 -6
-        /* Methods called by bulk generated hotplug devices below */
-        Method (PCEJ, 1, NotSerialized) {
-            // _EJ0 method - eject callback
-            Store(ShiftLeft(1, Arg0), B0EJ)
-            Return (0x0)
-        }

[...]

Quoted 15 lines
src/acpi-dsdt.dsl +0 -15
-        /* 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)
-        }

[...]

src/acpi-dsdt.dsl DefinitionBlock ( · line 843

Quoted 4 lines
src/acpi-dsdt.dsl +0 -4
-        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

#2 lore raw
Quoted 70 lines · Kevin O'Connor
Quoted 9 lines · 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.

[...]

Quoted 19 lines
src/acpi-dsdt.dsl +0 -19
-            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,
-            }
-

[...]

Quoted 9 lines
src/acpi-dsdt.dsl +0 -9
-    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)
-        }

[...]

Quoted 6 lines
src/acpi-dsdt.dsl +0 -6
-        /* Methods called by bulk generated hotplug devices below */
-        Method (PCEJ, 1, NotSerialized) {
-            // _EJ0 method - eject callback
-            Store(ShiftLeft(1, Arg0), B0EJ)
-            Return (0x0)
-        }

[...]

Quoted 15 lines
src/acpi-dsdt.dsl +0 -15
-        /* 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)
-        }

[...]

src/acpi-dsdt.dsl DefinitionBlock ( · line 843

Quoted 4 lines
src/acpi-dsdt.dsl +0 -4
-        Method(_L01) {
-            // PCI hotplug event
-            Return(\_SB.PCI0.PCNF())
-        }
Quoted 4 lines

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?

Signature · 1 line
MST
#3 lore raw
Quoted 7 lines · Michael S. Tsirkin
Quoted 3 lines · Kevin O'Connor

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

#4 lore raw

At 11/03/2011 08:30 AM, Kevin O'Connor Write:

Quoted 5 lines · Michael S. Tsirkin
Quoted 4 lines · Kevin O'Connor
Quoted 3 lines

Can we leave these parts in the DSDT and only move the bulk generated stuff to the SSDT?

Quoted 8 lines
Quoted 3 lines

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

Quoted 17 lines

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
[email protected]
http://www.seabios.org/mailman/listinfo/seabios

#5 lore raw
Quoted 8 lines · Wen Congyang

At 11/03/2011 08:30 AM, Kevin O'Connor Write:

Quoted 4 lines

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?

Quoted 2 lines

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

#6 lore raw

At 11/03/2011 09:36 AM, Kevin O'Connor Write:

Quoted 11 lines · Wen Congyang
Quoted 8 lines

At 11/03/2011 08:30 AM, Kevin O'Connor Write:

Quoted 4 lines

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.

Quoted 5 lines

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?

Quoted 7 lines
Quoted 2 lines

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

Quoted 3 lines

-Kevin

#7 lore raw
In reply to Kevin O'Connor
Quoted 7 lines · Kevin O'Connor
Quoted 6 lines · Wen Congyang

At 11/03/2011 08:30 AM, Kevin O'Connor Write:

Quoted 4 lines

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.

Quoted 6 lines
Quoted 2 lines

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.

Signature · 1 line
MST
#8 lore raw
In reply to Kevin O'Connor
Quoted 14 lines · Kevin O'Connor
Quoted 7 lines · Michael S. Tsirkin
Quoted 3 lines · Kevin O'Connor

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.

Quoted 7 lines

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?

Quoted 1 line

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.

Quoted 1 line

-Kevin

#9 lore raw
Quoted 19 lines · Michael S. Tsirkin
Quoted 18 lines · Kevin O'Connor
Quoted 7 lines · Michael S. Tsirkin
Quoted 3 lines · Kevin O'Connor

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).

Quoted 5 lines
Quoted 3 lines

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,

Quoted 11 lines
Quoted 1 line

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.

Quoted 1 line

-Kevin

_______________________________________________
SeaBIOS mailing list
[email protected]
http://www.seabios.org/mailman/listinfo/seabios

Signature · 1 line
yamahata
#10 lore raw
In reply to Michael S. Tsirkin

At 11/02/2011 03:11 AM, Michael S. Tsirkin Write:

Quoted 10 lines

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 ]---
Quoted 2 lines
#11 lore raw

At 11/02/2011 02:08 PM, Wen Congyang Write:

Quoted 6 lines

At 11/02/2011 03:11 AM, Michael S. Tsirkin Write:

Quoted 5 lines

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.

Quoted 8 lines
Quoted 5 lines

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