* [Qemu-devel] [PATCH v6 1/5] pc: acpi-build: cleanup AcpiPmInfo initialization
2015-01-28 14:34 [Qemu-devel] [PATCH v6 0/5] pc: acpi: various fixes and cleanups Igor Mammedov
@ 2015-01-28 14:34 ` Igor Mammedov
2015-01-28 15:22 ` Michael S. Tsirkin
2015-01-28 14:34 ` [Qemu-devel] [PATCH v6 2/5] acpi: move generic aml building helpers into dedictated file Igor Mammedov
` (3 subsequent siblings)
4 siblings, 1 reply; 16+ messages in thread
From: Igor Mammedov @ 2015-01-28 14:34 UTC (permalink / raw)
To: qemu-devel; +Cc: mst
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>
Reviewed-by: Marcel Apfelbaum <marcel@redhat.com>
---
hw/i386/acpi-build.c | 8 ++------
1 file changed, 2 insertions(+), 6 deletions(-)
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 4944249..6f8d4e3 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
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [Qemu-devel] [PATCH v6 1/5] pc: acpi-build: cleanup AcpiPmInfo initialization
2015-01-28 14:34 ` [Qemu-devel] [PATCH v6 1/5] pc: acpi-build: cleanup AcpiPmInfo initialization Igor Mammedov
@ 2015-01-28 15:22 ` Michael S. Tsirkin
0 siblings, 0 replies; 16+ messages in thread
From: Michael S. Tsirkin @ 2015-01-28 15:22 UTC (permalink / raw)
To: Igor Mammedov; +Cc: qemu-devel
On Wed, Jan 28, 2015 at 02:34:46PM +0000, Igor Mammedov wrote:
> zero initialize AcpiPmInfo struct to reduce code bloat
> a little bit.
>
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> Reviewed-by: Claudio Fontana <claudio.fontana@huawei.com>
> Reviewed-by: Marcel Apfelbaum <marcel@redhat.com>
Pls drop this, I prefer consistent initialization.
> ---
> 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 4944249..6f8d4e3 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
^ permalink raw reply [flat|nested] 16+ messages in thread
* [Qemu-devel] [PATCH v6 2/5] acpi: move generic aml building helpers into dedictated file
2015-01-28 14:34 [Qemu-devel] [PATCH v6 0/5] pc: acpi: various fixes and cleanups Igor Mammedov
2015-01-28 14:34 ` [Qemu-devel] [PATCH v6 1/5] pc: acpi-build: cleanup AcpiPmInfo initialization Igor Mammedov
@ 2015-01-28 14:34 ` Igor Mammedov
2015-01-28 14:34 ` [Qemu-devel] [PATCH v6 3/5] acpi: add build_append_namestring() helper Igor Mammedov
` (2 subsequent siblings)
4 siblings, 0 replies; 16+ messages in thread
From: Igor Mammedov @ 2015-01-28 14:34 UTC (permalink / raw)
To: qemu-devel; +Cc: 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-build-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 6f8d4e3..a92d008 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"
@@ -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/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] 16+ messages in thread* [Qemu-devel] [PATCH v6 3/5] acpi: add build_append_namestring() helper
2015-01-28 14:34 [Qemu-devel] [PATCH v6 0/5] pc: acpi: various fixes and cleanups Igor Mammedov
2015-01-28 14:34 ` [Qemu-devel] [PATCH v6 1/5] pc: acpi-build: cleanup AcpiPmInfo initialization Igor Mammedov
2015-01-28 14:34 ` [Qemu-devel] [PATCH v6 2/5] acpi: move generic aml building helpers into dedictated file Igor Mammedov
@ 2015-01-28 14:34 ` Igor Mammedov
2015-01-28 15:16 ` Michael S. Tsirkin
2015-01-28 14:34 ` [Qemu-devel] [PATCH v6 4/5] acpi: drop min-bytes in build_package() Igor Mammedov
2015-01-28 14:34 ` [Qemu-devel] [PATCH v6 5/5] pc: acpi-build: simplify PCI bus tree generation Igor Mammedov
4 siblings, 1 reply; 16+ messages in thread
From: Igor Mammedov @ 2015-01-28 14:34 UTC (permalink / raw)
To: qemu-devel; +Cc: 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 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>
Acked-by: Marcel Apfelbaum <marcel@redhat.com>
---
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 | 92 ++++++++++++++++++++++++++++++++++++++++++++-
hw/i386/acpi-build.c | 35 ++++++++---------
include/hw/acpi/aml-build.h | 2 +-
3 files changed, 108 insertions(+), 21 deletions(-)
diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
index b28c1f4..ed4ab56 100644
--- a/hw/acpi/aml-build.c
+++ b/hw/acpi/aml-build.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,96 @@ build_append_nameseg(GArray *array, const char *format, ...)
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 == '^') {
+ 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: {
+ const uint8_t prefix_opcode = 0x2E; /* DualNamePrefix */
+
+ g_array_append_val(array, prefix_opcode);
+ build_append_nameseg(array, "%s", s);
+ build_append_nameseg(array, "%s", segs[1]);
+ break;
+ }
+ default: {
+ const uint8_t prefix_opcode = 0x2F; /* MultiNamePrefix */
+
+ g_array_append_val(array, prefix_opcode);
+ 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;
+ } /* default */
+ } /* switch (seg_count) */
+ 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 a92d008..380799b 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/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] 16+ messages in thread* Re: [Qemu-devel] [PATCH v6 3/5] acpi: add build_append_namestring() helper
2015-01-28 14:34 ` [Qemu-devel] [PATCH v6 3/5] acpi: add build_append_namestring() helper Igor Mammedov
@ 2015-01-28 15:16 ` Michael S. Tsirkin
2015-01-28 15:44 ` Igor Mammedov
2015-01-30 11:46 ` Igor Mammedov
0 siblings, 2 replies; 16+ messages in thread
From: Michael S. Tsirkin @ 2015-01-28 15:16 UTC (permalink / raw)
To: Igor Mammedov; +Cc: qemu-devel
On Wed, Jan 28, 2015 at 02:34:48PM +0000, Igor Mammedov wrote:
> Use build_append_namestring() instead of build_append_nameseg()
> So user won't have to care whether name is NameSeg, NamePath or
> NameString.
>
> See for refernce ACPI 5.0: 20.2.2 Name Objects Encoding
>
typo
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> Reviewed-by: Claudio Fontana <claudio.fontana@huawei.com>
> Acked-by: Marcel Apfelbaum <marcel@redhat.com>
> ---
> 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 | 92 ++++++++++++++++++++++++++++++++++++++++++++-
> hw/i386/acpi-build.c | 35 ++++++++---------
> include/hw/acpi/aml-build.h | 2 +-
> 3 files changed, 108 insertions(+), 21 deletions(-)
>
> diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
> index b28c1f4..ed4ab56 100644
> --- a/hw/acpi/aml-build.c
> +++ b/hw/acpi/aml-build.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,96 @@ build_append_nameseg(GArray *array, const char *format, ...)
> 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;
>
How about we split out formatting?
Make +build_append_namestringv acceptconst char **segs, int nsegs,
put all code above to build_append_namestring.
> + /*
> + * 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: {
> + const uint8_t prefix_opcode = 0x2E; /* DualNamePrefix */
const is pretty bogus here.
> +
> + g_array_append_val(array, prefix_opcode);
> + build_append_nameseg(array, "%s", s);
So nameseg only ever gets %s now?
In that case, move varg parsing out of there,
make it simply assept char*
> + build_append_nameseg(array, "%s", segs[1]);
> + break;
> + }
> + default: {
> + const uint8_t prefix_opcode = 0x2F; /* MultiNamePrefix */
And here.
> + g_array_append_val(array, prefix_opcode);
> + 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;
> + } /* default */
> + } /* switch (seg_count) */
And the only reason for the extra {} is the local variable -
just declare it at top of function and assign here, then you
won't have these weird double }} things, and you won't need to
add comments after } which is really confusing IMO.
Or drop the variable:
g_array_append_val(array, 0x2F); /* MultiNamePrefix */
is just as clear.
> + 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 a92d008..380799b 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);
Pls align continuation at opening (.
> - 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/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 [flat|nested] 16+ messages in thread* Re: [Qemu-devel] [PATCH v6 3/5] acpi: add build_append_namestring() helper
2015-01-28 15:16 ` Michael S. Tsirkin
@ 2015-01-28 15:44 ` Igor Mammedov
2015-01-28 16:07 ` Michael S. Tsirkin
2015-01-30 11:46 ` Igor Mammedov
1 sibling, 1 reply; 16+ messages in thread
From: Igor Mammedov @ 2015-01-28 15:44 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: qemu-devel
On Wed, 28 Jan 2015 17:16:17 +0200
"Michael S. Tsirkin" <mst@redhat.com> wrote:
> On Wed, Jan 28, 2015 at 02:34:48PM +0000, Igor Mammedov wrote:
> > Use build_append_namestring() instead of build_append_nameseg()
> > So user won't have to care whether name is NameSeg, NamePath or
> > NameString.
> >
> > See for refernce ACPI 5.0: 20.2.2 Name Objects Encoding
> >
>
> typo
will fix
>
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > Reviewed-by: Claudio Fontana <claudio.fontana@huawei.com>
> > Acked-by: Marcel Apfelbaum <marcel@redhat.com>
> > ---
> > 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 | 92 ++++++++++++++++++++++++++++++++++++++++++++-
> > hw/i386/acpi-build.c | 35 ++++++++---------
> > include/hw/acpi/aml-build.h | 2 +-
> > 3 files changed, 108 insertions(+), 21 deletions(-)
> >
> > diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
> > index b28c1f4..ed4ab56 100644
> > --- a/hw/acpi/aml-build.c
> > +++ b/hw/acpi/aml-build.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,96 @@ build_append_nameseg(GArray *array, const char *format, ...)
> > 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;
> >
> How about we split out formatting?
> Make +build_append_namestringv acceptconst char **segs, int nsegs,
> put all code above to build_append_namestring.
ok
>
> > + /*
> > + * 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: {
> > + const uint8_t prefix_opcode = 0x2E; /* DualNamePrefix */
>
> const is pretty bogus here.
it's like in above block: const uint8_t nullpath = 0;
>
> > +
> > + g_array_append_val(array, prefix_opcode);
> > + build_append_nameseg(array, "%s", s);
>
> So nameseg only ever gets %s now?
> In that case, move varg parsing out of there,
> make it simply assept char*
ok
>
> > + build_append_nameseg(array, "%s", segs[1]);
> > + break;
> > + }
> > + default: {
> > + const uint8_t prefix_opcode = 0x2F; /* MultiNamePrefix */
>
> And here.
>
> > + g_array_append_val(array, prefix_opcode);
> > + 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;
> > + } /* default */
> > + } /* switch (seg_count) */
>
> And the only reason for the extra {} is the local variable -
> just declare it at top of function and assign here, then you
> won't have these weird double }} things, and you won't need to
> add comments after } which is really confusing IMO.
>
> Or drop the variable:
>
> g_array_append_val(array, 0x2F); /* MultiNamePrefix */
g_array_append_val() doesn't work with literals, hence variable.
>
> is just as clear.
>
>
> > + 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 a92d008..380799b 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);
>
> Pls align continuation at opening (.
ok
>
> > - 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/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 [flat|nested] 16+ messages in thread* Re: [Qemu-devel] [PATCH v6 3/5] acpi: add build_append_namestring() helper
2015-01-28 15:44 ` Igor Mammedov
@ 2015-01-28 16:07 ` Michael S. Tsirkin
0 siblings, 0 replies; 16+ messages in thread
From: Michael S. Tsirkin @ 2015-01-28 16:07 UTC (permalink / raw)
To: Igor Mammedov; +Cc: qemu-devel
On Wed, Jan 28, 2015 at 04:44:09PM +0100, Igor Mammedov wrote:
> On Wed, 28 Jan 2015 17:16:17 +0200
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
>
> > On Wed, Jan 28, 2015 at 02:34:48PM +0000, Igor Mammedov wrote:
> > > Use build_append_namestring() instead of build_append_nameseg()
> > > So user won't have to care whether name is NameSeg, NamePath or
> > > NameString.
> > >
> > > See for refernce ACPI 5.0: 20.2.2 Name Objects Encoding
> > >
> >
> > typo
> will fix
>
> >
> > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > > Reviewed-by: Claudio Fontana <claudio.fontana@huawei.com>
> > > Acked-by: Marcel Apfelbaum <marcel@redhat.com>
> > > ---
> > > 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 | 92 ++++++++++++++++++++++++++++++++++++++++++++-
> > > hw/i386/acpi-build.c | 35 ++++++++---------
> > > include/hw/acpi/aml-build.h | 2 +-
> > > 3 files changed, 108 insertions(+), 21 deletions(-)
> > >
> > > diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
> > > index b28c1f4..ed4ab56 100644
> > > --- a/hw/acpi/aml-build.c
> > > +++ b/hw/acpi/aml-build.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,96 @@ build_append_nameseg(GArray *array, const char *format, ...)
> > > 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;
> > >
> > How about we split out formatting?
> > Make +build_append_namestringv acceptconst char **segs, int nsegs,
> > put all code above to build_append_namestring.
> ok
>
> >
> > > + /*
> > > + * 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: {
> > > + const uint8_t prefix_opcode = 0x2E; /* DualNamePrefix */
> >
> > const is pretty bogus here.
> it's like in above block: const uint8_t nullpath = 0;
It's bogus there as well.
> >
> > > +
> > > + g_array_append_val(array, prefix_opcode);
> > > + build_append_nameseg(array, "%s", s);
> >
> > So nameseg only ever gets %s now?
> > In that case, move varg parsing out of there,
> > make it simply assept char*
> ok
>
> >
> > > + build_append_nameseg(array, "%s", segs[1]);
> > > + break;
> > > + }
> > > + default: {
> > > + const uint8_t prefix_opcode = 0x2F; /* MultiNamePrefix */
> >
> > And here.
> >
> > > + g_array_append_val(array, prefix_opcode);
> > > + 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;
> > > + } /* default */
> > > + } /* switch (seg_count) */
> >
> > And the only reason for the extra {} is the local variable -
> > just declare it at top of function and assign here, then you
> > won't have these weird double }} things, and you won't need to
> > add comments after } which is really confusing IMO.
> >
> > Or drop the variable:
> >
> > g_array_append_val(array, 0x2F); /* MultiNamePrefix */
> g_array_append_val() doesn't work with literals, hence variable.
OK but there's no need for extra {}, just add uint8_t opcode
at the top scope, and reuse.
>
> >
> > is just as clear.
> >
> >
> > > + 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 a92d008..380799b 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);
> >
> > Pls align continuation at opening (.
> ok
>
> >
> > > - 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/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 [flat|nested] 16+ messages in thread
* Re: [Qemu-devel] [PATCH v6 3/5] acpi: add build_append_namestring() helper
2015-01-28 15:16 ` Michael S. Tsirkin
2015-01-28 15:44 ` Igor Mammedov
@ 2015-01-30 11:46 ` Igor Mammedov
2015-01-31 17:05 ` Michael S. Tsirkin
1 sibling, 1 reply; 16+ messages in thread
From: Igor Mammedov @ 2015-01-30 11:46 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: qemu-devel
On Wed, 28 Jan 2015 17:16:17 +0200
"Michael S. Tsirkin" <mst@redhat.com> wrote:
> On Wed, Jan 28, 2015 at 02:34:48PM +0000, Igor Mammedov wrote:
> > Use build_append_namestring() instead of build_append_nameseg()
> > So user won't have to care whether name is NameSeg, NamePath or
> > NameString.
> >
> > See for refernce ACPI 5.0: 20.2.2 Name Objects Encoding
> >
>
> typo
>
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > Reviewed-by: Claudio Fontana <claudio.fontana@huawei.com>
> > Acked-by: Marcel Apfelbaum <marcel@redhat.com>
> > ---
> > 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 | 92
> > ++++++++++++++++++++++++++++++++++++++++++++-
> > hw/i386/acpi-build.c | 35 ++++++++---------
> > include/hw/acpi/aml-build.h | 2 +- 3 files changed, 108
> > insertions(+), 21 deletions(-)
> >
> > diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
> > index b28c1f4..ed4ab56 100644
> > --- a/hw/acpi/aml-build.c
> > +++ b/hw/acpi/aml-build.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,96 @@ build_append_nameseg(GArray
> > *array, const char *format, ...) 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;
> >
> How about we split out formatting?
> Make +build_append_namestringv acceptconst char **segs, int nsegs,
> put all code above to build_append_namestring.
Can't do this, AML API calls which accept format string will
use build_append_namestringv()
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [Qemu-devel] [PATCH v6 3/5] acpi: add build_append_namestring() helper
2015-01-30 11:46 ` Igor Mammedov
@ 2015-01-31 17:05 ` Michael S. Tsirkin
2015-02-02 8:31 ` Igor Mammedov
0 siblings, 1 reply; 16+ messages in thread
From: Michael S. Tsirkin @ 2015-01-31 17:05 UTC (permalink / raw)
To: Igor Mammedov; +Cc: qemu-devel
On Fri, Jan 30, 2015 at 12:46:41PM +0100, Igor Mammedov wrote:
> On Wed, 28 Jan 2015 17:16:17 +0200
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
>
> > On Wed, Jan 28, 2015 at 02:34:48PM +0000, Igor Mammedov wrote:
> > > Use build_append_namestring() instead of build_append_nameseg()
> > > So user won't have to care whether name is NameSeg, NamePath or
> > > NameString.
> > >
> > > See for refernce ACPI 5.0: 20.2.2 Name Objects Encoding
> > >
> >
> > typo
> >
> > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > > Reviewed-by: Claudio Fontana <claudio.fontana@huawei.com>
> > > Acked-by: Marcel Apfelbaum <marcel@redhat.com>
> > > ---
> > > 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 | 92
> > > ++++++++++++++++++++++++++++++++++++++++++++-
> > > hw/i386/acpi-build.c | 35 ++++++++---------
> > > include/hw/acpi/aml-build.h | 2 +- 3 files changed, 108
> > > insertions(+), 21 deletions(-)
> > >
> > > diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
> > > index b28c1f4..ed4ab56 100644
> > > --- a/hw/acpi/aml-build.c
> > > +++ b/hw/acpi/aml-build.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,96 @@ build_append_nameseg(GArray
> > > *array, const char *format, ...) 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;
> > >
> > How about we split out formatting?
> > Make +build_append_namestringv acceptconst char **segs, int nsegs,
> > put all code above to build_append_namestring.
> Can't do this, AML API calls which accept format string will
> use build_append_namestringv()
OK but I still think it's a good idea to split this out,
and have a static function that accepts an array of segments.
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [Qemu-devel] [PATCH v6 3/5] acpi: add build_append_namestring() helper
2015-01-31 17:05 ` Michael S. Tsirkin
@ 2015-02-02 8:31 ` Igor Mammedov
0 siblings, 0 replies; 16+ messages in thread
From: Igor Mammedov @ 2015-02-02 8:31 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: qemu-devel
On Sat, 31 Jan 2015 19:05:47 +0200
"Michael S. Tsirkin" <mst@redhat.com> wrote:
> On Fri, Jan 30, 2015 at 12:46:41PM +0100, Igor Mammedov wrote:
> > On Wed, 28 Jan 2015 17:16:17 +0200
> > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> >
> > > On Wed, Jan 28, 2015 at 02:34:48PM +0000, Igor Mammedov wrote:
> > > > Use build_append_namestring() instead of build_append_nameseg()
> > > > So user won't have to care whether name is NameSeg, NamePath or
> > > > NameString.
> > > >
> > > > See for refernce ACPI 5.0: 20.2.2 Name Objects Encoding
> > > >
> > >
> > > typo
> > >
> > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > > > Reviewed-by: Claudio Fontana <claudio.fontana@huawei.com>
> > > > Acked-by: Marcel Apfelbaum <marcel@redhat.com>
> > > > ---
> > > > 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 | 92
> > > > ++++++++++++++++++++++++++++++++++++++++++++-
> > > > hw/i386/acpi-build.c | 35 ++++++++---------
> > > > include/hw/acpi/aml-build.h | 2 +- 3 files changed, 108
> > > > insertions(+), 21 deletions(-)
> > > >
> > > > diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
> > > > index b28c1f4..ed4ab56 100644
> > > > --- a/hw/acpi/aml-build.c
> > > > +++ b/hw/acpi/aml-build.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,96 @@ build_append_nameseg(GArray
> > > > *array, const char *format, ...) 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;
> > > >
> > > How about we split out formatting?
> > > Make +build_append_namestringv acceptconst char **segs, int nsegs,
> > > put all code above to build_append_namestring.
> > Can't do this, AML API calls which accept format string will
> > use build_append_namestringv()
>
> OK but I still think it's a good idea to split this out,
> and have a static function that accepts an array of segments.
It would be a static function with a single call site from another
static function. I see no point is doing it, on contrary it would be
confusing why do we have function if we do not reuse it.
I'd split out the moment we actually need it.
>
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* [Qemu-devel] [PATCH v6 4/5] acpi: drop min-bytes in build_package()
2015-01-28 14:34 [Qemu-devel] [PATCH v6 0/5] pc: acpi: various fixes and cleanups Igor Mammedov
` (2 preceding siblings ...)
2015-01-28 14:34 ` [Qemu-devel] [PATCH v6 3/5] acpi: add build_append_namestring() helper Igor Mammedov
@ 2015-01-28 14:34 ` Igor Mammedov
2015-01-28 14:34 ` [Qemu-devel] [PATCH v6 5/5] pc: acpi-build: simplify PCI bus tree generation Igor Mammedov
4 siblings, 0 replies; 16+ messages in thread
From: Igor Mammedov @ 2015-01-28 14:34 UTC (permalink / raw)
To: qemu-devel; +Cc: 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 ed4ab56..40e096d 100644
--- a/hw/acpi/aml-build.c
+++ b/hw/acpi/aml-build.c
@@ -169,7 +169,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;
@@ -185,11 +185,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;
@@ -222,15 +217,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 */
}
@@ -274,4 +269,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 380799b..b36ac45 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/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] 16+ messages in thread* [Qemu-devel] [PATCH v6 5/5] pc: acpi-build: simplify PCI bus tree generation
2015-01-28 14:34 [Qemu-devel] [PATCH v6 0/5] pc: acpi: various fixes and cleanups Igor Mammedov
` (3 preceding siblings ...)
2015-01-28 14:34 ` [Qemu-devel] [PATCH v6 4/5] acpi: drop min-bytes in build_package() Igor Mammedov
@ 2015-01-28 14:34 ` Igor Mammedov
2015-01-28 15:21 ` Michael S. Tsirkin
4 siblings, 1 reply; 16+ messages in thread
From: Igor Mammedov @ 2015-01-28 14:34 UTC (permalink / raw)
To: qemu-devel; +Cc: 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>
---
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 b36ac45..c5d1eba 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
^ permalink raw reply related [flat|nested] 16+ messages in thread* Re: [Qemu-devel] [PATCH v6 5/5] pc: acpi-build: simplify PCI bus tree generation
2015-01-28 14:34 ` [Qemu-devel] [PATCH v6 5/5] pc: acpi-build: simplify PCI bus tree generation Igor Mammedov
@ 2015-01-28 15:21 ` Michael S. Tsirkin
2015-01-28 15:53 ` Igor Mammedov
0 siblings, 1 reply; 16+ messages in thread
From: Michael S. Tsirkin @ 2015-01-28 15:21 UTC (permalink / raw)
To: Igor Mammedov; +Cc: qemu-devel
On Wed, Jan 28, 2015 at 02:34:50PM +0000, Igor Mammedov wrote:
> it basicaly does the same as original approach,
> * just without bus/notify tables tracking (less obscure)
> which is easier to follow.
> * drops unnecessary loops and bitmaps,
> creating devices and notification method in the same loop.
> * saves us ~100LOC
>
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> Reviewed-by: Marcel Apfelbaum <marcel@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 b36ac45..c5d1eba 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.
You can't remove it.
Check the commit log, and you will know why.
> + */
> + 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 */
So we have scope even if it's a non root device.
I'd prefer it that we put everything in the device
directly, when later you rewrite DSDT as well,
you will be able to do it for root as well.
> + 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
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [Qemu-devel] [PATCH v6 5/5] pc: acpi-build: simplify PCI bus tree generation
2015-01-28 15:21 ` Michael S. Tsirkin
@ 2015-01-28 15:53 ` Igor Mammedov
2015-01-28 16:02 ` Michael S. Tsirkin
0 siblings, 1 reply; 16+ messages in thread
From: Igor Mammedov @ 2015-01-28 15:53 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: qemu-devel
On Wed, 28 Jan 2015 17:21:21 +0200
"Michael S. Tsirkin" <mst@redhat.com> wrote:
> On Wed, Jan 28, 2015 at 02:34:50PM +0000, Igor Mammedov wrote:
> > it basicaly does the same as original approach,
> > * just without bus/notify tables tracking (less obscure)
> > which is easier to follow.
> > * drops unnecessary loops and bitmaps,
> > creating devices and notification method in the same loop.
> > * saves us ~100LOC
> >
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > Reviewed-by: Marcel Apfelbaum <marcel@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 b36ac45..c5d1eba 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.
>
> You can't remove it.
> Check the commit log, and you will know why.
I'll drop comment.
>
> > + */
> > + 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 */
>
> So we have scope even if it's a non root device.
> I'd prefer it that we put everything in the device
> directly, when later you rewrite DSDT as well,
> you will be able to do it for root as well.
yes, I did exactly this in AML series.
So lets leave this as it is, AML series will convert it to
Device(bridge) { child_socket_devices... }
format
>
>
> > + 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
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [Qemu-devel] [PATCH v6 5/5] pc: acpi-build: simplify PCI bus tree generation
2015-01-28 15:53 ` Igor Mammedov
@ 2015-01-28 16:02 ` Michael S. Tsirkin
0 siblings, 0 replies; 16+ messages in thread
From: Michael S. Tsirkin @ 2015-01-28 16:02 UTC (permalink / raw)
To: Igor Mammedov; +Cc: qemu-devel
On Wed, Jan 28, 2015 at 04:53:36PM +0100, Igor Mammedov wrote:
> On Wed, 28 Jan 2015 17:21:21 +0200
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
>
> > On Wed, Jan 28, 2015 at 02:34:50PM +0000, Igor Mammedov wrote:
> > > it basicaly does the same as original approach,
> > > * just without bus/notify tables tracking (less obscure)
> > > which is easier to follow.
> > > * drops unnecessary loops and bitmaps,
> > > creating devices and notification method in the same loop.
> > > * saves us ~100LOC
> > >
> > > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > > Reviewed-by: Marcel Apfelbaum <marcel@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 b36ac45..c5d1eba 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.
> >
> > You can't remove it.
> > Check the commit log, and you will know why.
> I'll drop comment.
>
> >
> > > + */
> > > + 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 */
> >
> > So we have scope even if it's a non root device.
> > I'd prefer it that we put everything in the device
> > directly, when later you rewrite DSDT as well,
> > you will be able to do it for root as well.
> yes, I did exactly this in AML series.
> So lets leave this as it is, AML series will convert it to
> Device(bridge) { child_socket_devices... }
> format
OK, but pls add a comment about this.
> >
> >
> > > + 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
^ permalink raw reply [flat|nested] 16+ messages in thread