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

[Igor Mammedov]

On Tue, 17 Feb 2015 17:35:55 +0100
"Michael S. Tsirkin" <address@hidden> wrote:

> On Mon, Feb 09, 2015 at 10:53:27AM +0000, Igor Mammedov wrote:
> > Signed-off-by: Igor Mammedov <address@hidden>
> > ---
> >  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.
it does, in ACPI 5.1,

20.2.1 Table and Table Header Encoding
AMLCode := DefBlockHeader TermList
DefBlockHeader := TableSignature TableLength SpecCompliance CheckSum OemID
OemTableID OemRevision CreatorID CreatorRevision

> 
> 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