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

[Michael S. Tsirkin]

On Wed, Feb 18, 2015 at 10:50:54AM +0100, Igor Mammedov wrote:
> 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.

things like
        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);
are kind of ugly, are they not? 8 is repeated 3 times here,
and you need to go look at spec to figure out what 8 stands for.

Besides, we aren't yet getting rid of build_header, are we?
How about this, we get rest of this code in, then
work on rewriting build_header everywhere?

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