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

[Igor Mammedov]

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

> On Tue, Feb 17, 2015 at 05:47:26PM +0100, Igor Mammedov wrote:
> > 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
> 
> So spec explains it very badly, but these all are
> fixed width fields.
> Filling them in using add_byte just makes for hard to
> read code.
If it were only add_byte, I'd agree but it basically composes
by whole fields and in this particular case is easy to
verify just by looking to spec, just like the rest of AML
terms does.

And one doesn't have to care about endianness because
build_append_value() packs integer fields as expected
by spec as opposed to using C struct-s.

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