* [Qemu-devel] [PATCH v3 1/8] pc: acpi: fix WindowsXP BSOD when memory hotplug is enabled
2014-12-19 11:46 [Qemu-devel] [PATCH v3 0/8] pc: acpi: various fixes and cleanups Igor Mammedov
@ 2014-12-19 11:46 ` Igor Mammedov
2014-12-19 11:46 ` [Qemu-devel] [PATCH v3 2/8] pc: acpi: decribe bridge device as not hotpluggable Igor Mammedov
` (6 subsequent siblings)
7 siblings, 0 replies; 22+ messages in thread
From: Igor Mammedov @ 2014-12-19 11:46 UTC (permalink / raw)
To: qemu-devel; +Cc: mst, claudio.fontana, marcel.a
ACPI parser in XP considers PNP0A06 devices of CPU and
memory hotplug as duplicates. Adding unique _UID
to CPU hotplug device fixes BSOD.
Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
hw/i386/acpi-dsdt-cpu-hotplug.dsl | 1 +
1 file changed, 1 insertion(+)
diff --git a/hw/i386/acpi-dsdt-cpu-hotplug.dsl b/hw/i386/acpi-dsdt-cpu-hotplug.dsl
index 34aab5a..268d870 100644
--- a/hw/i386/acpi-dsdt-cpu-hotplug.dsl
+++ b/hw/i386/acpi-dsdt-cpu-hotplug.dsl
@@ -94,6 +94,7 @@ Scope(\_SB) {
Device(CPU_HOTPLUG_RESOURCE_DEVICE) {
Name(_HID, EisaId("PNP0A06"))
+ Name(_UID, "CPU hotplug resources")
Name(_CRS, ResourceTemplate() {
IO(Decode16, CPU_STATUS_BASE, CPU_STATUS_BASE, 0, CPU_STATUS_LEN)
--
1.8.3.1
^ permalink raw reply related [flat|nested] 22+ messages in thread* [Qemu-devel] [PATCH v3 2/8] pc: acpi: decribe bridge device as not hotpluggable
2014-12-19 11:46 [Qemu-devel] [PATCH v3 0/8] pc: acpi: various fixes and cleanups Igor Mammedov
2014-12-19 11:46 ` [Qemu-devel] [PATCH v3 1/8] pc: acpi: fix WindowsXP BSOD when memory hotplug is enabled Igor Mammedov
@ 2014-12-19 11:46 ` Igor Mammedov
2015-01-19 12:46 ` Michael S. Tsirkin
2014-12-19 11:46 ` [Qemu-devel] [PATCH v3 3/8] pc: acpi-build: cleanup AcpiPmInfo initialization Igor Mammedov
` (5 subsequent siblings)
7 siblings, 1 reply; 22+ messages in thread
From: Igor Mammedov @ 2014-12-19 11:46 UTC (permalink / raw)
To: qemu-devel; +Cc: mst, claudio.fontana, marcel.a
when bridge hotplug is disabled, i.e. for machine
types less then 2.0, bridge device was created as
hotpluggable by mistake since commit 133a2da (2.1).
Fix it by just creating it as a present device as
it was done in 1.7.
Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
hw/i386/acpi-build.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index a4d0c0c..c151fde 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -919,7 +919,7 @@ static void build_pci_bus_end(PCIBus *bus, void *bus_state)
}
}
- if (!dc->hotpluggable || bridge_in_acpi) {
+ if (!dc->hotpluggable || pc->is_bridge) {
clear_bit(slot, slot_hotplug_enable);
}
}
--
1.8.3.1
^ permalink raw reply related [flat|nested] 22+ messages in thread* Re: [Qemu-devel] [PATCH v3 2/8] pc: acpi: decribe bridge device as not hotpluggable
2014-12-19 11:46 ` [Qemu-devel] [PATCH v3 2/8] pc: acpi: decribe bridge device as not hotpluggable Igor Mammedov
@ 2015-01-19 12:46 ` Michael S. Tsirkin
2015-01-20 9:22 ` Igor Mammedov
0 siblings, 1 reply; 22+ messages in thread
From: Michael S. Tsirkin @ 2015-01-19 12:46 UTC (permalink / raw)
To: Igor Mammedov; +Cc: claudio.fontana, qemu-devel, marcel.a
On Fri, Dec 19, 2014 at 11:46:58AM +0000, Igor Mammedov wrote:
> when bridge hotplug is disabled, i.e. for machine
> types less then 2.0, bridge device was created as
> hotpluggable by mistake since commit 133a2da (2.1).
> Fix it by just creating it as a present device as
> it was done in 1.7.
>
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
I still don't get this. Under 1.7 all devices are hotpluggable
unless disabled by device class. Bridges were hotpluggable too.
See 72c194f7e75cb64b2558111cb111adb49fbf4097, function
acpi_get_hotplug_info.
So if the idea is to match 1.7, this does not do it.
What, then, is the purpose of this patch?
> ---
> hw/i386/acpi-build.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index a4d0c0c..c151fde 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -919,7 +919,7 @@ static void build_pci_bus_end(PCIBus *bus, void *bus_state)
> }
> }
>
> - if (!dc->hotpluggable || bridge_in_acpi) {
> + if (!dc->hotpluggable || pc->is_bridge) {
> clear_bit(slot, slot_hotplug_enable);
> }
> }
> --
> 1.8.3.1
>
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [Qemu-devel] [PATCH v3 2/8] pc: acpi: decribe bridge device as not hotpluggable
2015-01-19 12:46 ` Michael S. Tsirkin
@ 2015-01-20 9:22 ` Igor Mammedov
0 siblings, 0 replies; 22+ messages in thread
From: Igor Mammedov @ 2015-01-20 9:22 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: claudio.fontana, qemu-devel, marcel.a
On Mon, 19 Jan 2015 14:46:38 +0200
"Michael S. Tsirkin" <mst@redhat.com> wrote:
> On Fri, Dec 19, 2014 at 11:46:58AM +0000, Igor Mammedov wrote:
> > when bridge hotplug is disabled, i.e. for machine
> > types less then 2.0, bridge device was created as
> > hotpluggable by mistake since commit 133a2da (2.1).
> > Fix it by just creating it as a present device as
> > it was done in 1.7.
> >
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
>
> I still don't get this. Under 1.7 all devices are hotpluggable
> unless disabled by device class. Bridges were hotpluggable too.
>
> See 72c194f7e75cb64b2558111cb111adb49fbf4097, function
> acpi_get_hotplug_info.
>
> So if the idea is to match 1.7, this does not do it.
Yep, I was wrong about 1.7.
Let's see:
1.7 - all PCI devices created as hotpluggable entries
99fd437d - creates bridge as non hotpluggable unconditionally
133a2da4 - creates bridge as
1 - non hotpluggable if bridge hotplug supported - subtree is creeated
2 - if bridge hotplug isn't supported, it creates bridge as hotpluggable - subtree is not created
so I guess, I should maintain what 133a2da4 does, drop this patch and amend 8/8
>
> What, then, is the purpose of this patch?
>
>
> > ---
> > hw/i386/acpi-build.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> > index a4d0c0c..c151fde 100644
> > --- a/hw/i386/acpi-build.c
> > +++ b/hw/i386/acpi-build.c
> > @@ -919,7 +919,7 @@ static void build_pci_bus_end(PCIBus *bus, void *bus_state)
> > }
> > }
> >
> > - if (!dc->hotpluggable || bridge_in_acpi) {
> > + if (!dc->hotpluggable || pc->is_bridge) {
> > clear_bit(slot, slot_hotplug_enable);
> > }
> > }
> > --
> > 1.8.3.1
> >
^ permalink raw reply [flat|nested] 22+ messages in thread
* [Qemu-devel] [PATCH v3 3/8] pc: acpi-build: cleanup AcpiPmInfo initialization
2014-12-19 11:46 [Qemu-devel] [PATCH v3 0/8] pc: acpi: various fixes and cleanups Igor Mammedov
2014-12-19 11:46 ` [Qemu-devel] [PATCH v3 1/8] pc: acpi: fix WindowsXP BSOD when memory hotplug is enabled Igor Mammedov
2014-12-19 11:46 ` [Qemu-devel] [PATCH v3 2/8] pc: acpi: decribe bridge device as not hotpluggable Igor Mammedov
@ 2014-12-19 11:46 ` Igor Mammedov
2014-12-19 11:47 ` [Qemu-devel] [PATCH v3 4/8] acpi: build_append_nameseg(): add padding if necessary Igor Mammedov
` (4 subsequent siblings)
7 siblings, 0 replies; 22+ messages in thread
From: Igor Mammedov @ 2014-12-19 11:46 UTC (permalink / raw)
To: qemu-devel; +Cc: mst, claudio.fontana, marcel.a
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 c151fde..0351363 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -169,6 +169,8 @@ static void acpi_get_pm_info(AcpiPmInfo *pm)
Object *obj = NULL;
QObject *o;
+ memset(pm, 0, sizeof(*pm));
+
if (piix) {
obj = piix;
}
@@ -181,22 +183,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] 22+ messages in thread* [Qemu-devel] [PATCH v3 4/8] acpi: build_append_nameseg(): add padding if necessary
2014-12-19 11:46 [Qemu-devel] [PATCH v3 0/8] pc: acpi: various fixes and cleanups Igor Mammedov
` (2 preceding siblings ...)
2014-12-19 11:46 ` [Qemu-devel] [PATCH v3 3/8] pc: acpi-build: cleanup AcpiPmInfo initialization Igor Mammedov
@ 2014-12-19 11:47 ` Igor Mammedov
2014-12-19 11:47 ` [Qemu-devel] [PATCH v3 5/8] acpi: move generic aml building helpers into dedictated file Igor Mammedov
` (3 subsequent siblings)
7 siblings, 0 replies; 22+ messages in thread
From: Igor Mammedov @ 2014-12-19 11:47 UTC (permalink / raw)
To: qemu-devel; +Cc: mst, claudio.fontana, marcel.a
According to ACPI spec NameSeg shorter than 4 characters
must be padded up to 4 characters with "_" symbol.
ACPI 5.0: 20.2.2 "Name Objects Encoding"
Do it in build_append_nameseg() so that caller shouldn't know
or care about it.
Signed-off-by: Igor Mammedov <imammedo@redhat.com>
Reviewed-by: Claudio Fontana <claudio.fontana@huawei.com>
---
v2:
* simplify padding, suggested by: "Michael S. Tsirkin" <mst@redhat.com>
---
hw/i386/acpi-build.c | 15 ++++++++++-----
1 file changed, 10 insertions(+), 5 deletions(-)
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 0351363..cab3c76 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -298,6 +298,8 @@ static inline void build_append_array(GArray *array, GArray *val)
g_array_append_vals(array, val->data, val->len);
}
+#define ACPI_NAMESEG_LEN 4
+
static void GCC_FMT_ATTR(2, 3)
build_append_nameseg(GArray *array, const char *format, ...)
{
@@ -310,8 +312,11 @@ build_append_nameseg(GArray *array, const char *format, ...)
len = vsnprintf(s, sizeof s, format, args);
va_end(args);
- assert(len == 4);
+ assert(len <= ACPI_NAMESEG_LEN);
+
g_array_append_vals(array, s, len);
+ /* Pad up to ACPI_NAMESEG_LEN characters if necessary. */
+ g_array_append_vals(array, "____", ACPI_NAMESEG_LEN - len);
}
/* 5.4 Definition Block Encoding */
@@ -852,7 +857,7 @@ static void build_pci_bus_end(PCIBus *bus, void *bus_state)
if (bus->parent_dev) {
op = 0x82; /* DeviceOp */
- build_append_nameseg(bus_table, "S%.02X_",
+ build_append_nameseg(bus_table, "S%.02X",
bus->parent_dev->devfn);
build_append_byte(bus_table, 0x08); /* NameOp */
build_append_nameseg(bus_table, "_SUN");
@@ -972,7 +977,7 @@ static void build_pci_bus_end(PCIBus *bus, void *bus_state)
build_append_int(notify, 0x1U << i);
build_append_byte(notify, 0x00); /* NullName */
build_append_byte(notify, 0x86); /* NotifyOp */
- build_append_nameseg(notify, "S%.02X_", PCI_DEVFN(i, 0));
+ build_append_nameseg(notify, "S%.02X", PCI_DEVFN(i, 0));
build_append_byte(notify, 0x69); /* Arg1Op */
/* Pack it up */
@@ -1029,7 +1034,7 @@ static void build_pci_bus_end(PCIBus *bus, void *bus_state)
if (bus->parent_dev) {
build_append_byte(parent->notify_table, '^'); /* ParentPrefixChar */
build_append_byte(parent->notify_table, 0x2E); /* DualNamePrefix */
- build_append_nameseg(parent->notify_table, "S%.02X_",
+ build_append_nameseg(parent->notify_table, "S%.02X",
bus->parent_dev->devfn);
build_append_nameseg(parent->notify_table, "PCNT");
}
@@ -1099,7 +1104,7 @@ build_ssdt(GArray *table_data, GArray *linker,
GArray *sb_scope = build_alloc_array();
uint8_t op = 0x10; /* ScopeOp */
- build_append_nameseg(sb_scope, "_SB_");
+ build_append_nameseg(sb_scope, "_SB");
/* build Processor object for each processor */
for (i = 0; i < acpi_cpus; i++) {
--
1.8.3.1
^ permalink raw reply related [flat|nested] 22+ messages in thread* [Qemu-devel] [PATCH v3 5/8] acpi: move generic aml building helpers into dedictated file
2014-12-19 11:46 [Qemu-devel] [PATCH v3 0/8] pc: acpi: various fixes and cleanups Igor Mammedov
` (3 preceding siblings ...)
2014-12-19 11:47 ` [Qemu-devel] [PATCH v3 4/8] acpi: build_append_nameseg(): add padding if necessary Igor Mammedov
@ 2014-12-19 11:47 ` Igor Mammedov
2015-01-19 15:08 ` Michael S. Tsirkin
2014-12-19 11:47 ` [Qemu-devel] [PATCH v3 6/8] acpi: add build_append_namestring() helper Igor Mammedov
` (2 subsequent siblings)
7 siblings, 1 reply; 22+ messages in thread
From: Igor Mammedov @ 2014-12-19 11:47 UTC (permalink / raw)
To: qemu-devel; +Cc: mst, claudio.fontana, marcel.a
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>
---
v2:
fix wrong ident in moved code
---
hw/acpi/Makefile.objs | 1 +
hw/acpi/acpi_gen_utils.c | 166 +++++++++++++++++++++++++++++++++++++++
hw/i386/acpi-build.c | 162 +-------------------------------------
include/hw/acpi/acpi_gen_utils.h | 23 ++++++
4 files changed, 192 insertions(+), 160 deletions(-)
create mode 100644 hw/acpi/acpi_gen_utils.c
create mode 100644 include/hw/acpi/acpi_gen_utils.h
diff --git a/hw/acpi/Makefile.objs b/hw/acpi/Makefile.objs
index acd2389..4237232 100644
--- a/hw/acpi/Makefile.objs
+++ b/hw/acpi/Makefile.objs
@@ -1,3 +1,4 @@
common-obj-$(CONFIG_ACPI) += core.o piix4.o ich9.o pcihp.o cpu_hotplug.o
common-obj-$(CONFIG_ACPI) += memory_hotplug.o
common-obj-$(CONFIG_ACPI) += acpi_interface.o
+common-obj-$(CONFIG_ACPI) += acpi_gen_utils.o
diff --git a/hw/acpi/acpi_gen_utils.c b/hw/acpi/acpi_gen_utils.c
new file mode 100644
index 0000000..5f64c37
--- /dev/null
+++ b/hw/acpi/acpi_gen_utils.c
@@ -0,0 +1,166 @@
+#include <stdio.h>
+#include <stdarg.h>
+#include <assert.h>
+#include <stdbool.h>
+#include "hw/acpi/acpi_gen_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 cab3c76..f7282ef 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_gen_utils.h"
+
#include "qapi/qmp/qint.h"
#include "qom/qom-qobject.h"
#include "exec/ram_addr.h"
@@ -273,166 +275,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_gen_utils.h b/include/hw/acpi/acpi_gen_utils.h
new file mode 100644
index 0000000..e6a0b28
--- /dev/null
+++ b/include/hw/acpi/acpi_gen_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] 22+ messages in thread* Re: [Qemu-devel] [PATCH v3 5/8] acpi: move generic aml building helpers into dedictated file
2014-12-19 11:47 ` [Qemu-devel] [PATCH v3 5/8] acpi: move generic aml building helpers into dedictated file Igor Mammedov
@ 2015-01-19 15:08 ` Michael S. Tsirkin
2015-01-20 9:24 ` Igor Mammedov
0 siblings, 1 reply; 22+ messages in thread
From: Michael S. Tsirkin @ 2015-01-19 15:08 UTC (permalink / raw)
To: Igor Mammedov; +Cc: claudio.fontana, qemu-devel, marcel.a
On Fri, Dec 19, 2014 at 11:47:01AM +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>
Pls rename acpi-build-utils: using dashes, and
build in name consistent with "build" in function
names.
> ---
> v2:
> fix wrong ident in moved code
> ---
> hw/acpi/Makefile.objs | 1 +
> hw/acpi/acpi_gen_utils.c | 166 +++++++++++++++++++++++++++++++++++++++
> hw/i386/acpi-build.c | 162 +-------------------------------------
> include/hw/acpi/acpi_gen_utils.h | 23 ++++++
> 4 files changed, 192 insertions(+), 160 deletions(-)
> create mode 100644 hw/acpi/acpi_gen_utils.c
> create mode 100644 include/hw/acpi/acpi_gen_utils.h
>
> diff --git a/hw/acpi/Makefile.objs b/hw/acpi/Makefile.objs
> index acd2389..4237232 100644
> --- a/hw/acpi/Makefile.objs
> +++ b/hw/acpi/Makefile.objs
> @@ -1,3 +1,4 @@
> common-obj-$(CONFIG_ACPI) += core.o piix4.o ich9.o pcihp.o cpu_hotplug.o
> common-obj-$(CONFIG_ACPI) += memory_hotplug.o
> common-obj-$(CONFIG_ACPI) += acpi_interface.o
> +common-obj-$(CONFIG_ACPI) += acpi_gen_utils.o
> diff --git a/hw/acpi/acpi_gen_utils.c b/hw/acpi/acpi_gen_utils.c
> new file mode 100644
> index 0000000..5f64c37
> --- /dev/null
> +++ b/hw/acpi/acpi_gen_utils.c
Pls copy header from hw/i386/acpi-build.c:
(we can drop Fabrice's and Kevin's copyright on this file since the specific
functions were all written by myself).
And I guess update the copyright.
/* Support for generating ACPI tables and passing them to Guests
*
* Copyright (C) 2014 Red Hat Inc
*
* Author: Michael S. Tsirkin <mst@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/>.
*/
> @@ -0,0 +1,166 @@
> +#include <stdio.h>
> +#include <stdarg.h>
> +#include <assert.h>
> +#include <stdbool.h>
> +#include "hw/acpi/acpi_gen_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 cab3c76..f7282ef 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_gen_utils.h"
> +
> #include "qapi/qmp/qint.h"
> #include "qom/qom-qobject.h"
> #include "exec/ram_addr.h"
> @@ -273,166 +275,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_gen_utils.h b/include/hw/acpi/acpi_gen_utils.h
> new file mode 100644
> index 0000000..e6a0b28
> --- /dev/null
> +++ b/include/hw/acpi/acpi_gen_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] 22+ messages in thread* Re: [Qemu-devel] [PATCH v3 5/8] acpi: move generic aml building helpers into dedictated file
2015-01-19 15:08 ` Michael S. Tsirkin
@ 2015-01-20 9:24 ` Igor Mammedov
2015-01-20 9:38 ` Michael S. Tsirkin
0 siblings, 1 reply; 22+ messages in thread
From: Igor Mammedov @ 2015-01-20 9:24 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: claudio.fontana, qemu-devel, marcel.a
On Mon, 19 Jan 2015 17:08:43 +0200
"Michael S. Tsirkin" <mst@redhat.com> wrote:
> On Fri, Dec 19, 2014 at 11:47:01AM +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>
>
> Pls rename acpi-build-utils: using dashes, and
> build in name consistent with "build" in function
> names.
Sure.
Do you mean s/acpi_gen_utils/acpi-build-utils/
>
>
>
> > ---
> > v2:
> > fix wrong ident in moved code
> > ---
> > hw/acpi/Makefile.objs | 1 +
> > hw/acpi/acpi_gen_utils.c | 166 +++++++++++++++++++++++++++++++++++++++
> > hw/i386/acpi-build.c | 162 +-------------------------------------
> > include/hw/acpi/acpi_gen_utils.h | 23 ++++++
> > 4 files changed, 192 insertions(+), 160 deletions(-)
> > create mode 100644 hw/acpi/acpi_gen_utils.c
> > create mode 100644 include/hw/acpi/acpi_gen_utils.h
> >
> > diff --git a/hw/acpi/Makefile.objs b/hw/acpi/Makefile.objs
> > index acd2389..4237232 100644
> > --- a/hw/acpi/Makefile.objs
> > +++ b/hw/acpi/Makefile.objs
> > @@ -1,3 +1,4 @@
> > common-obj-$(CONFIG_ACPI) += core.o piix4.o ich9.o pcihp.o cpu_hotplug.o
> > common-obj-$(CONFIG_ACPI) += memory_hotplug.o
> > common-obj-$(CONFIG_ACPI) += acpi_interface.o
> > +common-obj-$(CONFIG_ACPI) += acpi_gen_utils.o
> > diff --git a/hw/acpi/acpi_gen_utils.c b/hw/acpi/acpi_gen_utils.c
> > new file mode 100644
> > index 0000000..5f64c37
> > --- /dev/null
> > +++ b/hw/acpi/acpi_gen_utils.c
>
> Pls copy header from hw/i386/acpi-build.c:
> (we can drop Fabrice's and Kevin's copyright on this file since the specific
> functions were all written by myself).
> And I guess update the copyright.
>
> /* Support for generating ACPI tables and passing them to Guests
> *
> * Copyright (C) 2014 Red Hat Inc
> *
> * Author: Michael S. Tsirkin <mst@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/>.
> */
>
>
>
>
> > @@ -0,0 +1,166 @@
> > +#include <stdio.h>
> > +#include <stdarg.h>
> > +#include <assert.h>
> > +#include <stdbool.h>
> > +#include "hw/acpi/acpi_gen_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 cab3c76..f7282ef 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_gen_utils.h"
> > +
> > #include "qapi/qmp/qint.h"
> > #include "qom/qom-qobject.h"
> > #include "exec/ram_addr.h"
> > @@ -273,166 +275,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_gen_utils.h b/include/hw/acpi/acpi_gen_utils.h
> > new file mode 100644
> > index 0000000..e6a0b28
> > --- /dev/null
> > +++ b/include/hw/acpi/acpi_gen_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] 22+ messages in thread* Re: [Qemu-devel] [PATCH v3 5/8] acpi: move generic aml building helpers into dedictated file
2015-01-20 9:24 ` Igor Mammedov
@ 2015-01-20 9:38 ` Michael S. Tsirkin
0 siblings, 0 replies; 22+ messages in thread
From: Michael S. Tsirkin @ 2015-01-20 9:38 UTC (permalink / raw)
To: Igor Mammedov; +Cc: claudio.fontana, qemu-devel, marcel.a
On Tue, Jan 20, 2015 at 10:24:41AM +0100, Igor Mammedov wrote:
> On Mon, 19 Jan 2015 17:08:43 +0200
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
>
> > On Fri, Dec 19, 2014 at 11:47:01AM +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>
> >
> > Pls rename acpi-build-utils: using dashes, and
> > build in name consistent with "build" in function
> > names.
> Sure.
> Do you mean s/acpi_gen_utils/acpi-build-utils/
Exactly. I'd do so myself, but it's best to keep the copyrights
straight from the beginning, and you have a bunch of follow-up
patches as well.
> >
> >
> >
> > > ---
> > > v2:
> > > fix wrong ident in moved code
> > > ---
> > > hw/acpi/Makefile.objs | 1 +
> > > hw/acpi/acpi_gen_utils.c | 166 +++++++++++++++++++++++++++++++++++++++
> > > hw/i386/acpi-build.c | 162 +-------------------------------------
> > > include/hw/acpi/acpi_gen_utils.h | 23 ++++++
> > > 4 files changed, 192 insertions(+), 160 deletions(-)
> > > create mode 100644 hw/acpi/acpi_gen_utils.c
> > > create mode 100644 include/hw/acpi/acpi_gen_utils.h
> > >
> > > diff --git a/hw/acpi/Makefile.objs b/hw/acpi/Makefile.objs
> > > index acd2389..4237232 100644
> > > --- a/hw/acpi/Makefile.objs
> > > +++ b/hw/acpi/Makefile.objs
> > > @@ -1,3 +1,4 @@
> > > common-obj-$(CONFIG_ACPI) += core.o piix4.o ich9.o pcihp.o cpu_hotplug.o
> > > common-obj-$(CONFIG_ACPI) += memory_hotplug.o
> > > common-obj-$(CONFIG_ACPI) += acpi_interface.o
> > > +common-obj-$(CONFIG_ACPI) += acpi_gen_utils.o
> > > diff --git a/hw/acpi/acpi_gen_utils.c b/hw/acpi/acpi_gen_utils.c
> > > new file mode 100644
> > > index 0000000..5f64c37
> > > --- /dev/null
> > > +++ b/hw/acpi/acpi_gen_utils.c
> >
> > Pls copy header from hw/i386/acpi-build.c:
> > (we can drop Fabrice's and Kevin's copyright on this file since the specific
> > functions were all written by myself).
> > And I guess update the copyright.
> >
> > /* Support for generating ACPI tables and passing them to Guests
> > *
> > * Copyright (C) 2014 Red Hat Inc
> > *
> > * Author: Michael S. Tsirkin <mst@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/>.
> > */
> >
> >
> >
> >
> > > @@ -0,0 +1,166 @@
> > > +#include <stdio.h>
> > > +#include <stdarg.h>
> > > +#include <assert.h>
> > > +#include <stdbool.h>
> > > +#include "hw/acpi/acpi_gen_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 cab3c76..f7282ef 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_gen_utils.h"
> > > +
> > > #include "qapi/qmp/qint.h"
> > > #include "qom/qom-qobject.h"
> > > #include "exec/ram_addr.h"
> > > @@ -273,166 +275,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_gen_utils.h b/include/hw/acpi/acpi_gen_utils.h
> > > new file mode 100644
> > > index 0000000..e6a0b28
> > > --- /dev/null
> > > +++ b/include/hw/acpi/acpi_gen_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] 22+ messages in thread
* [Qemu-devel] [PATCH v3 6/8] acpi: add build_append_namestring() helper
2014-12-19 11:46 [Qemu-devel] [PATCH v3 0/8] pc: acpi: various fixes and cleanups Igor Mammedov
` (4 preceding siblings ...)
2014-12-19 11:47 ` [Qemu-devel] [PATCH v3 5/8] acpi: move generic aml building helpers into dedictated file Igor Mammedov
@ 2014-12-19 11:47 ` Igor Mammedov
2014-12-19 13:29 ` Claudio Fontana
2015-01-28 7:43 ` [Qemu-devel] [PATCH v3 " Michael S. Tsirkin
2014-12-19 11:47 ` [Qemu-devel] [PATCH v3 7/8] acpi: drop min-bytes in build_package() Igor Mammedov
2014-12-19 11:47 ` [Qemu-devel] [PATCH v3 8/8] pc: acpi-build: simplify PCI bus tree generation Igor Mammedov
7 siblings, 2 replies; 22+ messages in thread
From: Igor Mammedov @ 2014-12-19 11:47 UTC (permalink / raw)
To: qemu-devel; +Cc: mst, claudio.fontana, marcel.a
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>
---
v2:
assert on wrong Segcount earlier and extend condition to
seg_count <= 0xFF || seg_count < 1
---
hw/acpi/acpi_gen_utils.c | 90 +++++++++++++++++++++++++++++++++++++++-
hw/i386/acpi-build.c | 35 +++++++---------
include/hw/acpi/acpi_gen_utils.h | 2 +-
3 files changed, 106 insertions(+), 21 deletions(-)
diff --git a/hw/acpi/acpi_gen_utils.c b/hw/acpi/acpi_gen_utils.c
index 5f64c37..d5fca8e 100644
--- a/hw/acpi/acpi_gen_utils.c
+++ b/hw/acpi/acpi_gen_utils.c
@@ -31,7 +31,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 */
@@ -50,6 +50,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 <= 0xFF || seg_count < 1);
+
+ /* 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 f7282ef..7642f6d 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -279,7 +279,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;
@@ -571,7 +571,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);
@@ -699,24 +699,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 {
@@ -819,7 +819,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 */
@@ -843,12 +843,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 */
}
@@ -874,11 +874,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");
}
}
@@ -946,7 +943,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++) {
@@ -966,7 +963,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_gen_utils.h b/include/hw/acpi/acpi_gen_utils.h
index e6a0b28..fd50625 100644
--- a/include/hw/acpi/acpi_gen_utils.h
+++ b/include/hw/acpi/acpi_gen_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] 22+ messages in thread* Re: [Qemu-devel] [PATCH v3 6/8] acpi: add build_append_namestring() helper
2014-12-19 11:47 ` [Qemu-devel] [PATCH v3 6/8] acpi: add build_append_namestring() helper Igor Mammedov
@ 2014-12-19 13:29 ` Claudio Fontana
2014-12-19 14:27 ` [Qemu-devel] [PATCH v4 " Igor Mammedov
2015-01-28 7:43 ` [Qemu-devel] [PATCH v3 " Michael S. Tsirkin
1 sibling, 1 reply; 22+ messages in thread
From: Claudio Fontana @ 2014-12-19 13:29 UTC (permalink / raw)
To: Igor Mammedov, qemu-devel; +Cc: mst, marcel.a
On 19.12.2014 12:47, 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>
> ---
> v2:
> assert on wrong Segcount earlier and extend condition to
> seg_count <= 0xFF || seg_count < 1
> ---
> hw/acpi/acpi_gen_utils.c | 90 +++++++++++++++++++++++++++++++++++++++-
> hw/i386/acpi-build.c | 35 +++++++---------
> include/hw/acpi/acpi_gen_utils.h | 2 +-
> 3 files changed, 106 insertions(+), 21 deletions(-)
>
> diff --git a/hw/acpi/acpi_gen_utils.c b/hw/acpi/acpi_gen_utils.c
> index 5f64c37..d5fca8e 100644
> --- a/hw/acpi/acpi_gen_utils.c
> +++ b/hw/acpi/acpi_gen_utils.c
> @@ -31,7 +31,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 */
> @@ -50,6 +50,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 <= 0xFF || seg_count < 1);
hmm that doesn't seem right. If seg_count is 0, you want to fail.
assert(seg_count <= 0xff && seg_count != 0);
or
assert(seg_count > 0 && seg_count <= 255);
or something like that..
> +
> + /* 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 f7282ef..7642f6d 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -279,7 +279,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;
> @@ -571,7 +571,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);
> @@ -699,24 +699,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 {
> @@ -819,7 +819,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 */
> @@ -843,12 +843,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 */
> }
>
> @@ -874,11 +874,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");
> }
> }
>
> @@ -946,7 +943,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++) {
> @@ -966,7 +963,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_gen_utils.h b/include/hw/acpi/acpi_gen_utils.h
> index e6a0b28..fd50625 100644
> --- a/include/hw/acpi/acpi_gen_utils.h
> +++ b/include/hw/acpi/acpi_gen_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);
>
--
Claudio Fontana
Server Virtualization Architect
Huawei Technologies Duesseldorf GmbH
Riesstraße 25 - 80992 München
office: +49 89 158834 4135
mobile: +49 15253060158
^ permalink raw reply [flat|nested] 22+ messages in thread* [Qemu-devel] [PATCH v4 6/8] acpi: add build_append_namestring() helper
2014-12-19 13:29 ` Claudio Fontana
@ 2014-12-19 14:27 ` Igor Mammedov
2014-12-19 14:45 ` Claudio Fontana
0 siblings, 1 reply; 22+ messages in thread
From: Igor Mammedov @ 2014-12-19 14:27 UTC (permalink / raw)
To: qemu-devel; +Cc: mst, claudio.fontana, marcel.a
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>
---
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_gen_utils.c | 90 +++++++++++++++++++++++++++++++++++++++-
hw/i386/acpi-build.c | 35 +++++++---------
include/hw/acpi/acpi_gen_utils.h | 2 +-
3 files changed, 106 insertions(+), 21 deletions(-)
diff --git a/hw/acpi/acpi_gen_utils.c b/hw/acpi/acpi_gen_utils.c
index 5f64c37..b6b2956 100644
--- a/hw/acpi/acpi_gen_utils.c
+++ b/hw/acpi/acpi_gen_utils.c
@@ -31,7 +31,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 */
@@ -50,6 +50,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 f7282ef..7642f6d 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -279,7 +279,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;
@@ -571,7 +571,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);
@@ -699,24 +699,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 {
@@ -819,7 +819,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 */
@@ -843,12 +843,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 */
}
@@ -874,11 +874,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");
}
}
@@ -946,7 +943,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++) {
@@ -966,7 +963,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_gen_utils.h b/include/hw/acpi/acpi_gen_utils.h
index e6a0b28..fd50625 100644
--- a/include/hw/acpi/acpi_gen_utils.h
+++ b/include/hw/acpi/acpi_gen_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] 22+ messages in thread* Re: [Qemu-devel] [PATCH v4 6/8] acpi: add build_append_namestring() helper
2014-12-19 14:27 ` [Qemu-devel] [PATCH v4 " Igor Mammedov
@ 2014-12-19 14:45 ` Claudio Fontana
0 siblings, 0 replies; 22+ messages in thread
From: Claudio Fontana @ 2014-12-19 14:45 UTC (permalink / raw)
To: Igor Mammedov, qemu-devel; +Cc: mst, marcel.a
Reviewed-by: Claudio Fontana <claudio.fontana@huawei.com>
On 19.12.2014 15:27, 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>
> ---
> 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_gen_utils.c | 90 +++++++++++++++++++++++++++++++++++++++-
> hw/i386/acpi-build.c | 35 +++++++---------
> include/hw/acpi/acpi_gen_utils.h | 2 +-
> 3 files changed, 106 insertions(+), 21 deletions(-)
>
> diff --git a/hw/acpi/acpi_gen_utils.c b/hw/acpi/acpi_gen_utils.c
> index 5f64c37..b6b2956 100644
> --- a/hw/acpi/acpi_gen_utils.c
> +++ b/hw/acpi/acpi_gen_utils.c
> @@ -31,7 +31,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 */
> @@ -50,6 +50,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 f7282ef..7642f6d 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -279,7 +279,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;
> @@ -571,7 +571,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);
> @@ -699,24 +699,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 {
> @@ -819,7 +819,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 */
> @@ -843,12 +843,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 */
> }
>
> @@ -874,11 +874,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");
> }
> }
>
> @@ -946,7 +943,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++) {
> @@ -966,7 +963,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_gen_utils.h b/include/hw/acpi/acpi_gen_utils.h
> index e6a0b28..fd50625 100644
> --- a/include/hw/acpi/acpi_gen_utils.h
> +++ b/include/hw/acpi/acpi_gen_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);
>
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [Qemu-devel] [PATCH v3 6/8] acpi: add build_append_namestring() helper
2014-12-19 11:47 ` [Qemu-devel] [PATCH v3 6/8] acpi: add build_append_namestring() helper Igor Mammedov
2014-12-19 13:29 ` Claudio Fontana
@ 2015-01-28 7:43 ` Michael S. Tsirkin
2015-01-28 12:32 ` Igor Mammedov
1 sibling, 1 reply; 22+ messages in thread
From: Michael S. Tsirkin @ 2015-01-28 7:43 UTC (permalink / raw)
To: Igor Mammedov; +Cc: claudio.fontana, qemu-devel, marcel.a
On Fri, Dec 19, 2014 at 11:47:02AM +0000, Igor Mammedov wrote:
> Use build_append_namestring() instead of build_append_nameseg()
> So user won't have to care whether name is NameSeg, NamePath or
> NameString.
>
> See for refernce ACPI 5.0: 20.2.2 Name Objects Encoding
>
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
> v2:
> assert on wrong Segcount earlier and extend condition to
> seg_count <= 0xFF || seg_count < 1
> ---
> hw/acpi/acpi_gen_utils.c | 90 +++++++++++++++++++++++++++++++++++++++-
> hw/i386/acpi-build.c | 35 +++++++---------
> include/hw/acpi/acpi_gen_utils.h | 2 +-
> 3 files changed, 106 insertions(+), 21 deletions(-)
>
> diff --git a/hw/acpi/acpi_gen_utils.c b/hw/acpi/acpi_gen_utils.c
> index 5f64c37..d5fca8e 100644
> --- a/hw/acpi/acpi_gen_utils.c
> +++ b/hw/acpi/acpi_gen_utils.c
> @@ -31,7 +31,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 */
> @@ -50,6 +50,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;
> +
This kind of name is explicitly against out coding style.
variables are lower case, types are mixed case, macros are
upper case.
there's a single user, just do the usual
0x2E /* DualNamePrefix */;
0x2F /* MultiNamePrefix */
> +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 <= 0xFF || seg_count < 1);
> +
> + /* 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 f7282ef..7642f6d 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -279,7 +279,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;
> @@ -571,7 +571,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);
> @@ -699,24 +699,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 {
> @@ -819,7 +819,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 */
> @@ -843,12 +843,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 */
> }
>
> @@ -874,11 +874,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");
> }
> }
>
> @@ -946,7 +943,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++) {
> @@ -966,7 +963,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_gen_utils.h b/include/hw/acpi/acpi_gen_utils.h
> index e6a0b28..fd50625 100644
> --- a/include/hw/acpi/acpi_gen_utils.h
> +++ b/include/hw/acpi/acpi_gen_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 [flat|nested] 22+ messages in thread* Re: [Qemu-devel] [PATCH v3 6/8] acpi: add build_append_namestring() helper
2015-01-28 7:43 ` [Qemu-devel] [PATCH v3 " Michael S. Tsirkin
@ 2015-01-28 12:32 ` Igor Mammedov
0 siblings, 0 replies; 22+ messages in thread
From: Igor Mammedov @ 2015-01-28 12:32 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: claudio.fontana, qemu-devel, marcel.a
On Wed, 28 Jan 2015 09:43:02 +0200
"Michael S. Tsirkin" <mst@redhat.com> wrote:
> On Fri, Dec 19, 2014 at 11:47:02AM +0000, Igor Mammedov wrote:
> > Use build_append_namestring() instead of build_append_nameseg()
> > So user won't have to care whether name is NameSeg, NamePath or
> > NameString.
> >
> > See for refernce ACPI 5.0: 20.2.2 Name Objects Encoding
> >
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > ---
> > v2:
> > assert on wrong Segcount earlier and extend condition to
> > seg_count <= 0xFF || seg_count < 1
> > ---
> > hw/acpi/acpi_gen_utils.c | 90 +++++++++++++++++++++++++++++++++++++++-
> > hw/i386/acpi-build.c | 35 +++++++---------
> > include/hw/acpi/acpi_gen_utils.h | 2 +-
> > 3 files changed, 106 insertions(+), 21 deletions(-)
> >
> > diff --git a/hw/acpi/acpi_gen_utils.c b/hw/acpi/acpi_gen_utils.c
> > index 5f64c37..d5fca8e 100644
> > --- a/hw/acpi/acpi_gen_utils.c
> > +++ b/hw/acpi/acpi_gen_utils.c
> > @@ -31,7 +31,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 */
> > @@ -50,6 +50,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;
> > +
>
> This kind of name is explicitly against out coding style.
> variables are lower case, types are mixed case, macros are
> upper case.
>
> there's a single user, just do the usual
> 0x2E /* DualNamePrefix */;
> 0x2F /* MultiNamePrefix */
Ok, I'll fix it.
>
> > +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 <= 0xFF || seg_count < 1);
> > +
> > + /* 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 f7282ef..7642f6d 100644
> > --- a/hw/i386/acpi-build.c
> > +++ b/hw/i386/acpi-build.c
> > @@ -279,7 +279,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;
> > @@ -571,7 +571,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);
> > @@ -699,24 +699,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 {
> > @@ -819,7 +819,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 */
> > @@ -843,12 +843,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 */
> > }
> >
> > @@ -874,11 +874,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");
> > }
> > }
> >
> > @@ -946,7 +943,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++) {
> > @@ -966,7 +963,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_gen_utils.h b/include/hw/acpi/acpi_gen_utils.h
> > index e6a0b28..fd50625 100644
> > --- a/include/hw/acpi/acpi_gen_utils.h
> > +++ b/include/hw/acpi/acpi_gen_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 [flat|nested] 22+ messages in thread
* [Qemu-devel] [PATCH v3 7/8] acpi: drop min-bytes in build_package()
2014-12-19 11:46 [Qemu-devel] [PATCH v3 0/8] pc: acpi: various fixes and cleanups Igor Mammedov
` (5 preceding siblings ...)
2014-12-19 11:47 ` [Qemu-devel] [PATCH v3 6/8] acpi: add build_append_namestring() helper Igor Mammedov
@ 2014-12-19 11:47 ` Igor Mammedov
2015-01-20 9:30 ` Claudio Fontana
2014-12-19 11:47 ` [Qemu-devel] [PATCH v3 8/8] pc: acpi-build: simplify PCI bus tree generation Igor Mammedov
7 siblings, 1 reply; 22+ messages in thread
From: Igor Mammedov @ 2014-12-19 11:47 UTC (permalink / raw)
To: qemu-devel; +Cc: mst, claudio.fontana, marcel.a
Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
hw/acpi/acpi_gen_utils.c | 14 ++++----------
hw/i386/acpi-build.c | 13 ++++++-------
include/hw/acpi/acpi_gen_utils.h | 4 ++--
3 files changed, 12 insertions(+), 19 deletions(-)
diff --git a/hw/acpi/acpi_gen_utils.c b/hw/acpi/acpi_gen_utils.c
index d5fca8e..eee8066 100644
--- a/hw/acpi/acpi_gen_utils.c
+++ b/hw/acpi/acpi_gen_utils.c
@@ -146,7 +146,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;
@@ -162,11 +162,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;
@@ -199,15 +194,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 */
}
@@ -251,4 +246,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 7642f6d..94202b5 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -289,7 +289,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);
@@ -310,7 +310,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);
@@ -823,7 +823,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);
@@ -864,7 +864,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 */
@@ -987,7 +987,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);
}
@@ -1035,8 +1035,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_gen_utils.h b/include/hw/acpi/acpi_gen_utils.h
index fd50625..199f003 100644
--- a/include/hw/acpi/acpi_gen_utils.h
+++ b/include/hw/acpi/acpi_gen_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] 22+ messages in thread* Re: [Qemu-devel] [PATCH v3 7/8] acpi: drop min-bytes in build_package()
2014-12-19 11:47 ` [Qemu-devel] [PATCH v3 7/8] acpi: drop min-bytes in build_package() Igor Mammedov
@ 2015-01-20 9:30 ` Claudio Fontana
0 siblings, 0 replies; 22+ messages in thread
From: Claudio Fontana @ 2015-01-20 9:30 UTC (permalink / raw)
To: Igor Mammedov, qemu-devel; +Cc: mst, marcel.a
Reviewed-by: Claudio Fontana <claudio.fontana@huawei.com>
On 19.12.2014 12:47, Igor Mammedov wrote:
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
> hw/acpi/acpi_gen_utils.c | 14 ++++----------
> hw/i386/acpi-build.c | 13 ++++++-------
> include/hw/acpi/acpi_gen_utils.h | 4 ++--
> 3 files changed, 12 insertions(+), 19 deletions(-)
>
> diff --git a/hw/acpi/acpi_gen_utils.c b/hw/acpi/acpi_gen_utils.c
> index d5fca8e..eee8066 100644
> --- a/hw/acpi/acpi_gen_utils.c
> +++ b/hw/acpi/acpi_gen_utils.c
> @@ -146,7 +146,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;
> @@ -162,11 +162,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;
>
> @@ -199,15 +194,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 */
> }
>
> @@ -251,4 +246,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 7642f6d..94202b5 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -289,7 +289,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);
> @@ -310,7 +310,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);
>
> @@ -823,7 +823,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);
>
> @@ -864,7 +864,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 */
> @@ -987,7 +987,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);
> }
> @@ -1035,8 +1035,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_gen_utils.h b/include/hw/acpi/acpi_gen_utils.h
> index fd50625..199f003 100644
> --- a/include/hw/acpi/acpi_gen_utils.h
> +++ b/include/hw/acpi/acpi_gen_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);
>
--
Claudio Fontana
Server Virtualization Architect
Huawei Technologies Duesseldorf GmbH
Riesstraße 25 - 80992 München
office: +49 89 158834 4135
mobile: +49 15253060158
^ permalink raw reply [flat|nested] 22+ messages in thread
* [Qemu-devel] [PATCH v3 8/8] pc: acpi-build: simplify PCI bus tree generation
2014-12-19 11:46 [Qemu-devel] [PATCH v3 0/8] pc: acpi: various fixes and cleanups Igor Mammedov
` (6 preceding siblings ...)
2014-12-19 11:47 ` [Qemu-devel] [PATCH v3 7/8] acpi: drop min-bytes in build_package() Igor Mammedov
@ 2014-12-19 11:47 ` Igor Mammedov
2015-01-19 15:23 ` Michael S. Tsirkin
7 siblings, 1 reply; 22+ messages in thread
From: Igor Mammedov @ 2014-12-19 11:47 UTC (permalink / raw)
To: qemu-devel; +Cc: mst, claudio.fontana, marcel.a
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
change in behavior:
* generate hotpluggable device entries for empty slots
only if BUS itself is hotpluggable, otherwise do not
create them.
Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
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 | 265 +++++++++++++++------------------------------------
1 file changed, 79 insertions(+), 186 deletions(-)
diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
index 94202b5..a893f5e 100644
--- a/hw/i386/acpi-build.c
+++ b/hw/i386/acpi-build.c
@@ -103,7 +103,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;
@@ -647,69 +646,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");
}
@@ -718,171 +685,103 @@ 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;
PCIDevice *pdev = bus->devices[i];
int slot = PCI_SLOT(i);
- 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);
+ }
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;
}
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 || pc->is_bridge) {
- 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 && !pc->is_bridge) {
+ 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);
+ if (pc->is_bridge) {
+ 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)
@@ -1014,7 +913,6 @@ build_ssdt(GArray *table_data, GArray *linker,
}
{
- AcpiBuildPciBusHotplugState hotplug_state;
Object *pci_host;
PCIBus *bus = NULL;
bool ambiguous;
@@ -1024,16 +922,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] 22+ messages in thread* Re: [Qemu-devel] [PATCH v3 8/8] pc: acpi-build: simplify PCI bus tree generation
2014-12-19 11:47 ` [Qemu-devel] [PATCH v3 8/8] pc: acpi-build: simplify PCI bus tree generation Igor Mammedov
@ 2015-01-19 15:23 ` Michael S. Tsirkin
2015-01-20 9:39 ` Igor Mammedov
0 siblings, 1 reply; 22+ messages in thread
From: Michael S. Tsirkin @ 2015-01-19 15:23 UTC (permalink / raw)
To: Igor Mammedov; +Cc: claudio.fontana, qemu-devel, marcel.a
On Fri, Dec 19, 2014 at 11:47:04AM +0000, Igor Mammedov wrote:
> it basicaly does the same as original approach,
> * just without bus/notify tables tracking (less obscure)
> which is easier to follow.
> * drops unnecessary loops and bitmaps,
> creating devices and notification method in the same loop.
> * saves us ~100LOC
>
> change in behavior:
> * generate hotpluggable device entries for empty slots
> only if BUS itself is hotpluggable, otherwise do not
> create them.
Hmm so how do you create a machine where this applies?
Can you clarify?
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
Overall looks fine to me, I'm fine with appying this after rebase.
Although I would prefer it if we didn't add so many scope
operations: I think it's cleaner to add content to devices
directly, but not a must.
> ---
> 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 | 265 +++++++++++++++------------------------------------
> 1 file changed, 79 insertions(+), 186 deletions(-)
>
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index 94202b5..a893f5e 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -103,7 +103,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;
> @@ -647,69 +646,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");
> }
>
> @@ -718,171 +685,103 @@ 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;
> PCIDevice *pdev = bus->devices[i];
> int slot = PCI_SLOT(i);
> - 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);
> + }
> 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;
> }
>
> 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 || pc->is_bridge) {
> - 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 && !pc->is_bridge) {
> + 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);
> + if (pc->is_bridge) {
> + 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)
> @@ -1014,7 +913,6 @@ build_ssdt(GArray *table_data, GArray *linker,
> }
>
> {
> - AcpiBuildPciBusHotplugState hotplug_state;
> Object *pci_host;
> PCIBus *bus = NULL;
> bool ambiguous;
> @@ -1024,16 +922,11 @@ build_ssdt(GArray *table_data, GArray *linker,
> bus = PCI_HOST_BRIDGE(pci_host)->bus;
> }
>
> - build_pci_bus_state_init(&hotplug_state, NULL, pm->pcihp_bridge_en);
> -
> if (bus) {
> /* Scan all PCI buses. Generate tables to support hotplug. */
> - pci_for_each_bus_depth_first(bus, build_pci_bus_begin,
> - build_pci_bus_end, &hotplug_state);
> + build_append_pci_bus_devices(sb_scope, bus,
> + pm->pcihp_bridge_en);
> }
> -
> - build_append_array(sb_scope, hotplug_state.device_table);
> - build_pci_bus_state_cleanup(&hotplug_state);
> }
> build_package(sb_scope, op);
> build_append_array(table_data, sb_scope);
> --
> 1.8.3.1
>
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [Qemu-devel] [PATCH v3 8/8] pc: acpi-build: simplify PCI bus tree generation
2015-01-19 15:23 ` Michael S. Tsirkin
@ 2015-01-20 9:39 ` Igor Mammedov
0 siblings, 0 replies; 22+ messages in thread
From: Igor Mammedov @ 2015-01-20 9:39 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: claudio.fontana, qemu-devel, marcel.a
On Mon, 19 Jan 2015 17:23:41 +0200
"Michael S. Tsirkin" <mst@redhat.com> wrote:
> On Fri, Dec 19, 2014 at 11:47:04AM +0000, Igor Mammedov wrote:
> > it basicaly does the same as original approach,
> > * just without bus/notify tables tracking (less obscure)
> > which is easier to follow.
> > * drops unnecessary loops and bitmaps,
> > creating devices and notification method in the same loop.
> > * saves us ~100LOC
> >
> > change in behavior:
> > * generate hotpluggable device entries for empty slots
> > only if BUS itself is hotpluggable, otherwise do not
> > create them.
>
> Hmm so how do you create a machine where this applies?
> Can you clarify?
it only can happen if bridge device marked as non hotpluggable,
i.e. not happens in practice for now. Perhaps I should drop
this comment altogether.
>
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
>
> Overall looks fine to me, I'm fine with appying this after rebase.
> Although I would prefer it if we didn't add so many scope
> operations: I think it's cleaner to add content to devices
> directly, but not a must.
scope here is used to avoid construction of pcinohp template manually
by hand. and reuse current pcihohp template. However it should be easy
to drop scopes in following conversion where devices are constructed
manually. I'll take care of it in that series during its rebasing.
>
> > ---
> > 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 | 265 +++++++++++++++------------------------------------
> > 1 file changed, 79 insertions(+), 186 deletions(-)
> >
> > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> > index 94202b5..a893f5e 100644
> > --- a/hw/i386/acpi-build.c
> > +++ b/hw/i386/acpi-build.c
> > @@ -103,7 +103,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;
> > @@ -647,69 +646,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");
> > }
> >
> > @@ -718,171 +685,103 @@ 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;
> > PCIDevice *pdev = bus->devices[i];
> > int slot = PCI_SLOT(i);
> > - 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);
> > + }
> > 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;
> > }
> >
> > 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 || pc->is_bridge) {
> > - 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 && !pc->is_bridge) {
> > + 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);
> > + if (pc->is_bridge) {
> > + 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)
> > @@ -1014,7 +913,6 @@ build_ssdt(GArray *table_data, GArray *linker,
> > }
> >
> > {
> > - AcpiBuildPciBusHotplugState hotplug_state;
> > Object *pci_host;
> > PCIBus *bus = NULL;
> > bool ambiguous;
> > @@ -1024,16 +922,11 @@ build_ssdt(GArray *table_data, GArray *linker,
> > bus = PCI_HOST_BRIDGE(pci_host)->bus;
> > }
> >
> > - build_pci_bus_state_init(&hotplug_state, NULL, pm->pcihp_bridge_en);
> > -
> > if (bus) {
> > /* Scan all PCI buses. Generate tables to support hotplug. */
> > - pci_for_each_bus_depth_first(bus, build_pci_bus_begin,
> > - build_pci_bus_end, &hotplug_state);
> > + build_append_pci_bus_devices(sb_scope, bus,
> > + pm->pcihp_bridge_en);
> > }
> > -
> > - build_append_array(sb_scope, hotplug_state.device_table);
> > - build_pci_bus_state_cleanup(&hotplug_state);
> > }
> > build_package(sb_scope, op);
> > build_append_array(table_data, sb_scope);
> > --
> > 1.8.3.1
> >
>
^ permalink raw reply [flat|nested] 22+ messages in thread