All of lore.kernel.org
 help / color / mirror / Atom feed
* [Qemu-devel] [PATCH v7 0/5] pc: acpi: various fixes and cleanups
@ 2015-01-30 13:29 Igor Mammedov
  2015-01-30 13:29 ` [Qemu-devel] [PATCH v7 1/5] acpi: move generic aml building helpers into dedictated file Igor Mammedov
                   ` (4 more replies)
  0 siblings, 5 replies; 6+ messages in thread
From: Igor Mammedov @ 2015-01-30 13:29 UTC (permalink / raw)
  To: qemu-devel; +Cc: marcel.a, mst

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

changes from v6:
 * drop "[PATCH v6 1/5] pc: acpi-build: cleanup AcpiPmInfo initialization"
 * fixup and cleanup build_append_namestring patch as Michael requested
 * add extra patch based on "acpi-build: skip hotplugged bridges"
   http://lists.gnu.org/archive/html/qemu-devel/2015-01/msg04085.html
   which applies on top of this series

changes from v5:
 * fix codding style issue with var names
 * fix Copyright from 2014 to 2015
 * rename acpi-build-utils.[ch] to aml-build.[ch]

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_v7



Igor Mammedov (5):
  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
  acpi-build: skip hotplugged bridges

 hw/acpi/Makefile.objs       |   1 +
 hw/acpi/aml-build.c         | 259 +++++++++++++++++++++++++
 hw/i386/acpi-build.c        | 456 ++++++++++----------------------------------
 include/hw/acpi/aml-build.h |  23 +++
 4 files changed, 380 insertions(+), 359 deletions(-)
 create mode 100644 hw/acpi/aml-build.c
 create mode 100644 include/hw/acpi/aml-build.h

-- 
1.8.3.1

^ permalink raw reply	[flat|nested] 6+ messages in thread

* [Qemu-devel] [PATCH v7 1/5] acpi: move generic aml building helpers into dedictated file
  2015-01-30 13:29 [Qemu-devel] [PATCH v7 0/5] pc: acpi: various fixes and cleanups Igor Mammedov
@ 2015-01-30 13:29 ` Igor Mammedov
  2015-01-30 13:29 ` [Qemu-devel] [PATCH v7 2/5] acpi: add build_append_namestring() helper Igor Mammedov
                   ` (3 subsequent siblings)
  4 siblings, 0 replies; 6+ messages in thread
From: Igor Mammedov @ 2015-01-30 13:29 UTC (permalink / raw)
  To: qemu-devel; +Cc: marcel.a, mst

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>
Acked-by: Marcel Apfelbaum <marcel@redhat.com>
---
v4:
  * rename acpi_gen_utils.[ch] to aml-build.[ch]
  * fix Copyright from 2014 to 2015
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

fixup

rename to hw/acpi/aml-build.c
---
 hw/acpi/Makefile.objs       |   1 +
 hw/acpi/aml-build.c         | 187 ++++++++++++++++++++++++++++++++++++++++++++
 hw/i386/acpi-build.c        | 162 +-------------------------------------
 include/hw/acpi/aml-build.h |  23 ++++++
 4 files changed, 213 insertions(+), 160 deletions(-)
 create mode 100644 hw/acpi/aml-build.c
 create mode 100644 include/hw/acpi/aml-build.h

diff --git a/hw/acpi/Makefile.objs b/hw/acpi/Makefile.objs
index ee82073..b9fefa7 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) += aml-build.o
diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
new file mode 100644
index 0000000..b28c1f4
--- /dev/null
+++ b/hw/acpi/aml-build.c
@@ -0,0 +1,187 @@
+/* Support for generating ACPI tables and passing them to Guests
+ *
+ * Copyright (C) 2015 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/aml-build.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 4944249..b3bd269 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/aml-build.h"
+
 #include "qapi/qmp/qint.h"
 #include "qom/qom-qobject.h"
 #include "exec/ram_addr.h"
@@ -280,166 +282,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/aml-build.h b/include/hw/acpi/aml-build.h
new file mode 100644
index 0000000..e6a0b28
--- /dev/null
+++ b/include/hw/acpi/aml-build.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

^ permalink raw reply related	[flat|nested] 6+ messages in thread

* [Qemu-devel] [PATCH v7 2/5] acpi: add build_append_namestring() helper
  2015-01-30 13:29 [Qemu-devel] [PATCH v7 0/5] pc: acpi: various fixes and cleanups Igor Mammedov
  2015-01-30 13:29 ` [Qemu-devel] [PATCH v7 1/5] acpi: move generic aml building helpers into dedictated file Igor Mammedov
@ 2015-01-30 13:29 ` Igor Mammedov
  2015-01-30 13:29 ` [Qemu-devel] [PATCH v7 3/5] acpi: drop min-bytes in build_package() Igor Mammedov
                   ` (2 subsequent siblings)
  4 siblings, 0 replies; 6+ messages in thread
From: Igor Mammedov @ 2015-01-30 13:29 UTC (permalink / raw)
  To: qemu-devel; +Cc: marcel.a, mst

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 reference ACPI 5.0: 20.2.2 Name Objects Encoding

Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
v5:
 * replace g_array_append_val() with build_append_byte() and
   use literals instead of temporary variables
 * fix indent in acpi-build.c
 * build_append_nameseg() no longer used with format string
   replace variable args prototype with fixed one that takes
   only "char *seg" as argument
v4:
 * fix codding style of ACPI_DualNamePrefix & ACPI_MultiNamePrefix
   it's imposible to use literals as suggested due to
   g_array_append_val() requiring reference value

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/aml-build.c         | 96 ++++++++++++++++++++++++++++++++++++++++-----
 hw/i386/acpi-build.c        | 37 ++++++++---------
 include/hw/acpi/aml-build.h |  2 +-
 3 files changed, 105 insertions(+), 30 deletions(-)

diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
index b28c1f4..6a02474 100644
--- a/hw/acpi/aml-build.c
+++ b/hw/acpi/aml-build.c
@@ -23,6 +23,7 @@
 #include <stdarg.h>
 #include <assert.h>
 #include <stdbool.h>
+#include <string.h>
 #include "hw/acpi/aml-build.h"
 
 GArray *build_alloc_array(void)
@@ -52,25 +53,102 @@ void build_append_array(GArray *array, GArray *val)
 
 #define ACPI_NAMESEG_LEN 4
 
-void GCC_FMT_ATTR(2, 3)
-build_append_nameseg(GArray *array, const char *format, ...)
+static void
+build_append_nameseg(GArray *array, const char *seg)
 {
     /* 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);
 
+    len = strlen(seg);
     assert(len <= ACPI_NAMESEG_LEN);
 
-    g_array_append_vals(array, s, len);
+    g_array_append_vals(array, seg, len);
     /* Pad up to ACPI_NAMESEG_LEN characters if necessary. */
     g_array_append_vals(array, "____", ACPI_NAMESEG_LEN - len);
 }
 
+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 == '^') {
+        build_append_byte(array, *s);
+        ++s;
+    }
+
+    switch (seg_count) {
+    case 1:
+        if (!*s) {
+            build_append_byte(array, 0x0); /* NullName */
+        } else {
+            build_append_nameseg(array, s);
+        }
+        break;
+
+    case 2:
+        build_append_byte(array, 0x2E); /* DualNamePrefix */
+        build_append_nameseg(array, s);
+        build_append_nameseg(array, segs[1]);
+        break;
+    default:
+        build_append_byte(array, 0x2F); /* MultiNamePrefix */
+        build_append_byte(array, seg_count);
+
+        /* handle the 1st segment manually due to prefix/root path */
+        build_append_nameseg(array, s);
+
+        /* add the rest of segments */
+        segs_iter = segs + 1;
+        while (*segs_iter) {
+            build_append_nameseg(array, *segs_iter);
+            ++segs_iter;
+        }
+        break;
+    }
+    g_strfreev(segs);
+}
+
+void 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 b3bd269..4784756 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -286,7 +286,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;
@@ -578,7 +578,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);
@@ -706,24 +706,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 {
@@ -826,7 +826,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 */
@@ -850,12 +850,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 */
         }
 
@@ -881,11 +881,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",
-                                 bus->parent_dev->devfn);
-            build_append_nameseg(parent->notify_table, "PCNT");
+            build_append_namestring(parent->notify_table, "^PCNT.S%.02X",
+                                    bus->parent_dev->devfn);
         }
     }
 
@@ -953,7 +950,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++) {
@@ -973,7 +970,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/aml-build.h b/include/hw/acpi/aml-build.h
index e6a0b28..fd50625 100644
--- a/include/hw/acpi/aml-build.h
+++ b/include/hw/acpi/aml-build.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

^ permalink raw reply related	[flat|nested] 6+ messages in thread

* [Qemu-devel] [PATCH v7 3/5] acpi: drop min-bytes in build_package()
  2015-01-30 13:29 [Qemu-devel] [PATCH v7 0/5] pc: acpi: various fixes and cleanups Igor Mammedov
  2015-01-30 13:29 ` [Qemu-devel] [PATCH v7 1/5] acpi: move generic aml building helpers into dedictated file Igor Mammedov
  2015-01-30 13:29 ` [Qemu-devel] [PATCH v7 2/5] acpi: add build_append_namestring() helper Igor Mammedov
@ 2015-01-30 13:29 ` Igor Mammedov
  2015-01-30 13:29 ` [Qemu-devel] [PATCH v7 4/5] pc: acpi-build: simplify PCI bus tree generation Igor Mammedov
  2015-01-30 13:29 ` [Qemu-devel] [PATCH v7 5/5] acpi-build: skip hotplugged bridges Igor Mammedov
  4 siblings, 0 replies; 6+ messages in thread
From: Igor Mammedov @ 2015-01-30 13:29 UTC (permalink / raw)
  To: qemu-devel; +Cc: marcel.a, mst

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

diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
index 6a02474..bcb288e 100644
--- a/hw/acpi/aml-build.c
+++ b/hw/acpi/aml-build.c
@@ -157,7 +157,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;
@@ -173,11 +173,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;
 
@@ -210,15 +205,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 */
 }
 
@@ -262,4 +257,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 4784756..6a1d9ab 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -296,7 +296,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);
@@ -317,7 +317,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);
 
@@ -830,7 +830,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);
 
@@ -871,7 +871,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 */
@@ -994,7 +994,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);
         }
@@ -1042,8 +1042,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/aml-build.h b/include/hw/acpi/aml-build.h
index fd50625..199f003 100644
--- a/include/hw/acpi/aml-build.h
+++ b/include/hw/acpi/aml-build.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

^ permalink raw reply related	[flat|nested] 6+ messages in thread

* [Qemu-devel] [PATCH v7 4/5] pc: acpi-build: simplify PCI bus tree generation
  2015-01-30 13:29 [Qemu-devel] [PATCH v7 0/5] pc: acpi: various fixes and cleanups Igor Mammedov
                   ` (2 preceding siblings ...)
  2015-01-30 13:29 ` [Qemu-devel] [PATCH v7 3/5] acpi: drop min-bytes in build_package() Igor Mammedov
@ 2015-01-30 13:29 ` Igor Mammedov
  2015-01-30 13:29 ` [Qemu-devel] [PATCH v7 5/5] acpi-build: skip hotplugged bridges Igor Mammedov
  4 siblings, 0 replies; 6+ messages in thread
From: Igor Mammedov @ 2015-01-30 13:29 UTC (permalink / raw)
  To: qemu-devel; +Cc: marcel.a, mst

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>
Reviewed-by: Marcel Apfelbaum <marcel@redhat.com>
---
v5:
  * drop wrong comment
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 | 272 ++++++++++++++++-----------------------------------
 1 file changed, 87 insertions(+), 185 deletions(-)

diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 6a1d9ab..f91b7cf 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;
@@ -654,69 +653,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");
     }
 
@@ -725,17 +692,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;
@@ -744,152 +703,101 @@ 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 {
+                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)
@@ -1021,7 +929,6 @@ build_ssdt(GArray *table_data, GArray *linker,
         }
 
         {
-            AcpiBuildPciBusHotplugState hotplug_state;
             Object *pci_host;
             PCIBus *bus = NULL;
             bool ambiguous;
@@ -1031,16 +938,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

^ permalink raw reply related	[flat|nested] 6+ messages in thread

* [Qemu-devel] [PATCH v7 5/5] acpi-build: skip hotplugged bridges
  2015-01-30 13:29 [Qemu-devel] [PATCH v7 0/5] pc: acpi: various fixes and cleanups Igor Mammedov
                   ` (3 preceding siblings ...)
  2015-01-30 13:29 ` [Qemu-devel] [PATCH v7 4/5] pc: acpi-build: simplify PCI bus tree generation Igor Mammedov
@ 2015-01-30 13:29 ` Igor Mammedov
  4 siblings, 0 replies; 6+ messages in thread
From: Igor Mammedov @ 2015-01-30 13:29 UTC (permalink / raw)
  To: qemu-devel; +Cc: marcel.a, mst

Hotplugged bridges don't get bsel allocated so acpi hotplug doesn't
work for them anyway, also it causes ACPI tables size change across
reboot when bridge was hotplugged before reboot, which doesn't work
with immutable RSDP.
This patch works around static RSDP issue, where if ACPI tables blob
changes it size across reboots RSDT shifts up or down and RSDP no
longer ponts to it, as result guest can't find/initialize ACPI tables
correctly. Which causes BSOD for Windows guests.

With this patch slot where bridge was hotplugged will keep the same
description, i.e. as hotpluggable slot and also hotplugged bridge
subtree won't be build, keeping size of ACPI tables blob the same.

Subtree for bridge is build only for cold-plugged bridges.

based on "Michael S. Tsirkin" <mst@redhat.com> patch
http://lists.gnu.org/archive/html/qemu-devel/2015-01/msg04085.html
but a bit simpler

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

diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index f91b7cf..27adfb9 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -723,7 +723,7 @@ static void build_append_pci_bus_devices(GArray *parent_scope, PCIBus *bus,
         if (pc->class_id == PCI_CLASS_BRIDGE_ISA) {
             continue;
         }
-        bridge_in_acpi = pc->is_bridge && pcihp_bridge_en;
+        bridge_in_acpi = pc->is_bridge && pcihp_bridge_en && !dc->hotpluggable;
 
         if (pc->class_id == PCI_CLASS_DISPLAY_VGA) {
 
@@ -751,9 +751,7 @@ static void build_append_pci_bus_devices(GArray *parent_scope, PCIBus *bus,
             memcpy(pcihp, ACPI_PCINOHP_AML, ACPI_PCINOHP_SIZEOF);
             patch_pcinohp(slot, pcihp);
 
-            /* When hotplug for bridges is enabled, bridges that are
-             * described in ACPI separately aren't themselves hot-pluggable.
-             */
+            /* Describe coldplugged bridges in ACPI */
             if (bridge_in_acpi) {
                 PCIBus *sec_bus = pci_bridge_get_sec_bus(PCI_BRIDGE(pdev));
 
-- 
1.8.3.1

^ permalink raw reply related	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2015-01-30 13:30 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-01-30 13:29 [Qemu-devel] [PATCH v7 0/5] pc: acpi: various fixes and cleanups Igor Mammedov
2015-01-30 13:29 ` [Qemu-devel] [PATCH v7 1/5] acpi: move generic aml building helpers into dedictated file Igor Mammedov
2015-01-30 13:29 ` [Qemu-devel] [PATCH v7 2/5] acpi: add build_append_namestring() helper Igor Mammedov
2015-01-30 13:29 ` [Qemu-devel] [PATCH v7 3/5] acpi: drop min-bytes in build_package() Igor Mammedov
2015-01-30 13:29 ` [Qemu-devel] [PATCH v7 4/5] pc: acpi-build: simplify PCI bus tree generation Igor Mammedov
2015-01-30 13:29 ` [Qemu-devel] [PATCH v7 5/5] acpi-build: skip hotplugged bridges Igor Mammedov

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.