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

[Michael S. Tsirkin]

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 <address@hidden>
> ---
>  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.


> +
> +void aml_append(AcpiAml *parent_ctx, AcpiAml child)
> +{
> +    switch (child.block_flags) {
> +    case EXT_PACKAGE:
> +        build_extop_package(child.buf, child.op);
> +        break;
> +
> +    case PACKAGE:
> +        build_package(child.buf, child.op);
> +        break;
> +
> +    case 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]
> +         */

Bad english. Let's quote verbatim:
        If the checksum field is zero, the
        resource data is treated as if the checksum operation succeeded.
Also best to quote earliest spec available so we know it's
compatible with old guests:
Spec 1.0b, 6.4.2.8 End Tag


> +        build_append_byte(child.buf, 0);
> +        /* fall through, to pack resources in buffer */
> +    case BUFFER:
> +        build_prepend_int(child.buf, child.buf->len);
> +        build_package(child.buf, child.op);
> +        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 <glib.h>
>  #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