From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:33505) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YNnZP-0001Kw-6A for qemu-devel@nongnu.org; Tue, 17 Feb 2015 14:12:32 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YNnZK-0008GG-FB for qemu-devel@nongnu.org; Tue, 17 Feb 2015 14:12:31 -0500 Received: from mx1.redhat.com ([209.132.183.28]:49447) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YNnZK-0008G6-58 for qemu-devel@nongnu.org; Tue, 17 Feb 2015 14:12:26 -0500 Date: Tue, 17 Feb 2015 20:12:16 +0100 From: "Michael S. Tsirkin" Message-ID: <20150217191216.GA29473@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> <20150217185011.437bf89d@nial.brq.redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20150217185011.437bf89d@nial.brq.redhat.com> 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: Igor Mammedov Cc: drjones@redhat.com, zhaoshenglong@huawei.com, claudio.fontana@huawei.com, qemu-devel@nongnu.org, marcel.a@redhat.com On Tue, Feb 17, 2015 at 06:50:11PM +0100, Igor Mammedov wrote: > 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. definition block is not an AML thing, is it? It's an ASL construct I think. AML calls them tables, e.g.: 20.2.1 Table and Table Header Encoding > > 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. We have different definitions of scary. C structures is the standard way to lay out memory. > > > > > + 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() So just call 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. It's not easy to use, but it's not too hard either. As there's a single instance now, let's merge the rest, we'll work on a cleaner interface for build_header. this is not it. > 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. Unlikely. > PS: > My secret goal is to bury linker and use it only when one > have to do so :), seriously. You are doing too many things at once. We need a minimal patchset to start merging your work. so drop this part please, just use build_header. > > > > > > > + 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 > >