* [Qemu-devel] [PATCH v5 1/5] pc: acpi-build: cleanup AcpiPmInfo initialization
2015-01-21 9:09 [Qemu-devel] [PATCH v5 0/5] pc: acpi: various fixes and cleanups Igor Mammedov
@ 2015-01-21 9:09 ` Igor Mammedov
2015-01-21 9:11 ` Marcel Apfelbaum
2015-01-21 9:09 ` [Qemu-devel] [PATCH v5 2/5] acpi: move generic aml building helpers into dedictated file Igor Mammedov
` (3 subsequent siblings)
4 siblings, 1 reply; 13+ messages in thread
From: Igor Mammedov @ 2015-01-21 9:09 UTC (permalink / raw)
To: qemu-devel; +Cc: marcel.a, 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>
---
hw/i386/acpi-build.c | 8 ++------
1 file changed, 2 insertions(+), 6 deletions(-)
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 77a124e..4c0536f 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -172,6 +172,8 @@ static void acpi_get_pm_info(AcpiPmInfo *pm)
Object *obj = NULL;
QObject *o;
+ memset(pm, 0, sizeof(*pm));
+
if (piix) {
obj = piix;
}
@@ -184,22 +186,16 @@ static void acpi_get_pm_info(AcpiPmInfo *pm)
o = object_property_get_qobject(obj, ACPI_PM_PROP_S3_DISABLED, NULL);
if (o) {
pm->s3_disabled = qint_get_int(qobject_to_qint(o));
- } else {
- pm->s3_disabled = false;
}
qobject_decref(o);
o = object_property_get_qobject(obj, ACPI_PM_PROP_S4_DISABLED, NULL);
if (o) {
pm->s4_disabled = qint_get_int(qobject_to_qint(o));
- } else {
- pm->s4_disabled = false;
}
qobject_decref(o);
o = object_property_get_qobject(obj, ACPI_PM_PROP_S4_VAL, NULL);
if (o) {
pm->s4_val = qint_get_int(qobject_to_qint(o));
- } else {
- pm->s4_val = false;
}
qobject_decref(o);
--
1.8.3.1
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [Qemu-devel] [PATCH v5 1/5] pc: acpi-build: cleanup AcpiPmInfo initialization
2015-01-21 9:09 ` [Qemu-devel] [PATCH v5 1/5] pc: acpi-build: cleanup AcpiPmInfo initialization Igor Mammedov
@ 2015-01-21 9:11 ` Marcel Apfelbaum
0 siblings, 0 replies; 13+ messages in thread
From: Marcel Apfelbaum @ 2015-01-21 9:11 UTC (permalink / raw)
To: Igor Mammedov, qemu-devel; +Cc: marcel.a, mst
On 01/21/2015 11:09 AM, Igor Mammedov wrote:
> zero initialize AcpiPmInfo struct to reduce code bloat
> a little bit.
>
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> Reviewed-by: Claudio Fontana <claudio.fontana@huawei.com>
> ---
> hw/i386/acpi-build.c | 8 ++------
> 1 file changed, 2 insertions(+), 6 deletions(-)
>
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index 77a124e..4c0536f 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -172,6 +172,8 @@ static void acpi_get_pm_info(AcpiPmInfo *pm)
> Object *obj = NULL;
> QObject *o;
>
> + memset(pm, 0, sizeof(*pm));
> +
> if (piix) {
> obj = piix;
> }
> @@ -184,22 +186,16 @@ static void acpi_get_pm_info(AcpiPmInfo *pm)
> o = object_property_get_qobject(obj, ACPI_PM_PROP_S3_DISABLED, NULL);
> if (o) {
> pm->s3_disabled = qint_get_int(qobject_to_qint(o));
> - } else {
> - pm->s3_disabled = false;
> }
> qobject_decref(o);
> o = object_property_get_qobject(obj, ACPI_PM_PROP_S4_DISABLED, NULL);
> if (o) {
> pm->s4_disabled = qint_get_int(qobject_to_qint(o));
> - } else {
> - pm->s4_disabled = false;
> }
> qobject_decref(o);
> o = object_property_get_qobject(obj, ACPI_PM_PROP_S4_VAL, NULL);
> if (o) {
> pm->s4_val = qint_get_int(qobject_to_qint(o));
> - } else {
> - pm->s4_val = false;
> }
> qobject_decref(o);
>
>
Reviewed-by: Marcel Apfelbaum <marcel@redhat.com>
^ permalink raw reply [flat|nested] 13+ messages in thread
* [Qemu-devel] [PATCH v5 2/5] acpi: move generic aml building helpers into dedictated file
2015-01-21 9:09 [Qemu-devel] [PATCH v5 0/5] pc: acpi: various fixes and cleanups Igor Mammedov
2015-01-21 9:09 ` [Qemu-devel] [PATCH v5 1/5] pc: acpi-build: cleanup AcpiPmInfo initialization Igor Mammedov
@ 2015-01-21 9:09 ` Igor Mammedov
2015-01-21 9:13 ` Marcel Apfelbaum
2015-01-27 12:23 ` Michael S. Tsirkin
2015-01-21 9:09 ` [Qemu-devel] [PATCH v5 3/5] acpi: add build_append_namestring() helper Igor Mammedov
` (2 subsequent siblings)
4 siblings, 2 replies; 13+ messages in thread
From: Igor Mammedov @ 2015-01-21 9:09 UTC (permalink / raw)
To: qemu-devel; +Cc: marcel.a, mst
the will be later used for composing AML primitives
and all that could be reused later for ARM machines
as well.
Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
v3:
* rename acpi_gen_utils.[ch] to acpi-build-utils.[ch]
* copy GLP license block from acpi-build.c
v2:
* fix wrong ident in moved code
---
hw/acpi/Makefile.objs | 1 +
hw/acpi/acpi-build-utils.c | 187 +++++++++++++++++++++++++++++++++++++
hw/i386/acpi-build.c | 162 +-------------------------------
include/hw/acpi/acpi-build-utils.h | 23 +++++
4 files changed, 213 insertions(+), 160 deletions(-)
create mode 100644 hw/acpi/acpi-build-utils.c
create mode 100644 include/hw/acpi/acpi-build-utils.h
diff --git a/hw/acpi/Makefile.objs b/hw/acpi/Makefile.objs
index ee82073..cad0355 100644
--- a/hw/acpi/Makefile.objs
+++ b/hw/acpi/Makefile.objs
@@ -2,3 +2,4 @@ common-obj-$(CONFIG_ACPI) += core.o piix4.o ich9.o pcihp.o cpu_hotplug.o
common-obj-$(CONFIG_ACPI) += memory_hotplug.o
common-obj-$(CONFIG_ACPI) += acpi_interface.o
common-obj-$(CONFIG_ACPI) += bios-linker-loader.o
+common-obj-$(CONFIG_ACPI) += acpi-build-utils.o
diff --git a/hw/acpi/acpi-build-utils.c b/hw/acpi/acpi-build-utils.c
new file mode 100644
index 0000000..79aa610
--- /dev/null
+++ b/hw/acpi/acpi-build-utils.c
@@ -0,0 +1,187 @@
+/* Support for generating ACPI tables and passing them to Guests
+ *
+ * Copyright (C) 2014 Red Hat Inc
+ *
+ * Author: Michael S. Tsirkin <mst@redhat.com>
+ * Author: Igor Mammedov <imammedo@redhat.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+
+ * You should have received a copy of the GNU General Public License along
+ * with this program; if not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include <stdio.h>
+#include <stdarg.h>
+#include <assert.h>
+#include <stdbool.h>
+#include "hw/acpi/acpi-build-utils.h"
+
+GArray *build_alloc_array(void)
+{
+ return g_array_new(false, true /* clear */, 1);
+}
+
+void build_free_array(GArray *array)
+{
+ g_array_free(array, true);
+}
+
+void build_prepend_byte(GArray *array, uint8_t val)
+{
+ g_array_prepend_val(array, val);
+}
+
+void build_append_byte(GArray *array, uint8_t val)
+{
+ g_array_append_val(array, val);
+}
+
+void build_append_array(GArray *array, GArray *val)
+{
+ g_array_append_vals(array, val->data, val->len);
+}
+
+#define ACPI_NAMESEG_LEN 4
+
+void GCC_FMT_ATTR(2, 3)
+build_append_nameseg(GArray *array, const char *format, ...)
+{
+ /* It would be nicer to use g_string_vprintf but it's only there in 2.22 */
+ char s[] = "XXXX";
+ int len;
+ va_list args;
+
+ va_start(args, format);
+ len = vsnprintf(s, sizeof s, format, args);
+ va_end(args);
+
+ assert(len <= ACPI_NAMESEG_LEN);
+
+ g_array_append_vals(array, s, len);
+ /* Pad up to ACPI_NAMESEG_LEN characters if necessary. */
+ g_array_append_vals(array, "____", ACPI_NAMESEG_LEN - len);
+}
+
+/* 5.4 Definition Block Encoding */
+enum {
+ PACKAGE_LENGTH_1BYTE_SHIFT = 6, /* Up to 63 - use extra 2 bits. */
+ PACKAGE_LENGTH_2BYTE_SHIFT = 4,
+ PACKAGE_LENGTH_3BYTE_SHIFT = 12,
+ PACKAGE_LENGTH_4BYTE_SHIFT = 20,
+};
+
+void build_prepend_package_length(GArray *package, unsigned min_bytes)
+{
+ uint8_t byte;
+ unsigned length = package->len;
+ unsigned length_bytes;
+
+ if (length + 1 < (1 << PACKAGE_LENGTH_1BYTE_SHIFT)) {
+ length_bytes = 1;
+ } else if (length + 2 < (1 << PACKAGE_LENGTH_3BYTE_SHIFT)) {
+ length_bytes = 2;
+ } else if (length + 3 < (1 << PACKAGE_LENGTH_4BYTE_SHIFT)) {
+ length_bytes = 3;
+ } else {
+ length_bytes = 4;
+ }
+
+ /* Force length to at least min_bytes.
+ * This wastes memory but that's how bios did it.
+ */
+ length_bytes = MAX(length_bytes, min_bytes);
+
+ /* PkgLength is the length of the inclusive length of the data. */
+ length += length_bytes;
+
+ switch (length_bytes) {
+ case 1:
+ byte = length;
+ build_prepend_byte(package, byte);
+ return;
+ case 4:
+ byte = length >> PACKAGE_LENGTH_4BYTE_SHIFT;
+ build_prepend_byte(package, byte);
+ length &= (1 << PACKAGE_LENGTH_4BYTE_SHIFT) - 1;
+ /* fall through */
+ case 3:
+ byte = length >> PACKAGE_LENGTH_3BYTE_SHIFT;
+ build_prepend_byte(package, byte);
+ length &= (1 << PACKAGE_LENGTH_3BYTE_SHIFT) - 1;
+ /* fall through */
+ case 2:
+ byte = length >> PACKAGE_LENGTH_2BYTE_SHIFT;
+ build_prepend_byte(package, byte);
+ length &= (1 << PACKAGE_LENGTH_2BYTE_SHIFT) - 1;
+ /* fall through */
+ }
+ /*
+ * Most significant two bits of byte zero indicate how many following bytes
+ * are in PkgLength encoding.
+ */
+ byte = ((length_bytes - 1) << PACKAGE_LENGTH_1BYTE_SHIFT) | length;
+ build_prepend_byte(package, byte);
+}
+
+void build_package(GArray *package, uint8_t op, unsigned min_bytes)
+{
+ build_prepend_package_length(package, min_bytes);
+ build_prepend_byte(package, op);
+}
+
+void build_extop_package(GArray *package, uint8_t op)
+{
+ build_package(package, op, 1);
+ build_prepend_byte(package, 0x5B); /* ExtOpPrefix */
+}
+
+void build_append_value(GArray *table, uint32_t value, int size)
+{
+ uint8_t prefix;
+ int i;
+
+ switch (size) {
+ case 1:
+ prefix = 0x0A; /* BytePrefix */
+ break;
+ case 2:
+ prefix = 0x0B; /* WordPrefix */
+ break;
+ case 4:
+ prefix = 0x0C; /* DWordPrefix */
+ break;
+ default:
+ assert(0);
+ return;
+ }
+ build_append_byte(table, prefix);
+ for (i = 0; i < size; ++i) {
+ build_append_byte(table, value & 0xFF);
+ value = value >> 8;
+ }
+}
+
+void build_append_int(GArray *table, uint32_t value)
+{
+ if (value == 0x00) {
+ build_append_byte(table, 0x00); /* ZeroOp */
+ } else if (value == 0x01) {
+ build_append_byte(table, 0x01); /* OneOp */
+ } else if (value <= 0xFF) {
+ build_append_value(table, value, 1);
+ } else if (value <= 0xFFFF) {
+ build_append_value(table, value, 2);
+ } else {
+ build_append_value(table, value, 4);
+ }
+}
+
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 4c0536f..8ee15ab 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -54,6 +54,8 @@
#include "hw/i386/q35-acpi-dsdt.hex"
#include "hw/i386/acpi-dsdt.hex"
+#include "hw/acpi/acpi-build-utils.h"
+
#include "qapi/qmp/qint.h"
#include "qom/qom-qobject.h"
#include "exec/ram_addr.h"
@@ -277,166 +279,6 @@ build_header(GArray *linker, GArray *table_data,
table_data->data, h, len, &h->checksum);
}
-static inline GArray *build_alloc_array(void)
-{
- return g_array_new(false, true /* clear */, 1);
-}
-
-static inline void build_free_array(GArray *array)
-{
- g_array_free(array, true);
-}
-
-static inline void build_prepend_byte(GArray *array, uint8_t val)
-{
- g_array_prepend_val(array, val);
-}
-
-static inline void build_append_byte(GArray *array, uint8_t val)
-{
- g_array_append_val(array, val);
-}
-
-static inline void build_append_array(GArray *array, GArray *val)
-{
- g_array_append_vals(array, val->data, val->len);
-}
-
-#define ACPI_NAMESEG_LEN 4
-
-static void GCC_FMT_ATTR(2, 3)
-build_append_nameseg(GArray *array, const char *format, ...)
-{
- /* It would be nicer to use g_string_vprintf but it's only there in 2.22 */
- char s[] = "XXXX";
- int len;
- va_list args;
-
- va_start(args, format);
- len = vsnprintf(s, sizeof s, format, args);
- va_end(args);
-
- assert(len <= ACPI_NAMESEG_LEN);
-
- g_array_append_vals(array, s, len);
- /* Pad up to ACPI_NAMESEG_LEN characters if necessary. */
- g_array_append_vals(array, "____", ACPI_NAMESEG_LEN - len);
-}
-
-/* 5.4 Definition Block Encoding */
-enum {
- PACKAGE_LENGTH_1BYTE_SHIFT = 6, /* Up to 63 - use extra 2 bits. */
- PACKAGE_LENGTH_2BYTE_SHIFT = 4,
- PACKAGE_LENGTH_3BYTE_SHIFT = 12,
- PACKAGE_LENGTH_4BYTE_SHIFT = 20,
-};
-
-static void build_prepend_package_length(GArray *package, unsigned min_bytes)
-{
- uint8_t byte;
- unsigned length = package->len;
- unsigned length_bytes;
-
- if (length + 1 < (1 << PACKAGE_LENGTH_1BYTE_SHIFT)) {
- length_bytes = 1;
- } else if (length + 2 < (1 << PACKAGE_LENGTH_3BYTE_SHIFT)) {
- length_bytes = 2;
- } else if (length + 3 < (1 << PACKAGE_LENGTH_4BYTE_SHIFT)) {
- length_bytes = 3;
- } else {
- length_bytes = 4;
- }
-
- /* Force length to at least min_bytes.
- * This wastes memory but that's how bios did it.
- */
- length_bytes = MAX(length_bytes, min_bytes);
-
- /* PkgLength is the length of the inclusive length of the data. */
- length += length_bytes;
-
- switch (length_bytes) {
- case 1:
- byte = length;
- build_prepend_byte(package, byte);
- return;
- case 4:
- byte = length >> PACKAGE_LENGTH_4BYTE_SHIFT;
- build_prepend_byte(package, byte);
- length &= (1 << PACKAGE_LENGTH_4BYTE_SHIFT) - 1;
- /* fall through */
- case 3:
- byte = length >> PACKAGE_LENGTH_3BYTE_SHIFT;
- build_prepend_byte(package, byte);
- length &= (1 << PACKAGE_LENGTH_3BYTE_SHIFT) - 1;
- /* fall through */
- case 2:
- byte = length >> PACKAGE_LENGTH_2BYTE_SHIFT;
- build_prepend_byte(package, byte);
- length &= (1 << PACKAGE_LENGTH_2BYTE_SHIFT) - 1;
- /* fall through */
- }
- /*
- * Most significant two bits of byte zero indicate how many following bytes
- * are in PkgLength encoding.
- */
- byte = ((length_bytes - 1) << PACKAGE_LENGTH_1BYTE_SHIFT) | length;
- build_prepend_byte(package, byte);
-}
-
-static void build_package(GArray *package, uint8_t op, unsigned min_bytes)
-{
- build_prepend_package_length(package, min_bytes);
- build_prepend_byte(package, op);
-}
-
-static void build_extop_package(GArray *package, uint8_t op)
-{
- build_package(package, op, 1);
- build_prepend_byte(package, 0x5B); /* ExtOpPrefix */
-}
-
-static void build_append_value(GArray *table, uint32_t value, int size)
-{
- uint8_t prefix;
- int i;
-
- switch (size) {
- case 1:
- prefix = 0x0A; /* BytePrefix */
- break;
- case 2:
- prefix = 0x0B; /* WordPrefix */
- break;
- case 4:
- prefix = 0x0C; /* DWordPrefix */
- break;
- default:
- assert(0);
- return;
- }
- build_append_byte(table, prefix);
- for (i = 0; i < size; ++i) {
- build_append_byte(table, value & 0xFF);
- value = value >> 8;
- }
-}
-
-static void build_append_int(GArray *table, uint32_t value)
-{
- if (value == 0x00) {
- build_append_byte(table, 0x00); /* ZeroOp */
- } else if (value == 0x01) {
- build_append_byte(table, 0x01); /* OneOp */
- } else if (value <= 0xFF) {
- build_append_value(table, value, 1);
- } else if (value <= 0xFFFF) {
- build_append_value(table, value, 2);
- } else {
- build_append_value(table, value, 4);
- }
-}
-
static GArray *build_alloc_method(const char *name, uint8_t arg_count)
{
GArray *method = build_alloc_array();
diff --git a/include/hw/acpi/acpi-build-utils.h b/include/hw/acpi/acpi-build-utils.h
new file mode 100644
index 0000000..e6a0b28
--- /dev/null
+++ b/include/hw/acpi/acpi-build-utils.h
@@ -0,0 +1,23 @@
+#ifndef HW_ACPI_GEN_UTILS_H
+#define HW_ACPI_GEN_UTILS_H
+
+#include <stdint.h>
+#include <glib.h>
+#include "qemu/compiler.h"
+
+GArray *build_alloc_array(void);
+void build_free_array(GArray *array);
+void build_prepend_byte(GArray *array, uint8_t val);
+void build_append_byte(GArray *array, uint8_t val);
+void build_append_array(GArray *array, GArray *val);
+
+void GCC_FMT_ATTR(2, 3)
+build_append_nameseg(GArray *array, const char *format, ...);
+
+void build_prepend_package_length(GArray *package, unsigned min_bytes);
+void build_package(GArray *package, uint8_t op, unsigned min_bytes);
+void build_append_value(GArray *table, uint32_t value, int size);
+void build_append_int(GArray *table, uint32_t value);
+void build_extop_package(GArray *package, uint8_t op);
+
+#endif
--
1.8.3.1
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [Qemu-devel] [PATCH v5 2/5] acpi: move generic aml building helpers into dedictated file
2015-01-21 9:09 ` [Qemu-devel] [PATCH v5 2/5] acpi: move generic aml building helpers into dedictated file Igor Mammedov
@ 2015-01-21 9:13 ` Marcel Apfelbaum
2015-01-27 12:23 ` Michael S. Tsirkin
1 sibling, 0 replies; 13+ messages in thread
From: Marcel Apfelbaum @ 2015-01-21 9:13 UTC (permalink / raw)
To: Igor Mammedov, qemu-devel; +Cc: marcel.a, mst
On 01/21/2015 11:09 AM, Igor Mammedov wrote:
> the will be later used for composing AML primitives
> and all that could be reused later for ARM machines
> as well.
>
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
> v3:
> * rename acpi_gen_utils.[ch] to acpi-build-utils.[ch]
> * copy GLP license block from acpi-build.c
> v2:
> * fix wrong ident in moved code
> ---
> hw/acpi/Makefile.objs | 1 +
> hw/acpi/acpi-build-utils.c | 187 +++++++++++++++++++++++++++++++++++++
> hw/i386/acpi-build.c | 162 +-------------------------------
> include/hw/acpi/acpi-build-utils.h | 23 +++++
> 4 files changed, 213 insertions(+), 160 deletions(-)
> create mode 100644 hw/acpi/acpi-build-utils.c
> create mode 100644 include/hw/acpi/acpi-build-utils.h
>
> diff --git a/hw/acpi/Makefile.objs b/hw/acpi/Makefile.objs
> index ee82073..cad0355 100644
> --- a/hw/acpi/Makefile.objs
> +++ b/hw/acpi/Makefile.objs
> @@ -2,3 +2,4 @@ common-obj-$(CONFIG_ACPI) += core.o piix4.o ich9.o pcihp.o cpu_hotplug.o
> common-obj-$(CONFIG_ACPI) += memory_hotplug.o
> common-obj-$(CONFIG_ACPI) += acpi_interface.o
> common-obj-$(CONFIG_ACPI) += bios-linker-loader.o
> +common-obj-$(CONFIG_ACPI) += acpi-build-utils.o
> diff --git a/hw/acpi/acpi-build-utils.c b/hw/acpi/acpi-build-utils.c
> new file mode 100644
> index 0000000..79aa610
> --- /dev/null
> +++ b/hw/acpi/acpi-build-utils.c
> @@ -0,0 +1,187 @@
> +/* Support for generating ACPI tables and passing them to Guests
> + *
> + * Copyright (C) 2014 Red Hat Inc
^^^^
2015?
Michael, can you change this on the fly?
Other than that:
Acked-by: Marcel Apfelbaum <marcel@redhat.com>
> + *
> + * Author: Michael S. Tsirkin <mst@redhat.com>
> + * Author: Igor Mammedov <imammedo@redhat.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> +
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> +
> + * You should have received a copy of the GNU General Public License along
> + * with this program; if not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#include <stdio.h>
> +#include <stdarg.h>
> +#include <assert.h>
> +#include <stdbool.h>
> +#include "hw/acpi/acpi-build-utils.h"
> +
> +GArray *build_alloc_array(void)
> +{
> + return g_array_new(false, true /* clear */, 1);
> +}
> +
> +void build_free_array(GArray *array)
> +{
> + g_array_free(array, true);
> +}
> +
> +void build_prepend_byte(GArray *array, uint8_t val)
> +{
> + g_array_prepend_val(array, val);
> +}
> +
> +void build_append_byte(GArray *array, uint8_t val)
> +{
> + g_array_append_val(array, val);
> +}
> +
> +void build_append_array(GArray *array, GArray *val)
> +{
> + g_array_append_vals(array, val->data, val->len);
> +}
> +
> +#define ACPI_NAMESEG_LEN 4
> +
> +void GCC_FMT_ATTR(2, 3)
> +build_append_nameseg(GArray *array, const char *format, ...)
> +{
> + /* It would be nicer to use g_string_vprintf but it's only there in 2.22 */
> + char s[] = "XXXX";
> + int len;
> + va_list args;
> +
> + va_start(args, format);
> + len = vsnprintf(s, sizeof s, format, args);
> + va_end(args);
> +
> + assert(len <= ACPI_NAMESEG_LEN);
> +
> + g_array_append_vals(array, s, len);
> + /* Pad up to ACPI_NAMESEG_LEN characters if necessary. */
> + g_array_append_vals(array, "____", ACPI_NAMESEG_LEN - len);
> +}
> +
> +/* 5.4 Definition Block Encoding */
> +enum {
> + PACKAGE_LENGTH_1BYTE_SHIFT = 6, /* Up to 63 - use extra 2 bits. */
> + PACKAGE_LENGTH_2BYTE_SHIFT = 4,
> + PACKAGE_LENGTH_3BYTE_SHIFT = 12,
> + PACKAGE_LENGTH_4BYTE_SHIFT = 20,
> +};
> +
> +void build_prepend_package_length(GArray *package, unsigned min_bytes)
> +{
> + uint8_t byte;
> + unsigned length = package->len;
> + unsigned length_bytes;
> +
> + if (length + 1 < (1 << PACKAGE_LENGTH_1BYTE_SHIFT)) {
> + length_bytes = 1;
> + } else if (length + 2 < (1 << PACKAGE_LENGTH_3BYTE_SHIFT)) {
> + length_bytes = 2;
> + } else if (length + 3 < (1 << PACKAGE_LENGTH_4BYTE_SHIFT)) {
> + length_bytes = 3;
> + } else {
> + length_bytes = 4;
> + }
> +
> + /* Force length to at least min_bytes.
> + * This wastes memory but that's how bios did it.
> + */
> + length_bytes = MAX(length_bytes, min_bytes);
> +
> + /* PkgLength is the length of the inclusive length of the data. */
> + length += length_bytes;
> +
> + switch (length_bytes) {
> + case 1:
> + byte = length;
> + build_prepend_byte(package, byte);
> + return;
> + case 4:
> + byte = length >> PACKAGE_LENGTH_4BYTE_SHIFT;
> + build_prepend_byte(package, byte);
> + length &= (1 << PACKAGE_LENGTH_4BYTE_SHIFT) - 1;
> + /* fall through */
> + case 3:
> + byte = length >> PACKAGE_LENGTH_3BYTE_SHIFT;
> + build_prepend_byte(package, byte);
> + length &= (1 << PACKAGE_LENGTH_3BYTE_SHIFT) - 1;
> + /* fall through */
> + case 2:
> + byte = length >> PACKAGE_LENGTH_2BYTE_SHIFT;
> + build_prepend_byte(package, byte);
> + length &= (1 << PACKAGE_LENGTH_2BYTE_SHIFT) - 1;
> + /* fall through */
> + }
> + /*
> + * Most significant two bits of byte zero indicate how many following bytes
> + * are in PkgLength encoding.
> + */
> + byte = ((length_bytes - 1) << PACKAGE_LENGTH_1BYTE_SHIFT) | length;
> + build_prepend_byte(package, byte);
> +}
> +
> +void build_package(GArray *package, uint8_t op, unsigned min_bytes)
> +{
> + build_prepend_package_length(package, min_bytes);
> + build_prepend_byte(package, op);
> +}
> +
> +void build_extop_package(GArray *package, uint8_t op)
> +{
> + build_package(package, op, 1);
> + build_prepend_byte(package, 0x5B); /* ExtOpPrefix */
> +}
> +
> +void build_append_value(GArray *table, uint32_t value, int size)
> +{
> + uint8_t prefix;
> + int i;
> +
> + switch (size) {
> + case 1:
> + prefix = 0x0A; /* BytePrefix */
> + break;
> + case 2:
> + prefix = 0x0B; /* WordPrefix */
> + break;
> + case 4:
> + prefix = 0x0C; /* DWordPrefix */
> + break;
> + default:
> + assert(0);
> + return;
> + }
> + build_append_byte(table, prefix);
> + for (i = 0; i < size; ++i) {
> + build_append_byte(table, value & 0xFF);
> + value = value >> 8;
> + }
> +}
> +
> +void build_append_int(GArray *table, uint32_t value)
> +{
> + if (value == 0x00) {
> + build_append_byte(table, 0x00); /* ZeroOp */
> + } else if (value == 0x01) {
> + build_append_byte(table, 0x01); /* OneOp */
> + } else if (value <= 0xFF) {
> + build_append_value(table, value, 1);
> + } else if (value <= 0xFFFF) {
> + build_append_value(table, value, 2);
> + } else {
> + build_append_value(table, value, 4);
> + }
> +}
> +
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index 4c0536f..8ee15ab 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -54,6 +54,8 @@
> #include "hw/i386/q35-acpi-dsdt.hex"
> #include "hw/i386/acpi-dsdt.hex"
>
> +#include "hw/acpi/acpi-build-utils.h"
> +
> #include "qapi/qmp/qint.h"
> #include "qom/qom-qobject.h"
> #include "exec/ram_addr.h"
> @@ -277,166 +279,6 @@ build_header(GArray *linker, GArray *table_data,
> table_data->data, h, len, &h->checksum);
> }
>
> -static inline GArray *build_alloc_array(void)
> -{
> - return g_array_new(false, true /* clear */, 1);
> -}
> -
> -static inline void build_free_array(GArray *array)
> -{
> - g_array_free(array, true);
> -}
> -
> -static inline void build_prepend_byte(GArray *array, uint8_t val)
> -{
> - g_array_prepend_val(array, val);
> -}
> -
> -static inline void build_append_byte(GArray *array, uint8_t val)
> -{
> - g_array_append_val(array, val);
> -}
> -
> -static inline void build_append_array(GArray *array, GArray *val)
> -{
> - g_array_append_vals(array, val->data, val->len);
> -}
> -
> -#define ACPI_NAMESEG_LEN 4
> -
> -static void GCC_FMT_ATTR(2, 3)
> -build_append_nameseg(GArray *array, const char *format, ...)
> -{
> - /* It would be nicer to use g_string_vprintf but it's only there in 2.22 */
> - char s[] = "XXXX";
> - int len;
> - va_list args;
> -
> - va_start(args, format);
> - len = vsnprintf(s, sizeof s, format, args);
> - va_end(args);
> -
> - assert(len <= ACPI_NAMESEG_LEN);
> -
> - g_array_append_vals(array, s, len);
> - /* Pad up to ACPI_NAMESEG_LEN characters if necessary. */
> - g_array_append_vals(array, "____", ACPI_NAMESEG_LEN - len);
> -}
> -
> -/* 5.4 Definition Block Encoding */
> -enum {
> - PACKAGE_LENGTH_1BYTE_SHIFT = 6, /* Up to 63 - use extra 2 bits. */
> - PACKAGE_LENGTH_2BYTE_SHIFT = 4,
> - PACKAGE_LENGTH_3BYTE_SHIFT = 12,
> - PACKAGE_LENGTH_4BYTE_SHIFT = 20,
> -};
> -
> -static void build_prepend_package_length(GArray *package, unsigned min_bytes)
> -{
> - uint8_t byte;
> - unsigned length = package->len;
> - unsigned length_bytes;
> -
> - if (length + 1 < (1 << PACKAGE_LENGTH_1BYTE_SHIFT)) {
> - length_bytes = 1;
> - } else if (length + 2 < (1 << PACKAGE_LENGTH_3BYTE_SHIFT)) {
> - length_bytes = 2;
> - } else if (length + 3 < (1 << PACKAGE_LENGTH_4BYTE_SHIFT)) {
> - length_bytes = 3;
> - } else {
> - length_bytes = 4;
> - }
> -
> - /* Force length to at least min_bytes.
> - * This wastes memory but that's how bios did it.
> - */
> - length_bytes = MAX(length_bytes, min_bytes);
> -
> - /* PkgLength is the length of the inclusive length of the data. */
> - length += length_bytes;
> -
> - switch (length_bytes) {
> - case 1:
> - byte = length;
> - build_prepend_byte(package, byte);
> - return;
> - case 4:
> - byte = length >> PACKAGE_LENGTH_4BYTE_SHIFT;
> - build_prepend_byte(package, byte);
> - length &= (1 << PACKAGE_LENGTH_4BYTE_SHIFT) - 1;
> - /* fall through */
> - case 3:
> - byte = length >> PACKAGE_LENGTH_3BYTE_SHIFT;
> - build_prepend_byte(package, byte);
> - length &= (1 << PACKAGE_LENGTH_3BYTE_SHIFT) - 1;
> - /* fall through */
> - case 2:
> - byte = length >> PACKAGE_LENGTH_2BYTE_SHIFT;
> - build_prepend_byte(package, byte);
> - length &= (1 << PACKAGE_LENGTH_2BYTE_SHIFT) - 1;
> - /* fall through */
> - }
> - /*
> - * Most significant two bits of byte zero indicate how many following bytes
> - * are in PkgLength encoding.
> - */
> - byte = ((length_bytes - 1) << PACKAGE_LENGTH_1BYTE_SHIFT) | length;
> - build_prepend_byte(package, byte);
> -}
> -
> -static void build_package(GArray *package, uint8_t op, unsigned min_bytes)
> -{
> - build_prepend_package_length(package, min_bytes);
> - build_prepend_byte(package, op);
> -}
> -
> -static void build_extop_package(GArray *package, uint8_t op)
> -{
> - build_package(package, op, 1);
> - build_prepend_byte(package, 0x5B); /* ExtOpPrefix */
> -}
> -
> -static void build_append_value(GArray *table, uint32_t value, int size)
> -{
> - uint8_t prefix;
> - int i;
> -
> - switch (size) {
> - case 1:
> - prefix = 0x0A; /* BytePrefix */
> - break;
> - case 2:
> - prefix = 0x0B; /* WordPrefix */
> - break;
> - case 4:
> - prefix = 0x0C; /* DWordPrefix */
> - break;
> - default:
> - assert(0);
> - return;
> - }
> - build_append_byte(table, prefix);
> - for (i = 0; i < size; ++i) {
> - build_append_byte(table, value & 0xFF);
> - value = value >> 8;
> - }
> -}
> -
> -static void build_append_int(GArray *table, uint32_t value)
> -{
> - if (value == 0x00) {
> - build_append_byte(table, 0x00); /* ZeroOp */
> - } else if (value == 0x01) {
> - build_append_byte(table, 0x01); /* OneOp */
> - } else if (value <= 0xFF) {
> - build_append_value(table, value, 1);
> - } else if (value <= 0xFFFF) {
> - build_append_value(table, value, 2);
> - } else {
> - build_append_value(table, value, 4);
> - }
> -}
> -
> static GArray *build_alloc_method(const char *name, uint8_t arg_count)
> {
> GArray *method = build_alloc_array();
> diff --git a/include/hw/acpi/acpi-build-utils.h b/include/hw/acpi/acpi-build-utils.h
> new file mode 100644
> index 0000000..e6a0b28
> --- /dev/null
> +++ b/include/hw/acpi/acpi-build-utils.h
> @@ -0,0 +1,23 @@
> +#ifndef HW_ACPI_GEN_UTILS_H
> +#define HW_ACPI_GEN_UTILS_H
> +
> +#include <stdint.h>
> +#include <glib.h>
> +#include "qemu/compiler.h"
> +
> +GArray *build_alloc_array(void);
> +void build_free_array(GArray *array);
> +void build_prepend_byte(GArray *array, uint8_t val);
> +void build_append_byte(GArray *array, uint8_t val);
> +void build_append_array(GArray *array, GArray *val);
> +
> +void GCC_FMT_ATTR(2, 3)
> +build_append_nameseg(GArray *array, const char *format, ...);
> +
> +void build_prepend_package_length(GArray *package, unsigned min_bytes);
> +void build_package(GArray *package, uint8_t op, unsigned min_bytes);
> +void build_append_value(GArray *table, uint32_t value, int size);
> +void build_append_int(GArray *table, uint32_t value);
> +void build_extop_package(GArray *package, uint8_t op);
> +
> +#endif
>
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [Qemu-devel] [PATCH v5 2/5] acpi: move generic aml building helpers into dedictated file
2015-01-21 9:09 ` [Qemu-devel] [PATCH v5 2/5] acpi: move generic aml building helpers into dedictated file Igor Mammedov
2015-01-21 9:13 ` Marcel Apfelbaum
@ 2015-01-27 12:23 ` Michael S. Tsirkin
2015-01-28 12:34 ` Igor Mammedov
1 sibling, 1 reply; 13+ messages in thread
From: Michael S. Tsirkin @ 2015-01-27 12:23 UTC (permalink / raw)
To: Igor Mammedov; +Cc: qemu-devel, marcel.a
On Wed, Jan 21, 2015 at 09:09:10AM +0000, Igor Mammedov wrote:
> the will be later used for composing AML primitives
> and all that could be reused later for ARM machines
> as well.
>
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
OK so the only thing holding this up is really that
you have indicated that in the follow-up patches
you want to add here more stuff with
aml_ prefix instead of acpi_.
In that case let's name this file aml-build.c?
> ---
> v3:
> * rename acpi_gen_utils.[ch] to acpi-build-utils.[ch]
> * copy GLP license block from acpi-build.c
> v2:
> * fix wrong ident in moved code
> ---
> hw/acpi/Makefile.objs | 1 +
> hw/acpi/acpi-build-utils.c | 187 +++++++++++++++++++++++++++++++++++++
> hw/i386/acpi-build.c | 162 +-------------------------------
> include/hw/acpi/acpi-build-utils.h | 23 +++++
> 4 files changed, 213 insertions(+), 160 deletions(-)
> create mode 100644 hw/acpi/acpi-build-utils.c
> create mode 100644 include/hw/acpi/acpi-build-utils.h
>
> diff --git a/hw/acpi/Makefile.objs b/hw/acpi/Makefile.objs
> index ee82073..cad0355 100644
> --- a/hw/acpi/Makefile.objs
> +++ b/hw/acpi/Makefile.objs
> @@ -2,3 +2,4 @@ common-obj-$(CONFIG_ACPI) += core.o piix4.o ich9.o pcihp.o cpu_hotplug.o
> common-obj-$(CONFIG_ACPI) += memory_hotplug.o
> common-obj-$(CONFIG_ACPI) += acpi_interface.o
> common-obj-$(CONFIG_ACPI) += bios-linker-loader.o
> +common-obj-$(CONFIG_ACPI) += acpi-build-utils.o
> diff --git a/hw/acpi/acpi-build-utils.c b/hw/acpi/acpi-build-utils.c
> new file mode 100644
> index 0000000..79aa610
> --- /dev/null
> +++ b/hw/acpi/acpi-build-utils.c
> @@ -0,0 +1,187 @@
> +/* Support for generating ACPI tables and passing them to Guests
> + *
> + * Copyright (C) 2014 Red Hat Inc
> + *
> + * Author: Michael S. Tsirkin <mst@redhat.com>
> + * Author: Igor Mammedov <imammedo@redhat.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> +
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> +
> + * You should have received a copy of the GNU General Public License along
> + * with this program; if not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#include <stdio.h>
> +#include <stdarg.h>
> +#include <assert.h>
> +#include <stdbool.h>
> +#include "hw/acpi/acpi-build-utils.h"
> +
> +GArray *build_alloc_array(void)
> +{
> + return g_array_new(false, true /* clear */, 1);
> +}
> +
> +void build_free_array(GArray *array)
> +{
> + g_array_free(array, true);
> +}
> +
> +void build_prepend_byte(GArray *array, uint8_t val)
> +{
> + g_array_prepend_val(array, val);
> +}
> +
> +void build_append_byte(GArray *array, uint8_t val)
> +{
> + g_array_append_val(array, val);
> +}
> +
> +void build_append_array(GArray *array, GArray *val)
> +{
> + g_array_append_vals(array, val->data, val->len);
> +}
> +
> +#define ACPI_NAMESEG_LEN 4
> +
> +void GCC_FMT_ATTR(2, 3)
> +build_append_nameseg(GArray *array, const char *format, ...)
> +{
> + /* It would be nicer to use g_string_vprintf but it's only there in 2.22 */
> + char s[] = "XXXX";
> + int len;
> + va_list args;
> +
> + va_start(args, format);
> + len = vsnprintf(s, sizeof s, format, args);
> + va_end(args);
> +
> + assert(len <= ACPI_NAMESEG_LEN);
> +
> + g_array_append_vals(array, s, len);
> + /* Pad up to ACPI_NAMESEG_LEN characters if necessary. */
> + g_array_append_vals(array, "____", ACPI_NAMESEG_LEN - len);
> +}
> +
> +/* 5.4 Definition Block Encoding */
> +enum {
> + PACKAGE_LENGTH_1BYTE_SHIFT = 6, /* Up to 63 - use extra 2 bits. */
> + PACKAGE_LENGTH_2BYTE_SHIFT = 4,
> + PACKAGE_LENGTH_3BYTE_SHIFT = 12,
> + PACKAGE_LENGTH_4BYTE_SHIFT = 20,
> +};
> +
> +void build_prepend_package_length(GArray *package, unsigned min_bytes)
> +{
> + uint8_t byte;
> + unsigned length = package->len;
> + unsigned length_bytes;
> +
> + if (length + 1 < (1 << PACKAGE_LENGTH_1BYTE_SHIFT)) {
> + length_bytes = 1;
> + } else if (length + 2 < (1 << PACKAGE_LENGTH_3BYTE_SHIFT)) {
> + length_bytes = 2;
> + } else if (length + 3 < (1 << PACKAGE_LENGTH_4BYTE_SHIFT)) {
> + length_bytes = 3;
> + } else {
> + length_bytes = 4;
> + }
> +
> + /* Force length to at least min_bytes.
> + * This wastes memory but that's how bios did it.
> + */
> + length_bytes = MAX(length_bytes, min_bytes);
> +
> + /* PkgLength is the length of the inclusive length of the data. */
> + length += length_bytes;
> +
> + switch (length_bytes) {
> + case 1:
> + byte = length;
> + build_prepend_byte(package, byte);
> + return;
> + case 4:
> + byte = length >> PACKAGE_LENGTH_4BYTE_SHIFT;
> + build_prepend_byte(package, byte);
> + length &= (1 << PACKAGE_LENGTH_4BYTE_SHIFT) - 1;
> + /* fall through */
> + case 3:
> + byte = length >> PACKAGE_LENGTH_3BYTE_SHIFT;
> + build_prepend_byte(package, byte);
> + length &= (1 << PACKAGE_LENGTH_3BYTE_SHIFT) - 1;
> + /* fall through */
> + case 2:
> + byte = length >> PACKAGE_LENGTH_2BYTE_SHIFT;
> + build_prepend_byte(package, byte);
> + length &= (1 << PACKAGE_LENGTH_2BYTE_SHIFT) - 1;
> + /* fall through */
> + }
> + /*
> + * Most significant two bits of byte zero indicate how many following bytes
> + * are in PkgLength encoding.
> + */
> + byte = ((length_bytes - 1) << PACKAGE_LENGTH_1BYTE_SHIFT) | length;
> + build_prepend_byte(package, byte);
> +}
> +
> +void build_package(GArray *package, uint8_t op, unsigned min_bytes)
> +{
> + build_prepend_package_length(package, min_bytes);
> + build_prepend_byte(package, op);
> +}
> +
> +void build_extop_package(GArray *package, uint8_t op)
> +{
> + build_package(package, op, 1);
> + build_prepend_byte(package, 0x5B); /* ExtOpPrefix */
> +}
> +
> +void build_append_value(GArray *table, uint32_t value, int size)
> +{
> + uint8_t prefix;
> + int i;
> +
> + switch (size) {
> + case 1:
> + prefix = 0x0A; /* BytePrefix */
> + break;
> + case 2:
> + prefix = 0x0B; /* WordPrefix */
> + break;
> + case 4:
> + prefix = 0x0C; /* DWordPrefix */
> + break;
> + default:
> + assert(0);
> + return;
> + }
> + build_append_byte(table, prefix);
> + for (i = 0; i < size; ++i) {
> + build_append_byte(table, value & 0xFF);
> + value = value >> 8;
> + }
> +}
> +
> +void build_append_int(GArray *table, uint32_t value)
> +{
> + if (value == 0x00) {
> + build_append_byte(table, 0x00); /* ZeroOp */
> + } else if (value == 0x01) {
> + build_append_byte(table, 0x01); /* OneOp */
> + } else if (value <= 0xFF) {
> + build_append_value(table, value, 1);
> + } else if (value <= 0xFFFF) {
> + build_append_value(table, value, 2);
> + } else {
> + build_append_value(table, value, 4);
> + }
> +}
> +
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index 4c0536f..8ee15ab 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -54,6 +54,8 @@
> #include "hw/i386/q35-acpi-dsdt.hex"
> #include "hw/i386/acpi-dsdt.hex"
>
> +#include "hw/acpi/acpi-build-utils.h"
> +
> #include "qapi/qmp/qint.h"
> #include "qom/qom-qobject.h"
> #include "exec/ram_addr.h"
> @@ -277,166 +279,6 @@ build_header(GArray *linker, GArray *table_data,
> table_data->data, h, len, &h->checksum);
> }
>
> -static inline GArray *build_alloc_array(void)
> -{
> - return g_array_new(false, true /* clear */, 1);
> -}
> -
> -static inline void build_free_array(GArray *array)
> -{
> - g_array_free(array, true);
> -}
> -
> -static inline void build_prepend_byte(GArray *array, uint8_t val)
> -{
> - g_array_prepend_val(array, val);
> -}
> -
> -static inline void build_append_byte(GArray *array, uint8_t val)
> -{
> - g_array_append_val(array, val);
> -}
> -
> -static inline void build_append_array(GArray *array, GArray *val)
> -{
> - g_array_append_vals(array, val->data, val->len);
> -}
> -
> -#define ACPI_NAMESEG_LEN 4
> -
> -static void GCC_FMT_ATTR(2, 3)
> -build_append_nameseg(GArray *array, const char *format, ...)
> -{
> - /* It would be nicer to use g_string_vprintf but it's only there in 2.22 */
> - char s[] = "XXXX";
> - int len;
> - va_list args;
> -
> - va_start(args, format);
> - len = vsnprintf(s, sizeof s, format, args);
> - va_end(args);
> -
> - assert(len <= ACPI_NAMESEG_LEN);
> -
> - g_array_append_vals(array, s, len);
> - /* Pad up to ACPI_NAMESEG_LEN characters if necessary. */
> - g_array_append_vals(array, "____", ACPI_NAMESEG_LEN - len);
> -}
> -
> -/* 5.4 Definition Block Encoding */
> -enum {
> - PACKAGE_LENGTH_1BYTE_SHIFT = 6, /* Up to 63 - use extra 2 bits. */
> - PACKAGE_LENGTH_2BYTE_SHIFT = 4,
> - PACKAGE_LENGTH_3BYTE_SHIFT = 12,
> - PACKAGE_LENGTH_4BYTE_SHIFT = 20,
> -};
> -
> -static void build_prepend_package_length(GArray *package, unsigned min_bytes)
> -{
> - uint8_t byte;
> - unsigned length = package->len;
> - unsigned length_bytes;
> -
> - if (length + 1 < (1 << PACKAGE_LENGTH_1BYTE_SHIFT)) {
> - length_bytes = 1;
> - } else if (length + 2 < (1 << PACKAGE_LENGTH_3BYTE_SHIFT)) {
> - length_bytes = 2;
> - } else if (length + 3 < (1 << PACKAGE_LENGTH_4BYTE_SHIFT)) {
> - length_bytes = 3;
> - } else {
> - length_bytes = 4;
> - }
> -
> - /* Force length to at least min_bytes.
> - * This wastes memory but that's how bios did it.
> - */
> - length_bytes = MAX(length_bytes, min_bytes);
> -
> - /* PkgLength is the length of the inclusive length of the data. */
> - length += length_bytes;
> -
> - switch (length_bytes) {
> - case 1:
> - byte = length;
> - build_prepend_byte(package, byte);
> - return;
> - case 4:
> - byte = length >> PACKAGE_LENGTH_4BYTE_SHIFT;
> - build_prepend_byte(package, byte);
> - length &= (1 << PACKAGE_LENGTH_4BYTE_SHIFT) - 1;
> - /* fall through */
> - case 3:
> - byte = length >> PACKAGE_LENGTH_3BYTE_SHIFT;
> - build_prepend_byte(package, byte);
> - length &= (1 << PACKAGE_LENGTH_3BYTE_SHIFT) - 1;
> - /* fall through */
> - case 2:
> - byte = length >> PACKAGE_LENGTH_2BYTE_SHIFT;
> - build_prepend_byte(package, byte);
> - length &= (1 << PACKAGE_LENGTH_2BYTE_SHIFT) - 1;
> - /* fall through */
> - }
> - /*
> - * Most significant two bits of byte zero indicate how many following bytes
> - * are in PkgLength encoding.
> - */
> - byte = ((length_bytes - 1) << PACKAGE_LENGTH_1BYTE_SHIFT) | length;
> - build_prepend_byte(package, byte);
> -}
> -
> -static void build_package(GArray *package, uint8_t op, unsigned min_bytes)
> -{
> - build_prepend_package_length(package, min_bytes);
> - build_prepend_byte(package, op);
> -}
> -
> -static void build_extop_package(GArray *package, uint8_t op)
> -{
> - build_package(package, op, 1);
> - build_prepend_byte(package, 0x5B); /* ExtOpPrefix */
> -}
> -
> -static void build_append_value(GArray *table, uint32_t value, int size)
> -{
> - uint8_t prefix;
> - int i;
> -
> - switch (size) {
> - case 1:
> - prefix = 0x0A; /* BytePrefix */
> - break;
> - case 2:
> - prefix = 0x0B; /* WordPrefix */
> - break;
> - case 4:
> - prefix = 0x0C; /* DWordPrefix */
> - break;
> - default:
> - assert(0);
> - return;
> - }
> - build_append_byte(table, prefix);
> - for (i = 0; i < size; ++i) {
> - build_append_byte(table, value & 0xFF);
> - value = value >> 8;
> - }
> -}
> -
> -static void build_append_int(GArray *table, uint32_t value)
> -{
> - if (value == 0x00) {
> - build_append_byte(table, 0x00); /* ZeroOp */
> - } else if (value == 0x01) {
> - build_append_byte(table, 0x01); /* OneOp */
> - } else if (value <= 0xFF) {
> - build_append_value(table, value, 1);
> - } else if (value <= 0xFFFF) {
> - build_append_value(table, value, 2);
> - } else {
> - build_append_value(table, value, 4);
> - }
> -}
> -
> static GArray *build_alloc_method(const char *name, uint8_t arg_count)
> {
> GArray *method = build_alloc_array();
> diff --git a/include/hw/acpi/acpi-build-utils.h b/include/hw/acpi/acpi-build-utils.h
> new file mode 100644
> index 0000000..e6a0b28
> --- /dev/null
> +++ b/include/hw/acpi/acpi-build-utils.h
> @@ -0,0 +1,23 @@
> +#ifndef HW_ACPI_GEN_UTILS_H
> +#define HW_ACPI_GEN_UTILS_H
> +
> +#include <stdint.h>
> +#include <glib.h>
> +#include "qemu/compiler.h"
> +
> +GArray *build_alloc_array(void);
> +void build_free_array(GArray *array);
> +void build_prepend_byte(GArray *array, uint8_t val);
> +void build_append_byte(GArray *array, uint8_t val);
> +void build_append_array(GArray *array, GArray *val);
> +
> +void GCC_FMT_ATTR(2, 3)
> +build_append_nameseg(GArray *array, const char *format, ...);
> +
> +void build_prepend_package_length(GArray *package, unsigned min_bytes);
> +void build_package(GArray *package, uint8_t op, unsigned min_bytes);
> +void build_append_value(GArray *table, uint32_t value, int size);
> +void build_append_int(GArray *table, uint32_t value);
> +void build_extop_package(GArray *package, uint8_t op);
> +
> +#endif
> --
> 1.8.3.1
>
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [Qemu-devel] [PATCH v5 2/5] acpi: move generic aml building helpers into dedictated file
2015-01-27 12:23 ` Michael S. Tsirkin
@ 2015-01-28 12:34 ` Igor Mammedov
0 siblings, 0 replies; 13+ messages in thread
From: Igor Mammedov @ 2015-01-28 12:34 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: qemu-devel, marcel.a
On Tue, 27 Jan 2015 14:23:18 +0200
"Michael S. Tsirkin" <mst@redhat.com> wrote:
> On Wed, Jan 21, 2015 at 09:09:10AM +0000, Igor Mammedov wrote:
> > the will be later used for composing AML primitives
> > and all that could be reused later for ARM machines
> > as well.
> >
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
>
> OK so the only thing holding this up is really that
> you have indicated that in the follow-up patches
> you want to add here more stuff with
> aml_ prefix instead of acpi_.
>
> In that case let's name this file aml-build.c?
sure, I'll rename it.
>
>
> > ---
> > v3:
> > * rename acpi_gen_utils.[ch] to acpi-build-utils.[ch]
> > * copy GLP license block from acpi-build.c
> > v2:
> > * fix wrong ident in moved code
> > ---
> > hw/acpi/Makefile.objs | 1 +
> > hw/acpi/acpi-build-utils.c | 187 +++++++++++++++++++++++++++++++++++++
> > hw/i386/acpi-build.c | 162 +-------------------------------
> > include/hw/acpi/acpi-build-utils.h | 23 +++++
> > 4 files changed, 213 insertions(+), 160 deletions(-)
> > create mode 100644 hw/acpi/acpi-build-utils.c
> > create mode 100644 include/hw/acpi/acpi-build-utils.h
> >
> > diff --git a/hw/acpi/Makefile.objs b/hw/acpi/Makefile.objs
> > index ee82073..cad0355 100644
> > --- a/hw/acpi/Makefile.objs
> > +++ b/hw/acpi/Makefile.objs
> > @@ -2,3 +2,4 @@ common-obj-$(CONFIG_ACPI) += core.o piix4.o ich9.o pcihp.o cpu_hotplug.o
> > common-obj-$(CONFIG_ACPI) += memory_hotplug.o
> > common-obj-$(CONFIG_ACPI) += acpi_interface.o
> > common-obj-$(CONFIG_ACPI) += bios-linker-loader.o
> > +common-obj-$(CONFIG_ACPI) += acpi-build-utils.o
> > diff --git a/hw/acpi/acpi-build-utils.c b/hw/acpi/acpi-build-utils.c
> > new file mode 100644
> > index 0000000..79aa610
> > --- /dev/null
> > +++ b/hw/acpi/acpi-build-utils.c
> > @@ -0,0 +1,187 @@
> > +/* Support for generating ACPI tables and passing them to Guests
> > + *
> > + * Copyright (C) 2014 Red Hat Inc
> > + *
> > + * Author: Michael S. Tsirkin <mst@redhat.com>
> > + * Author: Igor Mammedov <imammedo@redhat.com>
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License as published by
> > + * the Free Software Foundation; either version 2 of the License, or
> > + * (at your option) any later version.
> > +
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> > + * GNU General Public License for more details.
> > +
> > + * You should have received a copy of the GNU General Public License along
> > + * with this program; if not, see <http://www.gnu.org/licenses/>.
> > + */
> > +
> > +#include <stdio.h>
> > +#include <stdarg.h>
> > +#include <assert.h>
> > +#include <stdbool.h>
> > +#include "hw/acpi/acpi-build-utils.h"
> > +
> > +GArray *build_alloc_array(void)
> > +{
> > + return g_array_new(false, true /* clear */, 1);
> > +}
> > +
> > +void build_free_array(GArray *array)
> > +{
> > + g_array_free(array, true);
> > +}
> > +
> > +void build_prepend_byte(GArray *array, uint8_t val)
> > +{
> > + g_array_prepend_val(array, val);
> > +}
> > +
> > +void build_append_byte(GArray *array, uint8_t val)
> > +{
> > + g_array_append_val(array, val);
> > +}
> > +
> > +void build_append_array(GArray *array, GArray *val)
> > +{
> > + g_array_append_vals(array, val->data, val->len);
> > +}
> > +
> > +#define ACPI_NAMESEG_LEN 4
> > +
> > +void GCC_FMT_ATTR(2, 3)
> > +build_append_nameseg(GArray *array, const char *format, ...)
> > +{
> > + /* It would be nicer to use g_string_vprintf but it's only there in 2.22 */
> > + char s[] = "XXXX";
> > + int len;
> > + va_list args;
> > +
> > + va_start(args, format);
> > + len = vsnprintf(s, sizeof s, format, args);
> > + va_end(args);
> > +
> > + assert(len <= ACPI_NAMESEG_LEN);
> > +
> > + g_array_append_vals(array, s, len);
> > + /* Pad up to ACPI_NAMESEG_LEN characters if necessary. */
> > + g_array_append_vals(array, "____", ACPI_NAMESEG_LEN - len);
> > +}
> > +
> > +/* 5.4 Definition Block Encoding */
> > +enum {
> > + PACKAGE_LENGTH_1BYTE_SHIFT = 6, /* Up to 63 - use extra 2 bits. */
> > + PACKAGE_LENGTH_2BYTE_SHIFT = 4,
> > + PACKAGE_LENGTH_3BYTE_SHIFT = 12,
> > + PACKAGE_LENGTH_4BYTE_SHIFT = 20,
> > +};
> > +
> > +void build_prepend_package_length(GArray *package, unsigned min_bytes)
> > +{
> > + uint8_t byte;
> > + unsigned length = package->len;
> > + unsigned length_bytes;
> > +
> > + if (length + 1 < (1 << PACKAGE_LENGTH_1BYTE_SHIFT)) {
> > + length_bytes = 1;
> > + } else if (length + 2 < (1 << PACKAGE_LENGTH_3BYTE_SHIFT)) {
> > + length_bytes = 2;
> > + } else if (length + 3 < (1 << PACKAGE_LENGTH_4BYTE_SHIFT)) {
> > + length_bytes = 3;
> > + } else {
> > + length_bytes = 4;
> > + }
> > +
> > + /* Force length to at least min_bytes.
> > + * This wastes memory but that's how bios did it.
> > + */
> > + length_bytes = MAX(length_bytes, min_bytes);
> > +
> > + /* PkgLength is the length of the inclusive length of the data. */
> > + length += length_bytes;
> > +
> > + switch (length_bytes) {
> > + case 1:
> > + byte = length;
> > + build_prepend_byte(package, byte);
> > + return;
> > + case 4:
> > + byte = length >> PACKAGE_LENGTH_4BYTE_SHIFT;
> > + build_prepend_byte(package, byte);
> > + length &= (1 << PACKAGE_LENGTH_4BYTE_SHIFT) - 1;
> > + /* fall through */
> > + case 3:
> > + byte = length >> PACKAGE_LENGTH_3BYTE_SHIFT;
> > + build_prepend_byte(package, byte);
> > + length &= (1 << PACKAGE_LENGTH_3BYTE_SHIFT) - 1;
> > + /* fall through */
> > + case 2:
> > + byte = length >> PACKAGE_LENGTH_2BYTE_SHIFT;
> > + build_prepend_byte(package, byte);
> > + length &= (1 << PACKAGE_LENGTH_2BYTE_SHIFT) - 1;
> > + /* fall through */
> > + }
> > + /*
> > + * Most significant two bits of byte zero indicate how many following bytes
> > + * are in PkgLength encoding.
> > + */
> > + byte = ((length_bytes - 1) << PACKAGE_LENGTH_1BYTE_SHIFT) | length;
> > + build_prepend_byte(package, byte);
> > +}
> > +
> > +void build_package(GArray *package, uint8_t op, unsigned min_bytes)
> > +{
> > + build_prepend_package_length(package, min_bytes);
> > + build_prepend_byte(package, op);
> > +}
> > +
> > +void build_extop_package(GArray *package, uint8_t op)
> > +{
> > + build_package(package, op, 1);
> > + build_prepend_byte(package, 0x5B); /* ExtOpPrefix */
> > +}
> > +
> > +void build_append_value(GArray *table, uint32_t value, int size)
> > +{
> > + uint8_t prefix;
> > + int i;
> > +
> > + switch (size) {
> > + case 1:
> > + prefix = 0x0A; /* BytePrefix */
> > + break;
> > + case 2:
> > + prefix = 0x0B; /* WordPrefix */
> > + break;
> > + case 4:
> > + prefix = 0x0C; /* DWordPrefix */
> > + break;
> > + default:
> > + assert(0);
> > + return;
> > + }
> > + build_append_byte(table, prefix);
> > + for (i = 0; i < size; ++i) {
> > + build_append_byte(table, value & 0xFF);
> > + value = value >> 8;
> > + }
> > +}
> > +
> > +void build_append_int(GArray *table, uint32_t value)
> > +{
> > + if (value == 0x00) {
> > + build_append_byte(table, 0x00); /* ZeroOp */
> > + } else if (value == 0x01) {
> > + build_append_byte(table, 0x01); /* OneOp */
> > + } else if (value <= 0xFF) {
> > + build_append_value(table, value, 1);
> > + } else if (value <= 0xFFFF) {
> > + build_append_value(table, value, 2);
> > + } else {
> > + build_append_value(table, value, 4);
> > + }
> > +}
> > +
> > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> > index 4c0536f..8ee15ab 100644
> > --- a/hw/i386/acpi-build.c
> > +++ b/hw/i386/acpi-build.c
> > @@ -54,6 +54,8 @@
> > #include "hw/i386/q35-acpi-dsdt.hex"
> > #include "hw/i386/acpi-dsdt.hex"
> >
> > +#include "hw/acpi/acpi-build-utils.h"
> > +
> > #include "qapi/qmp/qint.h"
> > #include "qom/qom-qobject.h"
> > #include "exec/ram_addr.h"
> > @@ -277,166 +279,6 @@ build_header(GArray *linker, GArray *table_data,
> > table_data->data, h, len, &h->checksum);
> > }
> >
> > -static inline GArray *build_alloc_array(void)
> > -{
> > - return g_array_new(false, true /* clear */, 1);
> > -}
> > -
> > -static inline void build_free_array(GArray *array)
> > -{
> > - g_array_free(array, true);
> > -}
> > -
> > -static inline void build_prepend_byte(GArray *array, uint8_t val)
> > -{
> > - g_array_prepend_val(array, val);
> > -}
> > -
> > -static inline void build_append_byte(GArray *array, uint8_t val)
> > -{
> > - g_array_append_val(array, val);
> > -}
> > -
> > -static inline void build_append_array(GArray *array, GArray *val)
> > -{
> > - g_array_append_vals(array, val->data, val->len);
> > -}
> > -
> > -#define ACPI_NAMESEG_LEN 4
> > -
> > -static void GCC_FMT_ATTR(2, 3)
> > -build_append_nameseg(GArray *array, const char *format, ...)
> > -{
> > - /* It would be nicer to use g_string_vprintf but it's only there in 2.22 */
> > - char s[] = "XXXX";
> > - int len;
> > - va_list args;
> > -
> > - va_start(args, format);
> > - len = vsnprintf(s, sizeof s, format, args);
> > - va_end(args);
> > -
> > - assert(len <= ACPI_NAMESEG_LEN);
> > -
> > - g_array_append_vals(array, s, len);
> > - /* Pad up to ACPI_NAMESEG_LEN characters if necessary. */
> > - g_array_append_vals(array, "____", ACPI_NAMESEG_LEN - len);
> > -}
> > -
> > -/* 5.4 Definition Block Encoding */
> > -enum {
> > - PACKAGE_LENGTH_1BYTE_SHIFT = 6, /* Up to 63 - use extra 2 bits. */
> > - PACKAGE_LENGTH_2BYTE_SHIFT = 4,
> > - PACKAGE_LENGTH_3BYTE_SHIFT = 12,
> > - PACKAGE_LENGTH_4BYTE_SHIFT = 20,
> > -};
> > -
> > -static void build_prepend_package_length(GArray *package, unsigned min_bytes)
> > -{
> > - uint8_t byte;
> > - unsigned length = package->len;
> > - unsigned length_bytes;
> > -
> > - if (length + 1 < (1 << PACKAGE_LENGTH_1BYTE_SHIFT)) {
> > - length_bytes = 1;
> > - } else if (length + 2 < (1 << PACKAGE_LENGTH_3BYTE_SHIFT)) {
> > - length_bytes = 2;
> > - } else if (length + 3 < (1 << PACKAGE_LENGTH_4BYTE_SHIFT)) {
> > - length_bytes = 3;
> > - } else {
> > - length_bytes = 4;
> > - }
> > -
> > - /* Force length to at least min_bytes.
> > - * This wastes memory but that's how bios did it.
> > - */
> > - length_bytes = MAX(length_bytes, min_bytes);
> > -
> > - /* PkgLength is the length of the inclusive length of the data. */
> > - length += length_bytes;
> > -
> > - switch (length_bytes) {
> > - case 1:
> > - byte = length;
> > - build_prepend_byte(package, byte);
> > - return;
> > - case 4:
> > - byte = length >> PACKAGE_LENGTH_4BYTE_SHIFT;
> > - build_prepend_byte(package, byte);
> > - length &= (1 << PACKAGE_LENGTH_4BYTE_SHIFT) - 1;
> > - /* fall through */
> > - case 3:
> > - byte = length >> PACKAGE_LENGTH_3BYTE_SHIFT;
> > - build_prepend_byte(package, byte);
> > - length &= (1 << PACKAGE_LENGTH_3BYTE_SHIFT) - 1;
> > - /* fall through */
> > - case 2:
> > - byte = length >> PACKAGE_LENGTH_2BYTE_SHIFT;
> > - build_prepend_byte(package, byte);
> > - length &= (1 << PACKAGE_LENGTH_2BYTE_SHIFT) - 1;
> > - /* fall through */
> > - }
> > - /*
> > - * Most significant two bits of byte zero indicate how many following bytes
> > - * are in PkgLength encoding.
> > - */
> > - byte = ((length_bytes - 1) << PACKAGE_LENGTH_1BYTE_SHIFT) | length;
> > - build_prepend_byte(package, byte);
> > -}
> > -
> > -static void build_package(GArray *package, uint8_t op, unsigned min_bytes)
> > -{
> > - build_prepend_package_length(package, min_bytes);
> > - build_prepend_byte(package, op);
> > -}
> > -
> > -static void build_extop_package(GArray *package, uint8_t op)
> > -{
> > - build_package(package, op, 1);
> > - build_prepend_byte(package, 0x5B); /* ExtOpPrefix */
> > -}
> > -
> > -static void build_append_value(GArray *table, uint32_t value, int size)
> > -{
> > - uint8_t prefix;
> > - int i;
> > -
> > - switch (size) {
> > - case 1:
> > - prefix = 0x0A; /* BytePrefix */
> > - break;
> > - case 2:
> > - prefix = 0x0B; /* WordPrefix */
> > - break;
> > - case 4:
> > - prefix = 0x0C; /* DWordPrefix */
> > - break;
> > - default:
> > - assert(0);
> > - return;
> > - }
> > - build_append_byte(table, prefix);
> > - for (i = 0; i < size; ++i) {
> > - build_append_byte(table, value & 0xFF);
> > - value = value >> 8;
> > - }
> > -}
> > -
> > -static void build_append_int(GArray *table, uint32_t value)
> > -{
> > - if (value == 0x00) {
> > - build_append_byte(table, 0x00); /* ZeroOp */
> > - } else if (value == 0x01) {
> > - build_append_byte(table, 0x01); /* OneOp */
> > - } else if (value <= 0xFF) {
> > - build_append_value(table, value, 1);
> > - } else if (value <= 0xFFFF) {
> > - build_append_value(table, value, 2);
> > - } else {
> > - build_append_value(table, value, 4);
> > - }
> > -}
> > -
> > static GArray *build_alloc_method(const char *name, uint8_t arg_count)
> > {
> > GArray *method = build_alloc_array();
> > diff --git a/include/hw/acpi/acpi-build-utils.h b/include/hw/acpi/acpi-build-utils.h
> > new file mode 100644
> > index 0000000..e6a0b28
> > --- /dev/null
> > +++ b/include/hw/acpi/acpi-build-utils.h
> > @@ -0,0 +1,23 @@
> > +#ifndef HW_ACPI_GEN_UTILS_H
> > +#define HW_ACPI_GEN_UTILS_H
> > +
> > +#include <stdint.h>
> > +#include <glib.h>
> > +#include "qemu/compiler.h"
> > +
> > +GArray *build_alloc_array(void);
> > +void build_free_array(GArray *array);
> > +void build_prepend_byte(GArray *array, uint8_t val);
> > +void build_append_byte(GArray *array, uint8_t val);
> > +void build_append_array(GArray *array, GArray *val);
> > +
> > +void GCC_FMT_ATTR(2, 3)
> > +build_append_nameseg(GArray *array, const char *format, ...);
> > +
> > +void build_prepend_package_length(GArray *package, unsigned min_bytes);
> > +void build_package(GArray *package, uint8_t op, unsigned min_bytes);
> > +void build_append_value(GArray *table, uint32_t value, int size);
> > +void build_append_int(GArray *table, uint32_t value);
> > +void build_extop_package(GArray *package, uint8_t op);
> > +
> > +#endif
> > --
> > 1.8.3.1
> >
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* [Qemu-devel] [PATCH v5 3/5] acpi: add build_append_namestring() helper
2015-01-21 9:09 [Qemu-devel] [PATCH v5 0/5] pc: acpi: various fixes and cleanups Igor Mammedov
2015-01-21 9:09 ` [Qemu-devel] [PATCH v5 1/5] pc: acpi-build: cleanup AcpiPmInfo initialization Igor Mammedov
2015-01-21 9:09 ` [Qemu-devel] [PATCH v5 2/5] acpi: move generic aml building helpers into dedictated file Igor Mammedov
@ 2015-01-21 9:09 ` Igor Mammedov
2015-01-21 9:22 ` Marcel Apfelbaum
2015-01-21 9:09 ` [Qemu-devel] [PATCH v5 4/5] acpi: drop min-bytes in build_package() Igor Mammedov
2015-01-21 9:09 ` [Qemu-devel] [PATCH v5 5/5] pc: acpi-build: simplify PCI bus tree generation Igor Mammedov
4 siblings, 1 reply; 13+ messages in thread
From: Igor Mammedov @ 2015-01-21 9:09 UTC (permalink / raw)
To: qemu-devel; +Cc: marcel.a, mst
Use build_append_namestring() instead of build_append_nameseg()
So user won't have to care whether name is NameSeg, NamePath or
NameString.
See for refernce ACPI 5.0: 20.2.2 Name Objects Encoding
Signed-off-by: Igor Mammedov <imammedo@redhat.com>
Reviewed-by: Claudio Fontana <claudio.fontana@huawei.com>
---
v3:
assert on wrong Segcount earlier and extend condition to
seg_count > 0 && seg_count <= 255
as suggested by Claudio Fontana <claudio.fontana@huawei.com>
---
hw/acpi/acpi-build-utils.c | 90 +++++++++++++++++++++++++++++++++++++-
hw/i386/acpi-build.c | 35 +++++++--------
include/hw/acpi/acpi-build-utils.h | 2 +-
3 files changed, 106 insertions(+), 21 deletions(-)
diff --git a/hw/acpi/acpi-build-utils.c b/hw/acpi/acpi-build-utils.c
index 79aa610..aed9066 100644
--- a/hw/acpi/acpi-build-utils.c
+++ b/hw/acpi/acpi-build-utils.c
@@ -52,7 +52,7 @@ void build_append_array(GArray *array, GArray *val)
#define ACPI_NAMESEG_LEN 4
-void GCC_FMT_ATTR(2, 3)
+static void GCC_FMT_ATTR(2, 3)
build_append_nameseg(GArray *array, const char *format, ...)
{
/* It would be nicer to use g_string_vprintf but it's only there in 2.22 */
@@ -71,6 +71,94 @@ build_append_nameseg(GArray *array, const char *format, ...)
g_array_append_vals(array, "____", ACPI_NAMESEG_LEN - len);
}
+static const uint8_t ACPI_DualNamePrefix = 0x2E;
+static const uint8_t ACPI_MultiNamePrefix = 0x2F;
+
+static void
+build_append_namestringv(GArray *array, const char *format, va_list ap)
+{
+ /* It would be nicer to use g_string_vprintf but it's only there in 2.22 */
+ char *s;
+ int len;
+ va_list va_len;
+ char **segs;
+ char **segs_iter;
+ int seg_count = 0;
+
+ va_copy(va_len, ap);
+ len = vsnprintf(NULL, 0, format, va_len);
+ va_end(va_len);
+ len += 1;
+ s = g_new(typeof(*s), len);
+
+ len = vsnprintf(s, len, format, ap);
+
+ segs = g_strsplit(s, ".", 0);
+ g_free(s);
+
+ /* count segments */
+ segs_iter = segs;
+ while (*segs_iter) {
+ ++segs_iter;
+ ++seg_count;
+ }
+ /*
+ * ACPI 5.0 spec: 20.2.2 Name Objects Encoding:
+ * "SegCount can be from 1 to 255"
+ */
+ assert(seg_count > 0 && seg_count <= 255);
+
+ /* handle RootPath || PrefixPath */
+ s = *segs;
+ while (*s == '\\' || *s == '^') {
+ g_array_append_val(array, *s);
+ ++s;
+ }
+
+ switch (seg_count) {
+ case 1:
+ if (!*s) { /* NullName */
+ const uint8_t nullpath = 0;
+ g_array_append_val(array, nullpath);
+ } else {
+ build_append_nameseg(array, "%s", s);
+ }
+ break;
+
+ case 2:
+ g_array_append_val(array, ACPI_DualNamePrefix);
+ build_append_nameseg(array, "%s", s);
+ build_append_nameseg(array, "%s", segs[1]);
+ break;
+
+ default:
+ g_array_append_val(array, ACPI_MultiNamePrefix);
+ g_array_append_val(array, seg_count);
+
+ /* handle the 1st segment manually due to prefix/root path */
+ build_append_nameseg(array, "%s", s);
+
+ /* add the rest of segments */
+ segs_iter = segs + 1;
+ while (*segs_iter) {
+ build_append_nameseg(array, "%s", *segs_iter);
+ ++segs_iter;
+ }
+ break;
+ }
+ g_strfreev(segs);
+}
+
+void GCC_FMT_ATTR(2, 3)
+build_append_namestring(GArray *array, const char *format, ...)
+{
+ va_list ap;
+
+ va_start(ap, format);
+ build_append_namestringv(array, format, ap);
+ va_end(ap);
+}
+
/* 5.4 Definition Block Encoding */
enum {
PACKAGE_LENGTH_1BYTE_SHIFT = 6, /* Up to 63 - use extra 2 bits. */
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 8ee15ab..e3ac1a14 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -283,7 +283,7 @@ static GArray *build_alloc_method(const char *name, uint8_t arg_count)
{
GArray *method = build_alloc_array();
- build_append_nameseg(method, "%s", name);
+ build_append_namestring(method, "%s", name);
build_append_byte(method, arg_count); /* MethodFlags: ArgCount */
return method;
@@ -575,7 +575,7 @@ build_append_notify_method(GArray *device, const char *name,
for (i = 0; i < count; i++) {
GArray *target = build_alloc_array();
- build_append_nameseg(target, format, i);
+ build_append_namestring(target, format, i);
assert(i < 256); /* Fits in 1 byte */
build_append_notify_target_ifequal(method, target, i, 1);
build_free_array(target);
@@ -703,24 +703,24 @@ static void build_pci_bus_end(PCIBus *bus, void *bus_state)
if (bus->parent_dev) {
op = 0x82; /* DeviceOp */
- build_append_nameseg(bus_table, "S%.02X",
+ build_append_namestring(bus_table, "S%.02X",
bus->parent_dev->devfn);
build_append_byte(bus_table, 0x08); /* NameOp */
- build_append_nameseg(bus_table, "_SUN");
+ build_append_namestring(bus_table, "_SUN");
build_append_value(bus_table, PCI_SLOT(bus->parent_dev->devfn), 1);
build_append_byte(bus_table, 0x08); /* NameOp */
- build_append_nameseg(bus_table, "_ADR");
+ build_append_namestring(bus_table, "_ADR");
build_append_value(bus_table, (PCI_SLOT(bus->parent_dev->devfn) << 16) |
PCI_FUNC(bus->parent_dev->devfn), 4);
} else {
op = 0x10; /* ScopeOp */;
- build_append_nameseg(bus_table, "PCI0");
+ build_append_namestring(bus_table, "PCI0");
}
bsel = object_property_get_qobject(OBJECT(bus), ACPI_PCIHP_PROP_BSEL, NULL);
if (bsel) {
build_append_byte(bus_table, 0x08); /* NameOp */
- build_append_nameseg(bus_table, "BSEL");
+ build_append_namestring(bus_table, "BSEL");
build_append_int(bus_table, qint_get_int(qobject_to_qint(bsel)));
memset(slot_hotplug_enable, 0xff, sizeof slot_hotplug_enable);
} else {
@@ -823,7 +823,7 @@ static void build_pci_bus_end(PCIBus *bus, void *bus_state)
build_append_int(notify, 0x1U << i);
build_append_byte(notify, 0x00); /* NullName */
build_append_byte(notify, 0x86); /* NotifyOp */
- build_append_nameseg(notify, "S%.02X", PCI_DEVFN(i, 0));
+ build_append_namestring(notify, "S%.02X", PCI_DEVFN(i, 0));
build_append_byte(notify, 0x69); /* Arg1Op */
/* Pack it up */
@@ -847,12 +847,12 @@ static void build_pci_bus_end(PCIBus *bus, void *bus_state)
if (bsel) {
build_append_byte(method, 0x70); /* StoreOp */
build_append_int(method, qint_get_int(qobject_to_qint(bsel)));
- build_append_nameseg(method, "BNUM");
- build_append_nameseg(method, "DVNT");
- build_append_nameseg(method, "PCIU");
+ build_append_namestring(method, "BNUM");
+ build_append_namestring(method, "DVNT");
+ build_append_namestring(method, "PCIU");
build_append_int(method, 1); /* Device Check */
- build_append_nameseg(method, "DVNT");
- build_append_nameseg(method, "PCID");
+ build_append_namestring(method, "DVNT");
+ build_append_namestring(method, "PCID");
build_append_int(method, 3); /* Eject Request */
}
@@ -878,11 +878,8 @@ static void build_pci_bus_end(PCIBus *bus, void *bus_state)
* At the moment this is not needed for root as we have a single root.
*/
if (bus->parent_dev) {
- build_append_byte(parent->notify_table, '^'); /* ParentPrefixChar */
- build_append_byte(parent->notify_table, 0x2E); /* DualNamePrefix */
- build_append_nameseg(parent->notify_table, "S%.02X",
+ build_append_namestring(parent->notify_table, "^PCNT.S%.02X",
bus->parent_dev->devfn);
- build_append_nameseg(parent->notify_table, "PCNT");
}
}
@@ -967,7 +964,7 @@ build_ssdt(GArray *table_data, GArray *linker,
GArray *sb_scope = build_alloc_array();
uint8_t op = 0x10; /* ScopeOp */
- build_append_nameseg(sb_scope, "_SB");
+ build_append_namestring(sb_scope, "_SB");
/* build Processor object for each processor */
for (i = 0; i < acpi_cpus; i++) {
@@ -987,7 +984,7 @@ build_ssdt(GArray *table_data, GArray *linker,
/* build "Name(CPON, Package() { One, One, ..., Zero, Zero, ... })" */
build_append_byte(sb_scope, 0x08); /* NameOp */
- build_append_nameseg(sb_scope, "CPON");
+ build_append_namestring(sb_scope, "CPON");
{
GArray *package = build_alloc_array();
diff --git a/include/hw/acpi/acpi-build-utils.h b/include/hw/acpi/acpi-build-utils.h
index e6a0b28..fd50625 100644
--- a/include/hw/acpi/acpi-build-utils.h
+++ b/include/hw/acpi/acpi-build-utils.h
@@ -12,7 +12,7 @@ void build_append_byte(GArray *array, uint8_t val);
void build_append_array(GArray *array, GArray *val);
void GCC_FMT_ATTR(2, 3)
-build_append_nameseg(GArray *array, const char *format, ...);
+build_append_namestring(GArray *array, const char *format, ...);
void build_prepend_package_length(GArray *package, unsigned min_bytes);
void build_package(GArray *package, uint8_t op, unsigned min_bytes);
--
1.8.3.1
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [Qemu-devel] [PATCH v5 3/5] acpi: add build_append_namestring() helper
2015-01-21 9:09 ` [Qemu-devel] [PATCH v5 3/5] acpi: add build_append_namestring() helper Igor Mammedov
@ 2015-01-21 9:22 ` Marcel Apfelbaum
0 siblings, 0 replies; 13+ messages in thread
From: Marcel Apfelbaum @ 2015-01-21 9:22 UTC (permalink / raw)
To: Igor Mammedov, qemu-devel; +Cc: marcel.a, mst
On 01/21/2015 11:09 AM, Igor Mammedov wrote:
> Use build_append_namestring() instead of build_append_nameseg()
> So user won't have to care whether name is NameSeg, NamePath or
> NameString.
>
> See for refernce ACPI 5.0: 20.2.2 Name Objects Encoding
>
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> Reviewed-by: Claudio Fontana <claudio.fontana@huawei.com>
> ---
> v3:
> assert on wrong Segcount earlier and extend condition to
> seg_count > 0 && seg_count <= 255
> as suggested by Claudio Fontana <claudio.fontana@huawei.com>
> ---
> hw/acpi/acpi-build-utils.c | 90 +++++++++++++++++++++++++++++++++++++-
> hw/i386/acpi-build.c | 35 +++++++--------
> include/hw/acpi/acpi-build-utils.h | 2 +-
> 3 files changed, 106 insertions(+), 21 deletions(-)
>
> diff --git a/hw/acpi/acpi-build-utils.c b/hw/acpi/acpi-build-utils.c
> index 79aa610..aed9066 100644
> --- a/hw/acpi/acpi-build-utils.c
> +++ b/hw/acpi/acpi-build-utils.c
> @@ -52,7 +52,7 @@ void build_append_array(GArray *array, GArray *val)
>
> #define ACPI_NAMESEG_LEN 4
>
> -void GCC_FMT_ATTR(2, 3)
> +static void GCC_FMT_ATTR(2, 3)
> build_append_nameseg(GArray *array, const char *format, ...)
> {
> /* It would be nicer to use g_string_vprintf but it's only there in 2.22 */
> @@ -71,6 +71,94 @@ build_append_nameseg(GArray *array, const char *format, ...)
> g_array_append_vals(array, "____", ACPI_NAMESEG_LEN - len);
> }
>
> +static const uint8_t ACPI_DualNamePrefix = 0x2E;
> +static const uint8_t ACPI_MultiNamePrefix = 0x2F;
> +
> +static void
> +build_append_namestringv(GArray *array, const char *format, va_list ap)
> +{
> + /* It would be nicer to use g_string_vprintf but it's only there in 2.22 */
> + char *s;
> + int len;
> + va_list va_len;
> + char **segs;
> + char **segs_iter;
> + int seg_count = 0;
> +
> + va_copy(va_len, ap);
> + len = vsnprintf(NULL, 0, format, va_len);
> + va_end(va_len);
> + len += 1;
> + s = g_new(typeof(*s), len);
> +
> + len = vsnprintf(s, len, format, ap);
> +
> + segs = g_strsplit(s, ".", 0);
> + g_free(s);
> +
> + /* count segments */
> + segs_iter = segs;
> + while (*segs_iter) {
> + ++segs_iter;
> + ++seg_count;
> + }
> + /*
> + * ACPI 5.0 spec: 20.2.2 Name Objects Encoding:
> + * "SegCount can be from 1 to 255"
> + */
> + assert(seg_count > 0 && seg_count <= 255);
> +
> + /* handle RootPath || PrefixPath */
> + s = *segs;
> + while (*s == '\\' || *s == '^') {
> + g_array_append_val(array, *s);
> + ++s;
> + }
> +
> + switch (seg_count) {
> + case 1:
> + if (!*s) { /* NullName */
> + const uint8_t nullpath = 0;
> + g_array_append_val(array, nullpath);
> + } else {
> + build_append_nameseg(array, "%s", s);
> + }
> + break;
> +
> + case 2:
> + g_array_append_val(array, ACPI_DualNamePrefix);
> + build_append_nameseg(array, "%s", s);
> + build_append_nameseg(array, "%s", segs[1]);
> + break;
> +
> + default:
> + g_array_append_val(array, ACPI_MultiNamePrefix);
> + g_array_append_val(array, seg_count);
> +
> + /* handle the 1st segment manually due to prefix/root path */
> + build_append_nameseg(array, "%s", s);
> +
> + /* add the rest of segments */
> + segs_iter = segs + 1;
> + while (*segs_iter) {
> + build_append_nameseg(array, "%s", *segs_iter);
> + ++segs_iter;
> + }
> + break;
> + }
> + g_strfreev(segs);
> +}
> +
> +void GCC_FMT_ATTR(2, 3)
> +build_append_namestring(GArray *array, const char *format, ...)
> +{
> + va_list ap;
> +
> + va_start(ap, format);
> + build_append_namestringv(array, format, ap);
> + va_end(ap);
> +}
> +
> /* 5.4 Definition Block Encoding */
> enum {
> PACKAGE_LENGTH_1BYTE_SHIFT = 6, /* Up to 63 - use extra 2 bits. */
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index 8ee15ab..e3ac1a14 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -283,7 +283,7 @@ static GArray *build_alloc_method(const char *name, uint8_t arg_count)
> {
> GArray *method = build_alloc_array();
>
> - build_append_nameseg(method, "%s", name);
> + build_append_namestring(method, "%s", name);
> build_append_byte(method, arg_count); /* MethodFlags: ArgCount */
>
> return method;
> @@ -575,7 +575,7 @@ build_append_notify_method(GArray *device, const char *name,
>
> for (i = 0; i < count; i++) {
> GArray *target = build_alloc_array();
> - build_append_nameseg(target, format, i);
> + build_append_namestring(target, format, i);
> assert(i < 256); /* Fits in 1 byte */
> build_append_notify_target_ifequal(method, target, i, 1);
> build_free_array(target);
> @@ -703,24 +703,24 @@ static void build_pci_bus_end(PCIBus *bus, void *bus_state)
>
> if (bus->parent_dev) {
> op = 0x82; /* DeviceOp */
> - build_append_nameseg(bus_table, "S%.02X",
> + build_append_namestring(bus_table, "S%.02X",
> bus->parent_dev->devfn);
> build_append_byte(bus_table, 0x08); /* NameOp */
> - build_append_nameseg(bus_table, "_SUN");
> + build_append_namestring(bus_table, "_SUN");
> build_append_value(bus_table, PCI_SLOT(bus->parent_dev->devfn), 1);
> build_append_byte(bus_table, 0x08); /* NameOp */
> - build_append_nameseg(bus_table, "_ADR");
> + build_append_namestring(bus_table, "_ADR");
> build_append_value(bus_table, (PCI_SLOT(bus->parent_dev->devfn) << 16) |
> PCI_FUNC(bus->parent_dev->devfn), 4);
> } else {
> op = 0x10; /* ScopeOp */;
> - build_append_nameseg(bus_table, "PCI0");
> + build_append_namestring(bus_table, "PCI0");
> }
>
> bsel = object_property_get_qobject(OBJECT(bus), ACPI_PCIHP_PROP_BSEL, NULL);
> if (bsel) {
> build_append_byte(bus_table, 0x08); /* NameOp */
> - build_append_nameseg(bus_table, "BSEL");
> + build_append_namestring(bus_table, "BSEL");
> build_append_int(bus_table, qint_get_int(qobject_to_qint(bsel)));
> memset(slot_hotplug_enable, 0xff, sizeof slot_hotplug_enable);
> } else {
> @@ -823,7 +823,7 @@ static void build_pci_bus_end(PCIBus *bus, void *bus_state)
> build_append_int(notify, 0x1U << i);
> build_append_byte(notify, 0x00); /* NullName */
> build_append_byte(notify, 0x86); /* NotifyOp */
> - build_append_nameseg(notify, "S%.02X", PCI_DEVFN(i, 0));
> + build_append_namestring(notify, "S%.02X", PCI_DEVFN(i, 0));
> build_append_byte(notify, 0x69); /* Arg1Op */
>
> /* Pack it up */
> @@ -847,12 +847,12 @@ static void build_pci_bus_end(PCIBus *bus, void *bus_state)
> if (bsel) {
> build_append_byte(method, 0x70); /* StoreOp */
> build_append_int(method, qint_get_int(qobject_to_qint(bsel)));
> - build_append_nameseg(method, "BNUM");
> - build_append_nameseg(method, "DVNT");
> - build_append_nameseg(method, "PCIU");
> + build_append_namestring(method, "BNUM");
> + build_append_namestring(method, "DVNT");
> + build_append_namestring(method, "PCIU");
> build_append_int(method, 1); /* Device Check */
> - build_append_nameseg(method, "DVNT");
> - build_append_nameseg(method, "PCID");
> + build_append_namestring(method, "DVNT");
> + build_append_namestring(method, "PCID");
> build_append_int(method, 3); /* Eject Request */
> }
>
> @@ -878,11 +878,8 @@ static void build_pci_bus_end(PCIBus *bus, void *bus_state)
> * At the moment this is not needed for root as we have a single root.
> */
> if (bus->parent_dev) {
> - build_append_byte(parent->notify_table, '^'); /* ParentPrefixChar */
> - build_append_byte(parent->notify_table, 0x2E); /* DualNamePrefix */
> - build_append_nameseg(parent->notify_table, "S%.02X",
> + build_append_namestring(parent->notify_table, "^PCNT.S%.02X",
> bus->parent_dev->devfn);
> - build_append_nameseg(parent->notify_table, "PCNT");
> }
> }
>
> @@ -967,7 +964,7 @@ build_ssdt(GArray *table_data, GArray *linker,
> GArray *sb_scope = build_alloc_array();
> uint8_t op = 0x10; /* ScopeOp */
>
> - build_append_nameseg(sb_scope, "_SB");
> + build_append_namestring(sb_scope, "_SB");
>
> /* build Processor object for each processor */
> for (i = 0; i < acpi_cpus; i++) {
> @@ -987,7 +984,7 @@ build_ssdt(GArray *table_data, GArray *linker,
>
> /* build "Name(CPON, Package() { One, One, ..., Zero, Zero, ... })" */
> build_append_byte(sb_scope, 0x08); /* NameOp */
> - build_append_nameseg(sb_scope, "CPON");
> + build_append_namestring(sb_scope, "CPON");
>
> {
> GArray *package = build_alloc_array();
> diff --git a/include/hw/acpi/acpi-build-utils.h b/include/hw/acpi/acpi-build-utils.h
> index e6a0b28..fd50625 100644
> --- a/include/hw/acpi/acpi-build-utils.h
> +++ b/include/hw/acpi/acpi-build-utils.h
> @@ -12,7 +12,7 @@ void build_append_byte(GArray *array, uint8_t val);
> void build_append_array(GArray *array, GArray *val);
>
> void GCC_FMT_ATTR(2, 3)
> -build_append_nameseg(GArray *array, const char *format, ...);
> +build_append_namestring(GArray *array, const char *format, ...);
>
> void build_prepend_package_length(GArray *package, unsigned min_bytes);
> void build_package(GArray *package, uint8_t op, unsigned min_bytes);
>
Acked-by: Marcel Apfelbaum <marcel@redhat.com>
^ permalink raw reply [flat|nested] 13+ messages in thread
* [Qemu-devel] [PATCH v5 4/5] acpi: drop min-bytes in build_package()
2015-01-21 9:09 [Qemu-devel] [PATCH v5 0/5] pc: acpi: various fixes and cleanups Igor Mammedov
` (2 preceding siblings ...)
2015-01-21 9:09 ` [Qemu-devel] [PATCH v5 3/5] acpi: add build_append_namestring() helper Igor Mammedov
@ 2015-01-21 9:09 ` Igor Mammedov
2015-01-21 9:25 ` Marcel Apfelbaum
2015-01-21 9:09 ` [Qemu-devel] [PATCH v5 5/5] pc: acpi-build: simplify PCI bus tree generation Igor Mammedov
4 siblings, 1 reply; 13+ messages in thread
From: Igor Mammedov @ 2015-01-21 9:09 UTC (permalink / raw)
To: qemu-devel; +Cc: marcel.a, mst
Signed-off-by: Igor Mammedov <imammedo@redhat.com>
Reviewed-by: Claudio Fontana <claudio.fontana@huawei.com>
---
hw/acpi/acpi-build-utils.c | 14 ++++----------
hw/i386/acpi-build.c | 13 ++++++-------
include/hw/acpi/acpi-build-utils.h | 4 ++--
3 files changed, 12 insertions(+), 19 deletions(-)
diff --git a/hw/acpi/acpi-build-utils.c b/hw/acpi/acpi-build-utils.c
index aed9066..602e68c 100644
--- a/hw/acpi/acpi-build-utils.c
+++ b/hw/acpi/acpi-build-utils.c
@@ -167,7 +167,7 @@ enum {
PACKAGE_LENGTH_4BYTE_SHIFT = 20,
};
-void build_prepend_package_length(GArray *package, unsigned min_bytes)
+void build_prepend_package_length(GArray *package)
{
uint8_t byte;
unsigned length = package->len;
@@ -183,11 +183,6 @@ void build_prepend_package_length(GArray *package, unsigned min_bytes)
length_bytes = 4;
}
- /* Force length to at least min_bytes.
- * This wastes memory but that's how bios did it.
- */
- length_bytes = MAX(length_bytes, min_bytes);
-
/* PkgLength is the length of the inclusive length of the data. */
length += length_bytes;
@@ -220,15 +215,15 @@ void build_prepend_package_length(GArray *package, unsigned min_bytes)
build_prepend_byte(package, byte);
}
-void build_package(GArray *package, uint8_t op, unsigned min_bytes)
+void build_package(GArray *package, uint8_t op)
{
- build_prepend_package_length(package, min_bytes);
+ build_prepend_package_length(package);
build_prepend_byte(package, op);
}
void build_extop_package(GArray *package, uint8_t op)
{
- build_package(package, op, 1);
+ build_package(package, op);
build_prepend_byte(package, 0x5B); /* ExtOpPrefix */
}
@@ -272,4 +267,3 @@ void build_append_int(GArray *table, uint32_t value)
build_append_value(table, value, 4);
}
}
-
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index e3ac1a14..a010f3b 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -293,7 +293,7 @@ static void build_append_and_cleanup_method(GArray *device, GArray *method)
{
uint8_t op = 0x14; /* MethodOp */
- build_package(method, op, 0);
+ build_package(method, op);
build_append_array(device, method);
build_free_array(method);
@@ -314,7 +314,7 @@ static void build_append_notify_target_ifequal(GArray *method,
build_append_byte(notify, 0x69); /* Arg1Op */
/* Pack it up */
- build_package(notify, op, 1);
+ build_package(notify, op);
build_append_array(method, notify);
@@ -827,7 +827,7 @@ static void build_pci_bus_end(PCIBus *bus, void *bus_state)
build_append_byte(notify, 0x69); /* Arg1Op */
/* Pack it up */
- build_package(notify, op, 0);
+ build_package(notify, op);
build_append_array(method, notify);
@@ -868,7 +868,7 @@ static void build_pci_bus_end(PCIBus *bus, void *bus_state)
if (bus->parent_dev) {
build_extop_package(bus_table, op);
} else {
- build_package(bus_table, op, 0);
+ build_package(bus_table, op);
}
/* Append our bus description to parent table */
@@ -1008,7 +1008,7 @@ build_ssdt(GArray *table_data, GArray *linker,
build_append_byte(package, b);
}
- build_package(package, op, 2);
+ build_package(package, op);
build_append_array(sb_scope, package);
build_free_array(package);
}
@@ -1056,8 +1056,7 @@ build_ssdt(GArray *table_data, GArray *linker,
build_append_array(sb_scope, hotplug_state.device_table);
build_pci_bus_state_cleanup(&hotplug_state);
}
-
- build_package(sb_scope, op, 3);
+ build_package(sb_scope, op);
build_append_array(table_data, sb_scope);
build_free_array(sb_scope);
}
diff --git a/include/hw/acpi/acpi-build-utils.h b/include/hw/acpi/acpi-build-utils.h
index fd50625..199f003 100644
--- a/include/hw/acpi/acpi-build-utils.h
+++ b/include/hw/acpi/acpi-build-utils.h
@@ -14,8 +14,8 @@ void build_append_array(GArray *array, GArray *val);
void GCC_FMT_ATTR(2, 3)
build_append_namestring(GArray *array, const char *format, ...);
-void build_prepend_package_length(GArray *package, unsigned min_bytes);
-void build_package(GArray *package, uint8_t op, unsigned min_bytes);
+void build_prepend_package_length(GArray *package);
+void build_package(GArray *package, uint8_t op);
void build_append_value(GArray *table, uint32_t value, int size);
void build_append_int(GArray *table, uint32_t value);
void build_extop_package(GArray *package, uint8_t op);
--
1.8.3.1
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [Qemu-devel] [PATCH v5 4/5] acpi: drop min-bytes in build_package()
2015-01-21 9:09 ` [Qemu-devel] [PATCH v5 4/5] acpi: drop min-bytes in build_package() Igor Mammedov
@ 2015-01-21 9:25 ` Marcel Apfelbaum
0 siblings, 0 replies; 13+ messages in thread
From: Marcel Apfelbaum @ 2015-01-21 9:25 UTC (permalink / raw)
To: Igor Mammedov, qemu-devel; +Cc: marcel.a, mst
On 01/21/2015 11:09 AM, Igor Mammedov wrote:
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> Reviewed-by: Claudio Fontana <claudio.fontana@huawei.com>
> ---
> hw/acpi/acpi-build-utils.c | 14 ++++----------
> hw/i386/acpi-build.c | 13 ++++++-------
> include/hw/acpi/acpi-build-utils.h | 4 ++--
> 3 files changed, 12 insertions(+), 19 deletions(-)
>
> diff --git a/hw/acpi/acpi-build-utils.c b/hw/acpi/acpi-build-utils.c
> index aed9066..602e68c 100644
> --- a/hw/acpi/acpi-build-utils.c
> +++ b/hw/acpi/acpi-build-utils.c
> @@ -167,7 +167,7 @@ enum {
> PACKAGE_LENGTH_4BYTE_SHIFT = 20,
> };
>
> -void build_prepend_package_length(GArray *package, unsigned min_bytes)
> +void build_prepend_package_length(GArray *package)
> {
> uint8_t byte;
> unsigned length = package->len;
> @@ -183,11 +183,6 @@ void build_prepend_package_length(GArray *package, unsigned min_bytes)
> length_bytes = 4;
> }
>
> - /* Force length to at least min_bytes.
> - * This wastes memory but that's how bios did it.
> - */
> - length_bytes = MAX(length_bytes, min_bytes);
> -
> /* PkgLength is the length of the inclusive length of the data. */
> length += length_bytes;
>
> @@ -220,15 +215,15 @@ void build_prepend_package_length(GArray *package, unsigned min_bytes)
> build_prepend_byte(package, byte);
> }
>
> -void build_package(GArray *package, uint8_t op, unsigned min_bytes)
> +void build_package(GArray *package, uint8_t op)
> {
> - build_prepend_package_length(package, min_bytes);
> + build_prepend_package_length(package);
> build_prepend_byte(package, op);
> }
>
> void build_extop_package(GArray *package, uint8_t op)
> {
> - build_package(package, op, 1);
> + build_package(package, op);
> build_prepend_byte(package, 0x5B); /* ExtOpPrefix */
> }
>
> @@ -272,4 +267,3 @@ void build_append_int(GArray *table, uint32_t value)
> build_append_value(table, value, 4);
> }
> }
> -
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index e3ac1a14..a010f3b 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -293,7 +293,7 @@ static void build_append_and_cleanup_method(GArray *device, GArray *method)
> {
> uint8_t op = 0x14; /* MethodOp */
>
> - build_package(method, op, 0);
> + build_package(method, op);
>
> build_append_array(device, method);
> build_free_array(method);
> @@ -314,7 +314,7 @@ static void build_append_notify_target_ifequal(GArray *method,
> build_append_byte(notify, 0x69); /* Arg1Op */
>
> /* Pack it up */
> - build_package(notify, op, 1);
> + build_package(notify, op);
>
> build_append_array(method, notify);
>
> @@ -827,7 +827,7 @@ static void build_pci_bus_end(PCIBus *bus, void *bus_state)
> build_append_byte(notify, 0x69); /* Arg1Op */
>
> /* Pack it up */
> - build_package(notify, op, 0);
> + build_package(notify, op);
>
> build_append_array(method, notify);
>
> @@ -868,7 +868,7 @@ static void build_pci_bus_end(PCIBus *bus, void *bus_state)
> if (bus->parent_dev) {
> build_extop_package(bus_table, op);
> } else {
> - build_package(bus_table, op, 0);
> + build_package(bus_table, op);
> }
>
> /* Append our bus description to parent table */
> @@ -1008,7 +1008,7 @@ build_ssdt(GArray *table_data, GArray *linker,
> build_append_byte(package, b);
> }
>
> - build_package(package, op, 2);
> + build_package(package, op);
> build_append_array(sb_scope, package);
> build_free_array(package);
> }
> @@ -1056,8 +1056,7 @@ build_ssdt(GArray *table_data, GArray *linker,
> build_append_array(sb_scope, hotplug_state.device_table);
> build_pci_bus_state_cleanup(&hotplug_state);
> }
> -
> - build_package(sb_scope, op, 3);
> + build_package(sb_scope, op);
> build_append_array(table_data, sb_scope);
> build_free_array(sb_scope);
> }
> diff --git a/include/hw/acpi/acpi-build-utils.h b/include/hw/acpi/acpi-build-utils.h
> index fd50625..199f003 100644
> --- a/include/hw/acpi/acpi-build-utils.h
> +++ b/include/hw/acpi/acpi-build-utils.h
> @@ -14,8 +14,8 @@ void build_append_array(GArray *array, GArray *val);
> void GCC_FMT_ATTR(2, 3)
> build_append_namestring(GArray *array, const char *format, ...);
>
> -void build_prepend_package_length(GArray *package, unsigned min_bytes);
> -void build_package(GArray *package, uint8_t op, unsigned min_bytes);
> +void build_prepend_package_length(GArray *package);
> +void build_package(GArray *package, uint8_t op);
> void build_append_value(GArray *table, uint32_t value, int size);
> void build_append_int(GArray *table, uint32_t value);
> void build_extop_package(GArray *package, uint8_t op);
>
Reviewed-by: Marcel Apfelbaum <marcel@redhat.com>
^ permalink raw reply [flat|nested] 13+ messages in thread
* [Qemu-devel] [PATCH v5 5/5] pc: acpi-build: simplify PCI bus tree generation
2015-01-21 9:09 [Qemu-devel] [PATCH v5 0/5] pc: acpi: various fixes and cleanups Igor Mammedov
` (3 preceding siblings ...)
2015-01-21 9:09 ` [Qemu-devel] [PATCH v5 4/5] acpi: drop min-bytes in build_package() Igor Mammedov
@ 2015-01-21 9:09 ` Igor Mammedov
2015-01-21 9:51 ` Marcel Apfelbaum
4 siblings, 1 reply; 13+ messages in thread
From: Igor Mammedov @ 2015-01-21 9:09 UTC (permalink / raw)
To: qemu-devel; +Cc: marcel.a, mst
it basicaly does the same as original approach,
* just without bus/notify tables tracking (less obscure)
which is easier to follow.
* drops unnecessary loops and bitmaps,
creating devices and notification method in the same loop.
* saves us ~100LOC
Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
v4:
* keep original logic of creating bridge devices as it was done
in 133a2da48 "pc: acpi: generate AML only for PCI0 ..."
* if bus is non hotpluggable, add child slots to bus as non
hotpluggable as it was done in original code.
v3:
* use hotpluggable device object instead of not hotpluggable
for non present devices, and add it only when bus itself is
hotpluggable
---
hw/i386/acpi-build.c | 275 +++++++++++++++++----------------------------------
1 file changed, 90 insertions(+), 185 deletions(-)
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index a010f3b..5255428 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -106,7 +106,6 @@ typedef struct AcpiPmInfo {
typedef struct AcpiMiscInfo {
bool has_hpet;
bool has_tpm;
- DECLARE_BITMAP(slot_hotplug_enable, PCI_SLOT_MAX);
const unsigned char *dsdt_code;
unsigned dsdt_size;
uint16_t pvpanic_port;
@@ -651,69 +650,37 @@ static void acpi_set_pci_info(void)
}
}
-static void build_pci_bus_state_init(AcpiBuildPciBusHotplugState *state,
- AcpiBuildPciBusHotplugState *parent,
- bool pcihp_bridge_en)
+static void build_append_pcihp_notify_entry(GArray *method, int slot)
{
- state->parent = parent;
- state->device_table = build_alloc_array();
- state->notify_table = build_alloc_array();
- state->pcihp_bridge_en = pcihp_bridge_en;
-}
-
-static void build_pci_bus_state_cleanup(AcpiBuildPciBusHotplugState *state)
-{
- build_free_array(state->device_table);
- build_free_array(state->notify_table);
-}
+ GArray *ifctx;
-static void *build_pci_bus_begin(PCIBus *bus, void *parent_state)
-{
- AcpiBuildPciBusHotplugState *parent = parent_state;
- AcpiBuildPciBusHotplugState *child = g_malloc(sizeof *child);
-
- build_pci_bus_state_init(child, parent, parent->pcihp_bridge_en);
+ ifctx = build_alloc_array();
+ build_append_byte(ifctx, 0x7B); /* AndOp */
+ build_append_byte(ifctx, 0x68); /* Arg0Op */
+ build_append_int(ifctx, 0x1U << slot);
+ build_append_byte(ifctx, 0x00); /* NullName */
+ build_append_byte(ifctx, 0x86); /* NotifyOp */
+ build_append_namestring(ifctx, "S%.02X", PCI_DEVFN(slot, 0));
+ build_append_byte(ifctx, 0x69); /* Arg1Op */
- return child;
+ /* Pack it up */
+ build_package(ifctx, 0xA0 /* IfOp */);
+ build_append_array(method, ifctx);
+ build_free_array(ifctx);
}
-static void build_pci_bus_end(PCIBus *bus, void *bus_state)
+static void build_append_pci_bus_devices(GArray *parent_scope, PCIBus *bus,
+ bool pcihp_bridge_en)
{
- AcpiBuildPciBusHotplugState *child = bus_state;
- AcpiBuildPciBusHotplugState *parent = child->parent;
GArray *bus_table = build_alloc_array();
- DECLARE_BITMAP(slot_hotplug_enable, PCI_SLOT_MAX);
- DECLARE_BITMAP(slot_device_present, PCI_SLOT_MAX);
- DECLARE_BITMAP(slot_device_system, PCI_SLOT_MAX);
- DECLARE_BITMAP(slot_device_vga, PCI_SLOT_MAX);
- DECLARE_BITMAP(slot_device_qxl, PCI_SLOT_MAX);
- uint8_t op;
- int i;
+ GArray *method = NULL;
QObject *bsel;
- GArray *method;
- bool bus_hotplug_support = false;
-
- /*
- * Skip bridge subtree creation if bridge hotplug is disabled
- * to make acpi tables compatible with legacy machine types.
- */
- if (!child->pcihp_bridge_en && bus->parent_dev) {
- return;
- }
+ PCIBus *sec;
+ int i;
if (bus->parent_dev) {
- op = 0x82; /* DeviceOp */
- build_append_namestring(bus_table, "S%.02X",
- bus->parent_dev->devfn);
- build_append_byte(bus_table, 0x08); /* NameOp */
- build_append_namestring(bus_table, "_SUN");
- build_append_value(bus_table, PCI_SLOT(bus->parent_dev->devfn), 1);
- build_append_byte(bus_table, 0x08); /* NameOp */
- build_append_namestring(bus_table, "_ADR");
- build_append_value(bus_table, (PCI_SLOT(bus->parent_dev->devfn) << 16) |
- PCI_FUNC(bus->parent_dev->devfn), 4);
+ build_append_namestring(bus_table, "S%.02X_", bus->parent_dev->devfn);
} else {
- op = 0x10; /* ScopeOp */;
build_append_namestring(bus_table, "PCI0");
}
@@ -722,17 +689,9 @@ static void build_pci_bus_end(PCIBus *bus, void *bus_state)
build_append_byte(bus_table, 0x08); /* NameOp */
build_append_namestring(bus_table, "BSEL");
build_append_int(bus_table, qint_get_int(qobject_to_qint(bsel)));
- memset(slot_hotplug_enable, 0xff, sizeof slot_hotplug_enable);
- } else {
- /* No bsel - no slots are hot-pluggable */
- memset(slot_hotplug_enable, 0x00, sizeof slot_hotplug_enable);
+ method = build_alloc_method("DVNT", 2);
}
- memset(slot_device_present, 0x00, sizeof slot_device_present);
- memset(slot_device_system, 0x00, sizeof slot_device_present);
- memset(slot_device_vga, 0x00, sizeof slot_device_vga);
- memset(slot_device_qxl, 0x00, sizeof slot_device_qxl);
-
for (i = 0; i < ARRAY_SIZE(bus->devices); i += PCI_FUNC_MAX) {
DeviceClass *dc;
PCIDeviceClass *pc;
@@ -741,152 +700,104 @@ static void build_pci_bus_end(PCIBus *bus, void *bus_state)
bool bridge_in_acpi;
if (!pdev) {
+ if (bsel) {
+ void *pcihp = acpi_data_push(bus_table, ACPI_PCIHP_SIZEOF);
+ memcpy(pcihp, ACPI_PCIHP_AML, ACPI_PCIHP_SIZEOF);
+ patch_pcihp(slot, pcihp);
+
+ build_append_pcihp_notify_entry(method, slot);
+ } else {
+ /* no much sense to create empty non hotpluggable slots,
+ * but it's what original code did before. TODO: remove it.
+ */
+ void *pcihp = acpi_data_push(bus_table, ACPI_PCINOHP_SIZEOF);
+ memcpy(pcihp, ACPI_PCINOHP_AML, ACPI_PCINOHP_SIZEOF);
+ patch_pcinohp(slot, pcihp);
+ }
continue;
}
- set_bit(slot, slot_device_present);
pc = PCI_DEVICE_GET_CLASS(pdev);
dc = DEVICE_GET_CLASS(pdev);
- /* When hotplug for bridges is enabled, bridges are
- * described in ACPI separately (see build_pci_bus_end).
- * In this case they aren't themselves hot-pluggable.
- */
- bridge_in_acpi = pc->is_bridge && child->pcihp_bridge_en;
-
- if (pc->class_id == PCI_CLASS_BRIDGE_ISA || bridge_in_acpi) {
- set_bit(slot, slot_device_system);
+ if (pc->class_id == PCI_CLASS_BRIDGE_ISA) {
+ continue;
}
+ bridge_in_acpi = pc->is_bridge && pcihp_bridge_en;
if (pc->class_id == PCI_CLASS_DISPLAY_VGA) {
- set_bit(slot, slot_device_vga);
if (object_dynamic_cast(OBJECT(pdev), "qxl-vga")) {
- set_bit(slot, slot_device_qxl);
+ void *pcihp = acpi_data_push(bus_table,
+ ACPI_PCIQXL_SIZEOF);
+ memcpy(pcihp, ACPI_PCIQXL_AML, ACPI_PCIQXL_SIZEOF);
+ patch_pciqxl(slot, pcihp);
+ } else {
+ void *pcihp = acpi_data_push(bus_table,
+ ACPI_PCIVGA_SIZEOF);
+ memcpy(pcihp, ACPI_PCIVGA_AML, ACPI_PCIVGA_SIZEOF);
+ patch_pcivga(slot, pcihp);
}
- }
-
- if (!dc->hotpluggable || bridge_in_acpi) {
- clear_bit(slot, slot_hotplug_enable);
- }
- }
-
- /* Append Device object for each slot */
- for (i = 0; i < PCI_SLOT_MAX; i++) {
- bool can_eject = test_bit(i, slot_hotplug_enable);
- bool present = test_bit(i, slot_device_present);
- bool vga = test_bit(i, slot_device_vga);
- bool qxl = test_bit(i, slot_device_qxl);
- bool system = test_bit(i, slot_device_system);
- if (can_eject) {
- void *pcihp = acpi_data_push(bus_table,
- ACPI_PCIHP_SIZEOF);
+ } else if (dc->hotpluggable && !bridge_in_acpi) {
+ void *pcihp = acpi_data_push(bus_table, ACPI_PCIHP_SIZEOF);
memcpy(pcihp, ACPI_PCIHP_AML, ACPI_PCIHP_SIZEOF);
- patch_pcihp(i, pcihp);
- bus_hotplug_support = true;
- } else if (qxl) {
- void *pcihp = acpi_data_push(bus_table,
- ACPI_PCIQXL_SIZEOF);
- memcpy(pcihp, ACPI_PCIQXL_AML, ACPI_PCIQXL_SIZEOF);
- patch_pciqxl(i, pcihp);
- } else if (vga) {
- void *pcihp = acpi_data_push(bus_table,
- ACPI_PCIVGA_SIZEOF);
- memcpy(pcihp, ACPI_PCIVGA_AML, ACPI_PCIVGA_SIZEOF);
- patch_pcivga(i, pcihp);
- } else if (system) {
- /* Nothing to do: system devices are in DSDT or in SSDT above. */
- } else if (present) {
- void *pcihp = acpi_data_push(bus_table,
- ACPI_PCINOHP_SIZEOF);
- memcpy(pcihp, ACPI_PCINOHP_AML, ACPI_PCINOHP_SIZEOF);
- patch_pcinohp(i, pcihp);
- }
- }
-
- if (bsel) {
- method = build_alloc_method("DVNT", 2);
-
- for (i = 0; i < PCI_SLOT_MAX; i++) {
- GArray *notify;
- uint8_t op;
+ patch_pcihp(slot, pcihp);
- if (!test_bit(i, slot_hotplug_enable)) {
- continue;
+ if (bsel) {
+ build_append_pcihp_notify_entry(method, slot);
}
+ } else {
+ void *pcihp = acpi_data_push(bus_table, ACPI_PCINOHP_SIZEOF);
+ memcpy(pcihp, ACPI_PCINOHP_AML, ACPI_PCINOHP_SIZEOF);
+ patch_pcinohp(slot, pcihp);
- notify = build_alloc_array();
- op = 0xA0; /* IfOp */
-
- build_append_byte(notify, 0x7B); /* AndOp */
- build_append_byte(notify, 0x68); /* Arg0Op */
- build_append_int(notify, 0x1U << i);
- build_append_byte(notify, 0x00); /* NullName */
- build_append_byte(notify, 0x86); /* NotifyOp */
- build_append_namestring(notify, "S%.02X", PCI_DEVFN(i, 0));
- build_append_byte(notify, 0x69); /* Arg1Op */
-
- /* Pack it up */
- build_package(notify, op);
-
- build_append_array(method, notify);
+ /* When hotplug for bridges is enabled, bridges that are
+ * described in ACPI separately aren't themselves hot-pluggable.
+ */
+ if (bridge_in_acpi) {
+ PCIBus *sec_bus = pci_bridge_get_sec_bus(PCI_BRIDGE(pdev));
- build_free_array(notify);
+ build_append_pci_bus_devices(bus_table, sec_bus,
+ pcihp_bridge_en);
+ }
}
+ }
+ if (bsel) {
build_append_and_cleanup_method(bus_table, method);
}
/* Append PCNT method to notify about events on local and child buses.
* Add unconditionally for root since DSDT expects it.
*/
- if (bus_hotplug_support || child->notify_table->len || !bus->parent_dev) {
- method = build_alloc_method("PCNT", 0);
-
- /* If bus supports hotplug select it and notify about local events */
- if (bsel) {
- build_append_byte(method, 0x70); /* StoreOp */
- build_append_int(method, qint_get_int(qobject_to_qint(bsel)));
- build_append_namestring(method, "BNUM");
- build_append_namestring(method, "DVNT");
- build_append_namestring(method, "PCIU");
- build_append_int(method, 1); /* Device Check */
- build_append_namestring(method, "DVNT");
- build_append_namestring(method, "PCID");
- build_append_int(method, 3); /* Eject Request */
- }
-
- /* Notify about child bus events in any case */
- build_append_array(method, child->notify_table);
-
- build_append_and_cleanup_method(bus_table, method);
-
- /* Append description of child buses */
- build_append_array(bus_table, child->device_table);
+ method = build_alloc_method("PCNT", 0);
- /* Pack it up */
- if (bus->parent_dev) {
- build_extop_package(bus_table, op);
- } else {
- build_package(bus_table, op);
- }
-
- /* Append our bus description to parent table */
- build_append_array(parent->device_table, bus_table);
+ /* If bus supports hotplug select it and notify about local events */
+ if (bsel) {
+ build_append_byte(method, 0x70); /* StoreOp */
+ build_append_int(method, qint_get_int(qobject_to_qint(bsel)));
+ build_append_namestring(method, "BNUM");
+ build_append_namestring(method, "DVNT");
+ build_append_namestring(method, "PCIU");
+ build_append_int(method, 1); /* Device Check */
+ build_append_namestring(method, "DVNT");
+ build_append_namestring(method, "PCID");
+ build_append_int(method, 3); /* Eject Request */
+ }
- /* Also tell parent how to notify us, invoking PCNT method.
- * At the moment this is not needed for root as we have a single root.
- */
- if (bus->parent_dev) {
- build_append_namestring(parent->notify_table, "^PCNT.S%.02X",
- bus->parent_dev->devfn);
+ /* Notify about child bus events in any case */
+ if (pcihp_bridge_en) {
+ QLIST_FOREACH(sec, &bus->child, sibling) {
+ build_append_namestring(method, "^S%.02X.PCNT",
+ sec->parent_dev->devfn);
}
}
- qobject_decref(bsel);
+ build_append_and_cleanup_method(bus_table, method);
+
+ build_package(bus_table, 0x10); /* ScopeOp */
+ build_append_array(parent_scope, bus_table);
build_free_array(bus_table);
- build_pci_bus_state_cleanup(child);
- g_free(child);
}
static void patch_pci_windows(PcPciInfo *pci, uint8_t *start, unsigned size)
@@ -1035,7 +946,6 @@ build_ssdt(GArray *table_data, GArray *linker,
}
{
- AcpiBuildPciBusHotplugState hotplug_state;
Object *pci_host;
PCIBus *bus = NULL;
bool ambiguous;
@@ -1045,16 +955,11 @@ build_ssdt(GArray *table_data, GArray *linker,
bus = PCI_HOST_BRIDGE(pci_host)->bus;
}
- build_pci_bus_state_init(&hotplug_state, NULL, pm->pcihp_bridge_en);
-
if (bus) {
/* Scan all PCI buses. Generate tables to support hotplug. */
- pci_for_each_bus_depth_first(bus, build_pci_bus_begin,
- build_pci_bus_end, &hotplug_state);
+ build_append_pci_bus_devices(sb_scope, bus,
+ pm->pcihp_bridge_en);
}
-
- build_append_array(sb_scope, hotplug_state.device_table);
- build_pci_bus_state_cleanup(&hotplug_state);
}
build_package(sb_scope, op);
build_append_array(table_data, sb_scope);
--
1.8.3.1
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [Qemu-devel] [PATCH v5 5/5] pc: acpi-build: simplify PCI bus tree generation
2015-01-21 9:09 ` [Qemu-devel] [PATCH v5 5/5] pc: acpi-build: simplify PCI bus tree generation Igor Mammedov
@ 2015-01-21 9:51 ` Marcel Apfelbaum
0 siblings, 0 replies; 13+ messages in thread
From: Marcel Apfelbaum @ 2015-01-21 9:51 UTC (permalink / raw)
To: Igor Mammedov, qemu-devel; +Cc: marcel.a, mst
On 01/21/2015 11:09 AM, Igor Mammedov wrote:
> it basicaly does the same as original approach,
> * just without bus/notify tables tracking (less obscure)
> which is easier to follow.
> * drops unnecessary loops and bitmaps,
> creating devices and notification method in the same loop.
> * saves us ~100LOC
>
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
> v4:
> * keep original logic of creating bridge devices as it was done
> in 133a2da48 "pc: acpi: generate AML only for PCI0 ..."
> * if bus is non hotpluggable, add child slots to bus as non
> hotpluggable as it was done in original code.
> v3:
> * use hotpluggable device object instead of not hotpluggable
> for non present devices, and add it only when bus itself is
> hotpluggable
> ---
> hw/i386/acpi-build.c | 275 +++++++++++++++++----------------------------------
> 1 file changed, 90 insertions(+), 185 deletions(-)
>
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index a010f3b..5255428 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -106,7 +106,6 @@ typedef struct AcpiPmInfo {
> typedef struct AcpiMiscInfo {
> bool has_hpet;
> bool has_tpm;
> - DECLARE_BITMAP(slot_hotplug_enable, PCI_SLOT_MAX);
> const unsigned char *dsdt_code;
> unsigned dsdt_size;
> uint16_t pvpanic_port;
> @@ -651,69 +650,37 @@ static void acpi_set_pci_info(void)
> }
> }
>
> -static void build_pci_bus_state_init(AcpiBuildPciBusHotplugState *state,
> - AcpiBuildPciBusHotplugState *parent,
> - bool pcihp_bridge_en)
> +static void build_append_pcihp_notify_entry(GArray *method, int slot)
> {
> - state->parent = parent;
> - state->device_table = build_alloc_array();
> - state->notify_table = build_alloc_array();
> - state->pcihp_bridge_en = pcihp_bridge_en;
> -}
> -
> -static void build_pci_bus_state_cleanup(AcpiBuildPciBusHotplugState *state)
> -{
> - build_free_array(state->device_table);
> - build_free_array(state->notify_table);
> -}
> + GArray *ifctx;
>
> -static void *build_pci_bus_begin(PCIBus *bus, void *parent_state)
> -{
> - AcpiBuildPciBusHotplugState *parent = parent_state;
> - AcpiBuildPciBusHotplugState *child = g_malloc(sizeof *child);
> -
> - build_pci_bus_state_init(child, parent, parent->pcihp_bridge_en);
> + ifctx = build_alloc_array();
> + build_append_byte(ifctx, 0x7B); /* AndOp */
> + build_append_byte(ifctx, 0x68); /* Arg0Op */
> + build_append_int(ifctx, 0x1U << slot);
> + build_append_byte(ifctx, 0x00); /* NullName */
> + build_append_byte(ifctx, 0x86); /* NotifyOp */
> + build_append_namestring(ifctx, "S%.02X", PCI_DEVFN(slot, 0));
> + build_append_byte(ifctx, 0x69); /* Arg1Op */
>
> - return child;
> + /* Pack it up */
> + build_package(ifctx, 0xA0 /* IfOp */);
> + build_append_array(method, ifctx);
> + build_free_array(ifctx);
> }
>
> -static void build_pci_bus_end(PCIBus *bus, void *bus_state)
> +static void build_append_pci_bus_devices(GArray *parent_scope, PCIBus *bus,
> + bool pcihp_bridge_en)
> {
> - AcpiBuildPciBusHotplugState *child = bus_state;
> - AcpiBuildPciBusHotplugState *parent = child->parent;
> GArray *bus_table = build_alloc_array();
> - DECLARE_BITMAP(slot_hotplug_enable, PCI_SLOT_MAX);
> - DECLARE_BITMAP(slot_device_present, PCI_SLOT_MAX);
> - DECLARE_BITMAP(slot_device_system, PCI_SLOT_MAX);
> - DECLARE_BITMAP(slot_device_vga, PCI_SLOT_MAX);
> - DECLARE_BITMAP(slot_device_qxl, PCI_SLOT_MAX);
> - uint8_t op;
> - int i;
> + GArray *method = NULL;
> QObject *bsel;
> - GArray *method;
> - bool bus_hotplug_support = false;
> -
> - /*
> - * Skip bridge subtree creation if bridge hotplug is disabled
> - * to make acpi tables compatible with legacy machine types.
> - */
> - if (!child->pcihp_bridge_en && bus->parent_dev) {
> - return;
> - }
> + PCIBus *sec;
> + int i;
>
> if (bus->parent_dev) {
> - op = 0x82; /* DeviceOp */
> - build_append_namestring(bus_table, "S%.02X",
> - bus->parent_dev->devfn);
> - build_append_byte(bus_table, 0x08); /* NameOp */
> - build_append_namestring(bus_table, "_SUN");
> - build_append_value(bus_table, PCI_SLOT(bus->parent_dev->devfn), 1);
> - build_append_byte(bus_table, 0x08); /* NameOp */
> - build_append_namestring(bus_table, "_ADR");
> - build_append_value(bus_table, (PCI_SLOT(bus->parent_dev->devfn) << 16) |
> - PCI_FUNC(bus->parent_dev->devfn), 4);
> + build_append_namestring(bus_table, "S%.02X_", bus->parent_dev->devfn);
> } else {
> - op = 0x10; /* ScopeOp */;
> build_append_namestring(bus_table, "PCI0");
> }
>
> @@ -722,17 +689,9 @@ static void build_pci_bus_end(PCIBus *bus, void *bus_state)
> build_append_byte(bus_table, 0x08); /* NameOp */
> build_append_namestring(bus_table, "BSEL");
> build_append_int(bus_table, qint_get_int(qobject_to_qint(bsel)));
> - memset(slot_hotplug_enable, 0xff, sizeof slot_hotplug_enable);
> - } else {
> - /* No bsel - no slots are hot-pluggable */
> - memset(slot_hotplug_enable, 0x00, sizeof slot_hotplug_enable);
> + method = build_alloc_method("DVNT", 2);
> }
>
> - memset(slot_device_present, 0x00, sizeof slot_device_present);
> - memset(slot_device_system, 0x00, sizeof slot_device_present);
> - memset(slot_device_vga, 0x00, sizeof slot_device_vga);
> - memset(slot_device_qxl, 0x00, sizeof slot_device_qxl);
> -
> for (i = 0; i < ARRAY_SIZE(bus->devices); i += PCI_FUNC_MAX) {
> DeviceClass *dc;
> PCIDeviceClass *pc;
> @@ -741,152 +700,104 @@ static void build_pci_bus_end(PCIBus *bus, void *bus_state)
> bool bridge_in_acpi;
>
> if (!pdev) {
> + if (bsel) {
> + void *pcihp = acpi_data_push(bus_table, ACPI_PCIHP_SIZEOF);
> + memcpy(pcihp, ACPI_PCIHP_AML, ACPI_PCIHP_SIZEOF);
> + patch_pcihp(slot, pcihp);
> +
> + build_append_pcihp_notify_entry(method, slot);
> + } else {
> + /* no much sense to create empty non hotpluggable slots,
> + * but it's what original code did before. TODO: remove it.
> + */
> + void *pcihp = acpi_data_push(bus_table, ACPI_PCINOHP_SIZEOF);
> + memcpy(pcihp, ACPI_PCINOHP_AML, ACPI_PCINOHP_SIZEOF);
> + patch_pcinohp(slot, pcihp);
> + }
> continue;
> }
>
> - set_bit(slot, slot_device_present);
> pc = PCI_DEVICE_GET_CLASS(pdev);
> dc = DEVICE_GET_CLASS(pdev);
>
> - /* When hotplug for bridges is enabled, bridges are
> - * described in ACPI separately (see build_pci_bus_end).
> - * In this case they aren't themselves hot-pluggable.
> - */
> - bridge_in_acpi = pc->is_bridge && child->pcihp_bridge_en;
> -
> - if (pc->class_id == PCI_CLASS_BRIDGE_ISA || bridge_in_acpi) {
> - set_bit(slot, slot_device_system);
> + if (pc->class_id == PCI_CLASS_BRIDGE_ISA) {
> + continue;
> }
> + bridge_in_acpi = pc->is_bridge && pcihp_bridge_en;
>
> if (pc->class_id == PCI_CLASS_DISPLAY_VGA) {
> - set_bit(slot, slot_device_vga);
>
> if (object_dynamic_cast(OBJECT(pdev), "qxl-vga")) {
> - set_bit(slot, slot_device_qxl);
> + void *pcihp = acpi_data_push(bus_table,
> + ACPI_PCIQXL_SIZEOF);
> + memcpy(pcihp, ACPI_PCIQXL_AML, ACPI_PCIQXL_SIZEOF);
> + patch_pciqxl(slot, pcihp);
> + } else {
> + void *pcihp = acpi_data_push(bus_table,
> + ACPI_PCIVGA_SIZEOF);
> + memcpy(pcihp, ACPI_PCIVGA_AML, ACPI_PCIVGA_SIZEOF);
> + patch_pcivga(slot, pcihp);
> }
> - }
> -
> - if (!dc->hotpluggable || bridge_in_acpi) {
> - clear_bit(slot, slot_hotplug_enable);
> - }
> - }
> -
> - /* Append Device object for each slot */
> - for (i = 0; i < PCI_SLOT_MAX; i++) {
> - bool can_eject = test_bit(i, slot_hotplug_enable);
> - bool present = test_bit(i, slot_device_present);
> - bool vga = test_bit(i, slot_device_vga);
> - bool qxl = test_bit(i, slot_device_qxl);
> - bool system = test_bit(i, slot_device_system);
> - if (can_eject) {
> - void *pcihp = acpi_data_push(bus_table,
> - ACPI_PCIHP_SIZEOF);
> + } else if (dc->hotpluggable && !bridge_in_acpi) {
> + void *pcihp = acpi_data_push(bus_table, ACPI_PCIHP_SIZEOF);
> memcpy(pcihp, ACPI_PCIHP_AML, ACPI_PCIHP_SIZEOF);
> - patch_pcihp(i, pcihp);
> - bus_hotplug_support = true;
> - } else if (qxl) {
> - void *pcihp = acpi_data_push(bus_table,
> - ACPI_PCIQXL_SIZEOF);
> - memcpy(pcihp, ACPI_PCIQXL_AML, ACPI_PCIQXL_SIZEOF);
> - patch_pciqxl(i, pcihp);
> - } else if (vga) {
> - void *pcihp = acpi_data_push(bus_table,
> - ACPI_PCIVGA_SIZEOF);
> - memcpy(pcihp, ACPI_PCIVGA_AML, ACPI_PCIVGA_SIZEOF);
> - patch_pcivga(i, pcihp);
> - } else if (system) {
> - /* Nothing to do: system devices are in DSDT or in SSDT above. */
> - } else if (present) {
> - void *pcihp = acpi_data_push(bus_table,
> - ACPI_PCINOHP_SIZEOF);
> - memcpy(pcihp, ACPI_PCINOHP_AML, ACPI_PCINOHP_SIZEOF);
> - patch_pcinohp(i, pcihp);
> - }
> - }
> -
> - if (bsel) {
> - method = build_alloc_method("DVNT", 2);
> -
> - for (i = 0; i < PCI_SLOT_MAX; i++) {
> - GArray *notify;
> - uint8_t op;
> + patch_pcihp(slot, pcihp);
>
> - if (!test_bit(i, slot_hotplug_enable)) {
> - continue;
> + if (bsel) {
> + build_append_pcihp_notify_entry(method, slot);
> }
> + } else {
> + void *pcihp = acpi_data_push(bus_table, ACPI_PCINOHP_SIZEOF);
> + memcpy(pcihp, ACPI_PCINOHP_AML, ACPI_PCINOHP_SIZEOF);
> + patch_pcinohp(slot, pcihp);
>
> - notify = build_alloc_array();
> - op = 0xA0; /* IfOp */
> -
> - build_append_byte(notify, 0x7B); /* AndOp */
> - build_append_byte(notify, 0x68); /* Arg0Op */
> - build_append_int(notify, 0x1U << i);
> - build_append_byte(notify, 0x00); /* NullName */
> - build_append_byte(notify, 0x86); /* NotifyOp */
> - build_append_namestring(notify, "S%.02X", PCI_DEVFN(i, 0));
> - build_append_byte(notify, 0x69); /* Arg1Op */
> -
> - /* Pack it up */
> - build_package(notify, op);
> -
> - build_append_array(method, notify);
> + /* When hotplug for bridges is enabled, bridges that are
> + * described in ACPI separately aren't themselves hot-pluggable.
> + */
> + if (bridge_in_acpi) {
> + PCIBus *sec_bus = pci_bridge_get_sec_bus(PCI_BRIDGE(pdev));
>
> - build_free_array(notify);
> + build_append_pci_bus_devices(bus_table, sec_bus,
> + pcihp_bridge_en);
> + }
> }
> + }
>
> + if (bsel) {
> build_append_and_cleanup_method(bus_table, method);
> }
>
> /* Append PCNT method to notify about events on local and child buses.
> * Add unconditionally for root since DSDT expects it.
> */
> - if (bus_hotplug_support || child->notify_table->len || !bus->parent_dev) {
> - method = build_alloc_method("PCNT", 0);
> -
> - /* If bus supports hotplug select it and notify about local events */
> - if (bsel) {
> - build_append_byte(method, 0x70); /* StoreOp */
> - build_append_int(method, qint_get_int(qobject_to_qint(bsel)));
> - build_append_namestring(method, "BNUM");
> - build_append_namestring(method, "DVNT");
> - build_append_namestring(method, "PCIU");
> - build_append_int(method, 1); /* Device Check */
> - build_append_namestring(method, "DVNT");
> - build_append_namestring(method, "PCID");
> - build_append_int(method, 3); /* Eject Request */
> - }
> -
> - /* Notify about child bus events in any case */
> - build_append_array(method, child->notify_table);
> -
> - build_append_and_cleanup_method(bus_table, method);
> -
> - /* Append description of child buses */
> - build_append_array(bus_table, child->device_table);
> + method = build_alloc_method("PCNT", 0);
>
> - /* Pack it up */
> - if (bus->parent_dev) {
> - build_extop_package(bus_table, op);
> - } else {
> - build_package(bus_table, op);
> - }
> -
> - /* Append our bus description to parent table */
> - build_append_array(parent->device_table, bus_table);
> + /* If bus supports hotplug select it and notify about local events */
> + if (bsel) {
> + build_append_byte(method, 0x70); /* StoreOp */
> + build_append_int(method, qint_get_int(qobject_to_qint(bsel)));
> + build_append_namestring(method, "BNUM");
> + build_append_namestring(method, "DVNT");
> + build_append_namestring(method, "PCIU");
> + build_append_int(method, 1); /* Device Check */
> + build_append_namestring(method, "DVNT");
> + build_append_namestring(method, "PCID");
> + build_append_int(method, 3); /* Eject Request */
> + }
>
> - /* Also tell parent how to notify us, invoking PCNT method.
> - * At the moment this is not needed for root as we have a single root.
> - */
> - if (bus->parent_dev) {
> - build_append_namestring(parent->notify_table, "^PCNT.S%.02X",
> - bus->parent_dev->devfn);
> + /* Notify about child bus events in any case */
> + if (pcihp_bridge_en) {
> + QLIST_FOREACH(sec, &bus->child, sibling) {
> + build_append_namestring(method, "^S%.02X.PCNT",
> + sec->parent_dev->devfn);
> }
> }
>
> - qobject_decref(bsel);
> + build_append_and_cleanup_method(bus_table, method);
> +
> + build_package(bus_table, 0x10); /* ScopeOp */
> + build_append_array(parent_scope, bus_table);
> build_free_array(bus_table);
> - build_pci_bus_state_cleanup(child);
> - g_free(child);
> }
>
> static void patch_pci_windows(PcPciInfo *pci, uint8_t *start, unsigned size)
> @@ -1035,7 +946,6 @@ build_ssdt(GArray *table_data, GArray *linker,
> }
>
> {
> - AcpiBuildPciBusHotplugState hotplug_state;
> Object *pci_host;
> PCIBus *bus = NULL;
> bool ambiguous;
> @@ -1045,16 +955,11 @@ build_ssdt(GArray *table_data, GArray *linker,
> bus = PCI_HOST_BRIDGE(pci_host)->bus;
> }
>
> - build_pci_bus_state_init(&hotplug_state, NULL, pm->pcihp_bridge_en);
> -
> if (bus) {
> /* Scan all PCI buses. Generate tables to support hotplug. */
> - pci_for_each_bus_depth_first(bus, build_pci_bus_begin,
> - build_pci_bus_end, &hotplug_state);
> + build_append_pci_bus_devices(sb_scope, bus,
> + pm->pcihp_bridge_en);
> }
> -
> - build_append_array(sb_scope, hotplug_state.device_table);
> - build_pci_bus_state_cleanup(&hotplug_state);
> }
> build_package(sb_scope, op);
> build_append_array(table_data, sb_scope);
>
This one is tricky to review...
I do agree with the concept, I like the simplification,
and no obvious errors were found.
So ... "light":
Reviewed-by: Marcel Apfelbaum <marcel@redhat.com>
^ permalink raw reply [flat|nested] 13+ messages in thread