[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

[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 
---
 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() {

[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 
---
 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 */
+        })
+    }
+}

[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
>
> 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
> >
> > 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 
---
 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)} ...}"

[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 
---
 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

[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 
---
 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"):

[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
>
> 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"):

it looks like something I've done before experimenting with containers:
 https://github.com/imammedo/seabios/commit/61b2eb0cecbba06b7a5f47c93a45bde84...

Acked-By: Igor Mammedov
> 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 
---
        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)
     }
 }

[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
>      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)
> }
> }