On Tue, 17 Feb 2015 20:46:32 +0100
"Michael S. Tsirkin" <m...@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 <imamm...@redhat.com>
>
> This should be split up, so we can see
> functionality added in the same patch where it's used.
> It's too hard to evaluate this alone.
I'll try to split it
>
>
> > ---
> > 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)
>
> What does this do?
that's block of code moved to helper you asked for in previous review
it finishes up AML buffer construction pretty much the same way as
build_package() does.
>
> > +{
> > + GArray *data = build_alloc_array();
> > +
> > + build_append_int(data, array->len);
> > + g_array_prepend_vals(array, data->data, data->len);
>
> Prepend is best avoided. Can't say whether it's necessary
> here.
can't be helped, it does similar thing to as adding PkgLength
>
> > + 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
>
> are treated?
sure
>
> > + * field is zero. [ACPI Spec 5.0, 6.4.2.9 End Tag]
>
> Pls always quote oldest spec, not latest one.
>
>
> > + */
> > + 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: {
> > + 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,
> > + 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);
> > + break;
> > + }
> > + default:
> > + 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"
>
> fw cfg stuff doesn't belong in aml generator.
>
> > +
> > +typedef enum {
> > + AML_HELPER = 0,
> > + AML_NON_BLOCK,
>
> What are the above two?
> The names aren't really helpful.
AML_HELPER - to distinguish terms that do not need extra processing
and just need to be copied into parent context, which is mostly
only helpers like aml_callXX(), terms put in resource template, e.t.c.
allocated by aml_alloc()
AML_NON_BLOCK - is used for simple inline terms with 1 byte opcode
allocated by aml_opcode(opcode)
the rest of the flags are used for more complex structures
which are allocated with aml_bundle(opcode, flags)
I've thought we came to agreement when we discussed this.
>
> > + AML_PACKAGE,
> > + AML_EXT_PACKAGE,
> > + AML_BUFFER,
>
> And what's a buffer?
DefBuffer := BufferOp PkgLength BufferSize ByteList
>
> > + 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
