From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:45315) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YNmHy-0003uP-Ai for qemu-devel@nongnu.org; Tue, 17 Feb 2015 12:50:27 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YNmHs-0000WN-Pt for qemu-devel@nongnu.org; Tue, 17 Feb 2015 12:50:26 -0500 Received: from mx1.redhat.com ([209.132.183.28]:42594) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YNmHs-0000WJ-IV for qemu-devel@nongnu.org; Tue, 17 Feb 2015 12:50:20 -0500 Date: Tue, 17 Feb 2015 18:50:11 +0100 From: Igor Mammedov Message-ID: <20150217185011.437bf89d@nial.brq.redhat.com> In-Reply-To: <20150217162616.GA23547@redhat.com> References: <1423479254-15342-1-git-send-email-imammedo@redhat.com> <1423479254-15342-2-git-send-email-imammedo@redhat.com> <20150217162616.GA23547@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v3 01/52] acpi: introduce AML composer aml_append() List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: "Michael S. Tsirkin" Cc: drjones@redhat.com, zhaoshenglong@huawei.com, claudio.fontana@huawei.com, qemu-devel@nongnu.org, marcel.a@redhat.com On Tue, 17 Feb 2015 17:26:16 +0100 "Michael S. Tsirkin" wrote: > On Mon, Feb 09, 2015 at 10:53:23AM +0000, Igor Mammedov wrote: > > Adds for dynamic AML creation, which will be used > > for piecing ASL/AML primitives together and hiding > > from user/caller details about how nested context > > should be closed/packed leaving less space for > > mistakes and necessity to know how AML should be > > encoded, allowing user to concentrate on ASL > > representation instead. > > > > For example it will allow to create AML like this: > > > > init_aml_allocator(); > > ... > > Aml *scope = aml_scope("PCI0") > > Aml *dev = aml_device("PM") > > aml_append(dev, aml_name_decl("_ADR", aml_int(addr))) > > aml_append(scope, dev); > > ... > > free_aml_allocator(); > > > > Signed-off-by: Igor Mammedov > > --- > > hw/acpi/aml-build.c | 91 +++++++++++++++++++++++++++++++++++++++++++++ > > hw/i386/acpi-build.c | 1 - > > include/hw/acpi/aml-build.h | 61 ++++++++++++++++++++++++++++++ > > 3 files changed, 152 insertions(+), 1 deletion(-) > > > > diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c > > index bcb288e..096f347 100644 > > --- a/hw/acpi/aml-build.c > > +++ b/hw/acpi/aml-build.c > > @@ -25,6 +25,8 @@ > > #include > > #include > > #include "hw/acpi/aml-build.h" > > +#include "qemu/bswap.h" > > +#include "hw/acpi/bios-linker-loader.h" > > > > GArray *build_alloc_array(void) > > { > > @@ -257,3 +259,92 @@ void build_append_int(GArray *table, uint32_t value) > > build_append_value(table, value, 4); > > } > > } > > + > > +static GPtrArray *alloc_list; > > + > > +static Aml *aml_alloc(void) > > +{ > > + Aml *var = g_new0(typeof(*var), 1); > > + > > + g_ptr_array_add(alloc_list, var); > > + var->block_flags = AML_HELPER; > > + var->buf = build_alloc_array(); > > + return var; > > +} > > + > > +static void aml_free(gpointer data) > > +{ > > + Aml *var = data; > > + build_free_array(var->buf); > > +} > > + > > +Aml *init_aml_allocator(GArray *linker) > > +{ > > + Aml *var; > > + > > + assert(!alloc_list); > > + alloc_list = g_ptr_array_new_with_free_func(aml_free); > > + var = aml_alloc(); > > + var->linker = linker; > > + return var; > > +} > > + > > +void free_aml_allocator(void) > > +{ > > + g_ptr_array_free(alloc_list, true); > > + alloc_list = 0; > > +} > > + > > +static void build_buffer(GArray *array, uint8_t op) > > +{ > > + GArray *data = build_alloc_array(); > > + > > + build_append_int(data, array->len); > > + g_array_prepend_vals(array, data->data, data->len); > > + build_free_array(data); > > + build_package(array, op); > > +} > > + > > +void aml_append(Aml *parent_ctx, Aml *child) > > +{ > > + switch (child->block_flags) { > > + case AML_NON_BLOCK: > > + build_append_byte(parent_ctx->buf, child->op); > > + break; > > + case AML_EXT_PACKAGE: > > + build_extop_package(child->buf, child->op); > > + break; > > + case AML_PACKAGE: > > + build_package(child->buf, child->op); > > + break; > > + case AML_RES_TEMPLATE: > > + build_append_byte(child->buf, 0x79); /* EndTag */ > > + /* > > + * checksum operations is treated as succeeded if checksum > > + * field is zero. [ACPI Spec 5.0, 6.4.2.9 End Tag] > > + */ > > + build_append_byte(child->buf, 0); > > + /* fall through, to pack resources in buffer */ > > + case AML_BUFFER: > > + build_buffer(child->buf, child->op); > > + break; > > + case AML_DEF_BLOCK: { > > This one is inelegant. > It's not a definition block, and the patching and it's definition block according to spec. > manual offset calculation are scary. it's the same thing as every user of build_header does every time, so it's less scary compared to build_header and done only in one place so user won't ever have to care about offsets. > > > + uint8_t *start = (uint8_t *)parent_ctx->buf->data + > > + parent_ctx->buf->len; > > + uint32_t le32_len = cpu_to_le32(child->buf->len); > > + > > + /* create linker entry for the DefinitionBlock */ > > + bios_linker_loader_add_checksum(parent_ctx->linker, > > + ACPI_BUILD_TABLE_FILE, > > Hard-coding file name here is very ugly. > All this does not belong in this file, this > is fw cfg interface. It's but, it's the only one file and so far we don't have plans to extend it and it does the same thing as build_header() > > > > + parent_ctx->buf->data, > > + start, child->buf->len, start + 9 /* checksum offset */); > > + > > + /* set DefinitionBlock length at TableLength offset*/ > > + memcpy(child->buf->data + 4, &le32_len, sizeof le32_len); > > > This make no sense. > In AML, there are no objects which are higher level > than the table. > > So for now, please just leave this alone: > drop linker completely, keep table_data > a simple GArray, not AML, and reuse build_header > to fill in headers. > > This way this file just deals with AML and ACPI spec. Using build_header() is problematic for 2 reasons: 1. it has to be done backwards, i.e. one has to reserve space for header first, than add content and then call build_header to patch header with current length and table definition data 2. when using build_header() user has to calculate offsets and lengths manually every time. While using aml_def_block() process is quite straightforward and follows the declarative flow as it is in ASL, which less confusing and error-prone, like: ssdt = aml_def_block(...); // add content to ssdt aml_append(tables_blob, ssdt); so hiding linker from user in this case makes usage simpler and more harder to screw up, which is the whole point of introducing this API. I have a further plans to make other tables use aml_def_block and get rid of acpi_add_table() which is also confusing and simplify RSDT building doing it transparently for user. That way both ARM and i386 targets would reuse the same code /API instead of re-implementing it in every target. User will just get complete tables blob from API and put it in respective fw_cfg file. I also wish that we have had only one file for ACPI tables (including RSDP) but ship already went away, maybe we can do it for ARM though. PS: My secret goal is to bury linker and use it only when one have to do so :), seriously. > > > > + break; > > + } > > + default: > > This should assert. sure > > > + break; > > + } > > + build_append_array(parent_ctx->buf, child->buf); > > +} > > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c > > index 6d84f38..237080f 100644 > > --- a/hw/i386/acpi-build.c > > +++ b/hw/i386/acpi-build.c > > @@ -258,7 +258,6 @@ static void acpi_get_pci_info(PcPciInfo *info) > > #define ACPI_BUILD_APPNAME6 "BOCHS " > > #define ACPI_BUILD_APPNAME4 "BXPC" > > > > -#define ACPI_BUILD_TABLE_FILE "etc/acpi/tables" > > #define ACPI_BUILD_RSDP_FILE "etc/acpi/rsdp" > > #define ACPI_BUILD_TPMLOG_FILE "etc/tpm/log" > > > > diff --git a/include/hw/acpi/aml-build.h b/include/hw/acpi/aml-build.h > > index 199f003..4033796 100644 > > --- a/include/hw/acpi/aml-build.h > > +++ b/include/hw/acpi/aml-build.h > > @@ -5,6 +5,67 @@ > > #include > > #include "qemu/compiler.h" > > > > +#define ACPI_BUILD_TABLE_FILE "etc/acpi/tables" > > + > > +typedef enum { > > + AML_HELPER = 0, > > + AML_NON_BLOCK, > > + AML_PACKAGE, > > + AML_EXT_PACKAGE, > > + AML_BUFFER, > > + AML_RES_TEMPLATE, > > + AML_DEF_BLOCK, > > +} AmlBlockFlags; > > + > > +struct Aml { > > + GArray *buf; > > + > > + /*< private >*/ > > + uint8_t op; > > + AmlBlockFlags block_flags; > > + GArray *linker; > > +}; > > +typedef struct Aml Aml; > > + > > +/** > > + * init_aml_allocator: > > + * @linker: linker that used by API for registering ACPI tables > > + * with linker firmware interfac > > + * > > + * Called for initializing API allocator which allow to use > > + * AML API. > > + * Returns: toplevel container which accumulates all other > > + * ACPI tables. > > + */ > > +Aml *init_aml_allocator(GArray *linker); > > + > > +/** > > + * free_aml_allocator: > > + * > > + * Releases all elements used by AML API, frees associated memory > > + * and invalidates AML allocator. After this call @init_aml_allocator > > + * should be called again if AML API is to be used again. > > + */ > > +void free_aml_allocator(void); > > + > > +/** > > + * aml_append: > > + * @parent_ctx: context to which @child element is added > > + * @child: element that is copied into @parent_ctx context > > + * > > + * Joins Aml elements together and helps to construct AML tables > > + * Examle of usage: > > + * Aml *table = aml_def_block("SSDT", ...); > > + * Aml *sb = aml_scope("\_SB"); > > + * Aml *dev = aml_device("PCI0"); > > + * > > + * aml_append(dev, aml_name_decl("HID", aml_eisaid("PNP0A03"))); > > + * aml_append(sb, dev); > > + * aml_append(table, sb); > > + */ > > +void aml_append(Aml *parent_ctx, Aml *child); > > + > > +/* other helpers */ > > GArray *build_alloc_array(void); > > void build_free_array(GArray *array); > > void build_prepend_byte(GArray *array, uint8_t val); > > -- > > 1.8.3.1 >