Re: [Qemu-devel] [PATCH v3 05/52] acpi: add aml_def_block() term

["Michael S. Tsirkin" <mst@redhat.com>]

On Mon, Feb 09, 2015 at 10:53:27AM +0000, Igor Mammedov wrote:
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
>  hw/acpi/aml-build.c         | 38 ++++++++++++++++++++++++++++++++++++++
>  hw/i386/acpi-build.c        |  4 ----
>  include/hw/acpi/aml-build.h | 10 ++++++++++
>  3 files changed, 48 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
> index 67d1371..cb1a1bd 100644
> --- a/hw/acpi/aml-build.c
> +++ b/hw/acpi/aml-build.c
> @@ -335,3 +335,41 @@ void aml_append(Aml *parent_ctx, Aml *child)
>      }
>      build_append_array(parent_ctx->buf, child->buf);
>  }
> +
> +/*
> + * ACPI 1.0b: 16.2.1 Top Level AML
> + *            5.2.3 System Description Table Header
> + *
> + * ACPI 5.0: 20.2.1 Table and Table Header Encoding
> + */
> +Aml *aml_def_block(const char *signature, uint8_t revision,
> +                   const char *oem_id, const char *oem_table_id,
> +                   uint32_t oem_revision, uint32_t creator_id,
> +                   uint32_t creator_revision)
> +{
> +    int len;
> +    Aml *var = aml_alloc();
> +    var->block_flags = AML_DEF_BLOCK;
> +
> +    assert(strlen(signature) == 4);
> +    g_array_append_vals(var->buf, signature, 4);
> +    build_append_value(var->buf, 0, 4); /* Length place holder */
> +    build_append_byte(var->buf, revision);
> +    build_append_byte(var->buf, 0); /* place holder for Checksum */
> +
> +    len = strlen(oem_id);
> +    assert(len <= 6);
> +    g_array_append_vals(var->buf, oem_id, len);
> +    g_array_append_vals(var->buf, "\0\0\0\0\0\0\0\0", 6 - len);
> +
> +    len = strlen(oem_table_id);
> +    assert(len <= 8);
> +    g_array_append_vals(var->buf, oem_table_id, len);
> +    g_array_append_vals(var->buf, "\0\0\0\0\0\0\0\0", 8 - len);
> +
> +    build_append_value(var->buf, oem_revision, 4);
> +    build_append_value(var->buf, creator_id, 4);
> +    build_append_value(var->buf, creator_revision, 4);
> +
> +    return var;


Please don't do this.
First, this is not a definition block encoding, so name
is wrong.

Second, we already have functions to fill in headers.
So just call them.

Filling structures byte by byte is unreadable -
I know ACPI spec often makes us do this but
when it doesn't, we should not do it.

Pls keep using build_header for now.


> +}
> diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> index a1bf450..553c86b 100644
> --- a/hw/i386/acpi-build.c
> +++ b/hw/i386/acpi-build.c
> @@ -254,10 +254,6 @@ static void acpi_get_pci_info(PcPciInfo *info)
>                                              NULL);
>  }
>  
> -#define ACPI_BUILD_APPNAME  "Bochs"
> -#define ACPI_BUILD_APPNAME6 "BOCHS "
> -#define ACPI_BUILD_APPNAME4 "BXPC"
> -
>  #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 4033796..2610336 100644
> --- a/include/hw/acpi/aml-build.h
> +++ b/include/hw/acpi/aml-build.h
> @@ -6,6 +6,10 @@
>  #include "qemu/compiler.h"
>  
>  #define ACPI_BUILD_TABLE_FILE "etc/acpi/tables"
> +#define ACPI_BUILD_APPNAME  "Bochs"
> +#define ACPI_BUILD_APPNAME6 "BOCHS "
> +#define ACPI_BUILD_APPNAME4 "BXPC"
> +#define ACPI_BUILD_APPNAME4_HEX 0x43505842
>  
>  typedef enum {
>      AML_HELPER = 0,
> @@ -65,6 +69,12 @@ void free_aml_allocator(void);
>   */
>  void aml_append(Aml *parent_ctx, Aml *child);
>  
> +/* Block AML object primitives */
> +Aml *aml_def_block(const char *signature, uint8_t revision,
> +                   const char *oem_id, const char *oem_table_id,
> +                   uint32_t oem_revision, uint32_t creator_id,
> +                   uint32_t creator_revision);
> +
>  /* other helpers */
>  GArray *build_alloc_array(void);
>  void build_free_array(GArray *array);
> -- 
> 1.8.3.1