From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:53618) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YEeTd-0006nJ-Ag for qemu-devel@nongnu.org; Fri, 23 Jan 2015 08:40:50 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YEeTY-0006dN-6O for qemu-devel@nongnu.org; Fri, 23 Jan 2015 08:40:45 -0500 Received: from mx1.redhat.com ([209.132.183.28]:53387) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YEeTX-0006d9-Vu for qemu-devel@nongnu.org; Fri, 23 Jan 2015 08:40:40 -0500 Date: Fri, 23 Jan 2015 14:40:30 +0100 From: Igor Mammedov Message-ID: <20150123144030.21142ed1@nial.brq.redhat.com> In-Reply-To: <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> <20150123132424.GA4579@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit 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: "Michael S. Tsirkin" Cc: pbonzini@redhat.com, drjones@redhat.com, claudio.fontana@huawei.com, qemu-devel@nongnu.org, marcel.a@redhat.com On Fri, 23 Jan 2015 15:24:24 +0200 "Michael S. Tsirkin" wrote: > 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? build_prepend_int() is a static/non public function, build_buffer() will also be static/non public function for use only by API internals. I pretty much hate long build_append_foo() names so I'm hiding all lowlevel constructs and try to expose only high-level ASL ones. Which makes me to think that we need to use asl_ prefix for API calls instead of acpi_ or aml_. > > > 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