From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:49660) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YEeE2-0008Ln-61 for qemu-devel@nongnu.org; Fri, 23 Jan 2015 08:24:39 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YEeDy-0008Tz-T8 for qemu-devel@nongnu.org; Fri, 23 Jan 2015 08:24:38 -0500 Received: from mx1.redhat.com ([209.132.183.28]:52404) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YEeDy-0008Tt-Ll for qemu-devel@nongnu.org; Fri, 23 Jan 2015 08:24:34 -0500 Date: Fri, 23 Jan 2015 15:24:24 +0200 From: "Michael S. Tsirkin" Message-ID: <20150123132424.GA4579@redhat.com> References: <1421938231-25698-1-git-send-email-imammedo@redhat.com> <1421938231-25698-2-git-send-email-imammedo@redhat.com> <20150123081119.GE26711@redhat.com> <20150123113529.7954abc0@nial.brq.redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20150123113529.7954abc0@nial.brq.redhat.com> Subject: Re: [Qemu-devel] [PATCH v2 01/47] acpi: introduce AML composer aml_append() List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Igor Mammedov Cc: pbonzini@redhat.com, drjones@redhat.com, claudio.fontana@huawei.com, qemu-devel@nongnu.org, marcel.a@redhat.com On Fri, Jan 23, 2015 at 11:35:29AM +0100, Igor Mammedov wrote: > On Fri, 23 Jan 2015 10:11:19 +0200 > "Michael S. Tsirkin" wrote: > > > On Thu, Jan 22, 2015 at 02:49:45PM +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: > > > > > > AcpiAml scope = acpi_scope("PCI0") > > > AcpiAml dev = acpi_device("PM") > > > aml_append(&dev, acpi_name_decl("_ADR", acpi_int(addr))) > > > aml_append(&scope, dev); > > > > > > Signed-off-by: Igor Mammedov > > > --- > > > hw/acpi/acpi-build-utils.c | 39 ++++++++++++++++++++++++++++++++++++++ > > > include/hw/acpi/acpi-build-utils.h | 16 ++++++++++++++++ > > > 2 files changed, 55 insertions(+) > > > > > > diff --git a/hw/acpi/acpi-build-utils.c b/hw/acpi/acpi-build-utils.c > > > index 602e68c..547ecaa 100644 > > > --- a/hw/acpi/acpi-build-utils.c > > > +++ b/hw/acpi/acpi-build-utils.c > > > @@ -267,3 +267,42 @@ void build_append_int(GArray *table, uint32_t value) > > > build_append_value(table, value, 4); > > > } > > > } > > > + > > > +static void build_prepend_int(GArray *array, uint32_t value) > > > +{ > > > + GArray *data = build_alloc_array(); > > > + > > > + build_append_int(data, value); > > > + g_array_prepend_vals(array, data->data, data->len); > > > + build_free_array(data); > > > +} > > > > I don't think prepend is generally justified: > > it makes code hard to follow and debug. > > > > Adding length is different: of course you need > > to first have the package before you can add length. > > > > We currently have build_prepend_package_length - just move it > > to utils, and use everywhere. > [...] > > > + case BUFFER: > > > + build_prepend_int(child.buf, child.buf->len); > > > + build_package(child.buf, child.op); > Buffer uses the same concept as package, but adds its own additional length. > Therefore I've added build_prepend_int(), > I can create build_buffer() and mimic build_package() Sounds good, pls do. The point is to avoid generic prepend calls as an external API. > but it won't change picture. It's a better API - what is meant by picture? > As for moving to to another file, during all this series lowlevel > build_(some_aml_related_costruct_helper)s are moved into this file > and should be make static to hide from user lowlevel helpers > (including build_package). > That will leave only high level API available. > > TODO for me: make sure that moved lowlevel helpers are static > > > > > + break; > > > + default: > > > + break; > > > + } > > > + build_append_array(parent_ctx->buf, child.buf); > > > + build_free_array(child.buf); > > > +} > > > diff --git a/include/hw/acpi/acpi-build-utils.h b/include/hw/acpi/acpi-build-utils.h > > > index 199f003..64e7ec3 100644 > > > --- a/include/hw/acpi/acpi-build-utils.h > > > +++ b/include/hw/acpi/acpi-build-utils.h > > > @@ -5,6 +5,22 @@ > > > #include > > > #include "qemu/compiler.h" > > > > > > +typedef enum { > > > + NON_BLOCK, > > > + PACKAGE, > > > + EXT_PACKAGE, > > > + BUFFER, > > > + RES_TEMPLATE, > > > +} AcpiBlockFlags; > > > + > > > +typedef struct AcpiAml { > > > + GArray *buf; > > > + uint8_t op; > > > + AcpiBlockFlags block_flags; > > > +} AcpiAml; > > > + > > > +void aml_append(AcpiAml *parent_ctx, AcpiAml child); > > > + > > > GArray *build_alloc_array(void); > > > void build_free_array(GArray *array); > > > void build_prepend_byte(GArray *array, uint8_t val); > > > -- > > > 1.8.3.1