[PATCH 0/6] Build PCI hotplug SSDT from a single template

[Paolo Bonzini]

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 and 2 prepare for the change,
by moving other pieces of ssdt-pcihd.dsl out of the way.  Patch 3 is also
a simple rename and patch 4 fixes a bug in acpi_extract.  Patches 5 and
6 finally do the movement.

Paolo Bonzini (6):
  acpi: move PCNT to DSDT
  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 PCI hotplug devices from a single template

 Makefile              |    2 +-
 src/acpi-dsdt.dsl     |   39 +++++++++++-
 src/acpi.c            |  161 ++++++++++++++++++++++++++-----------------------
 src/ssdt-pcihp.dsl    |  124 ++++---------------------------------
 src/ssdt-susp.dsl     |   41 +++++++++++++
 tools/acpi_extract.py |   30 ++++++++-
 6 files changed, 206 insertions(+), 191 deletions(-)
 create mode 100644 src/ssdt-susp.dsl

-- 
1.7.10.4

[PATCH 1/6] acpi: move PCNT to DSDT

[Paolo Bonzini]

The PCNT method does not need any patching, so it can stay in the DSDT
if we add an External declaration.  The AML code size does not change.

Signed-off-by: Paolo Bonzini <[e-mail]>
---
 src/acpi-dsdt.dsl  |   39 +++++++++++++++++++++++++++++++++++++--
 src/ssdt-pcihp.dsl |   37 -------------------------------------
 2 files changed, 37 insertions(+), 39 deletions(-)

diff --git a/src/acpi-dsdt.dsl b/src/acpi-dsdt.dsl
index 2060686..4ebee9f 100644
--- a/src/acpi-dsdt.dsl
+++ b/src/acpi-dsdt.dsl
@@ -540,8 +540,43 @@ DefinitionBlock (
             Return (0x0)
         }
 
-	/* Hotplug notification method supplied by SSDT */
-	External (\_SB.PCI0.PCNT, MethodObj)
+#define gen_pci_hotplug(slot)   \
+            External (\_SB.PCI0.S##slot, DeviceObj) \
+            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 hotplug notify method */
         Method(PCNF, 0) {
diff --git a/src/ssdt-pcihp.dsl b/src/ssdt-pcihp.dsl
index 12555e2..81c57b3 100644
--- a/src/ssdt-pcihp.dsl
+++ b/src/ssdt-pcihp.dsl
@@ -57,43 +57,6 @@ 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)
-        }
     }
 
     Scope(\) {
-- 
1.7.10.4

[PATCH 2/6] acpi: move s3/s4/s5 to build_ssdt

[Paolo Bonzini]

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 <[e-mail]>
---
 Makefile           |    2 +-
 src/acpi.c         |   41 ++++++++++++++++++++++++-----------------
 src/ssdt-pcihp.dsl |   36 ------------------------------------
 src/ssdt-susp.dsl  |   41 +++++++++++++++++++++++++++++++++++++++++
 4 files changed, 66 insertions(+), 54 deletions(-)
 create mode 100644 src/ssdt-susp.dsl

diff --git a/Makefile b/Makefile
index fe974f7..2ccf05b 100644
--- a/Makefile
+++ b/Makefile
@@ -228,7 +228,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 55e4607..e6a7ef5 100644
--- a/src/acpi.c
+++ b/src/acpi.c
@@ -407,26 +407,46 @@ 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)
+    int length = (sizeof(ssdp_susp_aml)
+                  + (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);
+    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';
@@ -518,8 +538,6 @@ extern void link_time_assertion(void);
 
 static void* build_pcihp(void)
 {
-    char *sys_states;
-    int sys_state_size;
     u32 rmvc_pcrm;
     int i;
 
@@ -551,19 +569,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 81c57b3..fd9c0bb 100644
--- a/src/ssdt-pcihp.dsl
+++ b/src/ssdt-pcihp.dsl
@@ -58,40 +58,4 @@ DefinitionBlock ("ssdt-pcihp.aml", "SSDT", 0x01, "BXPC", "BXSSDTPCIHP", 0x1)
         hotplug_slot(1e)
         hotplug_slot(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

[PATCH 2/6] acpi: move s3/s4/s5 to build_ssdt

[Igor Mammedov]

On 07/31/2012 11:52 AM, Paolo Bonzini wrote:
> 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 <[e-mail]>
> ---
>   Makefile           |    2 +-
>   src/acpi.c         |   41 ++++++++++++++++++++++++-----------------
>   src/ssdt-pcihp.dsl |   36 ------------------------------------
>   src/ssdt-susp.dsl  |   41 +++++++++++++++++++++++++++++++++++++++++
>   4 files changed, 66 insertions(+), 54 deletions(-)
>   create mode 100644 src/ssdt-susp.dsl
>
> diff --git a/Makefile b/Makefile
> index fe974f7..2ccf05b 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -228,7 +228,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 55e4607..e6a7ef5 100644
> --- a/src/acpi.c
> +++ b/src/acpi.c
> @@ -407,26 +407,46 @@ 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
> perhaps comment should reflect changes
> -    int length = ((1+3+4)
> +    int length = (sizeof(ssdp_susp_aml)
> +                  + (1+3+4)
> it would be nice to have a self documenting define here instead of numbers
> + (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);
> +    u8 *ssdt = malloc_high(length);

does sizeof(ssdp_susp_aml) includes sizeof(struct acpi_table_header)?
> 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';
> @@ -518,8 +538,6 @@ extern void link_time_assertion(void);
>
> static void* build_pcihp(void)
>   {
> -    char *sys_states;
> -    int sys_state_size;
>       u32 rmvc_pcrm;
>       int i;
>
> @@ -551,19 +569,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 81c57b3..fd9c0bb 100644
> --- a/src/ssdt-pcihp.dsl
> +++ b/src/ssdt-pcihp.dsl
> @@ -58,40 +58,4 @@ DefinitionBlock ("ssdt-pcihp.aml", "SSDT", 0x01, "BXPC", "BXSSDTPCIHP", 0x1)
>           hotplug_slot(1e)
>           hotplug_slot(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 */
> +        })
> +    }
> +}

[PATCH 2/6] acpi: move s3/s4/s5 to build_ssdt

[Paolo Bonzini]

Il 01/08/2012 10:05, Igor Mammedov ha scritto:
> On 07/31/2012 11:52 AM, Paolo Bonzini wrote:
> > 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 <[e-mail]>
> > ---
> >   Makefile           |    2 +-
> >   src/acpi.c         |   41 ++++++++++++++++++++++++-----------------
> >   src/ssdt-pcihp.dsl |   36 ------------------------------------
> >   src/ssdt-susp.dsl  |   41 +++++++++++++++++++++++++++++++++++++++++
> >   4 files changed, 66 insertions(+), 54 deletions(-)
> >   create mode 100644 src/ssdt-susp.dsl
> >
> > diff --git a/Makefile b/Makefile
> > index fe974f7..2ccf05b 100644
> > --- a/Makefile
> > +++ b/Makefile
> > @@ -228,7 +228,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 55e4607..e6a7ef5 100644
> > --- a/src/acpi.c
> > +++ b/src/acpi.c
> > @@ -407,26 +407,46 @@ 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
> > perhaps comment should reflect changes

It's already out of date, but you're right.  It can be fixed on top, or
I can respin.
> > -    int length = ((1+3+4)
> > +    int length = (sizeof(ssdp_susp_aml)
> > +                  + (1+3+4)
> > it would be nice to have a self documenting define here instead of numbers
> > + (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);
> > +    u8 *ssdt = malloc_high(length);
>
> does sizeof(ssdp_susp_aml) includes sizeof(struct acpi_table_header)?

Yes:

static unsigned char ssdp_susp_aml[] = {
0x53,   // 'S'
0x53,   // 'S'
0x44,   // 'D'
0x54,   // 'T'
0x4e,   // length
0x0,    // rev
0x0,
0x0,
0x1,
0x45,   // checksum
0x42,   // 'B'
0x58,   // 'X'

etc.
> > 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';
> > @@ -518,8 +538,6 @@ extern void link_time_assertion(void);
> >
> > static void* build_pcihp(void)
> >   {
> > -    char *sys_states;
> > -    int sys_state_size;
> >       u32 rmvc_pcrm;
> >       int i;
> >
> > @@ -551,19 +569,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 81c57b3..fd9c0bb 100644
> > --- a/src/ssdt-pcihp.dsl
> > +++ b/src/ssdt-pcihp.dsl
> > @@ -58,40 +58,4 @@ DefinitionBlock ("ssdt-pcihp.aml", "SSDT", 0x01,
> > "BXPC", "BXSSDTPCIHP", 0x1)
> >           hotplug_slot(1e)
> >           hotplug_slot(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 */
> > +        })
> > +    }
> > +}

[PATCH 3/6] acpi: rename Processor SSDT constants

[Paolo Bonzini]

A simple rename, to prepare for the later introduction of similar constants
for PCI slots.

Signed-off-by: Paolo Bonzini <[e-mail]>
---
 src/acpi.c |   25 ++++++++++++-------------
 1 file changed, 12 insertions(+), 13 deletions(-)

diff --git a/src/acpi.c b/src/acpi.c
index e6a7ef5..906237c 100644
--- a/src/acpi.c
+++ b/src/acpi.c
@@ -399,14 +399,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)
     // length = ScopeOp + procs + NTYF method + CPON package
     int length = (sizeof(ssdp_susp_aml)
                   + (1+3+4)
-                  + (acpi_cpus * SD_SIZEOF)
+                  + (acpi_cpus * PROC_SIZEOF)
                   + (1+2+5+(12*acpi_cpus))
                   + (6+2+1+(1*acpi_cpus))
                   + 17);
@@ -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

[PATCH 4/6] acpi_extract: fix off-by-one

[Paolo Bonzini]

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 <[e-mail]>
---
 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

[PATCH 5/6] acpi_extract: detect DeviceOp

[Paolo Bonzini]

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 <[e-mail]>
---
 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

[PATCH 5/6] acpi_extract: detect DeviceOp

[Igor Mammedov]

On Tue, Jul 31, 2012 at 11:52:12AM +0200, Paolo Bonzini wrote:
> 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 <[e-mail]>
> ---
>  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

it looks like something I've done before experimenting with containers:
 https://github.com/imammedo/seabios/commit/61b2eb0cecbba06b7a5f47c93a45bde84... 
 
Acked-By: Igor Mammedov <[e-mail]>
> _______________________________________________
> SeaBIOS mailing list
> [e-mail]
> http://www.seabios.org/mailman/listinfo/seabios

[PATCH 6/6] acpi: build PCI hotplug devices from a single template

[Paolo Bonzini]

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 <[e-mail]>
---
        On top of this: do we actually need the S01/S02 devices?  Their
        address is a duplicate of \_SB.PCI0.ISA and \_SB.PCI0.VGA's; and
        they are never removable so _EJ0 is always patched out.  The only
        addition is the _SUN, which can be added in the DSDT.

 src/acpi.c         |   97 +++++++++++++++++++++++++++-------------------------
 src/ssdt-pcihp.dsl |   51 +++++++--------------------
 2 files changed, 63 insertions(+), 85 deletions(-)

diff --git a/src/acpi.c b/src/acpi.c
index 906237c..2d4513b 100644
--- a/src/acpi.c
+++ b/src/acpi.c
@@ -405,11 +405,42 @@ 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 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(ssdp_pcihp_aml + PCIHP_OFFSET_EJ0, "_EJ0", 4)) {
+        warn_internalerror();
+    }
+    if (!eject) {
+        memcpy(ssdt_ptr + PCIHP_OFFSET_EJ0, "EJ0_", 4);
+    }
+}
+
 static void*
 build_ssdt(void)
 {
@@ -420,7 +451,9 @@ build_ssdt(void)
                   + (acpi_cpus * PROC_SIZEOF)
                   + (1+2+5+(12*acpi_cpus))
                   + (6+2+1+(1*acpi_cpus))
-                  + 17);
+                  + 17
+                  + (1+3+4)
+                  + (PCI_SLOTS * PCIHP_SIZEOF));
     u8 *ssdt = malloc_high(length);
     if (! ssdt) {
         warn_noalloc();
@@ -522,6 +555,23 @@ 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';
+
+    // 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;
+    }
+
     build_header((void*)ssdt, SSDT_SIGNATURE, ssdt_ptr - ssdt, 1);
 
     //hexdump(ssdt, ssdt_ptr - ssdt);
@@ -529,50 +579,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)
@@ -754,7 +760,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

[PATCH 6/6] acpi: build PCI hotplug devices from a single template

[Kevin O'Connor]

On Tue, Jul 31, 2012 at 11:52:13AM +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.

I agree with this patch series.  A couple of questions - why not
create the Notify stuff dynamically (as CPU hotplug does) instead of
moving it back to the dsdt?  If a device doesn't support hotplug, why
not skip creating the device instead of patching _EJ0?

-Kevin

[PATCH 6/6] acpi: build PCI hotplug devices from a single template

[Paolo Bonzini]

Il 01/08/2012 02:46, Kevin O'Connor ha scritto:
> I agree with this patch series.  A couple of questions - why not
> create the Notify stuff dynamically (as CPU hotplug does) instead of
> moving it back to the dsdt?

For no particular reason, except that I aimed at 100% correspondence
with the existing SSDT; cleanups can be done on top.  If I were to
resubmit the series with a dynamically-created PCNT, I would add that as
a seventh patch.
> If a device doesn't support hotplug, why
> not skip creating the device instead of patching _EJ0?

1) Because I didn't think of it. :)

2) Because I am not sure of the effect of skipping the _SUN method.

3) Because either you complicate the computation of the size of the
SSDT, or you do not save memory at all.

Paolo

[PATCH 6/6] acpi: build PCI hotplug devices from a single template

[Igor Mammedov]

On 07/31/2012 11:52 AM, 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.
>
> Signed-off-by: Paolo Bonzini <[e-mail]>
> ---
>          On top of this: do we actually need the S01/S02 devices?  Their
>          address is a duplicate of \_SB.PCI0.ISA and \_SB.PCI0.VGA's; and
>          they are never removable so _EJ0 is always patched out.  The only
>          addition is the _SUN, which can be added in the DSDT.
>
> src/acpi.c         |   97 +++++++++++++++++++++++++++-------------------------
>   src/ssdt-pcihp.dsl |   51 +++++++--------------------
>   2 files changed, 63 insertions(+), 85 deletions(-)
>
> diff --git a/src/acpi.c b/src/acpi.c
> index 906237c..2d4513b 100644
> --- a/src/acpi.c
> +++ b/src/acpi.c
> @@ -405,11 +405,42 @@ 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 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(ssdp_pcihp_aml + PCIHP_OFFSET_EJ0, "_EJ0", 4)) {
> +        warn_internalerror();
> +    }
> +    if (!eject) {
> +        memcpy(ssdt_ptr + PCIHP_OFFSET_EJ0, "EJ0_", 4);
> +    }
> +}
> +
>   static void*
>   build_ssdt(void)
>   {
> @@ -420,7 +451,9 @@ build_ssdt(void)
>                     + (acpi_cpus * PROC_SIZEOF)
>                     + (1+2+5+(12*acpi_cpus))
>                     + (6+2+1+(1*acpi_cpus))
> -                  + 17);
> +                  + 17
> +                  + (1+3+4)
> perhaps macros or const here would be better
> +                  + (PCI_SLOTS * PCIHP_SIZEOF));
>       u8 *ssdt = malloc_high(length);
>       if (! ssdt) {
>           warn_noalloc();
> @@ -522,6 +555,23 @@ 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);
> in case we later decide to add something to _SB scope, could it set len 
> to not remaining buffer length but rather actual size of the built PCI0 
> scope.

it also will help to get scope len right if we omit _EJ0 for non 
eject-able devices. we could just not copy method and fixup device len.
> +    *(ssdt_ptr++) = 'P';
> +    *(ssdt_ptr++) = 'C';
> +    *(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;
> +    }
> +
>       build_header((void*)ssdt, SSDT_SIGNATURE, ssdt_ptr - ssdt, 1);
>
> //hexdump(ssdt, ssdt_ptr - ssdt);
> @@ -529,50 +579,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)
> @@ -754,7 +760,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)
>       }
>   }