Re: [Qemu-devel] [PATCH v3 01/52] acpi: introduce AML composer aml_append()

["Michael S. Tsirkin" <mst@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" <mst@redhat.com> 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 <imammedo@redhat.com>
> > > ---
> > >  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 <stdbool.h>
> > >  #include <string.h>
> > >  #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 <glib.h>
> > >  #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
> >