[Qemu-devel] [PATCH v4 0/7] pc: acpi: various fixes and cleanups

[Qemu-devel] [PATCH v4 0/7] pc: acpi: various fixes and cleanups

[Igor Mammedov]

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

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

previous revision:
 [PATCH v3 0/8] pc: acpi: various fixes and cleanups
 http://lists.gnu.org/archive/html/qemu-devel/2014-12/msg03057.html

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

 hw/acpi/Makefile.objs              |   1 +
 hw/acpi/acpi-build-utils.c         | 269 +++++++++++++++++++++
 hw/i386/acpi-build.c               | 464 +++++++++----------------------------
 hw/i386/acpi-dsdt-cpu-hotplug.dsl  |   1 +
 include/hw/acpi/acpi-build-utils.h |  23 ++
 5 files changed, 398 insertions(+), 360 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 v4 1/7] pc: acpi: fix WindowsXP BSOD when memory hotplug is enabled

[Igor Mammedov]

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

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

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

[Qemu-devel] [PATCH v4 2/7] 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 6a2e9c5..e0b66e7 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 v4 3/7] acpi: build_append_nameseg(): add padding if necessary

[Igor Mammedov]

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

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

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

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

[Qemu-devel] [PATCH v4 4/7] 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 acd2389..bd79e91 100644
--- a/hw/acpi/Makefile.objs
+++ b/hw/acpi/Makefile.objs
@@ -1,3 +1,4 @@
 common-obj-$(CONFIG_ACPI) += core.o piix4.o ich9.o pcihp.o cpu_hotplug.o
 common-obj-$(CONFIG_ACPI) += memory_hotplug.o
 common-obj-$(CONFIG_ACPI) += acpi_interface.o
+common-obj-$(CONFIG_ACPI) += acpi-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 d857e4b..f6cccd6 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"
@@ -276,166 +278,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 v4 5/7] 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 f6cccd6..341b669 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -282,7 +282,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;
@@ -574,7 +574,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);
@@ -702,24 +702,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 {
@@ -822,7 +822,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 */
@@ -846,12 +846,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 */
         }
 
@@ -877,11 +877,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");
         }
     }
 
@@ -949,7 +946,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++) {
@@ -969,7 +966,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 v4 6/7] 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 341b669..5632fae 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -292,7 +292,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);
@@ -313,7 +313,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);
 
@@ -826,7 +826,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);
 
@@ -867,7 +867,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 */
@@ -990,7 +990,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);
         }
@@ -1038,8 +1038,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 v4 7/7] 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 5632fae..a8a1368 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;
@@ -650,69 +649,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");
     }
 
@@ -721,17 +688,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;
@@ -740,152 +699,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)
@@ -1017,7 +928,6 @@ build_ssdt(GArray *table_data, GArray *linker,
         }
 
         {
-            AcpiBuildPciBusHotplugState hotplug_state;
             Object *pci_host;
             PCIBus *bus = NULL;
             bool ambiguous;
@@ -1027,16 +937,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 v4 0/7] pc: acpi: various fixes and cleanups

[Michael S. Tsirkin]

On Tue, Jan 20, 2015 at 03:52:25PM +0000, Igor Mammedov wrote:
> NOTE to maintainer: please update test data (ACPI blobs) in test cases
> 
> 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
> 
> previous revision:
>  [PATCH v3 0/8] pc: acpi: various fixes and cleanups
>  http://lists.gnu.org/archive/html/qemu-devel/2014-12/msg03057.html
> 
> Igor Mammedov (7):
>   pc: acpi: fix WindowsXP BSOD when memory hotplug is enabled

At least this one is already in pci tree isn't it?
Could you pls rebase?
Or if there's a replacement patch, pls send fixup or
mention old one needs to be retracted in commit log.



>   pc: acpi-build: cleanup AcpiPmInfo initialization
>   acpi: build_append_nameseg(): add padding if necessary
>   acpi: move generic aml building helpers into dedictated file
>   acpi: add build_append_namestring() helper
>   acpi: drop min-bytes in build_package()
>   pc: acpi-build: simplify PCI bus tree generation
> 
>  hw/acpi/Makefile.objs              |   1 +
>  hw/acpi/acpi-build-utils.c         | 269 +++++++++++++++++++++
>  hw/i386/acpi-build.c               | 464 +++++++++----------------------------
>  hw/i386/acpi-dsdt-cpu-hotplug.dsl  |   1 +
>  include/hw/acpi/acpi-build-utils.h |  23 ++
>  5 files changed, 398 insertions(+), 360 deletions(-)
>  create mode 100644 hw/acpi/acpi-build-utils.c
>  create mode 100644 include/hw/acpi/acpi-build-utils.h
> 
> -- 
> 1.8.3.1

Re: [Qemu-devel] [PATCH v4 0/7] pc: acpi: various fixes and cleanups

[Igor Mammedov]

On Tue, 20 Jan 2015 18:29:07 +0200
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Tue, Jan 20, 2015 at 03:52:25PM +0000, Igor Mammedov wrote:
> > NOTE to maintainer: please update test data (ACPI blobs) in test cases
> > 
> > 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
> > 
> > previous revision:
> >  [PATCH v3 0/8] pc: acpi: various fixes and cleanups
> >  http://lists.gnu.org/archive/html/qemu-devel/2014-12/msg03057.html
> > 
> > Igor Mammedov (7):
> >   pc: acpi: fix WindowsXP BSOD when memory hotplug is enabled
> 
> At least this one is already in pci tree isn't it?
> Could you pls rebase?
> Or if there's a replacement patch, pls send fixup or
> mention old one needs to be retracted in commit log.
It's exact copy, you can just drop it.

> 
> 
> 
> >   pc: acpi-build: cleanup AcpiPmInfo initialization
> >   acpi: build_append_nameseg(): add padding if necessary
> >   acpi: move generic aml building helpers into dedictated file
> >   acpi: add build_append_namestring() helper
> >   acpi: drop min-bytes in build_package()
> >   pc: acpi-build: simplify PCI bus tree generation
> > 
> >  hw/acpi/Makefile.objs              |   1 +
> >  hw/acpi/acpi-build-utils.c         | 269 +++++++++++++++++++++
> >  hw/i386/acpi-build.c               | 464 +++++++++----------------------------
> >  hw/i386/acpi-dsdt-cpu-hotplug.dsl  |   1 +
> >  include/hw/acpi/acpi-build-utils.h |  23 ++
> >  5 files changed, 398 insertions(+), 360 deletions(-)
> >  create mode 100644 hw/acpi/acpi-build-utils.c
> >  create mode 100644 include/hw/acpi/acpi-build-utils.h
> > 
> > -- 
> > 1.8.3.1

Re: [Qemu-devel] [PATCH v4 0/7] pc: acpi: various fixes and cleanups

[Igor Mammedov]

On Tue, 20 Jan 2015 18:29:07 +0200
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Tue, Jan 20, 2015 at 03:52:25PM +0000, Igor Mammedov wrote:
> > NOTE to maintainer: please update test data (ACPI blobs) in test
> > cases
> > 
> > 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
> > 
> > previous revision:
> >  [PATCH v3 0/8] pc: acpi: various fixes and cleanups
> >  http://lists.gnu.org/archive/html/qemu-devel/2014-12/msg03057.html
> > 
> > Igor Mammedov (7):
> >   pc: acpi: fix WindowsXP BSOD when memory hotplug is enabled
> 
> At least this one is already in pci tree isn't it?
> Could you pls rebase?
> Or if there's a replacement patch, pls send fixup or
> mention old one needs to be retracted in commit log.
I've rebased it on PCI tree, there was a trivial conflict in 4/7
you can pull result from:
https://github.com/imammedo/qemu/commits/acpi_pci_gen_simplification_v4_PCI

> 
> 
> 
> >   pc: acpi-build: cleanup AcpiPmInfo initialization
> >   acpi: build_append_nameseg(): add padding if necessary
> >   acpi: move generic aml building helpers into dedictated file
> >   acpi: add build_append_namestring() helper
> >   acpi: drop min-bytes in build_package()
> >   pc: acpi-build: simplify PCI bus tree generation
> > 
> >  hw/acpi/Makefile.objs              |   1 +
> >  hw/acpi/acpi-build-utils.c         | 269 +++++++++++++++++++++
> >  hw/i386/acpi-build.c               | 464
> > +++++++++----------------------------
> > hw/i386/acpi-dsdt-cpu-hotplug.dsl  |   1 +
> > include/hw/acpi/acpi-build-utils.h |  23 ++ 5 files changed, 398
> > insertions(+), 360 deletions(-) create mode 100644
> > hw/acpi/acpi-build-utils.c create mode 100644
> > include/hw/acpi/acpi-build-utils.h
> > 
> > -- 
> > 1.8.3.1

Re: [Qemu-devel] [PATCH v4 0/7] pc: acpi: various fixes and cleanups

[Michael S. Tsirkin]

On Tue, Jan 20, 2015 at 09:38:18PM +0100, Igor Mammedov wrote:
> On Tue, 20 Jan 2015 18:29:07 +0200
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
> 
> > On Tue, Jan 20, 2015 at 03:52:25PM +0000, Igor Mammedov wrote:
> > > NOTE to maintainer: please update test data (ACPI blobs) in test
> > > cases
> > > 
> > > 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
> > > 
> > > previous revision:
> > >  [PATCH v3 0/8] pc: acpi: various fixes and cleanups
> > >  http://lists.gnu.org/archive/html/qemu-devel/2014-12/msg03057.html
> > > 
> > > Igor Mammedov (7):
> > >   pc: acpi: fix WindowsXP BSOD when memory hotplug is enabled
> > 
> > At least this one is already in pci tree isn't it?
> > Could you pls rebase?
> > Or if there's a replacement patch, pls send fixup or
> > mention old one needs to be retracted in commit log.
> I've rebased it on PCI tree, there was a trivial conflict in 4/7
> you can pull result from:
> https://github.com/imammedo/qemu/commits/acpi_pci_gen_simplification_v4_PCI

That's not a git url.
Please just repost, it's easier.

> > 
> > 
> > 
> > >   pc: acpi-build: cleanup AcpiPmInfo initialization
> > >   acpi: build_append_nameseg(): add padding if necessary
> > >   acpi: move generic aml building helpers into dedictated file
> > >   acpi: add build_append_namestring() helper
> > >   acpi: drop min-bytes in build_package()
> > >   pc: acpi-build: simplify PCI bus tree generation
> > > 
> > >  hw/acpi/Makefile.objs              |   1 +
> > >  hw/acpi/acpi-build-utils.c         | 269 +++++++++++++++++++++
> > >  hw/i386/acpi-build.c               | 464
> > > +++++++++----------------------------
> > > hw/i386/acpi-dsdt-cpu-hotplug.dsl  |   1 +
> > > include/hw/acpi/acpi-build-utils.h |  23 ++ 5 files changed, 398
> > > insertions(+), 360 deletions(-) create mode 100644
> > > hw/acpi/acpi-build-utils.c create mode 100644
> > > include/hw/acpi/acpi-build-utils.h
> > > 
> > > -- 
> > > 1.8.3.1