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

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

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" <mst@redhat.com> wrote:
> 
> > 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.
> 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

Sorry I'm not merging this in this form, we should use structures
where possible, this is where C shines, just use the language
properly.


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