[PATCH v2 0/6] Build PCI hotplug SSDT from a single template
More than 1kb of data is taken by the 32 copies of the PCI hotplug SSDT methods. We can build them from a single template like we do for CPUs. This series does exactly this. Patches 1 prepares for the change, by moving other pieces of ssdt-pcihd.dsl out of the way. Patch 2 is also a simple rename and patch 3 fixes a bug in acpi_extract. Patches 4 to 6 finally do the movement. v1->v2: document computation of length (patch 1, Igor) build PCNT dynamically (Kevin) Paolo Bonzini (6): acpi: move s3/s4/s5 to build_ssdt acpi: rename Processor SSDT constants acpi_extract: fix off-by-one acpi_extract: detect DeviceOp acpi: build PCNT dynamically acpi: build PCI hotplug devices from a single template Makefile | 2 +- src/acpi.c | 218 +++++++++++++++++++++++++++---------------------- src/ssdt-pcihp.dsl | 124 +++------------------------- src/ssdt-susp.dsl | 41 ++++++++++ tools/acpi_extract.py | 30 ++++++- 5 files changed, 203 insertions(+), 212 deletions(-) create mode 100644 src/ssdt-susp.dsl -- 1.7.10.4
Move the _S3/_S4/_S5 packages out of ssdt-pcihp.dsl and into a separate file. Correspondingly, move the patching from build_pcihp to build_ssdt. Place this part at the beginning of the SSDT. Offset computation is a bit simpler, and anyway the packages do not need to be inside Scope(_SB). Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> --- Makefile | 2 +- src/acpi.c | 50 ++++++++++++++++++++++++++++---------------------- src/ssdt-pcihp.dsl | 36 ------------------------------------ src/ssdt-susp.dsl | 41 +++++++++++++++++++++++++++++++++++++++++ 4 files changed, 70 insertions(+), 59 deletions(-) create mode 100644 src/ssdt-susp.dsl diff --git a/Makefile b/Makefile index dfdec5c..1167799 100644 --- a/Makefile +++ b/Makefile @@ -229,7 +229,7 @@ $(OUT)%.hex: src/%.dsl ./tools/acpi_extract_preprocess.py ./tools/acpi_extract.p $(Q)$(PYTHON) ./tools/acpi_extract.py $(OUT)$*.lst > $(OUT)$*.off $(Q)cat $(OUT)$*.off > $@ -$(OUT)ccode32flat.o: $(OUT)acpi-dsdt.hex $(OUT)ssdt-proc.hex $(OUT)ssdt-pcihp.hex +$(OUT)ccode32flat.o: $(OUT)acpi-dsdt.hex $(OUT)ssdt-proc.hex $(OUT)ssdt-pcihp.hex $(OUT)ssdt-susp.hex ################ Kconfig rules diff --git a/src/acpi.c b/src/acpi.c index 39b7172..641781f 100644 --- a/src/acpi.c +++ b/src/acpi.c @@ -408,26 +408,45 @@ encodeLen(u8 *ssdt_ptr, int length, int bytes) #define SSDT_SIGNATURE 0x54445353 // SSDT +#define SSDT_HEADER_LENGTH 36 + +#include "ssdt-susp.hex" + static void* build_ssdt(void) { int acpi_cpus = MaxCountCPUs > 0xff ? 0xff : MaxCountCPUs; - // length = ScopeOp + procs + NTYF method + CPON package - int length = ((1+3+4) - + (acpi_cpus * SD_SIZEOF) - + (1+2+5+(12*acpi_cpus)) - + (6+2+1+(1*acpi_cpus)) - + 17); - u8 *ssdt = malloc_high(sizeof(struct acpi_table_header) + length); + int length = (sizeof(ssdp_susp_aml) // _S3_ / _S4_ / _S5_ + + (1+3+4) // Scope(_SB_) + + (acpi_cpus * SD_SIZEOF) // procs + + (1+2+5+(12*acpi_cpus)) // NTFY + + (6+2+1+(1*acpi_cpus)) // CPON + + 17); // BDAT + u8 *ssdt = malloc_high(length); if (! ssdt) { warn_noalloc(); return NULL; } - u8 *ssdt_ptr = ssdt + sizeof(struct acpi_table_header); + u8 *ssdt_ptr = ssdt; + + // Copy header and encode fwcfg values in the S3_ / S4_ / S5_ packages + int sys_state_size; + char *sys_states = romfile_loadfile("etc/system-states", &sys_state_size); + if (!sys_states || sys_state_size != 6) + sys_states = (char[]){128, 0, 0, 129, 128, 128}; + + memcpy(ssdt_ptr, ssdp_susp_aml, sizeof(ssdp_susp_aml)); + if (!(sys_states[3] & 128)) + ssdt_ptr[acpi_s3_name[0]] = 'X'; + if (!(sys_states[4] & 128)) + ssdt_ptr[acpi_s4_name[0]] = 'X'; + else + ssdt_ptr[acpi_s4_pkg[0] + 1] = ssdt[acpi_s4_pkg[0] + 3] = sys_states[4] & 127; + ssdt_ptr += sizeof(ssdp_susp_aml); // build Scope(_SB_) header *(ssdt_ptr++) = 0x10; // ScopeOp - ssdt_ptr = encodeLen(ssdt_ptr, length-1, 3); + ssdt_ptr = encodeLen(ssdt_ptr, length - (ssdt_ptr - ssdt), 3); *(ssdt_ptr++) = '_'; *(ssdt_ptr++) = 'S'; *(ssdt_ptr++) = 'B'; @@ -520,8 +539,6 @@ extern void link_time_assertion(void); static void* build_pcihp(void) { - char *sys_states; - int sys_state_size; u32 rmvc_pcrm; int i; @@ -553,19 +570,8 @@ static void* build_pcihp(void) } } - sys_states = romfile_loadfile("etc/system-states", &sys_state_size); - if (!sys_states || sys_state_size != 6) - sys_states = (char[]){128, 0, 0, 129, 128, 128}; - - if (!(sys_states[3] & 128)) - ssdt[acpi_s3_name[0]] = 'X'; - if (!(sys_states[4] & 128)) - ssdt[acpi_s4_name[0]] = 'X'; - else - ssdt[acpi_s4_pkg[0] + 1] = ssdt[acpi_s4_pkg[0] + 3] = sys_states[4] & 127; ((struct acpi_table_header*)ssdt)->checksum = 0; ((struct acpi_table_header*)ssdt)->checksum -= checksum(ssdt, sizeof(ssdp_pcihp_aml)); - return ssdt; } diff --git a/src/ssdt-pcihp.dsl b/src/ssdt-pcihp.dsl index 12555e2..4b435b8 100644 --- a/src/ssdt-pcihp.dsl +++ b/src/ssdt-pcihp.dsl @@ -95,40 +95,4 @@ DefinitionBlock ("ssdt-pcihp.aml", "SSDT", 0x01, "BXPC", "BXSSDTPCIHP", 0x1) gen_pci_hotplug(1f) } } - - Scope(\) { -/**************************************************************** - * Suspend - ****************************************************************/ - - /* - * S3 (suspend-to-ram), S4 (suspend-to-disk) and S5 (power-off) type codes: - * must match piix4 emulation. - */ - - ACPI_EXTRACT_NAME_STRING acpi_s3_name - Name (_S3, Package (0x04) - { - One, /* PM1a_CNT.SLP_TYP */ - One, /* PM1b_CNT.SLP_TYP */ - Zero, /* reserved */ - Zero /* reserved */ - }) - ACPI_EXTRACT_NAME_STRING acpi_s4_name - ACPI_EXTRACT_PKG_START acpi_s4_pkg - Name (_S4, Package (0x04) - { - 0x2, /* PM1a_CNT.SLP_TYP */ - 0x2, /* PM1b_CNT.SLP_TYP */ - Zero, /* reserved */ - Zero /* reserved */ - }) - Name (_S5, Package (0x04) - { - Zero, /* PM1a_CNT.SLP_TYP */ - Zero, /* PM1b_CNT.SLP_TYP */ - Zero, /* reserved */ - Zero /* reserved */ - }) - } } diff --git a/src/ssdt-susp.dsl b/src/ssdt-susp.dsl new file mode 100644 index 0000000..0b3fa08 --- /dev/null +++ b/src/ssdt-susp.dsl @@ -0,0 +1,41 @@ +ACPI_EXTRACT_ALL_CODE ssdp_susp_aml + +DefinitionBlock ("ssdt-susp.aml", "SSDT", 0x01, "BXPC", "BXSSDTSUSP", 0x1) +{ + +/**************************************************************** + * Suspend + ****************************************************************/ + + Scope(\) { + /* + * S3 (suspend-to-ram), S4 (suspend-to-disk) and S5 (power-off) type codes: + * must match piix4 emulation. + */ + + ACPI_EXTRACT_NAME_STRING acpi_s3_name + Name (_S3, Package (0x04) + { + One, /* PM1a_CNT.SLP_TYP */ + One, /* PM1b_CNT.SLP_TYP */ + Zero, /* reserved */ + Zero /* reserved */ + }) + ACPI_EXTRACT_NAME_STRING acpi_s4_name + ACPI_EXTRACT_PKG_START acpi_s4_pkg + Name (_S4, Package (0x04) + { + 0x2, /* PM1a_CNT.SLP_TYP */ + 0x2, /* PM1b_CNT.SLP_TYP */ + Zero, /* reserved */ + Zero /* reserved */ + }) + Name (_S5, Package (0x04) + { + Zero, /* PM1a_CNT.SLP_TYP */ + Zero, /* PM1b_CNT.SLP_TYP */ + Zero, /* reserved */ + Zero /* reserved */ + }) + } +} -- 1.7.10.4
A simple rename, to prepare for the later introduction of similar constants for PCI slots. Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> --- src/acpi.c | 25 ++++++++++++------------- 1 file changed, 12 insertions(+), 13 deletions(-) diff --git a/src/acpi.c b/src/acpi.c index 641781f..7d22d22 100644 --- a/src/acpi.c +++ b/src/acpi.c @@ -400,14 +400,13 @@ encodeLen(u8 *ssdt_ptr, int length, int bytes) #include "ssdt-proc.hex" /* 0x5B 0x83 ProcessorOp PkgLength NameString ProcID */ -#define SD_OFFSET_CPUHEX (*ssdt_proc_name - *ssdt_proc_start + 2) -#define SD_OFFSET_CPUID1 (*ssdt_proc_name - *ssdt_proc_start + 4) -#define SD_OFFSET_CPUID2 (*ssdt_proc_id - *ssdt_proc_start) -#define SD_SIZEOF (*ssdt_proc_end - *ssdt_proc_start) -#define SD_PROC (ssdp_proc_aml + *ssdt_proc_start) +#define PROC_OFFSET_CPUHEX (*ssdt_proc_name - *ssdt_proc_start + 2) +#define PROC_OFFSET_CPUID1 (*ssdt_proc_name - *ssdt_proc_start + 4) +#define PROC_OFFSET_CPUID2 (*ssdt_proc_id - *ssdt_proc_start) +#define PROC_SIZEOF (*ssdt_proc_end - *ssdt_proc_start) +#define PROC_AML (ssdp_proc_aml + *ssdt_proc_start) #define SSDT_SIGNATURE 0x54445353 // SSDT - #define SSDT_HEADER_LENGTH 36 #include "ssdt-susp.hex" @@ -418,7 +417,7 @@ build_ssdt(void) int acpi_cpus = MaxCountCPUs > 0xff ? 0xff : MaxCountCPUs; int length = (sizeof(ssdp_susp_aml) // _S3_ / _S4_ / _S5_ + (1+3+4) // Scope(_SB_) - + (acpi_cpus * SD_SIZEOF) // procs + + (acpi_cpus * PROC_SIZEOF) // procs + (1+2+5+(12*acpi_cpus)) // NTFY + (6+2+1+(1*acpi_cpus)) // CPON + 17); // BDAT @@ -455,12 +454,12 @@ build_ssdt(void) // build Processor object for each processor int i; for (i=0; i<acpi_cpus; i++) { - memcpy(ssdt_ptr, SD_PROC, SD_SIZEOF); - ssdt_ptr[SD_OFFSET_CPUHEX] = getHex(i >> 4); - ssdt_ptr[SD_OFFSET_CPUHEX+1] = getHex(i); - ssdt_ptr[SD_OFFSET_CPUID1] = i; - ssdt_ptr[SD_OFFSET_CPUID2] = i; - ssdt_ptr += SD_SIZEOF; + memcpy(ssdt_ptr, PROC_AML, PROC_SIZEOF); + ssdt_ptr[PROC_OFFSET_CPUHEX] = getHex(i >> 4); + ssdt_ptr[PROC_OFFSET_CPUHEX+1] = getHex(i); + ssdt_ptr[PROC_OFFSET_CPUID1] = i; + ssdt_ptr[PROC_OFFSET_CPUID2] = i; + ssdt_ptr += PROC_SIZEOF; } // build "Method(NTFY, 2) {If (LEqual(Arg0, 0x00)) {Notify(CP00, Arg1)} ...}" -- 1.7.10.4
Single-byte package length values do use bits 4-5, and this will happen with the PCI hotplug devices. pkglenbytes has not yet been decremented, so multi-byte values are detected with pkglenbytes > 1. Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> --- tools/acpi_extract.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tools/acpi_extract.py b/tools/acpi_extract.py index 167a322..81fa4aa 100755 --- a/tools/acpi_extract.py +++ b/tools/acpi_extract.py @@ -93,7 +93,7 @@ def aml_pkglen(offset): pkglenbytes = aml_pkglen_bytes(offset) pkglen = aml[offset] & 0x3F # If multibyte, first nibble only uses bits 0-3 - if ((pkglenbytes > 0) and (pkglen & 0x30)): + if ((pkglenbytes > 1) and (pkglen & 0x30)): die("PkgLen bytes 0x%x but first nibble 0x%x expected 0x0X" % (pkglen, pkglen)) offset += 1 -- 1.7.10.4
On Thu, Aug 02, 2012 at 03:07:23PM +0200, Paolo Bonzini wrote:
Single-byte package length values do use bits 4-5, and this will happen with the PCI hotplug devices. pkglenbytes has not yet been decremented, so multi-byte values are detected with pkglenbytes > 1.
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Acked-by: Michael S. Tsirkin <mst@redhat.com>
--- tools/acpi_extract.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tools/acpi_extract.py b/tools/acpi_extract.py index 167a322..81fa4aa 100755 --- a/tools/acpi_extract.py +++ b/tools/acpi_extract.py @@ -93,7 +93,7 @@ def aml_pkglen(offset): pkglenbytes = aml_pkglen_bytes(offset) pkglen = aml[offset] & 0x3F # If multibyte, first nibble only uses bits 0-3 - if ((pkglenbytes > 0) and (pkglen & 0x30)): + if ((pkglenbytes > 1) and (pkglen & 0x30)): die("PkgLen bytes 0x%x but first nibble 0x%x expected 0x0X" % (pkglen, pkglen)) offset += 1 -- 1.7.10.4
The DeviceOp AML opcode has more or less the same structure as ProcessorOp, except that there is no processor ID. Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> --- tools/acpi_extract.py | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/tools/acpi_extract.py b/tools/acpi_extract.py index 81fa4aa..3295678 100755 --- a/tools/acpi_extract.py +++ b/tools/acpi_extract.py @@ -164,6 +164,28 @@ def aml_name_word_const(offset): def aml_name_byte_const(offset): return aml_data_byte_const(aml_name_string(offset) + 4) +def aml_device_start(offset): + #0x5B 0x82 DeviceOp PkgLength NameString + if ((aml[offset] != 0x5B) or (aml[offset + 1] != 0x82)): + die( "Name offset 0x%x: expected 0x5B 0x82 actual 0x%x 0x%x" % + (offset, aml[offset], aml[offset + 1])); + return offset + +def aml_device_string(offset): + #0x5B 0x82 DeviceOp PkgLength NameString + start = aml_device_start(offset) + offset += 2 + pkglenbytes = aml_pkglen_bytes(offset) + offset += pkglenbytes + return offset + +def aml_device_end(offset): + start = aml_device_start(offset) + offset += 2 + pkglenbytes = aml_pkglen_bytes(offset) + pkglen = aml_pkglen(offset) + return offset + pkglen + def aml_processor_start(offset): #0x5B 0x83 ProcessorOp PkgLength NameString ProcID if ((aml[offset] != 0x5B) or (aml[offset + 1] != 0x83)): @@ -271,6 +293,12 @@ for i in range(len(asl)): offset = aml_name_string(offset) elif (directive == "ACPI_EXTRACT_METHOD_STRING"): offset = aml_method_string(offset) + elif (directive == "ACPI_EXTRACT_DEVICE_START"): + offset = aml_device_start(offset) + elif (directive == "ACPI_EXTRACT_DEVICE_STRING"): + offset = aml_device_string(offset) + elif (directive == "ACPI_EXTRACT_DEVICE_END"): + offset = aml_device_end(offset) elif (directive == "ACPI_EXTRACT_PROCESSOR_START"): offset = aml_processor_start(offset) elif (directive == "ACPI_EXTRACT_PROCESSOR_STRING"): -- 1.7.10.4
We already build the CPU notification method in src/acpi.c, and we can reuse most of the code for PCI hotplug. Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> --- src/acpi.c | 68 +++++++++++++++++++++++++++++++++++----------------- src/ssdt-pcihp.dsl | 37 ---------------------------- 2 files changed, 46 insertions(+), 59 deletions(-) diff --git a/src/acpi.c b/src/acpi.c index 7d22d22..38b72fb 100644 --- a/src/acpi.c +++ b/src/acpi.c @@ -406,11 +406,43 @@ encodeLen(u8 *ssdt_ptr, int length, int bytes) #define PROC_SIZEOF (*ssdt_proc_end - *ssdt_proc_start) #define PROC_AML (ssdp_proc_aml + *ssdt_proc_start) +#define PCI_SLOTS 32 + #define SSDT_SIGNATURE 0x54445353 // SSDT #define SSDT_HEADER_LENGTH 36 #include "ssdt-susp.hex" +static u8* +build_notify(u8 *ssdt_ptr, const char *name, int skip, int count, + const char *target, int ofs) +{ + count -= skip; + + *(ssdt_ptr++) = 0x14; // MethodOp + ssdt_ptr = encodeLen(ssdt_ptr, 2+5+(12*count), 2); + memcpy(ssdt_ptr, name, 4); + ssdt_ptr += 4; + *(ssdt_ptr++) = 0x02; // MethodOp + + int i; + for (i = skip; count-- > 0; i++) { + *(ssdt_ptr++) = 0xA0; // IfOp + ssdt_ptr = encodeLen(ssdt_ptr, 11, 1); + *(ssdt_ptr++) = 0x93; // LEqualOp + *(ssdt_ptr++) = 0x68; // Arg0Op + *(ssdt_ptr++) = 0x0A; // BytePrefix + *(ssdt_ptr++) = i; + *(ssdt_ptr++) = 0x86; // NotifyOp + memcpy(ssdt_ptr, target, 4); + ssdt_ptr[ofs] = getHex(i >> 4); + ssdt_ptr[ofs + 1] = getHex(i); + ssdt_ptr += 4; + *(ssdt_ptr++) = 0x69; // Arg1Op + } + return ssdt_ptr; +} + static void* build_ssdt(void) { @@ -420,7 +452,9 @@ build_ssdt(void) + (acpi_cpus * PROC_SIZEOF) // procs + (1+2+5+(12*acpi_cpus)) // NTFY + (6+2+1+(1*acpi_cpus)) // CPON - + 17); // BDAT + + 17 // BDAT + + (1+3+4) // Scope(PCI0) + + (1+2+5+(12*(PCI_SLOTS - 1)))); // PCNT u8 *ssdt = malloc_high(length); if (! ssdt) { warn_noalloc(); @@ -464,27 +498,7 @@ build_ssdt(void) // build "Method(NTFY, 2) {If (LEqual(Arg0, 0x00)) {Notify(CP00, Arg1)} ...}" // Arg0 = Processor ID = APIC ID - *(ssdt_ptr++) = 0x14; // MethodOp - ssdt_ptr = encodeLen(ssdt_ptr, 2+5+(12*acpi_cpus), 2); - *(ssdt_ptr++) = 'N'; - *(ssdt_ptr++) = 'T'; - *(ssdt_ptr++) = 'F'; - *(ssdt_ptr++) = 'Y'; - *(ssdt_ptr++) = 0x02; - for (i=0; i<acpi_cpus; i++) { - *(ssdt_ptr++) = 0xA0; // IfOp - ssdt_ptr = encodeLen(ssdt_ptr, 11, 1); - *(ssdt_ptr++) = 0x93; // LEqualOp - *(ssdt_ptr++) = 0x68; // Arg0Op - *(ssdt_ptr++) = 0x0A; // BytePrefix - *(ssdt_ptr++) = i; - *(ssdt_ptr++) = 0x86; // NotifyOp - *(ssdt_ptr++) = 'C'; - *(ssdt_ptr++) = 'P'; - *(ssdt_ptr++) = getHex(i >> 4); - *(ssdt_ptr++) = getHex(i); - *(ssdt_ptr++) = 0x69; // Arg1Op - } + ssdt_ptr = build_notify(ssdt_ptr, "NTFY", 0, acpi_cpus, "CP00", 2); // build "Name(CPON, Package() { One, One, ..., Zero, Zero, ... })" *(ssdt_ptr++) = 0x08; // NameOp @@ -523,6 +537,16 @@ build_ssdt(void) *(u32*)ssdt_ptr = sizeof(struct bfld); ssdt_ptr += 4; + // build Scope(PCI0) opcode + *(ssdt_ptr++) = 0x10; // ScopeOp + ssdt_ptr = encodeLen(ssdt_ptr, length - (ssdt_ptr - ssdt), 3); + *(ssdt_ptr++) = 'P'; + *(ssdt_ptr++) = 'C'; + *(ssdt_ptr++) = 'I'; + *(ssdt_ptr++) = '0'; + + ssdt_ptr = build_notify(ssdt_ptr, "PCNT", 1, PCI_SLOTS, "S00_", 1); + build_header((void*)ssdt, SSDT_SIGNATURE, ssdt_ptr - ssdt, 1); //hexdump(ssdt, ssdt_ptr - ssdt); diff --git a/src/ssdt-pcihp.dsl b/src/ssdt-pcihp.dsl index 4b435b8..fd9c0bb 100644 --- a/src/ssdt-pcihp.dsl +++ b/src/ssdt-pcihp.dsl @@ -57,42 +57,5 @@ DefinitionBlock ("ssdt-pcihp.aml", "SSDT", 0x01, "BXPC", "BXSSDTPCIHP", 0x1) hotplug_slot(1d) hotplug_slot(1e) hotplug_slot(1f) - -#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) - } } } -- 1.7.10.4
More than 1kb of data is taken by the 32 copies of the PCI hotplug SSDT methods. We can build them from a single template like we do for CPUs (wrapped in a Scope(\_SB.PCI0) block). Three items differ for each slot: the device name, bits 16-23 of _ADR, the _SUN value. On top of this we have to rename the eject method for non-removable slots, like we already do in build_pcihp. There is a small change in the ASL: instead of including the number of the slot in the implementation of _EJ0, we just call _SUN. This is also similar to what we do for CPU hotplug. Once we do this, there is no need to keep a separate SSDT for PCI hotplug. Everything can reside in the same table. Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> --- src/acpi.c | 83 ++++++++++++++++++++++++---------------------------- src/ssdt-pcihp.dsl | 51 ++++++++------------------------ 2 files changed, 50 insertions(+), 84 deletions(-) diff --git a/src/acpi.c b/src/acpi.c index 38b72fb..31b4086 100644 --- a/src/acpi.c +++ b/src/acpi.c @@ -406,12 +406,22 @@ encodeLen(u8 *ssdt_ptr, int length, int bytes) #define PROC_SIZEOF (*ssdt_proc_end - *ssdt_proc_start) #define PROC_AML (ssdp_proc_aml + *ssdt_proc_start) +/* 0x5B 0x82 DeviceOp PkgLength NameString */ +#define PCIHP_OFFSET_HEX (*ssdt_pcihp_name - *ssdt_pcihp_start + 1) +#define PCIHP_OFFSET_ID (*ssdt_pcihp_id - *ssdt_pcihp_start) +#define PCIHP_OFFSET_ADR (*ssdt_pcihp_adr - *ssdt_pcihp_start) +#define PCIHP_OFFSET_EJ0 (*ssdt_pcihp_ej0 - *ssdt_pcihp_start) +#define PCIHP_SIZEOF (*ssdt_pcihp_end - *ssdt_pcihp_start) +#define PCIHP_AML (ssdp_pcihp_aml + *ssdt_pcihp_start) #define PCI_SLOTS 32 #define SSDT_SIGNATURE 0x54445353 // SSDT #define SSDT_HEADER_LENGTH 36 #include "ssdt-susp.hex" +#include "ssdt-pcihp.hex" + +#define PCI_RMV_BASE 0xae0c static u8* build_notify(u8 *ssdt_ptr, const char *name, int skip, int count, @@ -443,6 +453,24 @@ build_notify(u8 *ssdt_ptr, const char *name, int skip, int count, return ssdt_ptr; } +static void patch_pcihp(int slot, u8 *ssdt_ptr, u32 eject) +{ + ssdt_ptr[PCIHP_OFFSET_HEX] = getHex(slot >> 4); + ssdt_ptr[PCIHP_OFFSET_HEX+1] = getHex(slot); + ssdt_ptr[PCIHP_OFFSET_ID] = slot; + ssdt_ptr[PCIHP_OFFSET_ADR + 2] = slot; + + /* Runtime patching of EJ0: to disable hotplug for a slot, + * replace the method name: _EJ0 by EJ0_. */ + /* Sanity check */ + if (memcmp(ssdt_ptr + PCIHP_OFFSET_EJ0, "_EJ0", 4)) { + warn_internalerror(); + } + if (!eject) { + memcpy(ssdt_ptr + PCIHP_OFFSET_EJ0, "EJ0_", 4); + } +} + static void* build_ssdt(void) { @@ -454,6 +482,7 @@ build_ssdt(void) + (6+2+1+(1*acpi_cpus)) // CPON + 17 // BDAT + (1+3+4) // Scope(PCI0) + + ((PCI_SLOTS - 1) * PCIHP_SIZEOF) // slots + (1+2+5+(12*(PCI_SLOTS - 1)))); // PCNT u8 *ssdt = malloc_high(length); if (! ssdt) { @@ -545,6 +574,15 @@ build_ssdt(void) *(ssdt_ptr++) = 'I'; *(ssdt_ptr++) = '0'; + // build Device object for each slot + u32 rmvc_pcrm = inl(PCI_RMV_BASE); + for (i=1; i<PCI_SLOTS; i++) { + u32 eject = rmvc_pcrm & (0x1 << i); + memcpy(ssdt_ptr, PCIHP_AML, PCIHP_SIZEOF); + patch_pcihp(i, ssdt_ptr, eject != 0); + ssdt_ptr += PCIHP_SIZEOF; + } + ssdt_ptr = build_notify(ssdt_ptr, "PCNT", 1, PCI_SLOTS, "S00_", 1); build_header((void*)ssdt, SSDT_SIGNATURE, ssdt_ptr - ssdt, 1); @@ -554,50 +592,6 @@ build_ssdt(void) return ssdt; } -#include "ssdt-pcihp.hex" - -#define PCI_RMV_BASE 0xae0c - -extern void link_time_assertion(void); - -static void* build_pcihp(void) -{ - u32 rmvc_pcrm; - int i; - - u8 *ssdt = malloc_high(sizeof ssdp_pcihp_aml); - if (!ssdt) { - warn_noalloc(); - return NULL; - } - memcpy(ssdt, ssdp_pcihp_aml, sizeof ssdp_pcihp_aml); - - /* Runtime patching of EJ0: to disable hotplug for a slot, - * replace the method name: _EJ0 by EJ0_. */ - if (ARRAY_SIZE(aml_ej0_name) != ARRAY_SIZE(aml_adr_dword)) { - link_time_assertion(); - } - - rmvc_pcrm = inl(PCI_RMV_BASE); - 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; - /* Sanity check */ - if (memcmp(ssdp_pcihp_aml + aml_ej0_name[i], "_EJ0", 4)) { - warn_internalerror(); - free(ssdt); - return NULL; - } - if (!(rmvc_pcrm & (0x1 << slot))) { - memcpy(ssdt + aml_ej0_name[i], "EJ0_", 4); - } - } - - ((struct acpi_table_header*)ssdt)->checksum = 0; - ((struct acpi_table_header*)ssdt)->checksum -= checksum(ssdt, sizeof(ssdp_pcihp_aml)); - return ssdt; -} - #define HPET_SIGNATURE 0x54455048 // HPET static void* build_hpet(void) @@ -779,7 +773,6 @@ acpi_bios_init(void) ACPI_INIT_TABLE(build_madt()); ACPI_INIT_TABLE(build_hpet()); ACPI_INIT_TABLE(build_srat()); - ACPI_INIT_TABLE(build_pcihp()); u16 i, external_tables = qemu_cfg_acpi_additional_tables(); diff --git a/src/ssdt-pcihp.dsl b/src/ssdt-pcihp.dsl index fd9c0bb..cd66b83 100644 --- a/src/ssdt-pcihp.dsl +++ b/src/ssdt-pcihp.dsl @@ -12,50 +12,23 @@ DefinitionBlock ("ssdt-pcihp.aml", "SSDT", 0x01, "BXPC", "BXSSDTPCIHP", 0x1) External (\_SB.PCI0.PCEJ, MethodObj) Scope(\_SB.PCI0) { + /* Bulk generated PCI hotplug devices */ + ACPI_EXTRACT_DEVICE_START ssdt_pcihp_start + ACPI_EXTRACT_DEVICE_END ssdt_pcihp_end + ACPI_EXTRACT_DEVICE_STRING ssdt_pcihp_name + // Method _EJ0 can be patched by BIOS to EJ0_ // at runtime, if the slot is detected to not support hotplug. // Extract the offset of the address dword and the // _EJ0 name to allow this patching. -#define hotplug_slot(slot) \ - Device (S##slot) { \ - ACPI_EXTRACT_NAME_DWORD_CONST aml_adr_dword \ - Name (_ADR, 0x##slot##0000) \ - ACPI_EXTRACT_METHOD_STRING aml_ej0_name \ - Method (_EJ0, 1) { Return(PCEJ(0x##slot)) } \ - Name (_SUN, 0x##slot) \ + Device (SAA) { + ACPI_EXTRACT_NAME_BYTE_CONST ssdt_pcihp_id + Name (_SUN, 0xAA) + ACPI_EXTRACT_NAME_DWORD_CONST ssdt_pcihp_adr + Name (_ADR, 0xAA0000) + ACPI_EXTRACT_METHOD_STRING ssdt_pcihp_ej0 + Method (_EJ0, 1) { Return(PCEJ(_SUN)) } } - - 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) } } -- 1.7.10.4
On Thu, Aug 02, 2012 at 03:07:26PM +0200, Paolo Bonzini wrote:
More than 1kb of data is taken by the 32 copies of the PCI hotplug SSDT methods. We can build them from a single template like we do for CPUs (wrapped in a Scope(\_SB.PCI0) block).
Three items differ for each slot: the device name, bits 16-23 of _ADR, the _SUN value. On top of this we have to rename the eject method for non-removable slots, like we already do in build_pcihp.
There is a small change in the ASL: instead of including the number of the slot in the implementation of _EJ0, we just call _SUN. This is also similar to what we do for CPU hotplug.
Once we do this, there is no need to keep a separate SSDT for PCI hotplug. Everything can reside in the same table.
Size was never a reason to keep it separate. This was suggested by Kevin, the idea was to make it easier for people to override the hotplug logic. Does this still apply? Kevin?
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> --- src/acpi.c | 83 ++++++++++++++++++++++++---------------------------- src/ssdt-pcihp.dsl | 51 ++++++++------------------------ 2 files changed, 50 insertions(+), 84 deletions(-)
diff --git a/src/acpi.c b/src/acpi.c index 38b72fb..31b4086 100644 --- a/src/acpi.c +++ b/src/acpi.c @@ -406,12 +406,22 @@ encodeLen(u8 *ssdt_ptr, int length, int bytes) #define PROC_SIZEOF (*ssdt_proc_end - *ssdt_proc_start) #define PROC_AML (ssdp_proc_aml + *ssdt_proc_start)
+/* 0x5B 0x82 DeviceOp PkgLength NameString */ +#define PCIHP_OFFSET_HEX (*ssdt_pcihp_name - *ssdt_pcihp_start + 1) +#define PCIHP_OFFSET_ID (*ssdt_pcihp_id - *ssdt_pcihp_start) +#define PCIHP_OFFSET_ADR (*ssdt_pcihp_adr - *ssdt_pcihp_start) +#define PCIHP_OFFSET_EJ0 (*ssdt_pcihp_ej0 - *ssdt_pcihp_start) +#define PCIHP_SIZEOF (*ssdt_pcihp_end - *ssdt_pcihp_start) +#define PCIHP_AML (ssdp_pcihp_aml + *ssdt_pcihp_start) #define PCI_SLOTS 32
#define SSDT_SIGNATURE 0x54445353 // SSDT #define SSDT_HEADER_LENGTH 36
#include "ssdt-susp.hex" +#include "ssdt-pcihp.hex" + +#define PCI_RMV_BASE 0xae0c
static u8* build_notify(u8 *ssdt_ptr, const char *name, int skip, int count, @@ -443,6 +453,24 @@ build_notify(u8 *ssdt_ptr, const char *name, int skip, int count, return ssdt_ptr; }
+static void patch_pcihp(int slot, u8 *ssdt_ptr, u32 eject) +{ + ssdt_ptr[PCIHP_OFFSET_HEX] = getHex(slot >> 4); + ssdt_ptr[PCIHP_OFFSET_HEX+1] = getHex(slot); + ssdt_ptr[PCIHP_OFFSET_ID] = slot; + ssdt_ptr[PCIHP_OFFSET_ADR + 2] = slot; + + /* Runtime patching of EJ0: to disable hotplug for a slot, + * replace the method name: _EJ0 by EJ0_. */ + /* Sanity check */ + if (memcmp(ssdt_ptr + PCIHP_OFFSET_EJ0, "_EJ0", 4)) { + warn_internalerror(); + } + if (!eject) { + memcpy(ssdt_ptr + PCIHP_OFFSET_EJ0, "EJ0_", 4); + } +} + static void* build_ssdt(void) { @@ -454,6 +482,7 @@ build_ssdt(void) + (6+2+1+(1*acpi_cpus)) // CPON + 17 // BDAT + (1+3+4) // Scope(PCI0) + + ((PCI_SLOTS - 1) * PCIHP_SIZEOF) // slots + (1+2+5+(12*(PCI_SLOTS - 1)))); // PCNT u8 *ssdt = malloc_high(length); if (! ssdt) { @@ -545,6 +574,15 @@ build_ssdt(void) *(ssdt_ptr++) = 'I'; *(ssdt_ptr++) = '0';
+ // build Device object for each slot + u32 rmvc_pcrm = inl(PCI_RMV_BASE); + for (i=1; i<PCI_SLOTS; i++) { + u32 eject = rmvc_pcrm & (0x1 << i); + memcpy(ssdt_ptr, PCIHP_AML, PCIHP_SIZEOF); + patch_pcihp(i, ssdt_ptr, eject != 0); + ssdt_ptr += PCIHP_SIZEOF; + } + ssdt_ptr = build_notify(ssdt_ptr, "PCNT", 1, PCI_SLOTS, "S00_", 1);
build_header((void*)ssdt, SSDT_SIGNATURE, ssdt_ptr - ssdt, 1); @@ -554,50 +592,6 @@ build_ssdt(void) return ssdt; }
-#include "ssdt-pcihp.hex" - -#define PCI_RMV_BASE 0xae0c - -extern void link_time_assertion(void); - -static void* build_pcihp(void) -{ - u32 rmvc_pcrm; - int i; - - u8 *ssdt = malloc_high(sizeof ssdp_pcihp_aml); - if (!ssdt) { - warn_noalloc(); - return NULL; - } - memcpy(ssdt, ssdp_pcihp_aml, sizeof ssdp_pcihp_aml); - - /* Runtime patching of EJ0: to disable hotplug for a slot, - * replace the method name: _EJ0 by EJ0_. */ - if (ARRAY_SIZE(aml_ej0_name) != ARRAY_SIZE(aml_adr_dword)) { - link_time_assertion(); - } - - rmvc_pcrm = inl(PCI_RMV_BASE); - 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; - /* Sanity check */ - if (memcmp(ssdp_pcihp_aml + aml_ej0_name[i], "_EJ0", 4)) { - warn_internalerror(); - free(ssdt); - return NULL; - } - if (!(rmvc_pcrm & (0x1 << slot))) { - memcpy(ssdt + aml_ej0_name[i], "EJ0_", 4); - } - } - - ((struct acpi_table_header*)ssdt)->checksum = 0; - ((struct acpi_table_header*)ssdt)->checksum -= checksum(ssdt, sizeof(ssdp_pcihp_aml)); - return ssdt; -} - #define HPET_SIGNATURE 0x54455048 // HPET static void* build_hpet(void) @@ -779,7 +773,6 @@ acpi_bios_init(void) ACPI_INIT_TABLE(build_madt()); ACPI_INIT_TABLE(build_hpet()); ACPI_INIT_TABLE(build_srat()); - ACPI_INIT_TABLE(build_pcihp());
u16 i, external_tables = qemu_cfg_acpi_additional_tables();
diff --git a/src/ssdt-pcihp.dsl b/src/ssdt-pcihp.dsl index fd9c0bb..cd66b83 100644 --- a/src/ssdt-pcihp.dsl +++ b/src/ssdt-pcihp.dsl @@ -12,50 +12,23 @@ DefinitionBlock ("ssdt-pcihp.aml", "SSDT", 0x01, "BXPC", "BXSSDTPCIHP", 0x1) External (\_SB.PCI0.PCEJ, MethodObj)
Scope(\_SB.PCI0) { + /* Bulk generated PCI hotplug devices */ + ACPI_EXTRACT_DEVICE_START ssdt_pcihp_start + ACPI_EXTRACT_DEVICE_END ssdt_pcihp_end + ACPI_EXTRACT_DEVICE_STRING ssdt_pcihp_name + // Method _EJ0 can be patched by BIOS to EJ0_ // at runtime, if the slot is detected to not support hotplug. // Extract the offset of the address dword and the // _EJ0 name to allow this patching. -#define hotplug_slot(slot) \ - Device (S##slot) { \ - ACPI_EXTRACT_NAME_DWORD_CONST aml_adr_dword \ - Name (_ADR, 0x##slot##0000) \ - ACPI_EXTRACT_METHOD_STRING aml_ej0_name \ - Method (_EJ0, 1) { Return(PCEJ(0x##slot)) } \ - Name (_SUN, 0x##slot) \ + Device (SAA) { + ACPI_EXTRACT_NAME_BYTE_CONST ssdt_pcihp_id + Name (_SUN, 0xAA) + ACPI_EXTRACT_NAME_DWORD_CONST ssdt_pcihp_adr + Name (_ADR, 0xAA0000) + ACPI_EXTRACT_METHOD_STRING ssdt_pcihp_ej0 + Method (_EJ0, 1) { Return(PCEJ(_SUN)) } } - - 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) } } -- 1.7.10.4
On Fri, Aug 03, 2012 at 08:49:00PM +0300, Michael S. Tsirkin wrote:
On Thu, Aug 02, 2012 at 03:07:26PM +0200, Paolo Bonzini wrote:
More than 1kb of data is taken by the 32 copies of the PCI hotplug SSDT methods. We can build them from a single template like we do for CPUs (wrapped in a Scope(\_SB.PCI0) block).
Three items differ for each slot: the device name, bits 16-23 of _ADR, the _SUN value. On top of this we have to rename the eject method for non-removable slots, like we already do in build_pcihp.
There is a small change in the ASL: instead of including the number of the slot in the implementation of _EJ0, we just call _SUN. This is also similar to what we do for CPU hotplug.
Once we do this, there is no need to keep a separate SSDT for PCI hotplug. Everything can reside in the same table.
Size was never a reason to keep it separate. This was suggested by Kevin, the idea was to make it easier for people to override the hotplug logic. Does this still apply? Kevin?
I don't recall suggesting it being separate - my recollection was preferring to dynamically generate more of the PCI hotplug so as to be more like cpu hotplug. -Kevin
On Thu, Aug 02, 2012 at 03:07:20PM +0200, Paolo Bonzini wrote:
More than 1kb of data is taken by the 32 copies of the PCI hotplug SSDT methods. We can build them from a single template like we do for CPUs.
This series does exactly this. Patches 1 prepares for the change, by moving other pieces of ssdt-pcihd.dsl out of the way. Patch 2 is also a simple rename and patch 3 fixes a bug in acpi_extract. Patches 4 to 6 finally do the movement.
v1->v2: document computation of length (patch 1, Igor) build PCNT dynamically (Kevin)
Paolo Bonzini (6): acpi: move s3/s4/s5 to build_ssdt acpi: rename Processor SSDT constants acpi_extract: fix off-by-one acpi_extract: detect DeviceOp acpi: build PCNT dynamically acpi: build PCI hotplug devices from a single template
Makefile | 2 +- src/acpi.c | 218 +++++++++++++++++++++++++++---------------------- src/ssdt-pcihp.dsl | 124 +++------------------------- src/ssdt-susp.dsl | 41 ++++++++++ tools/acpi_extract.py | 30 ++++++- 5 files changed, 203 insertions(+), 212 deletions(-) create mode 100644 src/ssdt-susp.dsl
Jason, any input? I'm a bit concerned this will make bridge support that you have been working on harder ... or maybe not and it will make it easier?
-- 1.7.10.4
Il 03/08/2012 00:33, Michael S. Tsirkin ha scritto:
Paolo Bonzini (6): acpi: move s3/s4/s5 to build_ssdt acpi: rename Processor SSDT constants acpi_extract: fix off-by-one acpi_extract: detect DeviceOp acpi: build PCNT dynamically acpi: build PCI hotplug devices from a single template
Makefile | 2 +- src/acpi.c | 218 +++++++++++++++++++++++++++---------------------- src/ssdt-pcihp.dsl | 124 +++------------------------- src/ssdt-susp.dsl | 41 ++++++++++ tools/acpi_extract.py | 30 ++++++- 5 files changed, 203 insertions(+), 212 deletions(-) create mode 100644 src/ssdt-susp.dsl Jason, any input? I'm a bit concerned this will make bridge support that you have been working on harder ... or maybe not and it will make it easier?
I didn't think of this, but patch 1 should make it simpler (because then we have an SSDT purely for PCI hotplug---no matter if we use it as is or just as a building block); the others should be a wash. Paolo
On Fri, Aug 03, 2012 at 01:33:52AM +0300, Michael S. Tsirkin wrote:
On Thu, Aug 02, 2012 at 03:07:20PM +0200, Paolo Bonzini wrote:
More than 1kb of data is taken by the 32 copies of the PCI hotplug SSDT methods. We can build them from a single template like we do for CPUs.
This series does exactly this. Patches 1 prepares for the change, by moving other pieces of ssdt-pcihd.dsl out of the way. Patch 2 is also a simple rename and patch 3 fixes a bug in acpi_extract. Patches 4 to 6 finally do the movement.
v1->v2: document computation of length (patch 1, Igor) build PCNT dynamically (Kevin)
Paolo Bonzini (6): acpi: move s3/s4/s5 to build_ssdt acpi: rename Processor SSDT constants acpi_extract: fix off-by-one acpi_extract: detect DeviceOp acpi: build PCNT dynamically acpi: build PCI hotplug devices from a single template
Makefile | 2 +- src/acpi.c | 218 +++++++++++++++++++++++++++---------------------- src/ssdt-pcihp.dsl | 124 +++------------------------- src/ssdt-susp.dsl | 41 ++++++++++ tools/acpi_extract.py | 30 ++++++- 5 files changed, 203 insertions(+), 212 deletions(-) create mode 100644 src/ssdt-susp.dsl
Jason, any input? I'm a bit concerned this will make bridge support that you have been working on harder ... or maybe not and it will make it easier?
I think this patch should be ok. Its harder for me in the sense that I need to re-do my patch :) Here's my current ssdt-pcihp.dsl patch, maybe Paolo can spot if there would be any conflicts. I suspect, the auto generation that Paolo is doing is actually going to simplify things in general for these typtes of nested tables. Thanks, -Jason --- a/src/ssdt-pcihp.dsl +++ b/src/ssdt-pcihp.dsl @@ -17,82 +17,162 @@ DefinitionBlock ("ssdt-pcihp.aml", "SSDT", 0x01, "BXPC", "BXSSDTPCIHP", 0x1) // at runtime, if the slot is detected to not support hotplug. // Extract the offset of the address dword and the // _EJ0 name to allow this patching. -#define hotplug_slot(slot) \ - Device (S##slot) { \ - ACPI_EXTRACT_NAME_DWORD_CONST aml_adr_dword \ - Name (_ADR, 0x##slot##0000) \ - ACPI_EXTRACT_METHOD_STRING aml_ej0_name \ - Method (_EJ0, 1) { Return(PCEJ(0x##slot)) } \ - Name (_SUN, 0x##slot) \ - } +#define hotplug_level2_slot(slot1, slot2) \ + Device (S##slot2) { \ + Name (_ADR, 0x##slot2##0000) \ + Method (_EJ0, 1) { Return(PCEJ(0x##slot1, 0x##slot2)) } \ + Name (_SUN, 0x##slot2) \ + } \ + +#define hotplug_top_level_slot(slot) \ + Device (S##slot) { \ + ACPI_EXTRACT_NAME_DWORD_CONST aml_adr_dword \ + Name (_ADR,0x##slot##0000) \ + ACPI_EXTRACT_METHOD_STRING aml_ej0_name \ + Method (_EJ0, 1) { Return(PCEJ(0x##slot, 0x00)) } \ + Name (_SUN, 0x##slot) \ + hotplug_level2_slot(slot, 01) \ + hotplug_level2_slot(slot, 02) \ + hotplug_level2_slot(slot, 03) \ + hotplug_level2_slot(slot, 04) \ + hotplug_level2_slot(slot, 05) \ + hotplug_level2_slot(slot, 06) \ + hotplug_level2_slot(slot, 07) \ + hotplug_level2_slot(slot, 08) \ + hotplug_level2_slot(slot, 09) \ + hotplug_level2_slot(slot, 0a) \ + hotplug_level2_slot(slot, 0b) \ + hotplug_level2_slot(slot, 0c) \ + hotplug_level2_slot(slot, 0d) \ + hotplug_level2_slot(slot, 0e) \ + hotplug_level2_slot(slot, 0f) \ + hotplug_level2_slot(slot, 10) \ + hotplug_level2_slot(slot, 11) \ + hotplug_level2_slot(slot, 12) \ + hotplug_level2_slot(slot, 13) \ + hotplug_level2_slot(slot, 14) \ + hotplug_level2_slot(slot, 15) \ + hotplug_level2_slot(slot, 16) \ + hotplug_level2_slot(slot, 17) \ + hotplug_level2_slot(slot, 18) \ + hotplug_level2_slot(slot, 19) \ + hotplug_level2_slot(slot, 1a) \ + hotplug_level2_slot(slot, 1b) \ + hotplug_level2_slot(slot, 1c) \ + hotplug_level2_slot(slot, 1d) \ + hotplug_level2_slot(slot, 1e) \ + hotplug_level2_slot(slot, 1f) \ + } \ + + hotplug_top_level_slot(01) + hotplug_top_level_slot(02) + hotplug_top_level_slot(03) + hotplug_top_level_slot(04) + hotplug_top_level_slot(05) + hotplug_top_level_slot(06) + hotplug_top_level_slot(07) + hotplug_top_level_slot(08) + hotplug_top_level_slot(09) + hotplug_top_level_slot(0a) + hotplug_top_level_slot(0b) + hotplug_top_level_slot(0c) + hotplug_top_level_slot(0d) + hotplug_top_level_slot(0e) + hotplug_top_level_slot(0f) + hotplug_top_level_slot(10) + hotplug_top_level_slot(11) + hotplug_top_level_slot(12) + hotplug_top_level_slot(13) + hotplug_top_level_slot(14) + hotplug_top_level_slot(15) + hotplug_top_level_slot(16) + hotplug_top_level_slot(17) + hotplug_top_level_slot(18) + hotplug_top_level_slot(19) + hotplug_top_level_slot(1a) + hotplug_top_level_slot(1b) + hotplug_top_level_slot(1c) + hotplug_top_level_slot(1d) + hotplug_top_level_slot(1e) + hotplug_top_level_slot(1f) - 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) +#define gen_pci_level2_hotplug(slot1, slot2) \ + If (LEqual(Arg0, 0x##slot1)) { \ + If (LEqual(Arg1, 0x##slot2)) { \ + Notify(\_SB.PCI0.S##slot1.S##slot2, Arg2) \ + } \ + } \ -#define gen_pci_hotplug(slot) \ - If (LEqual(Arg0, 0x##slot)) { Notify(S##slot, Arg1) } +#define gen_pci_top_level_hotplug(slot) \ + If (LEqual(Arg1, Zero)) { \ + If (LEqual(Arg0, 0x##slot)) { \ + Notify(S##slot, Arg2) \ + } \ + } \ + gen_pci_level2_hotplug(slot, 01) \ + gen_pci_level2_hotplug(slot, 02) \ + gen_pci_level2_hotplug(slot, 03) \ + gen_pci_level2_hotplug(slot, 04) \ + gen_pci_level2_hotplug(slot, 05) \ + gen_pci_level2_hotplug(slot, 06) \ + gen_pci_level2_hotplug(slot, 07) \ + gen_pci_level2_hotplug(slot, 08) \ + gen_pci_level2_hotplug(slot, 09) \ + gen_pci_level2_hotplug(slot, 0a) \ + gen_pci_level2_hotplug(slot, 0b) \ + gen_pci_level2_hotplug(slot, 0c) \ + gen_pci_level2_hotplug(slot, 0d) \ + gen_pci_level2_hotplug(slot, 0e) \ + gen_pci_level2_hotplug(slot, 0f) \ + gen_pci_level2_hotplug(slot, 10) \ + gen_pci_level2_hotplug(slot, 11) \ + gen_pci_level2_hotplug(slot, 12) \ + gen_pci_level2_hotplug(slot, 13) \ + gen_pci_level2_hotplug(slot, 14) \ + gen_pci_level2_hotplug(slot, 15) \ + gen_pci_level2_hotplug(slot, 16) \ + gen_pci_level2_hotplug(slot, 17) \ + gen_pci_level2_hotplug(slot, 18) \ + gen_pci_level2_hotplug(slot, 19) \ + gen_pci_level2_hotplug(slot, 1a) \ + gen_pci_level2_hotplug(slot, 1b) \ + gen_pci_level2_hotplug(slot, 1c) \ + gen_pci_level2_hotplug(slot, 1d) \ + gen_pci_level2_hotplug(slot, 1e) \ + gen_pci_level2_hotplug(slot, 1f) \ - 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) + Method(PCNT, 3) { + gen_pci_top_level_hotplug(01) + gen_pci_top_level_hotplug(02) + gen_pci_top_level_hotplug(03) + gen_pci_top_level_hotplug(04) + gen_pci_top_level_hotplug(05) + gen_pci_top_level_hotplug(06) + gen_pci_top_level_hotplug(07) + gen_pci_top_level_hotplug(08) + gen_pci_top_level_hotplug(09) + gen_pci_top_level_hotplug(0a) + gen_pci_top_level_hotplug(0b) + gen_pci_top_level_hotplug(0c) + gen_pci_top_level_hotplug(0d) + gen_pci_top_level_hotplug(0e) + gen_pci_top_level_hotplug(0f) + gen_pci_top_level_hotplug(10) + gen_pci_top_level_hotplug(11) + gen_pci_top_level_hotplug(12) + gen_pci_top_level_hotplug(13) + gen_pci_top_level_hotplug(14) + gen_pci_top_level_hotplug(15) + gen_pci_top_level_hotplug(16) + gen_pci_top_level_hotplug(17) + gen_pci_top_level_hotplug(18) + gen_pci_top_level_hotplug(19) + gen_pci_top_level_hotplug(1a) + gen_pci_top_level_hotplug(1b) + gen_pci_top_level_hotplug(1c) + gen_pci_top_level_hotplug(1d) + gen_pci_top_level_hotplug(1e) + gen_pci_top_level_hotplug(1f) } }
Il 03/08/2012 16:53, Jason Baron ha scritto:
I think this patch should be ok. Its harder for me in the sense that I need to re-do my patch :)
Here's my current ssdt-pcihp.dsl patch, maybe Paolo can spot if there would be any conflicts. I suspect, the auto generation that Paolo is doing is actually going to simplify things in general for these typtes of nested tables.
There are two parts here. One is the actual declaration of the devices, and here I think my patch is actually going to simplify things a bit. The second level of devices can stay in ssdt-pcihp.dsl, and it will just work. The second is the huge PCNT method. I made a couple of (feeble) attempts at rewriting it using DerefOf, like this: Name(DEVS, Package(32) { Zero, ".S01_", ".S02_", ".S03_", ".S04_", ".S05_", ".S06_", ".S07_", ".S08_", ".S09_", ".S0A_", ".S0B_", ".S0C_", ".S0D_", ".S0E_", ".S0F_", ".S10_", ".S11_", ".S12_", ".S13_", ".S14_", ".S15_", ".S16_", ".S17_", ".S18_", ".S19_", ".S1A_", ".S1B_", ".S1C_", ".S1D_", ".S1E_", ".S1F_" }) Method(PCNT, 2) { If (LGreater(Arg0, Zero)) { Store("\\_SB.PCI0", Local0) Concatenate(Local0, DerefOf(Index(\_SB.PCI0.DEVS, Arg1)), Local0) Notify(DerefOf(Local0), Arg1) } } (the above for no bridges) but it doesn't work. The simplest possibility here is to just move it to the DSDT, as in my v1. Generating the nested AML ifs is a bit tedious to write, but the outcome should be easy to read. Paolo
On Fri, Aug 03, 2012 at 05:57:41PM +0200, Paolo Bonzini wrote:
Il 03/08/2012 16:53, Jason Baron ha scritto:
I think this patch should be ok. Its harder for me in the sense that I need to re-do my patch :)
Here's my current ssdt-pcihp.dsl patch, maybe Paolo can spot if there would be any conflicts. I suspect, the auto generation that Paolo is doing is actually going to simplify things in general for these typtes of nested tables.
There are two parts here.
One is the actual declaration of the devices, and here I think my patch is actually going to simplify things a bit. The second level of devices can stay in ssdt-pcihp.dsl, and it will just work.
Long term, it is worth exploring whether we can load second level dynamically when bridge is added by hotplug or detected by bus scan. Or even go wild and support bridges of arbitrary depth (though in practice 1 level is enough for pci and 2 levels with level 1 always having 1 child for pci express).
The second is the huge PCNT method. I made a couple of (feeble) attempts at rewriting it using DerefOf, like this:
Name(DEVS, Package(32) { Zero, ".S01_", ".S02_", ".S03_", ".S04_", ".S05_", ".S06_", ".S07_", ".S08_", ".S09_", ".S0A_", ".S0B_", ".S0C_", ".S0D_", ".S0E_", ".S0F_", ".S10_", ".S11_", ".S12_", ".S13_", ".S14_", ".S15_", ".S16_", ".S17_", ".S18_", ".S19_", ".S1A_", ".S1B_", ".S1C_", ".S1D_", ".S1E_", ".S1F_" })
Method(PCNT, 2) { If (LGreater(Arg0, Zero)) { Store("\\_SB.PCI0", Local0) Concatenate(Local0, DerefOf(Index(\_SB.PCI0.DEVS, Arg1)), Local0) Notify(DerefOf(Local0), Arg1) } }
(the above for no bridges) but it doesn't work.
The simplest possibility here is to just move it to the DSDT, as in my v1. Generating the nested AML ifs is a bit tedious to write, but the outcome should be easy to read.
Paolo
Il 03/08/2012 19:52, Michael S. Tsirkin ha scritto:
On Fri, Aug 03, 2012 at 05:57:41PM +0200, Paolo Bonzini wrote:
Il 03/08/2012 16:53, Jason Baron ha scritto:
I think this patch should be ok. Its harder for me in the sense that I need to re-do my patch :)
Here's my current ssdt-pcihp.dsl patch, maybe Paolo can spot if there would be any conflicts. I suspect, the auto generation that Paolo is doing is actually going to simplify things in general for these typtes of nested tables.
There are two parts here.
One is the actual declaration of the devices, and here I think my patch is actually going to simplify things a bit. The second level of devices can stay in ssdt-pcihp.dsl, and it will just work.
Long term, it is worth exploring whether we can load second level dynamically when bridge is added by hotplug or detected by bus scan.
This should be doable with LoadTable (to replace a non-bridge _SB.PCI0.Sxx with a bridge-aware definition). Even if it's long-term, it makes sense then to split now the huge PCNT method between a part that is in \_SB.PCI0 If (LEqual(Arg0, 0x##slot)) { If (LEqual(Arg1, Zero)) { Notify(S##slot, Arg2) } Else { \_SB.PCI0.S##slot.PCNT(Arg1, Arg2) } } and another that is in the per-slot devices. All in all, it doesn't seem like this series would add any hurdle, and it helps remaining clean. Where would you handle cold-plug? _PS0? Paolo
On Tue, Aug 07, 2012 at 02:07:08PM +0200, Paolo Bonzini wrote:
Il 03/08/2012 19:52, Michael S. Tsirkin ha scritto:
On Fri, Aug 03, 2012 at 05:57:41PM +0200, Paolo Bonzini wrote:
Il 03/08/2012 16:53, Jason Baron ha scritto:
I think this patch should be ok. Its harder for me in the sense that I need to re-do my patch :)
Here's my current ssdt-pcihp.dsl patch, maybe Paolo can spot if there would be any conflicts. I suspect, the auto generation that Paolo is doing is actually going to simplify things in general for these typtes of nested tables.
There are two parts here.
One is the actual declaration of the devices, and here I think my patch is actually going to simplify things a bit. The second level of devices can stay in ssdt-pcihp.dsl, and it will just work.
Long term, it is worth exploring whether we can load second level dynamically when bridge is added by hotplug or detected by bus scan.
This should be doable with LoadTable (to replace a non-bridge _SB.PCI0.Sxx with a bridge-aware definition). Even if it's long-term, it makes sense then to split now the huge PCNT method between a part that is in \_SB.PCI0
If (LEqual(Arg0, 0x##slot)) { If (LEqual(Arg1, Zero)) { Notify(S##slot, Arg2) } Else { \_SB.PCI0.S##slot.PCNT(Arg1, Arg2) } }
and another that is in the per-slot devices. All in all, it doesn't seem like this series would add any hurdle, and it helps remaining clean.
If there's no further comments on this, I'll commit this series from Paolo. -Kevin
On Fri, Aug 03, 2012 at 05:57:41PM +0200, Paolo Bonzini wrote:
Il 03/08/2012 16:53, Jason Baron ha scritto:
I think this patch should be ok. Its harder for me in the sense that I need to re-do my patch :)
Here's my current ssdt-pcihp.dsl patch, maybe Paolo can spot if there would be any conflicts. I suspect, the auto generation that Paolo is doing is actually going to simplify things in general for these typtes of nested tables.
There are two parts here.
One is the actual declaration of the devices, and here I think my patch is actually going to simplify things a bit. The second level of devices can stay in ssdt-pcihp.dsl, and it will just work.
The second is the huge PCNT method. I made a couple of (feeble) attempts at rewriting it using DerefOf, like this:
Name(DEVS, Package(32) { Zero, ".S01_", ".S02_", ".S03_", ".S04_", ".S05_", ".S06_",
When I was working on the CPU hotplug stuff I tried a couple of different ways (including DerefOf) to make calling Notify more dynamic. I couldn't find anyway to make it work which is why I ended up dynamically building a method with a list of objects to conditinally call Notify on. -Kevin
On Fri, Aug 03, 2012 at 05:57:41PM +0200, Paolo Bonzini wrote:
The simplest possibility here is to just move it to the DSDT, as in my v1. Generating the nested AML ifs is a bit tedious to write, but the outcome should be easy to read.
That would require statically generating some 961 Notify operations. That seems a waste. Similar to your v2 patch, it should be possible to dynamically generate a second-level notify that does the equivalent of: Method(PCN2, 2) { If (LEqual(Arg0, 0x0101)) { Notify(S01_.S01_, Arg1) } If (LEqual(Arg0, 0x0102)) { Notify(S01_.S02_, Arg1) } If (LEqual(Arg0, 0x0103)) { Notify(S01_.S03_, Arg1) } ... If (LEqual(Arg0, 0x011F)) { Notify(S01_.S1F_, Arg1) } If (LEqual(Arg0, 0x0201)) { Notify(S02_.S01_, Arg1) } ... } Then the DSDT's PCNF method can have the intelligence to call both PCNT() on the main slot and PCN2() in a loop for all the children of that slot. -Kevin
On Thu, Aug 02, 2012 at 03:07:20PM +0200, Paolo Bonzini wrote:
More than 1kb of data is taken by the 32 copies of the PCI hotplug SSDT methods. We can build them from a single template like we do for CPUs.
This series does exactly this. Patches 1 prepares for the change, by moving other pieces of ssdt-pcihd.dsl out of the way. Patch 2 is also a simple rename and patch 3 fixes a bug in acpi_extract. Patches 4 to 6 finally do the movement.
v1->v2: document computation of length (patch 1, Igor) build PCNT dynamically (Kevin)
The patch looks good to me. Lets see where the discusions go before committing. -Kevin
On Thu, Aug 02, 2012 at 03:07:20PM +0200, Paolo Bonzini wrote:
More than 1kb of data is taken by the 32 copies of the PCI hotplug SSDT methods. We can build them from a single template like we do for CPUs.
Thanks. I committed this patch series. -Kevin
participants (4)
-
Jason Baron -
Kevin O'Connor -
Michael S. Tsirkin -
Paolo Bonzini