[Qemu-devel] [PATCH v5 0/5] pc: acpi: various fixes and cleanups

[Qemu-devel] [PATCH v5 0/5] pc: acpi: various fixes and cleanups

[Igor Mammedov]

NOTE to maintainer: please update test data (ACPI blobs) in test cases

changes from v4:
 * rebased on top of PCI tree, dropping 2 patches
   that are already there

changes from v3:
 * rename acpi_gen_utils.[ch] to acpi-build-utils.[ch]
 * copy GLP license block from acpi-build.c
 * assert on wrong Segcount earlier and extend condition to
   seg_count > 0 && seg_count <= 255
 * drop "pc: acpi: decribe bridge device as not hotpluggable"
 * keep original logic of creating bridge devices as it was done
   in 133a2da48 "pc: acpi: generate AML only for PCI0 ..."
 * if bus is non hotpluggable, add child slots to bus as non
   hotpluggable as it was done in original code.

changes from v2:
 * codding style fixups
 * check for SegCount earlier
 * use hotpluggable device object instead of not hotpluggable
    for non present devices, and add it only when bus itself is
    hotpluggable

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_v4

Igor Mammedov (5):
  pc: acpi-build: cleanup AcpiPmInfo initialization
  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-build-utils.c         | 269 +++++++++++++++++++++
 hw/i386/acpi-build.c               | 469 ++++++++-----------------------------
 include/hw/acpi/acpi-build-utils.h |  23 ++
 4 files changed, 397 insertions(+), 365 deletions(-)
 create mode 100644 hw/acpi/acpi-build-utils.c
 create mode 100644 include/hw/acpi/acpi-build-utils.h

-- 
1.8.3.1

[Qemu-devel] [PATCH v5 1/5] 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>
Reviewed-by: Claudio Fontana <claudio.fontana@huawei.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 77a124e..4c0536f 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -172,6 +172,8 @@ static void acpi_get_pm_info(AcpiPmInfo *pm)
     Object *obj = NULL;
     QObject *o;
 
+    memset(pm, 0, sizeof(*pm));
+
     if (piix) {
         obj = piix;
     }
@@ -184,22 +186,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 v5 2/5] 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>
---
v3:
  * rename acpi_gen_utils.[ch] to acpi-build-utils.[ch]
  * copy GLP license block from acpi-build.c
v2:
  * fix wrong ident in moved code
---
 hw/acpi/Makefile.objs              |   1 +
 hw/acpi/acpi-build-utils.c         | 187 +++++++++++++++++++++++++++++++++++++
 hw/i386/acpi-build.c               | 162 +-------------------------------
 include/hw/acpi/acpi-build-utils.h |  23 +++++
 4 files changed, 213 insertions(+), 160 deletions(-)
 create mode 100644 hw/acpi/acpi-build-utils.c
 create mode 100644 include/hw/acpi/acpi-build-utils.h

diff --git a/hw/acpi/Makefile.objs b/hw/acpi/Makefile.objs
index ee82073..cad0355 100644
--- a/hw/acpi/Makefile.objs
+++ b/hw/acpi/Makefile.objs
@@ -2,3 +2,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) += bios-linker-loader.o
+common-obj-$(CONFIG_ACPI) += acpi-build-utils.o
diff --git a/hw/acpi/acpi-build-utils.c b/hw/acpi/acpi-build-utils.c
new file mode 100644
index 0000000..79aa610
--- /dev/null
+++ b/hw/acpi/acpi-build-utils.c
@@ -0,0 +1,187 @@
+/* Support for generating ACPI tables and passing them to Guests
+ *
+ * Copyright (C) 2014 Red Hat Inc
+ *
+ * Author: Michael S. Tsirkin <mst@redhat.com>
+ * Author: Igor Mammedov <imammedo@redhat.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+
+ * You should have received a copy of the GNU General Public License along
+ * with this program; if not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include <stdio.h>
+#include <stdarg.h>
+#include <assert.h>
+#include <stdbool.h>
+#include "hw/acpi/acpi-build-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 4c0536f..8ee15ab 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-build-utils.h"
+
 #include "qapi/qmp/qint.h"
 #include "qom/qom-qobject.h"
 #include "exec/ram_addr.h"
@@ -277,166 +279,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-build-utils.h b/include/hw/acpi/acpi-build-utils.h
new file mode 100644
index 0000000..e6a0b28
--- /dev/null
+++ b/include/hw/acpi/acpi-build-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 v5 3/5] 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>
Reviewed-by: Claudio Fontana <claudio.fontana@huawei.com>
---
v3:
 assert on wrong Segcount earlier and extend condition to
  seg_count > 0 && seg_count <= 255
 as suggested by Claudio Fontana <claudio.fontana@huawei.com>
---
 hw/acpi/acpi-build-utils.c         | 90 +++++++++++++++++++++++++++++++++++++-
 hw/i386/acpi-build.c               | 35 +++++++--------
 include/hw/acpi/acpi-build-utils.h |  2 +-
 3 files changed, 106 insertions(+), 21 deletions(-)

diff --git a/hw/acpi/acpi-build-utils.c b/hw/acpi/acpi-build-utils.c
index 79aa610..aed9066 100644
--- a/hw/acpi/acpi-build-utils.c
+++ b/hw/acpi/acpi-build-utils.c
@@ -52,7 +52,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 */
@@ -71,6 +71,94 @@ 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;
+    }
+    /*
+     * ACPI 5.0 spec: 20.2.2 Name Objects Encoding:
+     * "SegCount can be from 1 to 255"
+     */
+    assert(seg_count > 0 && seg_count <= 255);
+
+    /* 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);
+        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 8ee15ab..e3ac1a14 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -283,7 +283,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;
@@ -575,7 +575,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);
@@ -703,24 +703,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 {
@@ -823,7 +823,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 */
@@ -847,12 +847,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 */
         }
 
@@ -878,11 +878,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");
         }
     }
 
@@ -967,7 +964,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++) {
@@ -987,7 +984,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-build-utils.h b/include/hw/acpi/acpi-build-utils.h
index e6a0b28..fd50625 100644
--- a/include/hw/acpi/acpi-build-utils.h
+++ b/include/hw/acpi/acpi-build-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 v5 4/5] acpi: drop min-bytes in build_package()

[Igor Mammedov]

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

diff --git a/hw/acpi/acpi-build-utils.c b/hw/acpi/acpi-build-utils.c
index aed9066..602e68c 100644
--- a/hw/acpi/acpi-build-utils.c
+++ b/hw/acpi/acpi-build-utils.c
@@ -167,7 +167,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;
@@ -183,11 +183,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;
 
@@ -220,15 +215,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 */
 }
 
@@ -272,4 +267,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 e3ac1a14..a010f3b 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -293,7 +293,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);
@@ -314,7 +314,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);
 
@@ -827,7 +827,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);
 
@@ -868,7 +868,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 */
@@ -1008,7 +1008,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);
         }
@@ -1056,8 +1056,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-build-utils.h b/include/hw/acpi/acpi-build-utils.h
index fd50625..199f003 100644
--- a/include/hw/acpi/acpi-build-utils.h
+++ b/include/hw/acpi/acpi-build-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 v5 5/5] 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>
---
v4:
  * keep original logic of creating bridge devices as it was done
    in 133a2da48 "pc: acpi: generate AML only for PCI0 ..."
  * if bus is non hotpluggable, add child slots to bus as non
    hotpluggable as it was done in original code.
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 | 275 +++++++++++++++++----------------------------------
 1 file changed, 90 insertions(+), 185 deletions(-)

diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index a010f3b..5255428 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -106,7 +106,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;
@@ -651,69 +650,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");
     }
 
@@ -722,17 +689,9 @@ 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;
@@ -741,152 +700,104 @@ static void build_pci_bus_end(PCIBus *bus, void *bus_state)
         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);
+            } else {
+                /* no much sense to create empty non hotpluggable slots,
+                 * but it's what original code did before. TODO: remove it.
+                 */
+                void *pcihp = acpi_data_push(bus_table, ACPI_PCINOHP_SIZEOF);
+                memcpy(pcihp, ACPI_PCINOHP_AML, ACPI_PCINOHP_SIZEOF);
+                patch_pcinohp(slot, pcihp);
+            }
             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;
         }
+        bridge_in_acpi = pc->is_bridge && pcihp_bridge_en;
 
         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 || bridge_in_acpi) {
-            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 && !bridge_in_acpi) {
+            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);
+            /* When hotplug for bridges is enabled, bridges that are
+             * described in ACPI separately aren't themselves hot-pluggable.
+             */
+            if (bridge_in_acpi) {
+                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)
@@ -1035,7 +946,6 @@ build_ssdt(GArray *table_data, GArray *linker,
         }
 
         {
-            AcpiBuildPciBusHotplugState hotplug_state;
             Object *pci_host;
             PCIBus *bus = NULL;
             bool ambiguous;
@@ -1045,16 +955,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 v5 1/5] pc: acpi-build: cleanup AcpiPmInfo initialization

[Marcel Apfelbaum]

On 01/21/2015 11:09 AM, Igor Mammedov wrote:
> zero initialize AcpiPmInfo struct to reduce code bloat
> a little bit.
>
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> Reviewed-by: Claudio Fontana <claudio.fontana@huawei.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 77a124e..4c0536f 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -172,6 +172,8 @@ static void acpi_get_pm_info(AcpiPmInfo *pm)
>       Object *obj = NULL;
>       QObject *o;
>
> +    memset(pm, 0, sizeof(*pm));
> +
>       if (piix) {
>           obj = piix;
>       }
> @@ -184,22 +186,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: Marcel Apfelbaum <marcel@redhat.com>

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

[Marcel Apfelbaum]

On 01/21/2015 11:09 AM, 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>
> ---
> v3:
>    * rename acpi_gen_utils.[ch] to acpi-build-utils.[ch]
>    * copy GLP license block from acpi-build.c
> v2:
>    * fix wrong ident in moved code
> ---
>   hw/acpi/Makefile.objs              |   1 +
>   hw/acpi/acpi-build-utils.c         | 187 +++++++++++++++++++++++++++++++++++++
>   hw/i386/acpi-build.c               | 162 +-------------------------------
>   include/hw/acpi/acpi-build-utils.h |  23 +++++
>   4 files changed, 213 insertions(+), 160 deletions(-)
>   create mode 100644 hw/acpi/acpi-build-utils.c
>   create mode 100644 include/hw/acpi/acpi-build-utils.h
>
> diff --git a/hw/acpi/Makefile.objs b/hw/acpi/Makefile.objs
> index ee82073..cad0355 100644
> --- a/hw/acpi/Makefile.objs
> +++ b/hw/acpi/Makefile.objs
> @@ -2,3 +2,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) += bios-linker-loader.o
> +common-obj-$(CONFIG_ACPI) += acpi-build-utils.o
> diff --git a/hw/acpi/acpi-build-utils.c b/hw/acpi/acpi-build-utils.c
> new file mode 100644
> index 0000000..79aa610
> --- /dev/null
> +++ b/hw/acpi/acpi-build-utils.c
> @@ -0,0 +1,187 @@
> +/* Support for generating ACPI tables and passing them to Guests
> + *
> + * Copyright (C) 2014 Red Hat Inc
                     ^^^^
2015?
Michael, can you change this on the fly?

Other than that:
Acked-by: Marcel Apfelbaum <marcel@redhat.com>

> + *
> + * Author: Michael S. Tsirkin <mst@redhat.com>
> + * Author: Igor Mammedov <imammedo@redhat.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> +
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> +
> + * You should have received a copy of the GNU General Public License along
> + * with this program; if not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#include <stdio.h>
> +#include <stdarg.h>
> +#include <assert.h>
> +#include <stdbool.h>
> +#include "hw/acpi/acpi-build-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 4c0536f..8ee15ab 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-build-utils.h"
> +
>   #include "qapi/qmp/qint.h"
>   #include "qom/qom-qobject.h"
>   #include "exec/ram_addr.h"
> @@ -277,166 +279,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-build-utils.h b/include/hw/acpi/acpi-build-utils.h
> new file mode 100644
> index 0000000..e6a0b28
> --- /dev/null
> +++ b/include/hw/acpi/acpi-build-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 v5 3/5] acpi: add build_append_namestring() helper

[Marcel Apfelbaum]

On 01/21/2015 11:09 AM, 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>
> Reviewed-by: Claudio Fontana <claudio.fontana@huawei.com>
> ---
> v3:
>   assert on wrong Segcount earlier and extend condition to
>    seg_count > 0 && seg_count <= 255
>   as suggested by Claudio Fontana <claudio.fontana@huawei.com>
> ---
>   hw/acpi/acpi-build-utils.c         | 90 +++++++++++++++++++++++++++++++++++++-
>   hw/i386/acpi-build.c               | 35 +++++++--------
>   include/hw/acpi/acpi-build-utils.h |  2 +-
>   3 files changed, 106 insertions(+), 21 deletions(-)
>
> diff --git a/hw/acpi/acpi-build-utils.c b/hw/acpi/acpi-build-utils.c
> index 79aa610..aed9066 100644
> --- a/hw/acpi/acpi-build-utils.c
> +++ b/hw/acpi/acpi-build-utils.c
> @@ -52,7 +52,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 */
> @@ -71,6 +71,94 @@ 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;
> +    }
> +    /*
> +     * ACPI 5.0 spec: 20.2.2 Name Objects Encoding:
> +     * "SegCount can be from 1 to 255"
> +     */
> +    assert(seg_count > 0 && seg_count <= 255);
> +
> +    /* 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);
> +        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 8ee15ab..e3ac1a14 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -283,7 +283,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;
> @@ -575,7 +575,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);
> @@ -703,24 +703,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 {
> @@ -823,7 +823,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 */
> @@ -847,12 +847,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 */
>           }
>
> @@ -878,11 +878,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");
>           }
>       }
>
> @@ -967,7 +964,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++) {
> @@ -987,7 +984,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-build-utils.h b/include/hw/acpi/acpi-build-utils.h
> index e6a0b28..fd50625 100644
> --- a/include/hw/acpi/acpi-build-utils.h
> +++ b/include/hw/acpi/acpi-build-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);
>

Acked-by: Marcel Apfelbaum <marcel@redhat.com>

Re: [Qemu-devel] [PATCH v5 4/5] acpi: drop min-bytes in build_package()

[Marcel Apfelbaum]

On 01/21/2015 11:09 AM, Igor Mammedov wrote:
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> Reviewed-by: Claudio Fontana <claudio.fontana@huawei.com>
> ---
>   hw/acpi/acpi-build-utils.c         | 14 ++++----------
>   hw/i386/acpi-build.c               | 13 ++++++-------
>   include/hw/acpi/acpi-build-utils.h |  4 ++--
>   3 files changed, 12 insertions(+), 19 deletions(-)
>
> diff --git a/hw/acpi/acpi-build-utils.c b/hw/acpi/acpi-build-utils.c
> index aed9066..602e68c 100644
> --- a/hw/acpi/acpi-build-utils.c
> +++ b/hw/acpi/acpi-build-utils.c
> @@ -167,7 +167,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;
> @@ -183,11 +183,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;
>
> @@ -220,15 +215,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 */
>   }
>
> @@ -272,4 +267,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 e3ac1a14..a010f3b 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -293,7 +293,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);
> @@ -314,7 +314,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);
>
> @@ -827,7 +827,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);
>
> @@ -868,7 +868,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 */
> @@ -1008,7 +1008,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);
>           }
> @@ -1056,8 +1056,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-build-utils.h b/include/hw/acpi/acpi-build-utils.h
> index fd50625..199f003 100644
> --- a/include/hw/acpi/acpi-build-utils.h
> +++ b/include/hw/acpi/acpi-build-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);
>
Reviewed-by: Marcel Apfelbaum <marcel@redhat.com>

Re: [Qemu-devel] [PATCH v5 5/5] pc: acpi-build: simplify PCI bus tree generation

[Marcel Apfelbaum]

On 01/21/2015 11:09 AM, Igor Mammedov wrote:
> 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>
> ---
> v4:
>    * keep original logic of creating bridge devices as it was done
>      in 133a2da48 "pc: acpi: generate AML only for PCI0 ..."
>    * if bus is non hotpluggable, add child slots to bus as non
>      hotpluggable as it was done in original code.
> 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 | 275 +++++++++++++++++----------------------------------
>   1 file changed, 90 insertions(+), 185 deletions(-)
>
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index a010f3b..5255428 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -106,7 +106,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;
> @@ -651,69 +650,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");
>       }
>
> @@ -722,17 +689,9 @@ 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;
> @@ -741,152 +700,104 @@ static void build_pci_bus_end(PCIBus *bus, void *bus_state)
>           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);
> +            } else {
> +                /* no much sense to create empty non hotpluggable slots,
> +                 * but it's what original code did before. TODO: remove it.
> +                 */
> +                void *pcihp = acpi_data_push(bus_table, ACPI_PCINOHP_SIZEOF);
> +                memcpy(pcihp, ACPI_PCINOHP_AML, ACPI_PCINOHP_SIZEOF);
> +                patch_pcinohp(slot, pcihp);
> +            }
>               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;
>           }
> +        bridge_in_acpi = pc->is_bridge && pcihp_bridge_en;
>
>           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 || bridge_in_acpi) {
> -            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 && !bridge_in_acpi) {
> +            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);
> +            /* When hotplug for bridges is enabled, bridges that are
> +             * described in ACPI separately aren't themselves hot-pluggable.
> +             */
> +            if (bridge_in_acpi) {
> +                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)
> @@ -1035,7 +946,6 @@ build_ssdt(GArray *table_data, GArray *linker,
>           }
>
>           {
> -            AcpiBuildPciBusHotplugState hotplug_state;
>               Object *pci_host;
>               PCIBus *bus = NULL;
>               bool ambiguous;
> @@ -1045,16 +955,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);
>

This one is tricky to review...
I do agree with the concept, I like the simplification,
and no obvious errors were found.

So ... "light":
Reviewed-by: Marcel Apfelbaum <marcel@redhat.com>

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

[Michael S. Tsirkin]

On Wed, Jan 21, 2015 at 09:09:10AM +0000, 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>

OK so the only thing holding this up is really that
you have indicated that in the follow-up patches
you want to add here more stuff with
aml_ prefix instead of acpi_.

In that case let's name this file aml-build.c?


> ---
> v3:
>   * rename acpi_gen_utils.[ch] to acpi-build-utils.[ch]
>   * copy GLP license block from acpi-build.c
> v2:
>   * fix wrong ident in moved code
> ---
>  hw/acpi/Makefile.objs              |   1 +
>  hw/acpi/acpi-build-utils.c         | 187 +++++++++++++++++++++++++++++++++++++
>  hw/i386/acpi-build.c               | 162 +-------------------------------
>  include/hw/acpi/acpi-build-utils.h |  23 +++++
>  4 files changed, 213 insertions(+), 160 deletions(-)
>  create mode 100644 hw/acpi/acpi-build-utils.c
>  create mode 100644 include/hw/acpi/acpi-build-utils.h
> 
> diff --git a/hw/acpi/Makefile.objs b/hw/acpi/Makefile.objs
> index ee82073..cad0355 100644
> --- a/hw/acpi/Makefile.objs
> +++ b/hw/acpi/Makefile.objs
> @@ -2,3 +2,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) += bios-linker-loader.o
> +common-obj-$(CONFIG_ACPI) += acpi-build-utils.o
> diff --git a/hw/acpi/acpi-build-utils.c b/hw/acpi/acpi-build-utils.c
> new file mode 100644
> index 0000000..79aa610
> --- /dev/null
> +++ b/hw/acpi/acpi-build-utils.c
> @@ -0,0 +1,187 @@
> +/* Support for generating ACPI tables and passing them to Guests
> + *
> + * Copyright (C) 2014 Red Hat Inc
> + *
> + * Author: Michael S. Tsirkin <mst@redhat.com>
> + * Author: Igor Mammedov <imammedo@redhat.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> +
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> +
> + * You should have received a copy of the GNU General Public License along
> + * with this program; if not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#include <stdio.h>
> +#include <stdarg.h>
> +#include <assert.h>
> +#include <stdbool.h>
> +#include "hw/acpi/acpi-build-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 4c0536f..8ee15ab 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-build-utils.h"
> +
>  #include "qapi/qmp/qint.h"
>  #include "qom/qom-qobject.h"
>  #include "exec/ram_addr.h"
> @@ -277,166 +279,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-build-utils.h b/include/hw/acpi/acpi-build-utils.h
> new file mode 100644
> index 0000000..e6a0b28
> --- /dev/null
> +++ b/include/hw/acpi/acpi-build-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
> 

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

[Igor Mammedov]

On Tue, 27 Jan 2015 14:23:18 +0200
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Wed, Jan 21, 2015 at 09:09:10AM +0000, 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>
> 
> OK so the only thing holding this up is really that
> you have indicated that in the follow-up patches
> you want to add here more stuff with
> aml_ prefix instead of acpi_.
> 
> In that case let's name this file aml-build.c?
sure, I'll rename it.

> 
> 
> > ---
> > v3:
> >   * rename acpi_gen_utils.[ch] to acpi-build-utils.[ch]
> >   * copy GLP license block from acpi-build.c
> > v2:
> >   * fix wrong ident in moved code
> > ---
> >  hw/acpi/Makefile.objs              |   1 +
> >  hw/acpi/acpi-build-utils.c         | 187 +++++++++++++++++++++++++++++++++++++
> >  hw/i386/acpi-build.c               | 162 +-------------------------------
> >  include/hw/acpi/acpi-build-utils.h |  23 +++++
> >  4 files changed, 213 insertions(+), 160 deletions(-)
> >  create mode 100644 hw/acpi/acpi-build-utils.c
> >  create mode 100644 include/hw/acpi/acpi-build-utils.h
> > 
> > diff --git a/hw/acpi/Makefile.objs b/hw/acpi/Makefile.objs
> > index ee82073..cad0355 100644
> > --- a/hw/acpi/Makefile.objs
> > +++ b/hw/acpi/Makefile.objs
> > @@ -2,3 +2,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) += bios-linker-loader.o
> > +common-obj-$(CONFIG_ACPI) += acpi-build-utils.o
> > diff --git a/hw/acpi/acpi-build-utils.c b/hw/acpi/acpi-build-utils.c
> > new file mode 100644
> > index 0000000..79aa610
> > --- /dev/null
> > +++ b/hw/acpi/acpi-build-utils.c
> > @@ -0,0 +1,187 @@
> > +/* Support for generating ACPI tables and passing them to Guests
> > + *
> > + * Copyright (C) 2014 Red Hat Inc
> > + *
> > + * Author: Michael S. Tsirkin <mst@redhat.com>
> > + * Author: Igor Mammedov <imammedo@redhat.com>
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License as published by
> > + * the Free Software Foundation; either version 2 of the License, or
> > + * (at your option) any later version.
> > +
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > + * GNU General Public License for more details.
> > +
> > + * You should have received a copy of the GNU General Public License along
> > + * with this program; if not, see <http://www.gnu.org/licenses/>.
> > + */
> > +
> > +#include <stdio.h>
> > +#include <stdarg.h>
> > +#include <assert.h>
> > +#include <stdbool.h>
> > +#include "hw/acpi/acpi-build-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 4c0536f..8ee15ab 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-build-utils.h"
> > +
> >  #include "qapi/qmp/qint.h"
> >  #include "qom/qom-qobject.h"
> >  #include "exec/ram_addr.h"
> > @@ -277,166 +279,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-build-utils.h b/include/hw/acpi/acpi-build-utils.h
> > new file mode 100644
> > index 0000000..e6a0b28
> > --- /dev/null
> > +++ b/include/hw/acpi/acpi-build-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
> > 
>