[Qemu-devel] [PATCH V2 0/8] pc: acpi: various fixes and cleanups

[Qemu-devel] [PATCH V2 0/8] pc: acpi: various fixes and cleanups

[Igor Mammedov]

changes from v1:
 * drop: [PATCH 7/9] acpi: replace opencoded notify codes with named values
 * use Michael's suggestion to improve build_append_nameseg()
 * drop long scope names and go back to recursion,
   but still significantly simplify building of PCI tree

this series is an attempt to shave off a bunch of
not directly related patches from already big dynamic
AML series (although it's dependency for it)

Tested: on XPsp3 to WS2012R2 and REHL6/7 guests.

Git tree for testing:
 https://github.com/imammedo/qemu/commits/acpi_pci_gen_simplification_v2

Igor Mammedov (8):
  pc: acpi: fix WindowsXP BSOD when memory hotplug is enabled
  pc: acpi: decribe bridge device as not hotpluggable
  pc: acpi-build: cleanup AcpiPmInfo initialization
  acpi: build_append_nameseg(): add padding if necessary
  acpi: move generic aml building helpers into dedictated file
  acpi: add build_append_namestring() helper
  acpi: drop min-bytes in build_package()
  pc: acpi-build: simplify PCI bus tree generation

 hw/acpi/Makefile.objs             |   1 +
 hw/acpi/acpi_gen_utils.c          | 244 +++++++++++++++++++++
 hw/i386/acpi-build.c              | 451 ++++++++------------------------------
 hw/i386/acpi-dsdt-cpu-hotplug.dsl |   1 +
 include/hw/acpi/acpi_gen_utils.h  |  23 ++
 5 files changed, 363 insertions(+), 357 deletions(-)
 create mode 100644 hw/acpi/acpi_gen_utils.c
 create mode 100644 include/hw/acpi/acpi_gen_utils.h

-- 
1.8.3.1

[Qemu-devel] [PATCH V2 1/8] pc: acpi: fix WindowsXP BSOD when memory hotplug is enabled

[Igor Mammedov]

ACPI parser in XP considers PNP0A06 devices of CPU and
memory hotplug as duplicates. Adding unique _UID
to CPU hotplug device fixes BSOD.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 hw/i386/acpi-dsdt-cpu-hotplug.dsl | 1 +
 1 file changed, 1 insertion(+)

diff --git a/hw/i386/acpi-dsdt-cpu-hotplug.dsl b/hw/i386/acpi-dsdt-cpu-hotplug.dsl
index 34aab5a..268d870 100644
--- a/hw/i386/acpi-dsdt-cpu-hotplug.dsl
+++ b/hw/i386/acpi-dsdt-cpu-hotplug.dsl
@@ -94,6 +94,7 @@ Scope(\_SB) {
 
     Device(CPU_HOTPLUG_RESOURCE_DEVICE) {
         Name(_HID, EisaId("PNP0A06"))
+        Name(_UID, "CPU hotplug resources")
 
         Name(_CRS, ResourceTemplate() {
             IO(Decode16, CPU_STATUS_BASE, CPU_STATUS_BASE, 0, CPU_STATUS_LEN)
-- 
1.8.3.1

[Qemu-devel] [PATCH V2 2/8] pc: acpi: decribe bridge device as not hotpluggable

[Igor Mammedov]

when bridge hotplug is disabled, i.e. for machine
types less then 2.0, bridge device was created as
hotpluggable by mistake since commit 133a2da.

Fix it by just creating it as a present device.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 hw/i386/acpi-build.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index b37a397..1fb92e5 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -913,7 +913,7 @@ static void build_pci_bus_end(PCIBus *bus, void *bus_state)
             }
         }
 
-        if (!dc->hotpluggable || bridge_in_acpi) {
+        if (!dc->hotpluggable || pc->is_bridge) {
             clear_bit(slot, slot_hotplug_enable);
         }
     }
-- 
1.8.3.1

[Qemu-devel] [PATCH V2 3/8] pc: acpi-build: cleanup AcpiPmInfo initialization

[Igor Mammedov]

zero initialize AcpiPmInfo struct to reduce code bloat
a little bit.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 hw/i386/acpi-build.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 1fb92e5..f5ec66a 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -161,6 +161,8 @@ static void acpi_get_pm_info(AcpiPmInfo *pm)
     Object *obj = NULL;
     QObject *o;
 
+    memset(pm, 0, sizeof(*pm));
+
     if (piix) {
         obj = piix;
     }
@@ -173,22 +175,16 @@ static void acpi_get_pm_info(AcpiPmInfo *pm)
     o = object_property_get_qobject(obj, ACPI_PM_PROP_S3_DISABLED, NULL);
     if (o) {
         pm->s3_disabled = qint_get_int(qobject_to_qint(o));
-    } else {
-        pm->s3_disabled = false;
     }
     qobject_decref(o);
     o = object_property_get_qobject(obj, ACPI_PM_PROP_S4_DISABLED, NULL);
     if (o) {
         pm->s4_disabled = qint_get_int(qobject_to_qint(o));
-    } else {
-        pm->s4_disabled = false;
     }
     qobject_decref(o);
     o = object_property_get_qobject(obj, ACPI_PM_PROP_S4_VAL, NULL);
     if (o) {
         pm->s4_val = qint_get_int(qobject_to_qint(o));
-    } else {
-        pm->s4_val = false;
     }
     qobject_decref(o);
 
-- 
1.8.3.1

[Qemu-devel] [PATCH V2 4/8] acpi: build_append_nameseg(): add padding if necessary

[Igor Mammedov]

According to ACPI spec NameSeg shorter than 4 characters
must be padded up to 4 characters with "_" symbol.
ACPI 5.0:  20.2.2 "Name Objects Encoding"

Do it in build_append_nameseg() so that caller shouldn't know
or care about it.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
v2:
  * simplify padding, suggested by: "Michael S. Tsirkin" <mst@redhat.com>
---
 hw/i386/acpi-build.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index f5ec66a..2bf9a09 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -292,6 +292,8 @@ static inline void build_append_array(GArray *array, GArray *val)
     g_array_append_vals(array, val->data, val->len);
 }
 
+#define ACPI_NAMESEG_LEN 4
+
 static void GCC_FMT_ATTR(2, 3)
 build_append_nameseg(GArray *array, const char *format, ...)
 {
@@ -304,8 +306,11 @@ build_append_nameseg(GArray *array, const char *format, ...)
     len = vsnprintf(s, sizeof s, format, args);
     va_end(args);
 
-    assert(len == 4);
+    assert(len <= ACPI_NAMESEG_LEN);
+
     g_array_append_vals(array, s, len);
+    /* Pad up to ACPI_NAMESEG_LEN characters if necessary. */
+    g_array_append_vals(array, "____", ACPI_NAMESEG_LEN - len);
 }
 
 /* 5.4 Definition Block Encoding */
@@ -846,7 +851,7 @@ static void build_pci_bus_end(PCIBus *bus, void *bus_state)
 
     if (bus->parent_dev) {
         op = 0x82; /* DeviceOp */
-        build_append_nameseg(bus_table, "S%.02X_",
+        build_append_nameseg(bus_table, "S%.02X",
                              bus->parent_dev->devfn);
         build_append_byte(bus_table, 0x08); /* NameOp */
         build_append_nameseg(bus_table, "_SUN");
@@ -966,7 +971,7 @@ static void build_pci_bus_end(PCIBus *bus, void *bus_state)
             build_append_int(notify, 0x1U << i);
             build_append_byte(notify, 0x00); /* NullName */
             build_append_byte(notify, 0x86); /* NotifyOp */
-            build_append_nameseg(notify, "S%.02X_", PCI_DEVFN(i, 0));
+            build_append_nameseg(notify, "S%.02X", PCI_DEVFN(i, 0));
             build_append_byte(notify, 0x69); /* Arg1Op */
 
             /* Pack it up */
@@ -1023,7 +1028,7 @@ static void build_pci_bus_end(PCIBus *bus, void *bus_state)
         if (bus->parent_dev) {
             build_append_byte(parent->notify_table, '^'); /* ParentPrefixChar */
             build_append_byte(parent->notify_table, 0x2E); /* DualNamePrefix */
-            build_append_nameseg(parent->notify_table, "S%.02X_",
+            build_append_nameseg(parent->notify_table, "S%.02X",
                                  bus->parent_dev->devfn);
             build_append_nameseg(parent->notify_table, "PCNT");
         }
@@ -1093,7 +1098,7 @@ build_ssdt(GArray *table_data, GArray *linker,
         GArray *sb_scope = build_alloc_array();
         uint8_t op = 0x10; /* ScopeOp */
 
-        build_append_nameseg(sb_scope, "_SB_");
+        build_append_nameseg(sb_scope, "_SB");
 
         /* build Processor object for each processor */
         for (i = 0; i < acpi_cpus; i++) {
-- 
1.8.3.1

[Qemu-devel] [PATCH V2 5/8] acpi: move generic aml building helpers into dedictated file

[Igor Mammedov]

the will be later used for composing AML primitives
and all that could be reused later for ARM machines
as well.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 hw/acpi/Makefile.objs            |   1 +
 hw/acpi/acpi_gen_utils.c         | 166 +++++++++++++++++++++++++++++++++++++++
 hw/i386/acpi-build.c             | 162 +-------------------------------------
 include/hw/acpi/acpi_gen_utils.h |  23 ++++++
 4 files changed, 192 insertions(+), 160 deletions(-)
 create mode 100644 hw/acpi/acpi_gen_utils.c
 create mode 100644 include/hw/acpi/acpi_gen_utils.h

diff --git a/hw/acpi/Makefile.objs b/hw/acpi/Makefile.objs
index acd2389..4237232 100644
--- a/hw/acpi/Makefile.objs
+++ b/hw/acpi/Makefile.objs
@@ -1,3 +1,4 @@
 common-obj-$(CONFIG_ACPI) += core.o piix4.o ich9.o pcihp.o cpu_hotplug.o
 common-obj-$(CONFIG_ACPI) += memory_hotplug.o
 common-obj-$(CONFIG_ACPI) += acpi_interface.o
+common-obj-$(CONFIG_ACPI) += acpi_gen_utils.o
diff --git a/hw/acpi/acpi_gen_utils.c b/hw/acpi/acpi_gen_utils.c
new file mode 100644
index 0000000..82d9cb7
--- /dev/null
+++ b/hw/acpi/acpi_gen_utils.c
@@ -0,0 +1,166 @@
+#include <stdio.h>
+#include <stdarg.h>
+#include <assert.h>
+#include <stdbool.h>
+#include "hw/acpi/acpi_gen_utils.h"
+
+GArray *build_alloc_array(void)
+{
+        return g_array_new(false, true /* clear */, 1);
+}
+
+void build_free_array(GArray *array)
+{
+        g_array_free(array, true);
+}
+
+void build_prepend_byte(GArray *array, uint8_t val)
+{
+    g_array_prepend_val(array, val);
+}
+
+void build_append_byte(GArray *array, uint8_t val)
+{
+    g_array_append_val(array, val);
+}
+
+void build_append_array(GArray *array, GArray *val)
+{
+    g_array_append_vals(array, val->data, val->len);
+}
+
+#define ACPI_NAMESEG_LEN 4
+
+void GCC_FMT_ATTR(2, 3)
+build_append_nameseg(GArray *array, const char *format, ...)
+{
+    /* It would be nicer to use g_string_vprintf but it's only there in 2.22 */
+    char s[] = "XXXX";
+    int len;
+    va_list args;
+
+    va_start(args, format);
+    len = vsnprintf(s, sizeof s, format, args);
+    va_end(args);
+
+    assert(len <= ACPI_NAMESEG_LEN);
+
+    g_array_append_vals(array, s, len);
+    /* Pad up to ACPI_NAMESEG_LEN characters if necessary. */
+    g_array_append_vals(array, "____", ACPI_NAMESEG_LEN - len);
+}
+
+/* 5.4 Definition Block Encoding */
+enum {
+    PACKAGE_LENGTH_1BYTE_SHIFT = 6, /* Up to 63 - use extra 2 bits. */
+    PACKAGE_LENGTH_2BYTE_SHIFT = 4,
+    PACKAGE_LENGTH_3BYTE_SHIFT = 12,
+    PACKAGE_LENGTH_4BYTE_SHIFT = 20,
+};
+
+void build_prepend_package_length(GArray *package, unsigned min_bytes)
+{
+    uint8_t byte;
+    unsigned length = package->len;
+    unsigned length_bytes;
+
+    if (length + 1 < (1 << PACKAGE_LENGTH_1BYTE_SHIFT)) {
+        length_bytes = 1;
+    } else if (length + 2 < (1 << PACKAGE_LENGTH_3BYTE_SHIFT)) {
+        length_bytes = 2;
+    } else if (length + 3 < (1 << PACKAGE_LENGTH_4BYTE_SHIFT)) {
+        length_bytes = 3;
+    } else {
+        length_bytes = 4;
+    }
+
+    /* Force length to at least min_bytes.
+     * This wastes memory but that's how bios did it.
+     */
+    length_bytes = MAX(length_bytes, min_bytes);
+
+    /* PkgLength is the length of the inclusive length of the data. */
+    length += length_bytes;
+
+    switch (length_bytes) {
+    case 1:
+        byte = length;
+        build_prepend_byte(package, byte);
+        return;
+    case 4:
+        byte = length >> PACKAGE_LENGTH_4BYTE_SHIFT;
+        build_prepend_byte(package, byte);
+        length &= (1 << PACKAGE_LENGTH_4BYTE_SHIFT) - 1;
+        /* fall through */
+    case 3:
+        byte = length >> PACKAGE_LENGTH_3BYTE_SHIFT;
+        build_prepend_byte(package, byte);
+        length &= (1 << PACKAGE_LENGTH_3BYTE_SHIFT) - 1;
+        /* fall through */
+    case 2:
+        byte = length >> PACKAGE_LENGTH_2BYTE_SHIFT;
+        build_prepend_byte(package, byte);
+        length &= (1 << PACKAGE_LENGTH_2BYTE_SHIFT) - 1;
+        /* fall through */
+    }
+    /*
+     * Most significant two bits of byte zero indicate how many following bytes
+     * are in PkgLength encoding.
+     */
+    byte = ((length_bytes - 1) << PACKAGE_LENGTH_1BYTE_SHIFT) | length;
+    build_prepend_byte(package, byte);
+}
+
+void build_package(GArray *package, uint8_t op, unsigned min_bytes)
+{
+    build_prepend_package_length(package, min_bytes);
+    build_prepend_byte(package, op);
+}
+
+void build_extop_package(GArray *package, uint8_t op)
+{
+    build_package(package, op, 1);
+    build_prepend_byte(package, 0x5B); /* ExtOpPrefix */
+}
+
+void build_append_value(GArray *table, uint32_t value, int size)
+{
+    uint8_t prefix;
+    int i;
+
+    switch (size) {
+    case 1:
+        prefix = 0x0A; /* BytePrefix */
+        break;
+    case 2:
+        prefix = 0x0B; /* WordPrefix */
+        break;
+    case 4:
+        prefix = 0x0C; /* DWordPrefix */
+        break;
+    default:
+        assert(0);
+        return;
+    }
+    build_append_byte(table, prefix);
+    for (i = 0; i < size; ++i) {
+        build_append_byte(table, value & 0xFF);
+        value = value >> 8;
+    }
+}
+
+void build_append_int(GArray *table, uint32_t value)
+{
+    if (value == 0x00) {
+        build_append_byte(table, 0x00); /* ZeroOp */
+    } else if (value == 0x01) {
+        build_append_byte(table, 0x01); /* OneOp */
+    } else if (value <= 0xFF) {
+        build_append_value(table, value, 1);
+    } else if (value <= 0xFFFF) {
+        build_append_value(table, value, 2);
+    } else {
+        build_append_value(table, value, 4);
+    }
+}
+
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 2bf9a09..870e4a0 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -54,6 +54,8 @@
 #include "hw/i386/q35-acpi-dsdt.hex"
 #include "hw/i386/acpi-dsdt.hex"
 
+#include "hw/acpi/acpi_gen_utils.h"
+
 #include "qapi/qmp/qint.h"
 #include "qom/qom-qobject.h"
 #include "exec/ram_addr.h"
@@ -267,166 +269,6 @@ build_header(GArray *linker, GArray *table_data,
                                     table_data->data, h, len, &h->checksum);
 }
 
-static inline GArray *build_alloc_array(void)
-{
-        return g_array_new(false, true /* clear */, 1);
-}
-
-static inline void build_free_array(GArray *array)
-{
-        g_array_free(array, true);
-}
-
-static inline void build_prepend_byte(GArray *array, uint8_t val)
-{
-    g_array_prepend_val(array, val);
-}
-
-static inline void build_append_byte(GArray *array, uint8_t val)
-{
-    g_array_append_val(array, val);
-}
-
-static inline void build_append_array(GArray *array, GArray *val)
-{
-    g_array_append_vals(array, val->data, val->len);
-}
-
-#define ACPI_NAMESEG_LEN 4
-
-static void GCC_FMT_ATTR(2, 3)
-build_append_nameseg(GArray *array, const char *format, ...)
-{
-    /* It would be nicer to use g_string_vprintf but it's only there in 2.22 */
-    char s[] = "XXXX";
-    int len;
-    va_list args;
-
-    va_start(args, format);
-    len = vsnprintf(s, sizeof s, format, args);
-    va_end(args);
-
-    assert(len <= ACPI_NAMESEG_LEN);
-
-    g_array_append_vals(array, s, len);
-    /* Pad up to ACPI_NAMESEG_LEN characters if necessary. */
-    g_array_append_vals(array, "____", ACPI_NAMESEG_LEN - len);
-}
-
-/* 5.4 Definition Block Encoding */
-enum {
-    PACKAGE_LENGTH_1BYTE_SHIFT = 6, /* Up to 63 - use extra 2 bits. */
-    PACKAGE_LENGTH_2BYTE_SHIFT = 4,
-    PACKAGE_LENGTH_3BYTE_SHIFT = 12,
-    PACKAGE_LENGTH_4BYTE_SHIFT = 20,
-};
-
-static void build_prepend_package_length(GArray *package, unsigned min_bytes)
-{
-    uint8_t byte;
-    unsigned length = package->len;
-    unsigned length_bytes;
-
-    if (length + 1 < (1 << PACKAGE_LENGTH_1BYTE_SHIFT)) {
-        length_bytes = 1;
-    } else if (length + 2 < (1 << PACKAGE_LENGTH_3BYTE_SHIFT)) {
-        length_bytes = 2;
-    } else if (length + 3 < (1 << PACKAGE_LENGTH_4BYTE_SHIFT)) {
-        length_bytes = 3;
-    } else {
-        length_bytes = 4;
-    }
-
-    /* Force length to at least min_bytes.
-     * This wastes memory but that's how bios did it.
-     */
-    length_bytes = MAX(length_bytes, min_bytes);
-
-    /* PkgLength is the length of the inclusive length of the data. */
-    length += length_bytes;
-
-    switch (length_bytes) {
-    case 1:
-        byte = length;
-        build_prepend_byte(package, byte);
-        return;
-    case 4:
-        byte = length >> PACKAGE_LENGTH_4BYTE_SHIFT;
-        build_prepend_byte(package, byte);
-        length &= (1 << PACKAGE_LENGTH_4BYTE_SHIFT) - 1;
-        /* fall through */
-    case 3:
-        byte = length >> PACKAGE_LENGTH_3BYTE_SHIFT;
-        build_prepend_byte(package, byte);
-        length &= (1 << PACKAGE_LENGTH_3BYTE_SHIFT) - 1;
-        /* fall through */
-    case 2:
-        byte = length >> PACKAGE_LENGTH_2BYTE_SHIFT;
-        build_prepend_byte(package, byte);
-        length &= (1 << PACKAGE_LENGTH_2BYTE_SHIFT) - 1;
-        /* fall through */
-    }
-    /*
-     * Most significant two bits of byte zero indicate how many following bytes
-     * are in PkgLength encoding.
-     */
-    byte = ((length_bytes - 1) << PACKAGE_LENGTH_1BYTE_SHIFT) | length;
-    build_prepend_byte(package, byte);
-}
-
-static void build_package(GArray *package, uint8_t op, unsigned min_bytes)
-{
-    build_prepend_package_length(package, min_bytes);
-    build_prepend_byte(package, op);
-}
-
-static void build_extop_package(GArray *package, uint8_t op)
-{
-    build_package(package, op, 1);
-    build_prepend_byte(package, 0x5B); /* ExtOpPrefix */
-}
-
-static void build_append_value(GArray *table, uint32_t value, int size)
-{
-    uint8_t prefix;
-    int i;
-
-    switch (size) {
-    case 1:
-        prefix = 0x0A; /* BytePrefix */
-        break;
-    case 2:
-        prefix = 0x0B; /* WordPrefix */
-        break;
-    case 4:
-        prefix = 0x0C; /* DWordPrefix */
-        break;
-    default:
-        assert(0);
-        return;
-    }
-    build_append_byte(table, prefix);
-    for (i = 0; i < size; ++i) {
-        build_append_byte(table, value & 0xFF);
-        value = value >> 8;
-    }
-}
-
-static void build_append_int(GArray *table, uint32_t value)
-{
-    if (value == 0x00) {
-        build_append_byte(table, 0x00); /* ZeroOp */
-    } else if (value == 0x01) {
-        build_append_byte(table, 0x01); /* OneOp */
-    } else if (value <= 0xFF) {
-        build_append_value(table, value, 1);
-    } else if (value <= 0xFFFF) {
-        build_append_value(table, value, 2);
-    } else {
-        build_append_value(table, value, 4);
-    }
-}
-
 static GArray *build_alloc_method(const char *name, uint8_t arg_count)
 {
     GArray *method = build_alloc_array();
diff --git a/include/hw/acpi/acpi_gen_utils.h b/include/hw/acpi/acpi_gen_utils.h
new file mode 100644
index 0000000..e6a0b28
--- /dev/null
+++ b/include/hw/acpi/acpi_gen_utils.h
@@ -0,0 +1,23 @@
+#ifndef HW_ACPI_GEN_UTILS_H
+#define HW_ACPI_GEN_UTILS_H
+
+#include <stdint.h>
+#include <glib.h>
+#include "qemu/compiler.h"
+
+GArray *build_alloc_array(void);
+void build_free_array(GArray *array);
+void build_prepend_byte(GArray *array, uint8_t val);
+void build_append_byte(GArray *array, uint8_t val);
+void build_append_array(GArray *array, GArray *val);
+
+void GCC_FMT_ATTR(2, 3)
+build_append_nameseg(GArray *array, const char *format, ...);
+
+void build_prepend_package_length(GArray *package, unsigned min_bytes);
+void build_package(GArray *package, uint8_t op, unsigned min_bytes);
+void build_append_value(GArray *table, uint32_t value, int size);
+void build_append_int(GArray *table, uint32_t value);
+void build_extop_package(GArray *package, uint8_t op);
+
+#endif
-- 
1.8.3.1

[Qemu-devel] [PATCH V2 6/8] acpi: add build_append_namestring() helper

[Igor Mammedov]

Use build_append_namestring() instead of build_append_nameseg()
So user won't have to care whether name is NameSeg, NamePath or
NameString.

See for refernce ACPI 5.0: 20.2.2 Name Objects Encoding

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 hw/acpi/acpi_gen_utils.c         | 86 +++++++++++++++++++++++++++++++++++++++-
 hw/i386/acpi-build.c             | 35 ++++++++--------
 include/hw/acpi/acpi_gen_utils.h |  2 +-
 3 files changed, 102 insertions(+), 21 deletions(-)

diff --git a/hw/acpi/acpi_gen_utils.c b/hw/acpi/acpi_gen_utils.c
index 82d9cb7..2e52690 100644
--- a/hw/acpi/acpi_gen_utils.c
+++ b/hw/acpi/acpi_gen_utils.c
@@ -31,7 +31,7 @@ void build_append_array(GArray *array, GArray *val)
 
 #define ACPI_NAMESEG_LEN 4
 
-void GCC_FMT_ATTR(2, 3)
+static void GCC_FMT_ATTR(2, 3)
 build_append_nameseg(GArray *array, const char *format, ...)
 {
     /* It would be nicer to use g_string_vprintf but it's only there in 2.22 */
@@ -50,6 +50,90 @@ build_append_nameseg(GArray *array, const char *format, ...)
     g_array_append_vals(array, "____", ACPI_NAMESEG_LEN - len);
 }
 
+static const uint8_t ACPI_DualNamePrefix  = 0x2E;
+static const uint8_t ACPI_MultiNamePrefix = 0x2F;
+
+static void
+build_append_namestringv(GArray *array, const char *format, va_list ap)
+{
+    /* It would be nicer to use g_string_vprintf but it's only there in 2.22 */
+    char *s;
+    int len;
+    va_list va_len;
+    char **segs;
+    char **segs_iter;
+    int seg_count = 0;
+
+    va_copy(va_len, ap);
+    len = vsnprintf(NULL, 0, format, va_len);
+    va_end(va_len);
+    len += 1;
+    s = g_new(typeof(*s), len);
+
+    len = vsnprintf(s, len, format, ap);
+
+    segs = g_strsplit(s, ".", 0);
+    g_free(s);
+
+    /* count segments */
+    segs_iter = segs;
+    while (*segs_iter) {
+        ++segs_iter;
+        ++seg_count;
+    }
+
+    /* handle RootPath || PrefixPath */
+    s = *segs;
+    while (*s == '\\' || *s == '^') {
+        g_array_append_val(array, *s);
+        ++s;
+    }
+
+    switch (seg_count) {
+    case 1:
+        if (!*s) { /* NullName */
+            const uint8_t nullpath = 0;
+            g_array_append_val(array, nullpath);
+        } else {
+            build_append_nameseg(array, "%s", s);
+        }
+        break;
+
+    case 2:
+        g_array_append_val(array, ACPI_DualNamePrefix);
+        build_append_nameseg(array, "%s", s);
+        build_append_nameseg(array, "%s", segs[1]);
+        break;
+
+    default:
+        g_array_append_val(array, ACPI_MultiNamePrefix);
+        assert(seg_count <= 0xFF); /* ByteData Max */
+        g_array_append_val(array, seg_count);
+
+        /* handle the 1st segment manually due to prefix/root path */
+        build_append_nameseg(array, "%s", s);
+
+        /* add the rest of segments */
+        segs_iter = segs + 1;
+        while (*segs_iter) {
+            build_append_nameseg(array, "%s", *segs_iter);
+            ++segs_iter;
+        }
+        break;
+    }
+    g_strfreev(segs);
+}
+
+void GCC_FMT_ATTR(2, 3)
+build_append_namestring(GArray *array, const char *format, ...)
+{
+    va_list ap;
+
+    va_start(ap, format);
+    build_append_namestringv(array, format, ap);
+    va_end(ap);
+}
+
 /* 5.4 Definition Block Encoding */
 enum {
     PACKAGE_LENGTH_1BYTE_SHIFT = 6, /* Up to 63 - use extra 2 bits. */
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 870e4a0..0f6202d 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -273,7 +273,7 @@ static GArray *build_alloc_method(const char *name, uint8_t arg_count)
 {
     GArray *method = build_alloc_array();
 
-    build_append_nameseg(method, "%s", name);
+    build_append_namestring(method, "%s", name);
     build_append_byte(method, arg_count); /* MethodFlags: ArgCount */
 
     return method;
@@ -565,7 +565,7 @@ build_append_notify_method(GArray *device, const char *name,
 
     for (i = 0; i < count; i++) {
         GArray *target = build_alloc_array();
-        build_append_nameseg(target, format, i);
+        build_append_namestring(target, format, i);
         assert(i < 256); /* Fits in 1 byte */
         build_append_notify_target_ifequal(method, target, i, 1);
         build_free_array(target);
@@ -693,24 +693,24 @@ static void build_pci_bus_end(PCIBus *bus, void *bus_state)
 
     if (bus->parent_dev) {
         op = 0x82; /* DeviceOp */
-        build_append_nameseg(bus_table, "S%.02X",
+        build_append_namestring(bus_table, "S%.02X",
                              bus->parent_dev->devfn);
         build_append_byte(bus_table, 0x08); /* NameOp */
-        build_append_nameseg(bus_table, "_SUN");
+        build_append_namestring(bus_table, "_SUN");
         build_append_value(bus_table, PCI_SLOT(bus->parent_dev->devfn), 1);
         build_append_byte(bus_table, 0x08); /* NameOp */
-        build_append_nameseg(bus_table, "_ADR");
+        build_append_namestring(bus_table, "_ADR");
         build_append_value(bus_table, (PCI_SLOT(bus->parent_dev->devfn) << 16) |
                            PCI_FUNC(bus->parent_dev->devfn), 4);
     } else {
         op = 0x10; /* ScopeOp */;
-        build_append_nameseg(bus_table, "PCI0");
+        build_append_namestring(bus_table, "PCI0");
     }
 
     bsel = object_property_get_qobject(OBJECT(bus), ACPI_PCIHP_PROP_BSEL, NULL);
     if (bsel) {
         build_append_byte(bus_table, 0x08); /* NameOp */
-        build_append_nameseg(bus_table, "BSEL");
+        build_append_namestring(bus_table, "BSEL");
         build_append_int(bus_table, qint_get_int(qobject_to_qint(bsel)));
         memset(slot_hotplug_enable, 0xff, sizeof slot_hotplug_enable);
     } else {
@@ -813,7 +813,7 @@ static void build_pci_bus_end(PCIBus *bus, void *bus_state)
             build_append_int(notify, 0x1U << i);
             build_append_byte(notify, 0x00); /* NullName */
             build_append_byte(notify, 0x86); /* NotifyOp */
-            build_append_nameseg(notify, "S%.02X", PCI_DEVFN(i, 0));
+            build_append_namestring(notify, "S%.02X", PCI_DEVFN(i, 0));
             build_append_byte(notify, 0x69); /* Arg1Op */
 
             /* Pack it up */
@@ -837,12 +837,12 @@ static void build_pci_bus_end(PCIBus *bus, void *bus_state)
         if (bsel) {
             build_append_byte(method, 0x70); /* StoreOp */
             build_append_int(method, qint_get_int(qobject_to_qint(bsel)));
-            build_append_nameseg(method, "BNUM");
-            build_append_nameseg(method, "DVNT");
-            build_append_nameseg(method, "PCIU");
+            build_append_namestring(method, "BNUM");
+            build_append_namestring(method, "DVNT");
+            build_append_namestring(method, "PCIU");
             build_append_int(method, 1); /* Device Check */
-            build_append_nameseg(method, "DVNT");
-            build_append_nameseg(method, "PCID");
+            build_append_namestring(method, "DVNT");
+            build_append_namestring(method, "PCID");
             build_append_int(method, 3); /* Eject Request */
         }
 
@@ -868,11 +868,8 @@ static void build_pci_bus_end(PCIBus *bus, void *bus_state)
          * At the moment this is not needed for root as we have a single root.
          */
         if (bus->parent_dev) {
-            build_append_byte(parent->notify_table, '^'); /* ParentPrefixChar */
-            build_append_byte(parent->notify_table, 0x2E); /* DualNamePrefix */
-            build_append_nameseg(parent->notify_table, "S%.02X",
+            build_append_namestring(parent->notify_table, "^PCNT.S%.02X",
                                  bus->parent_dev->devfn);
-            build_append_nameseg(parent->notify_table, "PCNT");
         }
     }
 
@@ -940,7 +937,7 @@ build_ssdt(GArray *table_data, GArray *linker,
         GArray *sb_scope = build_alloc_array();
         uint8_t op = 0x10; /* ScopeOp */
 
-        build_append_nameseg(sb_scope, "_SB");
+        build_append_namestring(sb_scope, "_SB");
 
         /* build Processor object for each processor */
         for (i = 0; i < acpi_cpus; i++) {
@@ -960,7 +957,7 @@ build_ssdt(GArray *table_data, GArray *linker,
 
         /* build "Name(CPON, Package() { One, One, ..., Zero, Zero, ... })" */
         build_append_byte(sb_scope, 0x08); /* NameOp */
-        build_append_nameseg(sb_scope, "CPON");
+        build_append_namestring(sb_scope, "CPON");
 
         {
             GArray *package = build_alloc_array();
diff --git a/include/hw/acpi/acpi_gen_utils.h b/include/hw/acpi/acpi_gen_utils.h
index e6a0b28..fd50625 100644
--- a/include/hw/acpi/acpi_gen_utils.h
+++ b/include/hw/acpi/acpi_gen_utils.h
@@ -12,7 +12,7 @@ void build_append_byte(GArray *array, uint8_t val);
 void build_append_array(GArray *array, GArray *val);
 
 void GCC_FMT_ATTR(2, 3)
-build_append_nameseg(GArray *array, const char *format, ...);
+build_append_namestring(GArray *array, const char *format, ...);
 
 void build_prepend_package_length(GArray *package, unsigned min_bytes);
 void build_package(GArray *package, uint8_t op, unsigned min_bytes);
-- 
1.8.3.1

[Qemu-devel] [PATCH V2 7/8] acpi: drop min-bytes in build_package()

[Igor Mammedov]

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 hw/acpi/acpi_gen_utils.c         | 14 ++++----------
 hw/i386/acpi-build.c             | 13 ++++++-------
 include/hw/acpi/acpi_gen_utils.h |  4 ++--
 3 files changed, 12 insertions(+), 19 deletions(-)

diff --git a/hw/acpi/acpi_gen_utils.c b/hw/acpi/acpi_gen_utils.c
index 2e52690..291b938 100644
--- a/hw/acpi/acpi_gen_utils.c
+++ b/hw/acpi/acpi_gen_utils.c
@@ -142,7 +142,7 @@ enum {
     PACKAGE_LENGTH_4BYTE_SHIFT = 20,
 };
 
-void build_prepend_package_length(GArray *package, unsigned min_bytes)
+void build_prepend_package_length(GArray *package)
 {
     uint8_t byte;
     unsigned length = package->len;
@@ -158,11 +158,6 @@ void build_prepend_package_length(GArray *package, unsigned min_bytes)
         length_bytes = 4;
     }
 
-    /* Force length to at least min_bytes.
-     * This wastes memory but that's how bios did it.
-     */
-    length_bytes = MAX(length_bytes, min_bytes);
-
     /* PkgLength is the length of the inclusive length of the data. */
     length += length_bytes;
 
@@ -195,15 +190,15 @@ void build_prepend_package_length(GArray *package, unsigned min_bytes)
     build_prepend_byte(package, byte);
 }
 
-void build_package(GArray *package, uint8_t op, unsigned min_bytes)
+void build_package(GArray *package, uint8_t op)
 {
-    build_prepend_package_length(package, min_bytes);
+    build_prepend_package_length(package);
     build_prepend_byte(package, op);
 }
 
 void build_extop_package(GArray *package, uint8_t op)
 {
-    build_package(package, op, 1);
+    build_package(package, op);
     build_prepend_byte(package, 0x5B); /* ExtOpPrefix */
 }
 
@@ -247,4 +242,3 @@ void build_append_int(GArray *table, uint32_t value)
         build_append_value(table, value, 4);
     }
 }
-
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 0f6202d..707ac7e 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -283,7 +283,7 @@ static void build_append_and_cleanup_method(GArray *device, GArray *method)
 {
     uint8_t op = 0x14; /* MethodOp */
 
-    build_package(method, op, 0);
+    build_package(method, op);
 
     build_append_array(device, method);
     build_free_array(method);
@@ -304,7 +304,7 @@ static void build_append_notify_target_ifequal(GArray *method,
     build_append_byte(notify, 0x69); /* Arg1Op */
 
     /* Pack it up */
-    build_package(notify, op, 1);
+    build_package(notify, op);
 
     build_append_array(method, notify);
 
@@ -817,7 +817,7 @@ static void build_pci_bus_end(PCIBus *bus, void *bus_state)
             build_append_byte(notify, 0x69); /* Arg1Op */
 
             /* Pack it up */
-            build_package(notify, op, 0);
+            build_package(notify, op);
 
             build_append_array(method, notify);
 
@@ -858,7 +858,7 @@ static void build_pci_bus_end(PCIBus *bus, void *bus_state)
         if (bus->parent_dev) {
             build_extop_package(bus_table, op);
         } else {
-            build_package(bus_table, op, 0);
+            build_package(bus_table, op);
         }
 
         /* Append our bus description to parent table */
@@ -981,7 +981,7 @@ build_ssdt(GArray *table_data, GArray *linker,
                 build_append_byte(package, b);
             }
 
-            build_package(package, op, 2);
+            build_package(package, op);
             build_append_array(sb_scope, package);
             build_free_array(package);
         }
@@ -1029,8 +1029,7 @@ build_ssdt(GArray *table_data, GArray *linker,
             build_append_array(sb_scope, hotplug_state.device_table);
             build_pci_bus_state_cleanup(&hotplug_state);
         }
-
-        build_package(sb_scope, op, 3);
+        build_package(sb_scope, op);
         build_append_array(table_data, sb_scope);
         build_free_array(sb_scope);
     }
diff --git a/include/hw/acpi/acpi_gen_utils.h b/include/hw/acpi/acpi_gen_utils.h
index fd50625..199f003 100644
--- a/include/hw/acpi/acpi_gen_utils.h
+++ b/include/hw/acpi/acpi_gen_utils.h
@@ -14,8 +14,8 @@ void build_append_array(GArray *array, GArray *val);
 void GCC_FMT_ATTR(2, 3)
 build_append_namestring(GArray *array, const char *format, ...);
 
-void build_prepend_package_length(GArray *package, unsigned min_bytes);
-void build_package(GArray *package, uint8_t op, unsigned min_bytes);
+void build_prepend_package_length(GArray *package);
+void build_package(GArray *package, uint8_t op);
 void build_append_value(GArray *table, uint32_t value, int size);
 void build_append_int(GArray *table, uint32_t value);
 void build_extop_package(GArray *package, uint8_t op);
-- 
1.8.3.1

[Qemu-devel] [PATCH V2 8/8] pc: acpi-build: simplify PCI bus tree generation

[Igor Mammedov]

it basicaly does the same as original approach,
* just without bus/notify tables tracking (less obscure)
  which is easier to follow.
* drops unnecessary loops and bitmaps,
  creating devices and notification method in the same loop.
* saves us ~100LOC

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 hw/i386/acpi-build.c | 262 ++++++++++++++++-----------------------------------
 1 file changed, 80 insertions(+), 182 deletions(-)

diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 707ac7e..f18b294 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -95,7 +95,6 @@ typedef struct AcpiPmInfo {
 typedef struct AcpiMiscInfo {
     bool has_hpet;
     bool has_tpm;
-    DECLARE_BITMAP(slot_hotplug_enable, PCI_SLOT_MAX);
     const unsigned char *dsdt_code;
     unsigned dsdt_size;
     uint16_t pvpanic_port;
@@ -641,242 +640,147 @@ static void acpi_set_pci_info(void)
     }
 }
 
-static void build_pci_bus_state_init(AcpiBuildPciBusHotplugState *state,
-                                     AcpiBuildPciBusHotplugState *parent,
-                                     bool pcihp_bridge_en)
+static void build_append_pcihp_notify(GArray *method, int slot)
 {
-    state->parent = parent;
-    state->device_table = build_alloc_array();
-    state->notify_table = build_alloc_array();
-    state->pcihp_bridge_en = pcihp_bridge_en;
-}
+    GArray *notify;
 
-static void build_pci_bus_state_cleanup(AcpiBuildPciBusHotplugState *state)
-{
-    build_free_array(state->device_table);
-    build_free_array(state->notify_table);
+    notify = build_alloc_array();
+    build_append_byte(notify, 0x7B); /* AndOp */
+    build_append_byte(notify, 0x68); /* Arg0Op */
+    build_append_int(notify, 0x1U << slot);
+    build_append_byte(notify, 0x00); /* NullName */
+    build_append_byte(notify, 0x86); /* NotifyOp */
+    build_append_namestring(notify, "S%.02X", PCI_DEVFN(slot, 0));
+    build_append_byte(notify, 0x69); /* Arg1Op */
+
+    /* Pack it up */
+    build_package(notify, 0xA0 /* IfOp */);
+    build_append_array(method, notify);
+    build_free_array(notify);
 }
 
-static void *build_pci_bus_begin(PCIBus *bus, void *parent_state)
+static void build_append_pcihp_dev(GArray *table, int slot)
 {
-    AcpiBuildPciBusHotplugState *parent = parent_state;
-    AcpiBuildPciBusHotplugState *child = g_malloc(sizeof *child);
-
-    build_pci_bus_state_init(child, parent, parent->pcihp_bridge_en);
+    void *pcihp = acpi_data_push(table, ACPI_PCINOHP_SIZEOF);
 
-    return child;
+    memcpy(pcihp, ACPI_PCINOHP_AML, ACPI_PCINOHP_SIZEOF);
+    patch_pcinohp(slot, pcihp);
 }
 
-static void build_pci_bus_end(PCIBus *bus, void *bus_state)
+static void build_append_pci_bus_devices(GArray *parent_scope, PCIBus *bus,
+                                         bool pcihp_bridge_en)
 {
-    AcpiBuildPciBusHotplugState *child = bus_state;
-    AcpiBuildPciBusHotplugState *parent = child->parent;
     GArray *bus_table = build_alloc_array();
-    DECLARE_BITMAP(slot_hotplug_enable, PCI_SLOT_MAX);
-    DECLARE_BITMAP(slot_device_present, PCI_SLOT_MAX);
-    DECLARE_BITMAP(slot_device_system, PCI_SLOT_MAX);
-    DECLARE_BITMAP(slot_device_vga, PCI_SLOT_MAX);
-    DECLARE_BITMAP(slot_device_qxl, PCI_SLOT_MAX);
-    uint8_t op;
-    int i;
+    GArray *method = NULL;
     QObject *bsel;
-    GArray *method;
-    bool bus_hotplug_support = false;
-
-    /*
-     * Skip bridge subtree creation if bridge hotplug is disabled
-     * to make acpi tables compatible with legacy machine types.
-     */
-    if (!child->pcihp_bridge_en && bus->parent_dev) {
-        return;
-    }
+    PCIBus *sec;
+    int i;
 
     if (bus->parent_dev) {
-        op = 0x82; /* DeviceOp */
-        build_append_namestring(bus_table, "S%.02X",
-                             bus->parent_dev->devfn);
-        build_append_byte(bus_table, 0x08); /* NameOp */
-        build_append_namestring(bus_table, "_SUN");
-        build_append_value(bus_table, PCI_SLOT(bus->parent_dev->devfn), 1);
-        build_append_byte(bus_table, 0x08); /* NameOp */
-        build_append_namestring(bus_table, "_ADR");
-        build_append_value(bus_table, (PCI_SLOT(bus->parent_dev->devfn) << 16) |
-                           PCI_FUNC(bus->parent_dev->devfn), 4);
+        build_append_namestring(bus_table, "S%.02X_", bus->parent_dev->devfn);
     } else {
-        op = 0x10; /* ScopeOp */;
         build_append_namestring(bus_table, "PCI0");
     }
 
-    bsel = object_property_get_qobject(OBJECT(bus), ACPI_PCIHP_PROP_BSEL, NULL);
+    bsel = object_property_get_qobject(OBJECT(bus), ACPI_PCIHP_PROP_BSEL,
+                                       NULL);
     if (bsel) {
         build_append_byte(bus_table, 0x08); /* NameOp */
         build_append_namestring(bus_table, "BSEL");
         build_append_int(bus_table, qint_get_int(qobject_to_qint(bsel)));
-        memset(slot_hotplug_enable, 0xff, sizeof slot_hotplug_enable);
-    } else {
-        /* No bsel - no slots are hot-pluggable */
-        memset(slot_hotplug_enable, 0x00, sizeof slot_hotplug_enable);
+        method = build_alloc_method("DVNT", 2);
     }
 
-    memset(slot_device_present, 0x00, sizeof slot_device_present);
-    memset(slot_device_system, 0x00, sizeof slot_device_present);
-    memset(slot_device_vga, 0x00, sizeof slot_device_vga);
-    memset(slot_device_qxl, 0x00, sizeof slot_device_qxl);
-
     for (i = 0; i < ARRAY_SIZE(bus->devices); i += PCI_FUNC_MAX) {
         DeviceClass *dc;
         PCIDeviceClass *pc;
         PCIDevice *pdev = bus->devices[i];
         int slot = PCI_SLOT(i);
-        bool bridge_in_acpi;
 
         if (!pdev) {
+            build_append_pcihp_dev(bus_table, slot);
+            if (method) {
+                build_append_pcihp_notify(method, slot);
+            }
             continue;
         }
 
-        set_bit(slot, slot_device_present);
         pc = PCI_DEVICE_GET_CLASS(pdev);
         dc = DEVICE_GET_CLASS(pdev);
 
-        /* When hotplug for bridges is enabled, bridges are
-         * described in ACPI separately (see build_pci_bus_end).
-         * In this case they aren't themselves hot-pluggable.
-         */
-        bridge_in_acpi = pc->is_bridge && child->pcihp_bridge_en;
-
-        if (pc->class_id == PCI_CLASS_BRIDGE_ISA || bridge_in_acpi) {
-            set_bit(slot, slot_device_system);
+        if (pc->class_id == PCI_CLASS_BRIDGE_ISA) {
+            continue;
         }
 
         if (pc->class_id == PCI_CLASS_DISPLAY_VGA) {
-            set_bit(slot, slot_device_vga);
 
             if (object_dynamic_cast(OBJECT(pdev), "qxl-vga")) {
-                set_bit(slot, slot_device_qxl);
+                void *pcihp = acpi_data_push(bus_table,
+                                             ACPI_PCIQXL_SIZEOF);
+                      memcpy(pcihp, ACPI_PCIQXL_AML, ACPI_PCIQXL_SIZEOF);
+                      patch_pciqxl(slot, pcihp);
+            } else {
+                void *pcihp = acpi_data_push(bus_table,
+                                             ACPI_PCIVGA_SIZEOF);
+                memcpy(pcihp, ACPI_PCIVGA_AML, ACPI_PCIVGA_SIZEOF);
+                patch_pcivga(slot, pcihp);
             }
-        }
-
-        if (!dc->hotpluggable || pc->is_bridge) {
-            clear_bit(slot, slot_hotplug_enable);
-        }
-    }
-
-    /* Append Device object for each slot */
-    for (i = 0; i < PCI_SLOT_MAX; i++) {
-        bool can_eject = test_bit(i, slot_hotplug_enable);
-        bool present = test_bit(i, slot_device_present);
-        bool vga = test_bit(i, slot_device_vga);
-        bool qxl = test_bit(i, slot_device_qxl);
-        bool system = test_bit(i, slot_device_system);
-        if (can_eject) {
+        } else if (dc->hotpluggable && !pc->is_bridge) {
             void *pcihp = acpi_data_push(bus_table,
                                          ACPI_PCIHP_SIZEOF);
             memcpy(pcihp, ACPI_PCIHP_AML, ACPI_PCIHP_SIZEOF);
-            patch_pcihp(i, pcihp);
-            bus_hotplug_support = true;
-        } else if (qxl) {
-            void *pcihp = acpi_data_push(bus_table,
-                                         ACPI_PCIQXL_SIZEOF);
-            memcpy(pcihp, ACPI_PCIQXL_AML, ACPI_PCIQXL_SIZEOF);
-            patch_pciqxl(i, pcihp);
-        } else if (vga) {
-            void *pcihp = acpi_data_push(bus_table,
-                                         ACPI_PCIVGA_SIZEOF);
-            memcpy(pcihp, ACPI_PCIVGA_AML, ACPI_PCIVGA_SIZEOF);
-            patch_pcivga(i, pcihp);
-        } else if (system) {
-            /* Nothing to do: system devices are in DSDT or in SSDT above. */
-        } else if (present) {
-            void *pcihp = acpi_data_push(bus_table,
-                                         ACPI_PCINOHP_SIZEOF);
-            memcpy(pcihp, ACPI_PCINOHP_AML, ACPI_PCINOHP_SIZEOF);
-            patch_pcinohp(i, pcihp);
-        }
-    }
-
-    if (bsel) {
-        method = build_alloc_method("DVNT", 2);
-
-        for (i = 0; i < PCI_SLOT_MAX; i++) {
-            GArray *notify;
-            uint8_t op;
+            patch_pcihp(slot, pcihp);
 
-            if (!test_bit(i, slot_hotplug_enable)) {
-                continue;
+            if (method) {
+                build_append_pcihp_notify(method, slot);
             }
+        } else {
+            build_append_pcihp_dev(bus_table, slot);
 
-            notify = build_alloc_array();
-            op = 0xA0; /* IfOp */
-
-            build_append_byte(notify, 0x7B); /* AndOp */
-            build_append_byte(notify, 0x68); /* Arg0Op */
-            build_append_int(notify, 0x1U << i);
-            build_append_byte(notify, 0x00); /* NullName */
-            build_append_byte(notify, 0x86); /* NotifyOp */
-            build_append_namestring(notify, "S%.02X", PCI_DEVFN(i, 0));
-            build_append_byte(notify, 0x69); /* Arg1Op */
-
-            /* Pack it up */
-            build_package(notify, op);
-
-            build_append_array(method, notify);
+            if (pc->is_bridge) {
+                PCIBus *sec_bus = pci_bridge_get_sec_bus(PCI_BRIDGE(pdev));
 
-            build_free_array(notify);
+                build_append_pci_bus_devices(bus_table, sec_bus,
+                                             pcihp_bridge_en);
+            }
         }
+    }
 
+    if (method) {
         build_append_and_cleanup_method(bus_table, method);
     }
 
     /* Append PCNT method to notify about events on local and child buses.
      * Add unconditionally for root since DSDT expects it.
      */
-    if (bus_hotplug_support || child->notify_table->len || !bus->parent_dev) {
-        method = build_alloc_method("PCNT", 0);
-
-        /* If bus supports hotplug select it and notify about local events */
-        if (bsel) {
-            build_append_byte(method, 0x70); /* StoreOp */
-            build_append_int(method, qint_get_int(qobject_to_qint(bsel)));
-            build_append_namestring(method, "BNUM");
-            build_append_namestring(method, "DVNT");
-            build_append_namestring(method, "PCIU");
-            build_append_int(method, 1); /* Device Check */
-            build_append_namestring(method, "DVNT");
-            build_append_namestring(method, "PCID");
-            build_append_int(method, 3); /* Eject Request */
-        }
-
-        /* Notify about child bus events in any case */
-        build_append_array(method, child->notify_table);
-
-        build_append_and_cleanup_method(bus_table, method);
-
-        /* Append description of child buses */
-        build_append_array(bus_table, child->device_table);
+    method = build_alloc_method("PCNT", 0);
 
-        /* Pack it up */
-        if (bus->parent_dev) {
-            build_extop_package(bus_table, op);
-        } else {
-            build_package(bus_table, op);
-        }
-
-        /* Append our bus description to parent table */
-        build_append_array(parent->device_table, bus_table);
+    /* If bus supports hotplug select it and notify about local events */
+    if (bsel) {
+        build_append_byte(method, 0x70); /* StoreOp */
+        build_append_int(method, qint_get_int(qobject_to_qint(bsel)));
+        build_append_namestring(method, "BNUM");
+        build_append_namestring(method, "DVNT");
+        build_append_namestring(method, "PCIU");
+        build_append_int(method, 1); /* Device Check */
+        build_append_namestring(method, "DVNT");
+        build_append_namestring(method, "PCID");
+        build_append_int(method, 3); /* Eject Request */
+    }
 
-        /* Also tell parent how to notify us, invoking PCNT method.
-         * At the moment this is not needed for root as we have a single root.
-         */
-        if (bus->parent_dev) {
-            build_append_namestring(parent->notify_table, "^PCNT.S%.02X",
-                                 bus->parent_dev->devfn);
+    /* Notify about child bus events in any case */
+    if (pcihp_bridge_en) {
+        QLIST_FOREACH(sec, &bus->child, sibling) {
+            build_append_namestring(method, "^S%.02X.PCNT",
+                                    sec->parent_dev->devfn);
         }
     }
 
-    qobject_decref(bsel);
+    build_append_and_cleanup_method(bus_table, method);
+
+    build_package(bus_table, 0x10); /* ScopeOp */
+    build_append_array(parent_scope, bus_table);
     build_free_array(bus_table);
-    build_pci_bus_state_cleanup(child);
-    g_free(child);
 }
 
 static void patch_pci_windows(PcPciInfo *pci, uint8_t *start, unsigned size)
@@ -1008,7 +912,6 @@ build_ssdt(GArray *table_data, GArray *linker,
         }
 
         {
-            AcpiBuildPciBusHotplugState hotplug_state;
             Object *pci_host;
             PCIBus *bus = NULL;
             bool ambiguous;
@@ -1018,16 +921,11 @@ build_ssdt(GArray *table_data, GArray *linker,
                 bus = PCI_HOST_BRIDGE(pci_host)->bus;
             }
 
-            build_pci_bus_state_init(&hotplug_state, NULL, pm->pcihp_bridge_en);
-
             if (bus) {
                 /* Scan all PCI buses. Generate tables to support hotplug. */
-                pci_for_each_bus_depth_first(bus, build_pci_bus_begin,
-                                             build_pci_bus_end, &hotplug_state);
+                build_append_pci_bus_devices(sb_scope, bus,
+                                             pm->pcihp_bridge_en);
             }
-
-            build_append_array(sb_scope, hotplug_state.device_table);
-            build_pci_bus_state_cleanup(&hotplug_state);
         }
         build_package(sb_scope, op);
         build_append_array(table_data, sb_scope);
-- 
1.8.3.1

[Qemu-devel] [PATCH V3 8/8] pc: acpi-build: simplify PCI bus tree generation

[Igor Mammedov]

it basicaly does the same as original approach,
* just without bus/notify tables tracking (less obscure)
  which is easier to follow.
* drops unnecessary loops and bitmaps,
  creating devices and notification method in the same loop.
* saves us ~100LOC

change in behavior:
* generate hotpluggable device entries for empty slots
  only if BUS itself is hotpluggable, otherwise do not
  create them.

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
v3:
  * use hotpluggable device object instead of not hotpluggable
    for non present devices, and add it only when bus itself is
    hotpluggable
---
 hw/i386/acpi-build.c | 265 +++++++++++++++------------------------------------
 1 file changed, 79 insertions(+), 186 deletions(-)

diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 707ac7e..da160c1 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -95,7 +95,6 @@ typedef struct AcpiPmInfo {
 typedef struct AcpiMiscInfo {
     bool has_hpet;
     bool has_tpm;
-    DECLARE_BITMAP(slot_hotplug_enable, PCI_SLOT_MAX);
     const unsigned char *dsdt_code;
     unsigned dsdt_size;
     uint16_t pvpanic_port;
@@ -641,69 +640,37 @@ static void acpi_set_pci_info(void)
     }
 }
 
-static void build_pci_bus_state_init(AcpiBuildPciBusHotplugState *state,
-                                     AcpiBuildPciBusHotplugState *parent,
-                                     bool pcihp_bridge_en)
+static void build_append_pcihp_notify_entry(GArray *method, int slot)
 {
-    state->parent = parent;
-    state->device_table = build_alloc_array();
-    state->notify_table = build_alloc_array();
-    state->pcihp_bridge_en = pcihp_bridge_en;
-}
-
-static void build_pci_bus_state_cleanup(AcpiBuildPciBusHotplugState *state)
-{
-    build_free_array(state->device_table);
-    build_free_array(state->notify_table);
-}
+    GArray *ifctx;
 
-static void *build_pci_bus_begin(PCIBus *bus, void *parent_state)
-{
-    AcpiBuildPciBusHotplugState *parent = parent_state;
-    AcpiBuildPciBusHotplugState *child = g_malloc(sizeof *child);
-
-    build_pci_bus_state_init(child, parent, parent->pcihp_bridge_en);
+    ifctx = build_alloc_array();
+    build_append_byte(ifctx, 0x7B); /* AndOp */
+    build_append_byte(ifctx, 0x68); /* Arg0Op */
+    build_append_int(ifctx, 0x1U << slot);
+    build_append_byte(ifctx, 0x00); /* NullName */
+    build_append_byte(ifctx, 0x86); /* NotifyOp */
+    build_append_namestring(ifctx, "S%.02X", PCI_DEVFN(slot, 0));
+    build_append_byte(ifctx, 0x69); /* Arg1Op */
 
-    return child;
+    /* Pack it up */
+    build_package(ifctx, 0xA0 /* IfOp */);
+    build_append_array(method, ifctx);
+    build_free_array(ifctx);
 }
 
-static void build_pci_bus_end(PCIBus *bus, void *bus_state)
+static void build_append_pci_bus_devices(GArray *parent_scope, PCIBus *bus,
+                                         bool pcihp_bridge_en)
 {
-    AcpiBuildPciBusHotplugState *child = bus_state;
-    AcpiBuildPciBusHotplugState *parent = child->parent;
     GArray *bus_table = build_alloc_array();
-    DECLARE_BITMAP(slot_hotplug_enable, PCI_SLOT_MAX);
-    DECLARE_BITMAP(slot_device_present, PCI_SLOT_MAX);
-    DECLARE_BITMAP(slot_device_system, PCI_SLOT_MAX);
-    DECLARE_BITMAP(slot_device_vga, PCI_SLOT_MAX);
-    DECLARE_BITMAP(slot_device_qxl, PCI_SLOT_MAX);
-    uint8_t op;
-    int i;
+    GArray *method = NULL;
     QObject *bsel;
-    GArray *method;
-    bool bus_hotplug_support = false;
-
-    /*
-     * Skip bridge subtree creation if bridge hotplug is disabled
-     * to make acpi tables compatible with legacy machine types.
-     */
-    if (!child->pcihp_bridge_en && bus->parent_dev) {
-        return;
-    }
+    PCIBus *sec;
+    int i;
 
     if (bus->parent_dev) {
-        op = 0x82; /* DeviceOp */
-        build_append_namestring(bus_table, "S%.02X",
-                             bus->parent_dev->devfn);
-        build_append_byte(bus_table, 0x08); /* NameOp */
-        build_append_namestring(bus_table, "_SUN");
-        build_append_value(bus_table, PCI_SLOT(bus->parent_dev->devfn), 1);
-        build_append_byte(bus_table, 0x08); /* NameOp */
-        build_append_namestring(bus_table, "_ADR");
-        build_append_value(bus_table, (PCI_SLOT(bus->parent_dev->devfn) << 16) |
-                           PCI_FUNC(bus->parent_dev->devfn), 4);
+        build_append_namestring(bus_table, "S%.02X_", bus->parent_dev->devfn);
     } else {
-        op = 0x10; /* ScopeOp */;
         build_append_namestring(bus_table, "PCI0");
     }
 
@@ -712,171 +679,103 @@ static void build_pci_bus_end(PCIBus *bus, void *bus_state)
         build_append_byte(bus_table, 0x08); /* NameOp */
         build_append_namestring(bus_table, "BSEL");
         build_append_int(bus_table, qint_get_int(qobject_to_qint(bsel)));
-        memset(slot_hotplug_enable, 0xff, sizeof slot_hotplug_enable);
-    } else {
-        /* No bsel - no slots are hot-pluggable */
-        memset(slot_hotplug_enable, 0x00, sizeof slot_hotplug_enable);
+        method = build_alloc_method("DVNT", 2);
     }
 
-    memset(slot_device_present, 0x00, sizeof slot_device_present);
-    memset(slot_device_system, 0x00, sizeof slot_device_present);
-    memset(slot_device_vga, 0x00, sizeof slot_device_vga);
-    memset(slot_device_qxl, 0x00, sizeof slot_device_qxl);
-
     for (i = 0; i < ARRAY_SIZE(bus->devices); i += PCI_FUNC_MAX) {
         DeviceClass *dc;
         PCIDeviceClass *pc;
         PCIDevice *pdev = bus->devices[i];
         int slot = PCI_SLOT(i);
-        bool bridge_in_acpi;
 
         if (!pdev) {
+            if (bsel) {
+                void *pcihp = acpi_data_push(bus_table, ACPI_PCIHP_SIZEOF);
+                memcpy(pcihp, ACPI_PCIHP_AML, ACPI_PCIHP_SIZEOF);
+                patch_pcihp(slot, pcihp);
+
+                build_append_pcihp_notify_entry(method, slot);
+            }
             continue;
         }
 
-        set_bit(slot, slot_device_present);
         pc = PCI_DEVICE_GET_CLASS(pdev);
         dc = DEVICE_GET_CLASS(pdev);
 
-        /* When hotplug for bridges is enabled, bridges are
-         * described in ACPI separately (see build_pci_bus_end).
-         * In this case they aren't themselves hot-pluggable.
-         */
-        bridge_in_acpi = pc->is_bridge && child->pcihp_bridge_en;
-
-        if (pc->class_id == PCI_CLASS_BRIDGE_ISA || bridge_in_acpi) {
-            set_bit(slot, slot_device_system);
+        if (pc->class_id == PCI_CLASS_BRIDGE_ISA) {
+            continue;
         }
 
         if (pc->class_id == PCI_CLASS_DISPLAY_VGA) {
-            set_bit(slot, slot_device_vga);
 
             if (object_dynamic_cast(OBJECT(pdev), "qxl-vga")) {
-                set_bit(slot, slot_device_qxl);
+                void *pcihp = acpi_data_push(bus_table,
+                                             ACPI_PCIQXL_SIZEOF);
+                      memcpy(pcihp, ACPI_PCIQXL_AML, ACPI_PCIQXL_SIZEOF);
+                      patch_pciqxl(slot, pcihp);
+            } else {
+                void *pcihp = acpi_data_push(bus_table,
+                                             ACPI_PCIVGA_SIZEOF);
+                memcpy(pcihp, ACPI_PCIVGA_AML, ACPI_PCIVGA_SIZEOF);
+                patch_pcivga(slot, pcihp);
             }
-        }
-
-        if (!dc->hotpluggable || pc->is_bridge) {
-            clear_bit(slot, slot_hotplug_enable);
-        }
-    }
-
-    /* Append Device object for each slot */
-    for (i = 0; i < PCI_SLOT_MAX; i++) {
-        bool can_eject = test_bit(i, slot_hotplug_enable);
-        bool present = test_bit(i, slot_device_present);
-        bool vga = test_bit(i, slot_device_vga);
-        bool qxl = test_bit(i, slot_device_qxl);
-        bool system = test_bit(i, slot_device_system);
-        if (can_eject) {
-            void *pcihp = acpi_data_push(bus_table,
-                                         ACPI_PCIHP_SIZEOF);
+        } else if (dc->hotpluggable && !pc->is_bridge) {
+            void *pcihp = acpi_data_push(bus_table, ACPI_PCIHP_SIZEOF);
             memcpy(pcihp, ACPI_PCIHP_AML, ACPI_PCIHP_SIZEOF);
-            patch_pcihp(i, pcihp);
-            bus_hotplug_support = true;
-        } else if (qxl) {
-            void *pcihp = acpi_data_push(bus_table,
-                                         ACPI_PCIQXL_SIZEOF);
-            memcpy(pcihp, ACPI_PCIQXL_AML, ACPI_PCIQXL_SIZEOF);
-            patch_pciqxl(i, pcihp);
-        } else if (vga) {
-            void *pcihp = acpi_data_push(bus_table,
-                                         ACPI_PCIVGA_SIZEOF);
-            memcpy(pcihp, ACPI_PCIVGA_AML, ACPI_PCIVGA_SIZEOF);
-            patch_pcivga(i, pcihp);
-        } else if (system) {
-            /* Nothing to do: system devices are in DSDT or in SSDT above. */
-        } else if (present) {
-            void *pcihp = acpi_data_push(bus_table,
-                                         ACPI_PCINOHP_SIZEOF);
-            memcpy(pcihp, ACPI_PCINOHP_AML, ACPI_PCINOHP_SIZEOF);
-            patch_pcinohp(i, pcihp);
-        }
-    }
-
-    if (bsel) {
-        method = build_alloc_method("DVNT", 2);
-
-        for (i = 0; i < PCI_SLOT_MAX; i++) {
-            GArray *notify;
-            uint8_t op;
+            patch_pcihp(slot, pcihp);
 
-            if (!test_bit(i, slot_hotplug_enable)) {
-                continue;
+            if (bsel) {
+                build_append_pcihp_notify_entry(method, slot);
             }
+        } else {
+            void *pcihp = acpi_data_push(bus_table, ACPI_PCINOHP_SIZEOF);
+            memcpy(pcihp, ACPI_PCINOHP_AML, ACPI_PCINOHP_SIZEOF);
+            patch_pcinohp(slot, pcihp);
 
-            notify = build_alloc_array();
-            op = 0xA0; /* IfOp */
-
-            build_append_byte(notify, 0x7B); /* AndOp */
-            build_append_byte(notify, 0x68); /* Arg0Op */
-            build_append_int(notify, 0x1U << i);
-            build_append_byte(notify, 0x00); /* NullName */
-            build_append_byte(notify, 0x86); /* NotifyOp */
-            build_append_namestring(notify, "S%.02X", PCI_DEVFN(i, 0));
-            build_append_byte(notify, 0x69); /* Arg1Op */
-
-            /* Pack it up */
-            build_package(notify, op);
-
-            build_append_array(method, notify);
+            if (pc->is_bridge) {
+                PCIBus *sec_bus = pci_bridge_get_sec_bus(PCI_BRIDGE(pdev));
 
-            build_free_array(notify);
+                build_append_pci_bus_devices(bus_table, sec_bus,
+                                             pcihp_bridge_en);
+            }
         }
+    }
 
+    if (bsel) {
         build_append_and_cleanup_method(bus_table, method);
     }
 
     /* Append PCNT method to notify about events on local and child buses.
      * Add unconditionally for root since DSDT expects it.
      */
-    if (bus_hotplug_support || child->notify_table->len || !bus->parent_dev) {
-        method = build_alloc_method("PCNT", 0);
-
-        /* If bus supports hotplug select it and notify about local events */
-        if (bsel) {
-            build_append_byte(method, 0x70); /* StoreOp */
-            build_append_int(method, qint_get_int(qobject_to_qint(bsel)));
-            build_append_namestring(method, "BNUM");
-            build_append_namestring(method, "DVNT");
-            build_append_namestring(method, "PCIU");
-            build_append_int(method, 1); /* Device Check */
-            build_append_namestring(method, "DVNT");
-            build_append_namestring(method, "PCID");
-            build_append_int(method, 3); /* Eject Request */
-        }
-
-        /* Notify about child bus events in any case */
-        build_append_array(method, child->notify_table);
-
-        build_append_and_cleanup_method(bus_table, method);
-
-        /* Append description of child buses */
-        build_append_array(bus_table, child->device_table);
+    method = build_alloc_method("PCNT", 0);
 
-        /* Pack it up */
-        if (bus->parent_dev) {
-            build_extop_package(bus_table, op);
-        } else {
-            build_package(bus_table, op);
-        }
-
-        /* Append our bus description to parent table */
-        build_append_array(parent->device_table, bus_table);
+    /* If bus supports hotplug select it and notify about local events */
+    if (bsel) {
+        build_append_byte(method, 0x70); /* StoreOp */
+        build_append_int(method, qint_get_int(qobject_to_qint(bsel)));
+        build_append_namestring(method, "BNUM");
+        build_append_namestring(method, "DVNT");
+        build_append_namestring(method, "PCIU");
+        build_append_int(method, 1); /* Device Check */
+        build_append_namestring(method, "DVNT");
+        build_append_namestring(method, "PCID");
+        build_append_int(method, 3); /* Eject Request */
+    }
 
-        /* Also tell parent how to notify us, invoking PCNT method.
-         * At the moment this is not needed for root as we have a single root.
-         */
-        if (bus->parent_dev) {
-            build_append_namestring(parent->notify_table, "^PCNT.S%.02X",
-                                 bus->parent_dev->devfn);
+    /* Notify about child bus events in any case */
+    if (pcihp_bridge_en) {
+        QLIST_FOREACH(sec, &bus->child, sibling) {
+            build_append_namestring(method, "^S%.02X.PCNT",
+                                    sec->parent_dev->devfn);
         }
     }
 
-    qobject_decref(bsel);
+    build_append_and_cleanup_method(bus_table, method);
+
+    build_package(bus_table, 0x10); /* ScopeOp */
+    build_append_array(parent_scope, bus_table);
     build_free_array(bus_table);
-    build_pci_bus_state_cleanup(child);
-    g_free(child);
 }
 
 static void patch_pci_windows(PcPciInfo *pci, uint8_t *start, unsigned size)
@@ -1008,7 +907,6 @@ build_ssdt(GArray *table_data, GArray *linker,
         }
 
         {
-            AcpiBuildPciBusHotplugState hotplug_state;
             Object *pci_host;
             PCIBus *bus = NULL;
             bool ambiguous;
@@ -1018,16 +916,11 @@ build_ssdt(GArray *table_data, GArray *linker,
                 bus = PCI_HOST_BRIDGE(pci_host)->bus;
             }
 
-            build_pci_bus_state_init(&hotplug_state, NULL, pm->pcihp_bridge_en);
-
             if (bus) {
                 /* Scan all PCI buses. Generate tables to support hotplug. */
-                pci_for_each_bus_depth_first(bus, build_pci_bus_begin,
-                                             build_pci_bus_end, &hotplug_state);
+                build_append_pci_bus_devices(sb_scope, bus,
+                                             pm->pcihp_bridge_en);
             }
-
-            build_append_array(sb_scope, hotplug_state.device_table);
-            build_pci_bus_state_cleanup(&hotplug_state);
         }
         build_package(sb_scope, op);
         build_append_array(table_data, sb_scope);
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH V2 3/8] pc: acpi-build: cleanup AcpiPmInfo initialization

[Claudio Fontana]

On 16.12.2014 11:58, Igor Mammedov wrote:
> zero initialize AcpiPmInfo struct to reduce code bloat
> a little bit.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
>  hw/i386/acpi-build.c | 8 ++------
>  1 file changed, 2 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index 1fb92e5..f5ec66a 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -161,6 +161,8 @@ static void acpi_get_pm_info(AcpiPmInfo *pm)
>      Object *obj = NULL;
>      QObject *o;
>  
> +    memset(pm, 0, sizeof(*pm));
> +
>      if (piix) {
>          obj = piix;
>      }
> @@ -173,22 +175,16 @@ static void acpi_get_pm_info(AcpiPmInfo *pm)
>      o = object_property_get_qobject(obj, ACPI_PM_PROP_S3_DISABLED, NULL);
>      if (o) {
>          pm->s3_disabled = qint_get_int(qobject_to_qint(o));
> -    } else {
> -        pm->s3_disabled = false;
>      }
>      qobject_decref(o);
>      o = object_property_get_qobject(obj, ACPI_PM_PROP_S4_DISABLED, NULL);
>      if (o) {
>          pm->s4_disabled = qint_get_int(qobject_to_qint(o));
> -    } else {
> -        pm->s4_disabled = false;
>      }
>      qobject_decref(o);
>      o = object_property_get_qobject(obj, ACPI_PM_PROP_S4_VAL, NULL);
>      if (o) {
>          pm->s4_val = qint_get_int(qobject_to_qint(o));
> -    } else {
> -        pm->s4_val = false;
>      }
>      qobject_decref(o);
>  
> 
Reviewed-by: Claudio Fontana <claudio.fontana@huawei.com>

-- 
Claudio Fontana
Server Virtualization Architect
Huawei Technologies Duesseldorf GmbH
Riesstraße 25 - 80992 München

office: +49 89 158834 4135
mobile: +49 15253060158

Re: [Qemu-devel] [PATCH V2 4/8] acpi: build_append_nameseg(): add padding if necessary

[Claudio Fontana]

On 16.12.2014 11:58, Igor Mammedov wrote:
> According to ACPI spec NameSeg shorter than 4 characters
> must be padded up to 4 characters with "_" symbol.
> ACPI 5.0:  20.2.2 "Name Objects Encoding"
> 
> Do it in build_append_nameseg() so that caller shouldn't know
> or care about it.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
> v2:
>   * simplify padding, suggested by: "Michael S. Tsirkin" <mst@redhat.com>
> ---
>  hw/i386/acpi-build.c | 15 ++++++++++-----
>  1 file changed, 10 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index f5ec66a..2bf9a09 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -292,6 +292,8 @@ static inline void build_append_array(GArray *array, GArray *val)
>      g_array_append_vals(array, val->data, val->len);
>  }
>  
> +#define ACPI_NAMESEG_LEN 4
> +
>  static void GCC_FMT_ATTR(2, 3)
>  build_append_nameseg(GArray *array, const char *format, ...)
>  {
> @@ -304,8 +306,11 @@ build_append_nameseg(GArray *array, const char *format, ...)
>      len = vsnprintf(s, sizeof s, format, args);
>      va_end(args);
>  
> -    assert(len == 4);
> +    assert(len <= ACPI_NAMESEG_LEN);
> +
>      g_array_append_vals(array, s, len);
> +    /* Pad up to ACPI_NAMESEG_LEN characters if necessary. */
> +    g_array_append_vals(array, "____", ACPI_NAMESEG_LEN - len);
>  }
>  
>  /* 5.4 Definition Block Encoding */
> @@ -846,7 +851,7 @@ static void build_pci_bus_end(PCIBus *bus, void *bus_state)
>  
>      if (bus->parent_dev) {
>          op = 0x82; /* DeviceOp */
> -        build_append_nameseg(bus_table, "S%.02X_",
> +        build_append_nameseg(bus_table, "S%.02X",
>                               bus->parent_dev->devfn);
>          build_append_byte(bus_table, 0x08); /* NameOp */
>          build_append_nameseg(bus_table, "_SUN");
> @@ -966,7 +971,7 @@ static void build_pci_bus_end(PCIBus *bus, void *bus_state)
>              build_append_int(notify, 0x1U << i);
>              build_append_byte(notify, 0x00); /* NullName */
>              build_append_byte(notify, 0x86); /* NotifyOp */
> -            build_append_nameseg(notify, "S%.02X_", PCI_DEVFN(i, 0));
> +            build_append_nameseg(notify, "S%.02X", PCI_DEVFN(i, 0));
>              build_append_byte(notify, 0x69); /* Arg1Op */
>  
>              /* Pack it up */
> @@ -1023,7 +1028,7 @@ static void build_pci_bus_end(PCIBus *bus, void *bus_state)
>          if (bus->parent_dev) {
>              build_append_byte(parent->notify_table, '^'); /* ParentPrefixChar */
>              build_append_byte(parent->notify_table, 0x2E); /* DualNamePrefix */
> -            build_append_nameseg(parent->notify_table, "S%.02X_",
> +            build_append_nameseg(parent->notify_table, "S%.02X",
>                                   bus->parent_dev->devfn);
>              build_append_nameseg(parent->notify_table, "PCNT");
>          }
> @@ -1093,7 +1098,7 @@ build_ssdt(GArray *table_data, GArray *linker,
>          GArray *sb_scope = build_alloc_array();
>          uint8_t op = 0x10; /* ScopeOp */
>  
> -        build_append_nameseg(sb_scope, "_SB_");
> +        build_append_nameseg(sb_scope, "_SB");
>  
>          /* build Processor object for each processor */
>          for (i = 0; i < acpi_cpus; i++) {
> 
Reviewed-by: Claudio Fontana <claudio.fontana@huawei.com>

-- 
Claudio Fontana
Server Virtualization Architect
Huawei Technologies Duesseldorf GmbH
Riesstraße 25 - 80992 München

office: +49 89 158834 4135
mobile: +49 15253060158

Re: [Qemu-devel] [PATCH V2 5/8] acpi: move generic aml building helpers into dedictated file

[Claudio Fontana]

On 16.12.2014 11:58, Igor Mammedov wrote:
> the will be later used for composing AML primitives
> and all that could be reused later for ARM machines
> as well.
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
>  hw/acpi/Makefile.objs            |   1 +
>  hw/acpi/acpi_gen_utils.c         | 166 +++++++++++++++++++++++++++++++++++++++
>  hw/i386/acpi-build.c             | 162 +-------------------------------------
>  include/hw/acpi/acpi_gen_utils.h |  23 ++++++
>  4 files changed, 192 insertions(+), 160 deletions(-)
>  create mode 100644 hw/acpi/acpi_gen_utils.c
>  create mode 100644 include/hw/acpi/acpi_gen_utils.h
> 
> diff --git a/hw/acpi/Makefile.objs b/hw/acpi/Makefile.objs
> index acd2389..4237232 100644
> --- a/hw/acpi/Makefile.objs
> +++ b/hw/acpi/Makefile.objs
> @@ -1,3 +1,4 @@
>  common-obj-$(CONFIG_ACPI) += core.o piix4.o ich9.o pcihp.o cpu_hotplug.o
>  common-obj-$(CONFIG_ACPI) += memory_hotplug.o
>  common-obj-$(CONFIG_ACPI) += acpi_interface.o
> +common-obj-$(CONFIG_ACPI) += acpi_gen_utils.o
> diff --git a/hw/acpi/acpi_gen_utils.c b/hw/acpi/acpi_gen_utils.c
> new file mode 100644
> index 0000000..82d9cb7
> --- /dev/null
> +++ b/hw/acpi/acpi_gen_utils.c
> @@ -0,0 +1,166 @@
> +#include <stdio.h>
> +#include <stdarg.h>
> +#include <assert.h>
> +#include <stdbool.h>
> +#include "hw/acpi/acpi_gen_utils.h"
> +
> +GArray *build_alloc_array(void)
> +{
> +        return g_array_new(false, true /* clear */, 1);
> +}
> +
> +void build_free_array(GArray *array)
> +{
> +        g_array_free(array, true);
> +}

indentation seems off in the two functions above.

> +
> +void build_prepend_byte(GArray *array, uint8_t val)
> +{
> +    g_array_prepend_val(array, val);
> +}
> +
> +void build_append_byte(GArray *array, uint8_t val)
> +{
> +    g_array_append_val(array, val);
> +}
> +
> +void build_append_array(GArray *array, GArray *val)
> +{
> +    g_array_append_vals(array, val->data, val->len);
> +}
> +
> +#define ACPI_NAMESEG_LEN 4
> +
> +void GCC_FMT_ATTR(2, 3)
> +build_append_nameseg(GArray *array, const char *format, ...)
> +{
> +    /* It would be nicer to use g_string_vprintf but it's only there in 2.22 */
> +    char s[] = "XXXX";
> +    int len;
> +    va_list args;
> +
> +    va_start(args, format);
> +    len = vsnprintf(s, sizeof s, format, args);
> +    va_end(args);
> +
> +    assert(len <= ACPI_NAMESEG_LEN);
> +
> +    g_array_append_vals(array, s, len);
> +    /* Pad up to ACPI_NAMESEG_LEN characters if necessary. */
> +    g_array_append_vals(array, "____", ACPI_NAMESEG_LEN - len);
> +}
> +
> +/* 5.4 Definition Block Encoding */
> +enum {
> +    PACKAGE_LENGTH_1BYTE_SHIFT = 6, /* Up to 63 - use extra 2 bits. */
> +    PACKAGE_LENGTH_2BYTE_SHIFT = 4,
> +    PACKAGE_LENGTH_3BYTE_SHIFT = 12,
> +    PACKAGE_LENGTH_4BYTE_SHIFT = 20,
> +};
> +
> +void build_prepend_package_length(GArray *package, unsigned min_bytes)
> +{
> +    uint8_t byte;
> +    unsigned length = package->len;
> +    unsigned length_bytes;
> +
> +    if (length + 1 < (1 << PACKAGE_LENGTH_1BYTE_SHIFT)) {
> +        length_bytes = 1;
> +    } else if (length + 2 < (1 << PACKAGE_LENGTH_3BYTE_SHIFT)) {
> +        length_bytes = 2;
> +    } else if (length + 3 < (1 << PACKAGE_LENGTH_4BYTE_SHIFT)) {
> +        length_bytes = 3;
> +    } else {
> +        length_bytes = 4;
> +    }
> +
> +    /* Force length to at least min_bytes.
> +     * This wastes memory but that's how bios did it.
> +     */
> +    length_bytes = MAX(length_bytes, min_bytes);
> +
> +    /* PkgLength is the length of the inclusive length of the data. */
> +    length += length_bytes;
> +
> +    switch (length_bytes) {
> +    case 1:
> +        byte = length;
> +        build_prepend_byte(package, byte);
> +        return;
> +    case 4:
> +        byte = length >> PACKAGE_LENGTH_4BYTE_SHIFT;
> +        build_prepend_byte(package, byte);
> +        length &= (1 << PACKAGE_LENGTH_4BYTE_SHIFT) - 1;
> +        /* fall through */
> +    case 3:
> +        byte = length >> PACKAGE_LENGTH_3BYTE_SHIFT;
> +        build_prepend_byte(package, byte);
> +        length &= (1 << PACKAGE_LENGTH_3BYTE_SHIFT) - 1;
> +        /* fall through */
> +    case 2:
> +        byte = length >> PACKAGE_LENGTH_2BYTE_SHIFT;
> +        build_prepend_byte(package, byte);
> +        length &= (1 << PACKAGE_LENGTH_2BYTE_SHIFT) - 1;
> +        /* fall through */
> +    }
> +    /*
> +     * Most significant two bits of byte zero indicate how many following bytes
> +     * are in PkgLength encoding.
> +     */
> +    byte = ((length_bytes - 1) << PACKAGE_LENGTH_1BYTE_SHIFT) | length;
> +    build_prepend_byte(package, byte);
> +}
> +
> +void build_package(GArray *package, uint8_t op, unsigned min_bytes)
> +{
> +    build_prepend_package_length(package, min_bytes);
> +    build_prepend_byte(package, op);
> +}
> +
> +void build_extop_package(GArray *package, uint8_t op)
> +{
> +    build_package(package, op, 1);
> +    build_prepend_byte(package, 0x5B); /* ExtOpPrefix */
> +}
> +
> +void build_append_value(GArray *table, uint32_t value, int size)
> +{
> +    uint8_t prefix;
> +    int i;
> +
> +    switch (size) {
> +    case 1:
> +        prefix = 0x0A; /* BytePrefix */
> +        break;
> +    case 2:
> +        prefix = 0x0B; /* WordPrefix */
> +        break;
> +    case 4:
> +        prefix = 0x0C; /* DWordPrefix */
> +        break;
> +    default:
> +        assert(0);
> +        return;
> +    }
> +    build_append_byte(table, prefix);
> +    for (i = 0; i < size; ++i) {
> +        build_append_byte(table, value & 0xFF);
> +        value = value >> 8;
> +    }
> +}
> +
> +void build_append_int(GArray *table, uint32_t value)
> +{
> +    if (value == 0x00) {
> +        build_append_byte(table, 0x00); /* ZeroOp */
> +    } else if (value == 0x01) {
> +        build_append_byte(table, 0x01); /* OneOp */
> +    } else if (value <= 0xFF) {
> +        build_append_value(table, value, 1);
> +    } else if (value <= 0xFFFF) {
> +        build_append_value(table, value, 2);
> +    } else {
> +        build_append_value(table, value, 4);
> +    }
> +}
> +
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index 2bf9a09..870e4a0 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -54,6 +54,8 @@
>  #include "hw/i386/q35-acpi-dsdt.hex"
>  #include "hw/i386/acpi-dsdt.hex"
>  
> +#include "hw/acpi/acpi_gen_utils.h"
> +
>  #include "qapi/qmp/qint.h"
>  #include "qom/qom-qobject.h"
>  #include "exec/ram_addr.h"
> @@ -267,166 +269,6 @@ build_header(GArray *linker, GArray *table_data,
>                                      table_data->data, h, len, &h->checksum);
>  }
>  
> -static inline GArray *build_alloc_array(void)
> -{
> -        return g_array_new(false, true /* clear */, 1);
> -}
> -
> -static inline void build_free_array(GArray *array)
> -{
> -        g_array_free(array, true);
> -}
> -
> -static inline void build_prepend_byte(GArray *array, uint8_t val)
> -{
> -    g_array_prepend_val(array, val);
> -}
> -
> -static inline void build_append_byte(GArray *array, uint8_t val)
> -{
> -    g_array_append_val(array, val);
> -}
> -
> -static inline void build_append_array(GArray *array, GArray *val)
> -{
> -    g_array_append_vals(array, val->data, val->len);
> -}
> -
> -#define ACPI_NAMESEG_LEN 4
> -
> -static void GCC_FMT_ATTR(2, 3)
> -build_append_nameseg(GArray *array, const char *format, ...)
> -{
> -    /* It would be nicer to use g_string_vprintf but it's only there in 2.22 */
> -    char s[] = "XXXX";
> -    int len;
> -    va_list args;
> -
> -    va_start(args, format);
> -    len = vsnprintf(s, sizeof s, format, args);
> -    va_end(args);
> -
> -    assert(len <= ACPI_NAMESEG_LEN);
> -
> -    g_array_append_vals(array, s, len);
> -    /* Pad up to ACPI_NAMESEG_LEN characters if necessary. */
> -    g_array_append_vals(array, "____", ACPI_NAMESEG_LEN - len);
> -}
> -
> -/* 5.4 Definition Block Encoding */
> -enum {
> -    PACKAGE_LENGTH_1BYTE_SHIFT = 6, /* Up to 63 - use extra 2 bits. */
> -    PACKAGE_LENGTH_2BYTE_SHIFT = 4,
> -    PACKAGE_LENGTH_3BYTE_SHIFT = 12,
> -    PACKAGE_LENGTH_4BYTE_SHIFT = 20,
> -};
> -
> -static void build_prepend_package_length(GArray *package, unsigned min_bytes)
> -{
> -    uint8_t byte;
> -    unsigned length = package->len;
> -    unsigned length_bytes;
> -
> -    if (length + 1 < (1 << PACKAGE_LENGTH_1BYTE_SHIFT)) {
> -        length_bytes = 1;
> -    } else if (length + 2 < (1 << PACKAGE_LENGTH_3BYTE_SHIFT)) {
> -        length_bytes = 2;
> -    } else if (length + 3 < (1 << PACKAGE_LENGTH_4BYTE_SHIFT)) {
> -        length_bytes = 3;
> -    } else {
> -        length_bytes = 4;
> -    }
> -
> -    /* Force length to at least min_bytes.
> -     * This wastes memory but that's how bios did it.
> -     */
> -    length_bytes = MAX(length_bytes, min_bytes);
> -
> -    /* PkgLength is the length of the inclusive length of the data. */
> -    length += length_bytes;
> -
> -    switch (length_bytes) {
> -    case 1:
> -        byte = length;
> -        build_prepend_byte(package, byte);
> -        return;
> -    case 4:
> -        byte = length >> PACKAGE_LENGTH_4BYTE_SHIFT;
> -        build_prepend_byte(package, byte);
> -        length &= (1 << PACKAGE_LENGTH_4BYTE_SHIFT) - 1;
> -        /* fall through */
> -    case 3:
> -        byte = length >> PACKAGE_LENGTH_3BYTE_SHIFT;
> -        build_prepend_byte(package, byte);
> -        length &= (1 << PACKAGE_LENGTH_3BYTE_SHIFT) - 1;
> -        /* fall through */
> -    case 2:
> -        byte = length >> PACKAGE_LENGTH_2BYTE_SHIFT;
> -        build_prepend_byte(package, byte);
> -        length &= (1 << PACKAGE_LENGTH_2BYTE_SHIFT) - 1;
> -        /* fall through */
> -    }
> -    /*
> -     * Most significant two bits of byte zero indicate how many following bytes
> -     * are in PkgLength encoding.
> -     */
> -    byte = ((length_bytes - 1) << PACKAGE_LENGTH_1BYTE_SHIFT) | length;
> -    build_prepend_byte(package, byte);
> -}
> -
> -static void build_package(GArray *package, uint8_t op, unsigned min_bytes)
> -{
> -    build_prepend_package_length(package, min_bytes);
> -    build_prepend_byte(package, op);
> -}
> -
> -static void build_extop_package(GArray *package, uint8_t op)
> -{
> -    build_package(package, op, 1);
> -    build_prepend_byte(package, 0x5B); /* ExtOpPrefix */
> -}
> -
> -static void build_append_value(GArray *table, uint32_t value, int size)
> -{
> -    uint8_t prefix;
> -    int i;
> -
> -    switch (size) {
> -    case 1:
> -        prefix = 0x0A; /* BytePrefix */
> -        break;
> -    case 2:
> -        prefix = 0x0B; /* WordPrefix */
> -        break;
> -    case 4:
> -        prefix = 0x0C; /* DWordPrefix */
> -        break;
> -    default:
> -        assert(0);
> -        return;
> -    }
> -    build_append_byte(table, prefix);
> -    for (i = 0; i < size; ++i) {
> -        build_append_byte(table, value & 0xFF);
> -        value = value >> 8;
> -    }
> -}
> -
> -static void build_append_int(GArray *table, uint32_t value)
> -{
> -    if (value == 0x00) {
> -        build_append_byte(table, 0x00); /* ZeroOp */
> -    } else if (value == 0x01) {
> -        build_append_byte(table, 0x01); /* OneOp */
> -    } else if (value <= 0xFF) {
> -        build_append_value(table, value, 1);
> -    } else if (value <= 0xFFFF) {
> -        build_append_value(table, value, 2);
> -    } else {
> -        build_append_value(table, value, 4);
> -    }
> -}
> -
>  static GArray *build_alloc_method(const char *name, uint8_t arg_count)
>  {
>      GArray *method = build_alloc_array();
> diff --git a/include/hw/acpi/acpi_gen_utils.h b/include/hw/acpi/acpi_gen_utils.h
> new file mode 100644
> index 0000000..e6a0b28
> --- /dev/null
> +++ b/include/hw/acpi/acpi_gen_utils.h
> @@ -0,0 +1,23 @@
> +#ifndef HW_ACPI_GEN_UTILS_H
> +#define HW_ACPI_GEN_UTILS_H
> +
> +#include <stdint.h>
> +#include <glib.h>
> +#include "qemu/compiler.h"
> +
> +GArray *build_alloc_array(void);
> +void build_free_array(GArray *array);
> +void build_prepend_byte(GArray *array, uint8_t val);
> +void build_append_byte(GArray *array, uint8_t val);
> +void build_append_array(GArray *array, GArray *val);
> +
> +void GCC_FMT_ATTR(2, 3)
> +build_append_nameseg(GArray *array, const char *format, ...);
> +
> +void build_prepend_package_length(GArray *package, unsigned min_bytes);
> +void build_package(GArray *package, uint8_t op, unsigned min_bytes);
> +void build_append_value(GArray *table, uint32_t value, int size);
> +void build_append_int(GArray *table, uint32_t value);
> +void build_extop_package(GArray *package, uint8_t op);
> +
> +#endif
> 


-- 
Claudio Fontana
Server Virtualization Architect
Huawei Technologies Duesseldorf GmbH
Riesstraße 25 - 80992 München

office: +49 89 158834 4135
mobile: +49 15253060158

Re: [Qemu-devel] [PATCH V2 5/8] acpi: move generic aml building helpers into dedictated file

[Igor Mammedov]

On Fri, 19 Dec 2014 10:31:18 +0100
Claudio Fontana <claudio.fontana@huawei.com> wrote:

> On 16.12.2014 11:58, Igor Mammedov wrote:
> > the will be later used for composing AML primitives
> > and all that could be reused later for ARM machines
> > as well.
> > 
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > ---
> >  hw/acpi/Makefile.objs            |   1 +
> >  hw/acpi/acpi_gen_utils.c         | 166 +++++++++++++++++++++++++++++++++++++++
> >  hw/i386/acpi-build.c             | 162 +-------------------------------------
> >  include/hw/acpi/acpi_gen_utils.h |  23 ++++++
> >  4 files changed, 192 insertions(+), 160 deletions(-)
> >  create mode 100644 hw/acpi/acpi_gen_utils.c
> >  create mode 100644 include/hw/acpi/acpi_gen_utils.h
> > 
> > diff --git a/hw/acpi/Makefile.objs b/hw/acpi/Makefile.objs
> > index acd2389..4237232 100644
> > --- a/hw/acpi/Makefile.objs
> > +++ b/hw/acpi/Makefile.objs
> > @@ -1,3 +1,4 @@
> >  common-obj-$(CONFIG_ACPI) += core.o piix4.o ich9.o pcihp.o cpu_hotplug.o
> >  common-obj-$(CONFIG_ACPI) += memory_hotplug.o
> >  common-obj-$(CONFIG_ACPI) += acpi_interface.o
> > +common-obj-$(CONFIG_ACPI) += acpi_gen_utils.o
> > diff --git a/hw/acpi/acpi_gen_utils.c b/hw/acpi/acpi_gen_utils.c
> > new file mode 100644
> > index 0000000..82d9cb7
> > --- /dev/null
> > +++ b/hw/acpi/acpi_gen_utils.c
> > @@ -0,0 +1,166 @@
> > +#include <stdio.h>
> > +#include <stdarg.h>
> > +#include <assert.h>
> > +#include <stdbool.h>
> > +#include "hw/acpi/acpi_gen_utils.h"
> > +
> > +GArray *build_alloc_array(void)
> > +{
> > +        return g_array_new(false, true /* clear */, 1);
> > +}
> > +
> > +void build_free_array(GArray *array)
> > +{
> > +        g_array_free(array, true);
> > +}
> 
> indentation seems off in the two functions above.
Moving bugs from original, I'll fix it up.

Thanks!
> 
> > +
> > +void build_prepend_byte(GArray *array, uint8_t val)
> > +{
> > +    g_array_prepend_val(array, val);
> > +}
> > +
> > +void build_append_byte(GArray *array, uint8_t val)
> > +{
> > +    g_array_append_val(array, val);
> > +}
> > +
> > +void build_append_array(GArray *array, GArray *val)
> > +{
> > +    g_array_append_vals(array, val->data, val->len);
> > +}
> > +
> > +#define ACPI_NAMESEG_LEN 4
> > +
> > +void GCC_FMT_ATTR(2, 3)
> > +build_append_nameseg(GArray *array, const char *format, ...)
> > +{
> > +    /* It would be nicer to use g_string_vprintf but it's only there in 2.22 */
> > +    char s[] = "XXXX";
> > +    int len;
> > +    va_list args;
> > +
> > +    va_start(args, format);
> > +    len = vsnprintf(s, sizeof s, format, args);
> > +    va_end(args);
> > +
> > +    assert(len <= ACPI_NAMESEG_LEN);
> > +
> > +    g_array_append_vals(array, s, len);
> > +    /* Pad up to ACPI_NAMESEG_LEN characters if necessary. */
> > +    g_array_append_vals(array, "____", ACPI_NAMESEG_LEN - len);
> > +}
> > +
> > +/* 5.4 Definition Block Encoding */
> > +enum {
> > +    PACKAGE_LENGTH_1BYTE_SHIFT = 6, /* Up to 63 - use extra 2 bits. */
> > +    PACKAGE_LENGTH_2BYTE_SHIFT = 4,
> > +    PACKAGE_LENGTH_3BYTE_SHIFT = 12,
> > +    PACKAGE_LENGTH_4BYTE_SHIFT = 20,
> > +};
> > +
> > +void build_prepend_package_length(GArray *package, unsigned min_bytes)
> > +{
> > +    uint8_t byte;
> > +    unsigned length = package->len;
> > +    unsigned length_bytes;
> > +
> > +    if (length + 1 < (1 << PACKAGE_LENGTH_1BYTE_SHIFT)) {
> > +        length_bytes = 1;
> > +    } else if (length + 2 < (1 << PACKAGE_LENGTH_3BYTE_SHIFT)) {
> > +        length_bytes = 2;
> > +    } else if (length + 3 < (1 << PACKAGE_LENGTH_4BYTE_SHIFT)) {
> > +        length_bytes = 3;
> > +    } else {
> > +        length_bytes = 4;
> > +    }
> > +
> > +    /* Force length to at least min_bytes.
> > +     * This wastes memory but that's how bios did it.
> > +     */
> > +    length_bytes = MAX(length_bytes, min_bytes);
> > +
> > +    /* PkgLength is the length of the inclusive length of the data. */
> > +    length += length_bytes;
> > +
> > +    switch (length_bytes) {
> > +    case 1:
> > +        byte = length;
> > +        build_prepend_byte(package, byte);
> > +        return;
> > +    case 4:
> > +        byte = length >> PACKAGE_LENGTH_4BYTE_SHIFT;
> > +        build_prepend_byte(package, byte);
> > +        length &= (1 << PACKAGE_LENGTH_4BYTE_SHIFT) - 1;
> > +        /* fall through */
> > +    case 3:
> > +        byte = length >> PACKAGE_LENGTH_3BYTE_SHIFT;
> > +        build_prepend_byte(package, byte);
> > +        length &= (1 << PACKAGE_LENGTH_3BYTE_SHIFT) - 1;
> > +        /* fall through */
> > +    case 2:
> > +        byte = length >> PACKAGE_LENGTH_2BYTE_SHIFT;
> > +        build_prepend_byte(package, byte);
> > +        length &= (1 << PACKAGE_LENGTH_2BYTE_SHIFT) - 1;
> > +        /* fall through */
> > +    }
> > +    /*
> > +     * Most significant two bits of byte zero indicate how many following bytes
> > +     * are in PkgLength encoding.
> > +     */
> > +    byte = ((length_bytes - 1) << PACKAGE_LENGTH_1BYTE_SHIFT) | length;
> > +    build_prepend_byte(package, byte);
> > +}
> > +
> > +void build_package(GArray *package, uint8_t op, unsigned min_bytes)
> > +{
> > +    build_prepend_package_length(package, min_bytes);
> > +    build_prepend_byte(package, op);
> > +}
> > +
> > +void build_extop_package(GArray *package, uint8_t op)
> > +{
> > +    build_package(package, op, 1);
> > +    build_prepend_byte(package, 0x5B); /* ExtOpPrefix */
> > +}
> > +
> > +void build_append_value(GArray *table, uint32_t value, int size)
> > +{
> > +    uint8_t prefix;
> > +    int i;
> > +
> > +    switch (size) {
> > +    case 1:
> > +        prefix = 0x0A; /* BytePrefix */
> > +        break;
> > +    case 2:
> > +        prefix = 0x0B; /* WordPrefix */
> > +        break;
> > +    case 4:
> > +        prefix = 0x0C; /* DWordPrefix */
> > +        break;
> > +    default:
> > +        assert(0);
> > +        return;
> > +    }
> > +    build_append_byte(table, prefix);
> > +    for (i = 0; i < size; ++i) {
> > +        build_append_byte(table, value & 0xFF);
> > +        value = value >> 8;
> > +    }
> > +}
> > +
> > +void build_append_int(GArray *table, uint32_t value)
> > +{
> > +    if (value == 0x00) {
> > +        build_append_byte(table, 0x00); /* ZeroOp */
> > +    } else if (value == 0x01) {
> > +        build_append_byte(table, 0x01); /* OneOp */
> > +    } else if (value <= 0xFF) {
> > +        build_append_value(table, value, 1);
> > +    } else if (value <= 0xFFFF) {
> > +        build_append_value(table, value, 2);
> > +    } else {
> > +        build_append_value(table, value, 4);
> > +    }
> > +}
> > +
> > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> > index 2bf9a09..870e4a0 100644
> > --- a/hw/i386/acpi-build.c
> > +++ b/hw/i386/acpi-build.c
> > @@ -54,6 +54,8 @@
> >  #include "hw/i386/q35-acpi-dsdt.hex"
> >  #include "hw/i386/acpi-dsdt.hex"
> >  
> > +#include "hw/acpi/acpi_gen_utils.h"
> > +
> >  #include "qapi/qmp/qint.h"
> >  #include "qom/qom-qobject.h"
> >  #include "exec/ram_addr.h"
> > @@ -267,166 +269,6 @@ build_header(GArray *linker, GArray *table_data,
> >                                      table_data->data, h, len, &h->checksum);
> >  }
> >  
> > -static inline GArray *build_alloc_array(void)
> > -{
> > -        return g_array_new(false, true /* clear */, 1);
> > -}
> > -
> > -static inline void build_free_array(GArray *array)
> > -{
> > -        g_array_free(array, true);
> > -}
> > -
> > -static inline void build_prepend_byte(GArray *array, uint8_t val)
> > -{
> > -    g_array_prepend_val(array, val);
> > -}
> > -
> > -static inline void build_append_byte(GArray *array, uint8_t val)
> > -{
> > -    g_array_append_val(array, val);
> > -}
> > -
> > -static inline void build_append_array(GArray *array, GArray *val)
> > -{
> > -    g_array_append_vals(array, val->data, val->len);
> > -}
> > -
> > -#define ACPI_NAMESEG_LEN 4
> > -
> > -static void GCC_FMT_ATTR(2, 3)
> > -build_append_nameseg(GArray *array, const char *format, ...)
> > -{
> > -    /* It would be nicer to use g_string_vprintf but it's only there in 2.22 */
> > -    char s[] = "XXXX";
> > -    int len;
> > -    va_list args;
> > -
> > -    va_start(args, format);
> > -    len = vsnprintf(s, sizeof s, format, args);
> > -    va_end(args);
> > -
> > -    assert(len <= ACPI_NAMESEG_LEN);
> > -
> > -    g_array_append_vals(array, s, len);
> > -    /* Pad up to ACPI_NAMESEG_LEN characters if necessary. */
> > -    g_array_append_vals(array, "____", ACPI_NAMESEG_LEN - len);
> > -}
> > -
> > -/* 5.4 Definition Block Encoding */
> > -enum {
> > -    PACKAGE_LENGTH_1BYTE_SHIFT = 6, /* Up to 63 - use extra 2 bits. */
> > -    PACKAGE_LENGTH_2BYTE_SHIFT = 4,
> > -    PACKAGE_LENGTH_3BYTE_SHIFT = 12,
> > -    PACKAGE_LENGTH_4BYTE_SHIFT = 20,
> > -};
> > -
> > -static void build_prepend_package_length(GArray *package, unsigned min_bytes)
> > -{
> > -    uint8_t byte;
> > -    unsigned length = package->len;
> > -    unsigned length_bytes;
> > -
> > -    if (length + 1 < (1 << PACKAGE_LENGTH_1BYTE_SHIFT)) {
> > -        length_bytes = 1;
> > -    } else if (length + 2 < (1 << PACKAGE_LENGTH_3BYTE_SHIFT)) {
> > -        length_bytes = 2;
> > -    } else if (length + 3 < (1 << PACKAGE_LENGTH_4BYTE_SHIFT)) {
> > -        length_bytes = 3;
> > -    } else {
> > -        length_bytes = 4;
> > -    }
> > -
> > -    /* Force length to at least min_bytes.
> > -     * This wastes memory but that's how bios did it.
> > -     */
> > -    length_bytes = MAX(length_bytes, min_bytes);
> > -
> > -    /* PkgLength is the length of the inclusive length of the data. */
> > -    length += length_bytes;
> > -
> > -    switch (length_bytes) {
> > -    case 1:
> > -        byte = length;
> > -        build_prepend_byte(package, byte);
> > -        return;
> > -    case 4:
> > -        byte = length >> PACKAGE_LENGTH_4BYTE_SHIFT;
> > -        build_prepend_byte(package, byte);
> > -        length &= (1 << PACKAGE_LENGTH_4BYTE_SHIFT) - 1;
> > -        /* fall through */
> > -    case 3:
> > -        byte = length >> PACKAGE_LENGTH_3BYTE_SHIFT;
> > -        build_prepend_byte(package, byte);
> > -        length &= (1 << PACKAGE_LENGTH_3BYTE_SHIFT) - 1;
> > -        /* fall through */
> > -    case 2:
> > -        byte = length >> PACKAGE_LENGTH_2BYTE_SHIFT;
> > -        build_prepend_byte(package, byte);
> > -        length &= (1 << PACKAGE_LENGTH_2BYTE_SHIFT) - 1;
> > -        /* fall through */
> > -    }
> > -    /*
> > -     * Most significant two bits of byte zero indicate how many following bytes
> > -     * are in PkgLength encoding.
> > -     */
> > -    byte = ((length_bytes - 1) << PACKAGE_LENGTH_1BYTE_SHIFT) | length;
> > -    build_prepend_byte(package, byte);
> > -}
> > -
> > -static void build_package(GArray *package, uint8_t op, unsigned min_bytes)
> > -{
> > -    build_prepend_package_length(package, min_bytes);
> > -    build_prepend_byte(package, op);
> > -}
> > -
> > -static void build_extop_package(GArray *package, uint8_t op)
> > -{
> > -    build_package(package, op, 1);
> > -    build_prepend_byte(package, 0x5B); /* ExtOpPrefix */
> > -}
> > -
> > -static void build_append_value(GArray *table, uint32_t value, int size)
> > -{
> > -    uint8_t prefix;
> > -    int i;
> > -
> > -    switch (size) {
> > -    case 1:
> > -        prefix = 0x0A; /* BytePrefix */
> > -        break;
> > -    case 2:
> > -        prefix = 0x0B; /* WordPrefix */
> > -        break;
> > -    case 4:
> > -        prefix = 0x0C; /* DWordPrefix */
> > -        break;
> > -    default:
> > -        assert(0);
> > -        return;
> > -    }
> > -    build_append_byte(table, prefix);
> > -    for (i = 0; i < size; ++i) {
> > -        build_append_byte(table, value & 0xFF);
> > -        value = value >> 8;
> > -    }
> > -}
> > -
> > -static void build_append_int(GArray *table, uint32_t value)
> > -{
> > -    if (value == 0x00) {
> > -        build_append_byte(table, 0x00); /* ZeroOp */
> > -    } else if (value == 0x01) {
> > -        build_append_byte(table, 0x01); /* OneOp */
> > -    } else if (value <= 0xFF) {
> > -        build_append_value(table, value, 1);
> > -    } else if (value <= 0xFFFF) {
> > -        build_append_value(table, value, 2);
> > -    } else {
> > -        build_append_value(table, value, 4);
> > -    }
> > -}
> > -
> >  static GArray *build_alloc_method(const char *name, uint8_t arg_count)
> >  {
> >      GArray *method = build_alloc_array();
> > diff --git a/include/hw/acpi/acpi_gen_utils.h b/include/hw/acpi/acpi_gen_utils.h
> > new file mode 100644
> > index 0000000..e6a0b28
> > --- /dev/null
> > +++ b/include/hw/acpi/acpi_gen_utils.h
> > @@ -0,0 +1,23 @@
> > +#ifndef HW_ACPI_GEN_UTILS_H
> > +#define HW_ACPI_GEN_UTILS_H
> > +
> > +#include <stdint.h>
> > +#include <glib.h>
> > +#include "qemu/compiler.h"
> > +
> > +GArray *build_alloc_array(void);
> > +void build_free_array(GArray *array);
> > +void build_prepend_byte(GArray *array, uint8_t val);
> > +void build_append_byte(GArray *array, uint8_t val);
> > +void build_append_array(GArray *array, GArray *val);
> > +
> > +void GCC_FMT_ATTR(2, 3)
> > +build_append_nameseg(GArray *array, const char *format, ...);
> > +
> > +void build_prepend_package_length(GArray *package, unsigned min_bytes);
> > +void build_package(GArray *package, uint8_t op, unsigned min_bytes);
> > +void build_append_value(GArray *table, uint32_t value, int size);
> > +void build_append_int(GArray *table, uint32_t value);
> > +void build_extop_package(GArray *package, uint8_t op);
> > +
> > +#endif
> > 
> 
> 

Re: [Qemu-devel] [PATCH V2 6/8] acpi: add build_append_namestring() helper

[Claudio Fontana]

On 16.12.2014 11:58, Igor Mammedov wrote:
> Use build_append_namestring() instead of build_append_nameseg()
> So user won't have to care whether name is NameSeg, NamePath or
> NameString.
> 
> See for refernce ACPI 5.0: 20.2.2 Name Objects Encoding
> 
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
>  hw/acpi/acpi_gen_utils.c         | 86 +++++++++++++++++++++++++++++++++++++++-
>  hw/i386/acpi-build.c             | 35 ++++++++--------
>  include/hw/acpi/acpi_gen_utils.h |  2 +-
>  3 files changed, 102 insertions(+), 21 deletions(-)
> 
> diff --git a/hw/acpi/acpi_gen_utils.c b/hw/acpi/acpi_gen_utils.c
> index 82d9cb7..2e52690 100644
> --- a/hw/acpi/acpi_gen_utils.c
> +++ b/hw/acpi/acpi_gen_utils.c
> @@ -31,7 +31,7 @@ void build_append_array(GArray *array, GArray *val)
>  
>  #define ACPI_NAMESEG_LEN 4
>  
> -void GCC_FMT_ATTR(2, 3)
> +static void GCC_FMT_ATTR(2, 3)
>  build_append_nameseg(GArray *array, const char *format, ...)
>  {
>      /* It would be nicer to use g_string_vprintf but it's only there in 2.22 */
> @@ -50,6 +50,90 @@ build_append_nameseg(GArray *array, const char *format, ...)
>      g_array_append_vals(array, "____", ACPI_NAMESEG_LEN - len);
>  }
>  
> +static const uint8_t ACPI_DualNamePrefix  = 0x2E;
> +static const uint8_t ACPI_MultiNamePrefix = 0x2F;
> +
> +static void
> +build_append_namestringv(GArray *array, const char *format, va_list ap)
> +{
> +    /* It would be nicer to use g_string_vprintf but it's only there in 2.22 */
> +    char *s;
> +    int len;
> +    va_list va_len;
> +    char **segs;
> +    char **segs_iter;
> +    int seg_count = 0;
> +
> +    va_copy(va_len, ap);
> +    len = vsnprintf(NULL, 0, format, va_len);
> +    va_end(va_len);
> +    len += 1;
> +    s = g_new(typeof(*s), len);
> +
> +    len = vsnprintf(s, len, format, ap);
> +
> +    segs = g_strsplit(s, ".", 0);
> +    g_free(s);
> +
> +    /* count segments */
> +    segs_iter = segs;
> +    while (*segs_iter) {
> +        ++segs_iter;
> +        ++seg_count;
> +    }

since SegCount is explicitly mentioned in 20.2.2 as being between 1 and 255,
why don't we assert right here (instead of below), and add an assert for seg_count != 0 as well while we're at it?

Thanks,

Claudio

> +
> +    /* handle RootPath || PrefixPath */
> +    s = *segs;
> +    while (*s == '\\' || *s == '^') {
> +        g_array_append_val(array, *s);
> +        ++s;
> +    }
> +
> +    switch (seg_count) {
> +    case 1:
> +        if (!*s) { /* NullName */
> +            const uint8_t nullpath = 0;
> +            g_array_append_val(array, nullpath);
> +        } else {
> +            build_append_nameseg(array, "%s", s);
> +        }
> +        break;
> +
> +    case 2:
> +        g_array_append_val(array, ACPI_DualNamePrefix);
> +        build_append_nameseg(array, "%s", s);
> +        build_append_nameseg(array, "%s", segs[1]);
> +        break;
> +
> +    default:
> +        g_array_append_val(array, ACPI_MultiNamePrefix);
> +        assert(seg_count <= 0xFF); /* ByteData Max */
> +        g_array_append_val(array, seg_count);
> +
> +        /* handle the 1st segment manually due to prefix/root path */
> +        build_append_nameseg(array, "%s", s);
> +
> +        /* add the rest of segments */
> +        segs_iter = segs + 1;
> +        while (*segs_iter) {
> +            build_append_nameseg(array, "%s", *segs_iter);
> +            ++segs_iter;
> +        }
> +        break;
> +    }
> +    g_strfreev(segs);
> +}
> +
> +void GCC_FMT_ATTR(2, 3)
> +build_append_namestring(GArray *array, const char *format, ...)
> +{
> +    va_list ap;
> +
> +    va_start(ap, format);
> +    build_append_namestringv(array, format, ap);
> +    va_end(ap);
> +}
> +
>  /* 5.4 Definition Block Encoding */
>  enum {
>      PACKAGE_LENGTH_1BYTE_SHIFT = 6, /* Up to 63 - use extra 2 bits. */
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index 870e4a0..0f6202d 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -273,7 +273,7 @@ static GArray *build_alloc_method(const char *name, uint8_t arg_count)
>  {
>      GArray *method = build_alloc_array();
>  
> -    build_append_nameseg(method, "%s", name);
> +    build_append_namestring(method, "%s", name);
>      build_append_byte(method, arg_count); /* MethodFlags: ArgCount */
>  
>      return method;
> @@ -565,7 +565,7 @@ build_append_notify_method(GArray *device, const char *name,
>  
>      for (i = 0; i < count; i++) {
>          GArray *target = build_alloc_array();
> -        build_append_nameseg(target, format, i);
> +        build_append_namestring(target, format, i);
>          assert(i < 256); /* Fits in 1 byte */
>          build_append_notify_target_ifequal(method, target, i, 1);
>          build_free_array(target);
> @@ -693,24 +693,24 @@ static void build_pci_bus_end(PCIBus *bus, void *bus_state)
>  
>      if (bus->parent_dev) {
>          op = 0x82; /* DeviceOp */
> -        build_append_nameseg(bus_table, "S%.02X",
> +        build_append_namestring(bus_table, "S%.02X",
>                               bus->parent_dev->devfn);
>          build_append_byte(bus_table, 0x08); /* NameOp */
> -        build_append_nameseg(bus_table, "_SUN");
> +        build_append_namestring(bus_table, "_SUN");
>          build_append_value(bus_table, PCI_SLOT(bus->parent_dev->devfn), 1);
>          build_append_byte(bus_table, 0x08); /* NameOp */
> -        build_append_nameseg(bus_table, "_ADR");
> +        build_append_namestring(bus_table, "_ADR");
>          build_append_value(bus_table, (PCI_SLOT(bus->parent_dev->devfn) << 16) |
>                             PCI_FUNC(bus->parent_dev->devfn), 4);
>      } else {
>          op = 0x10; /* ScopeOp */;
> -        build_append_nameseg(bus_table, "PCI0");
> +        build_append_namestring(bus_table, "PCI0");
>      }
>  
>      bsel = object_property_get_qobject(OBJECT(bus), ACPI_PCIHP_PROP_BSEL, NULL);
>      if (bsel) {
>          build_append_byte(bus_table, 0x08); /* NameOp */
> -        build_append_nameseg(bus_table, "BSEL");
> +        build_append_namestring(bus_table, "BSEL");
>          build_append_int(bus_table, qint_get_int(qobject_to_qint(bsel)));
>          memset(slot_hotplug_enable, 0xff, sizeof slot_hotplug_enable);
>      } else {
> @@ -813,7 +813,7 @@ static void build_pci_bus_end(PCIBus *bus, void *bus_state)
>              build_append_int(notify, 0x1U << i);
>              build_append_byte(notify, 0x00); /* NullName */
>              build_append_byte(notify, 0x86); /* NotifyOp */
> -            build_append_nameseg(notify, "S%.02X", PCI_DEVFN(i, 0));
> +            build_append_namestring(notify, "S%.02X", PCI_DEVFN(i, 0));
>              build_append_byte(notify, 0x69); /* Arg1Op */
>  
>              /* Pack it up */
> @@ -837,12 +837,12 @@ static void build_pci_bus_end(PCIBus *bus, void *bus_state)
>          if (bsel) {
>              build_append_byte(method, 0x70); /* StoreOp */
>              build_append_int(method, qint_get_int(qobject_to_qint(bsel)));
> -            build_append_nameseg(method, "BNUM");
> -            build_append_nameseg(method, "DVNT");
> -            build_append_nameseg(method, "PCIU");
> +            build_append_namestring(method, "BNUM");
> +            build_append_namestring(method, "DVNT");
> +            build_append_namestring(method, "PCIU");
>              build_append_int(method, 1); /* Device Check */
> -            build_append_nameseg(method, "DVNT");
> -            build_append_nameseg(method, "PCID");
> +            build_append_namestring(method, "DVNT");
> +            build_append_namestring(method, "PCID");
>              build_append_int(method, 3); /* Eject Request */
>          }
>  
> @@ -868,11 +868,8 @@ static void build_pci_bus_end(PCIBus *bus, void *bus_state)
>           * At the moment this is not needed for root as we have a single root.
>           */
>          if (bus->parent_dev) {
> -            build_append_byte(parent->notify_table, '^'); /* ParentPrefixChar */
> -            build_append_byte(parent->notify_table, 0x2E); /* DualNamePrefix */
> -            build_append_nameseg(parent->notify_table, "S%.02X",
> +            build_append_namestring(parent->notify_table, "^PCNT.S%.02X",
>                                   bus->parent_dev->devfn);
> -            build_append_nameseg(parent->notify_table, "PCNT");
>          }
>      }
>  
> @@ -940,7 +937,7 @@ build_ssdt(GArray *table_data, GArray *linker,
>          GArray *sb_scope = build_alloc_array();
>          uint8_t op = 0x10; /* ScopeOp */
>  
> -        build_append_nameseg(sb_scope, "_SB");
> +        build_append_namestring(sb_scope, "_SB");
>  
>          /* build Processor object for each processor */
>          for (i = 0; i < acpi_cpus; i++) {
> @@ -960,7 +957,7 @@ build_ssdt(GArray *table_data, GArray *linker,
>  
>          /* build "Name(CPON, Package() { One, One, ..., Zero, Zero, ... })" */
>          build_append_byte(sb_scope, 0x08); /* NameOp */
> -        build_append_nameseg(sb_scope, "CPON");
> +        build_append_namestring(sb_scope, "CPON");
>  
>          {
>              GArray *package = build_alloc_array();
> diff --git a/include/hw/acpi/acpi_gen_utils.h b/include/hw/acpi/acpi_gen_utils.h
> index e6a0b28..fd50625 100644
> --- a/include/hw/acpi/acpi_gen_utils.h
> +++ b/include/hw/acpi/acpi_gen_utils.h
> @@ -12,7 +12,7 @@ void build_append_byte(GArray *array, uint8_t val);
>  void build_append_array(GArray *array, GArray *val);
>  
>  void GCC_FMT_ATTR(2, 3)
> -build_append_nameseg(GArray *array, const char *format, ...);
> +build_append_namestring(GArray *array, const char *format, ...);
>  
>  void build_prepend_package_length(GArray *package, unsigned min_bytes);
>  void build_package(GArray *package, uint8_t op, unsigned min_bytes);
> 

Re: [Qemu-devel] [PATCH V2 6/8] acpi: add build_append_namestring() helper

[Igor Mammedov]

On Fri, 19 Dec 2014 10:53:03 +0100
Claudio Fontana <claudio.fontana@huawei.com> wrote:

> On 16.12.2014 11:58, Igor Mammedov wrote:
> > Use build_append_namestring() instead of build_append_nameseg()
> > So user won't have to care whether name is NameSeg, NamePath or
> > NameString.
> > 
> > See for refernce ACPI 5.0: 20.2.2 Name Objects Encoding
> > 
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > ---
> >  hw/acpi/acpi_gen_utils.c         | 86 +++++++++++++++++++++++++++++++++++++++-
> >  hw/i386/acpi-build.c             | 35 ++++++++--------
> >  include/hw/acpi/acpi_gen_utils.h |  2 +-
> >  3 files changed, 102 insertions(+), 21 deletions(-)
> > 
> > diff --git a/hw/acpi/acpi_gen_utils.c b/hw/acpi/acpi_gen_utils.c
> > index 82d9cb7..2e52690 100644
> > --- a/hw/acpi/acpi_gen_utils.c
> > +++ b/hw/acpi/acpi_gen_utils.c
> > @@ -31,7 +31,7 @@ void build_append_array(GArray *array, GArray *val)
> >  
> >  #define ACPI_NAMESEG_LEN 4
> >  
> > -void GCC_FMT_ATTR(2, 3)
> > +static void GCC_FMT_ATTR(2, 3)
> >  build_append_nameseg(GArray *array, const char *format, ...)
> >  {
> >      /* It would be nicer to use g_string_vprintf but it's only there in 2.22 */
> > @@ -50,6 +50,90 @@ build_append_nameseg(GArray *array, const char *format, ...)
> >      g_array_append_vals(array, "____", ACPI_NAMESEG_LEN - len);
> >  }
> >  
> > +static const uint8_t ACPI_DualNamePrefix  = 0x2E;
> > +static const uint8_t ACPI_MultiNamePrefix = 0x2F;
> > +
> > +static void
> > +build_append_namestringv(GArray *array, const char *format, va_list ap)
> > +{
> > +    /* It would be nicer to use g_string_vprintf but it's only there in 2.22 */
> > +    char *s;
> > +    int len;
> > +    va_list va_len;
> > +    char **segs;
> > +    char **segs_iter;
> > +    int seg_count = 0;
> > +
> > +    va_copy(va_len, ap);
> > +    len = vsnprintf(NULL, 0, format, va_len);
> > +    va_end(va_len);
> > +    len += 1;
> > +    s = g_new(typeof(*s), len);
> > +
> > +    len = vsnprintf(s, len, format, ap);
> > +
> > +    segs = g_strsplit(s, ".", 0);
> > +    g_free(s);
> > +
> > +    /* count segments */
> > +    segs_iter = segs;
> > +    while (*segs_iter) {
> > +        ++segs_iter;
> > +        ++seg_count;
> > +    }
> 
> since SegCount is explicitly mentioned in 20.2.2 as being between 1 and 255,
> why don't we assert right here (instead of below), and add an assert for seg_count != 0 as well while we're at it?
Sure, I'll move it here and add extra assert  as well.

> 
> Thanks,
> 
> Claudio
> 
> > +
> > +    /* handle RootPath || PrefixPath */
> > +    s = *segs;
> > +    while (*s == '\\' || *s == '^') {
> > +        g_array_append_val(array, *s);
> > +        ++s;
> > +    }
> > +
> > +    switch (seg_count) {
> > +    case 1:
> > +        if (!*s) { /* NullName */
> > +            const uint8_t nullpath = 0;
> > +            g_array_append_val(array, nullpath);
> > +        } else {
> > +            build_append_nameseg(array, "%s", s);
> > +        }
> > +        break;
> > +
> > +    case 2:
> > +        g_array_append_val(array, ACPI_DualNamePrefix);
> > +        build_append_nameseg(array, "%s", s);
> > +        build_append_nameseg(array, "%s", segs[1]);
> > +        break;
> > +
> > +    default:
> > +        g_array_append_val(array, ACPI_MultiNamePrefix);
> > +        assert(seg_count <= 0xFF); /* ByteData Max */
> > +        g_array_append_val(array, seg_count);
> > +
> > +        /* handle the 1st segment manually due to prefix/root path */
> > +        build_append_nameseg(array, "%s", s);
> > +
> > +        /* add the rest of segments */
> > +        segs_iter = segs + 1;
> > +        while (*segs_iter) {
> > +            build_append_nameseg(array, "%s", *segs_iter);
> > +            ++segs_iter;
> > +        }
> > +        break;
> > +    }
> > +    g_strfreev(segs);
> > +}
> > +
> > +void GCC_FMT_ATTR(2, 3)
> > +build_append_namestring(GArray *array, const char *format, ...)
> > +{
> > +    va_list ap;
> > +
> > +    va_start(ap, format);
> > +    build_append_namestringv(array, format, ap);
> > +    va_end(ap);
> > +}
> > +
> >  /* 5.4 Definition Block Encoding */
> >  enum {
> >      PACKAGE_LENGTH_1BYTE_SHIFT = 6, /* Up to 63 - use extra 2 bits. */
> > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> > index 870e4a0..0f6202d 100644
> > --- a/hw/i386/acpi-build.c
> > +++ b/hw/i386/acpi-build.c
> > @@ -273,7 +273,7 @@ static GArray *build_alloc_method(const char *name, uint8_t arg_count)
> >  {
> >      GArray *method = build_alloc_array();
> >  
> > -    build_append_nameseg(method, "%s", name);
> > +    build_append_namestring(method, "%s", name);
> >      build_append_byte(method, arg_count); /* MethodFlags: ArgCount */
> >  
> >      return method;
> > @@ -565,7 +565,7 @@ build_append_notify_method(GArray *device, const char *name,
> >  
> >      for (i = 0; i < count; i++) {
> >          GArray *target = build_alloc_array();
> > -        build_append_nameseg(target, format, i);
> > +        build_append_namestring(target, format, i);
> >          assert(i < 256); /* Fits in 1 byte */
> >          build_append_notify_target_ifequal(method, target, i, 1);
> >          build_free_array(target);
> > @@ -693,24 +693,24 @@ static void build_pci_bus_end(PCIBus *bus, void *bus_state)
> >  
> >      if (bus->parent_dev) {
> >          op = 0x82; /* DeviceOp */
> > -        build_append_nameseg(bus_table, "S%.02X",
> > +        build_append_namestring(bus_table, "S%.02X",
> >                               bus->parent_dev->devfn);
> >          build_append_byte(bus_table, 0x08); /* NameOp */
> > -        build_append_nameseg(bus_table, "_SUN");
> > +        build_append_namestring(bus_table, "_SUN");
> >          build_append_value(bus_table, PCI_SLOT(bus->parent_dev->devfn), 1);
> >          build_append_byte(bus_table, 0x08); /* NameOp */
> > -        build_append_nameseg(bus_table, "_ADR");
> > +        build_append_namestring(bus_table, "_ADR");
> >          build_append_value(bus_table, (PCI_SLOT(bus->parent_dev->devfn) << 16) |
> >                             PCI_FUNC(bus->parent_dev->devfn), 4);
> >      } else {
> >          op = 0x10; /* ScopeOp */;
> > -        build_append_nameseg(bus_table, "PCI0");
> > +        build_append_namestring(bus_table, "PCI0");
> >      }
> >  
> >      bsel = object_property_get_qobject(OBJECT(bus), ACPI_PCIHP_PROP_BSEL, NULL);
> >      if (bsel) {
> >          build_append_byte(bus_table, 0x08); /* NameOp */
> > -        build_append_nameseg(bus_table, "BSEL");
> > +        build_append_namestring(bus_table, "BSEL");
> >          build_append_int(bus_table, qint_get_int(qobject_to_qint(bsel)));
> >          memset(slot_hotplug_enable, 0xff, sizeof slot_hotplug_enable);
> >      } else {
> > @@ -813,7 +813,7 @@ static void build_pci_bus_end(PCIBus *bus, void *bus_state)
> >              build_append_int(notify, 0x1U << i);
> >              build_append_byte(notify, 0x00); /* NullName */
> >              build_append_byte(notify, 0x86); /* NotifyOp */
> > -            build_append_nameseg(notify, "S%.02X", PCI_DEVFN(i, 0));
> > +            build_append_namestring(notify, "S%.02X", PCI_DEVFN(i, 0));
> >              build_append_byte(notify, 0x69); /* Arg1Op */
> >  
> >              /* Pack it up */
> > @@ -837,12 +837,12 @@ static void build_pci_bus_end(PCIBus *bus, void *bus_state)
> >          if (bsel) {
> >              build_append_byte(method, 0x70); /* StoreOp */
> >              build_append_int(method, qint_get_int(qobject_to_qint(bsel)));
> > -            build_append_nameseg(method, "BNUM");
> > -            build_append_nameseg(method, "DVNT");
> > -            build_append_nameseg(method, "PCIU");
> > +            build_append_namestring(method, "BNUM");
> > +            build_append_namestring(method, "DVNT");
> > +            build_append_namestring(method, "PCIU");
> >              build_append_int(method, 1); /* Device Check */
> > -            build_append_nameseg(method, "DVNT");
> > -            build_append_nameseg(method, "PCID");
> > +            build_append_namestring(method, "DVNT");
> > +            build_append_namestring(method, "PCID");
> >              build_append_int(method, 3); /* Eject Request */
> >          }
> >  
> > @@ -868,11 +868,8 @@ static void build_pci_bus_end(PCIBus *bus, void *bus_state)
> >           * At the moment this is not needed for root as we have a single root.
> >           */
> >          if (bus->parent_dev) {
> > -            build_append_byte(parent->notify_table, '^'); /* ParentPrefixChar */
> > -            build_append_byte(parent->notify_table, 0x2E); /* DualNamePrefix */
> > -            build_append_nameseg(parent->notify_table, "S%.02X",
> > +            build_append_namestring(parent->notify_table, "^PCNT.S%.02X",
> >                                   bus->parent_dev->devfn);
> > -            build_append_nameseg(parent->notify_table, "PCNT");
> >          }
> >      }
> >  
> > @@ -940,7 +937,7 @@ build_ssdt(GArray *table_data, GArray *linker,
> >          GArray *sb_scope = build_alloc_array();
> >          uint8_t op = 0x10; /* ScopeOp */
> >  
> > -        build_append_nameseg(sb_scope, "_SB");
> > +        build_append_namestring(sb_scope, "_SB");
> >  
> >          /* build Processor object for each processor */
> >          for (i = 0; i < acpi_cpus; i++) {
> > @@ -960,7 +957,7 @@ build_ssdt(GArray *table_data, GArray *linker,
> >  
> >          /* build "Name(CPON, Package() { One, One, ..., Zero, Zero, ... })" */
> >          build_append_byte(sb_scope, 0x08); /* NameOp */
> > -        build_append_nameseg(sb_scope, "CPON");
> > +        build_append_namestring(sb_scope, "CPON");
> >  
> >          {
> >              GArray *package = build_alloc_array();
> > diff --git a/include/hw/acpi/acpi_gen_utils.h b/include/hw/acpi/acpi_gen_utils.h
> > index e6a0b28..fd50625 100644
> > --- a/include/hw/acpi/acpi_gen_utils.h
> > +++ b/include/hw/acpi/acpi_gen_utils.h
> > @@ -12,7 +12,7 @@ void build_append_byte(GArray *array, uint8_t val);
> >  void build_append_array(GArray *array, GArray *val);
> >  
> >  void GCC_FMT_ATTR(2, 3)
> > -build_append_nameseg(GArray *array, const char *format, ...);
> > +build_append_namestring(GArray *array, const char *format, ...);
> >  
> >  void build_prepend_package_length(GArray *package, unsigned min_bytes);
> >  void build_package(GArray *package, uint8_t op, unsigned min_bytes);
> > 
>