Re: [PATCH v2 01/35] acpi: add helper routines to initialize ACPI tables

[Igor Mammedov]

On Thu, 2 Sep 2021 14:56:00 +0200
Eric Auger <eauger@redhat.com> wrote:

> Hi Igor,
> 
> On 7/8/21 5:45 PM, Igor Mammedov wrote:
> > Patch introduces acpi_init_table()/acpi_table_composed() API
> > that hides pointer/offset arithmetic from user as opposed
> > to build_header(), to prevent errors caused by it [1].
> > 
> >  acpi_init_table():
> >      initializes table header and keeps track of
> >      table data/offsets
> >  acpi_table_composed():
> >      sets actual table length and tells bios loader
> >      where table is for the later initialization on
> >      guest side.  
> might be worth to put those comments in the code as doc comments since
> "_composed" terminology is not self-explanatory?

I'll add doc comments as suggested.
A better idea how to name function is welcome as well?


> > 1) commits
> >    bb9feea43179 x86: acpi: use offset instead of pointer when using 
> > build_header()
> >    4d027afeb3a9 Virt: ACPI: fix qemu assert due to re-assigned table data 
> > address
> > 
> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > ---
> >  include/hw/acpi/aml-build.h | 14 +++++++++
> >  hw/acpi/aml-build.c         | 58 +++++++++++++++++++++++++++++++++++++
> >  2 files changed, 72 insertions(+)
> > 
> > diff --git a/include/hw/acpi/aml-build.h b/include/hw/acpi/aml-build.h
> > index 471266d739..d590660bd2 100644
> > --- a/include/hw/acpi/aml-build.h
> > +++ b/include/hw/acpi/aml-build.h
> > @@ -413,6 +413,20 @@ Aml *aml_concatenate(Aml *source1, Aml *source2, Aml 
> > *target);
> >  Aml *aml_object_type(Aml *object);
> >  
> >  void build_append_int_noprefix(GArray *table, uint64_t value, int size);
> > +
> > +typedef struct AcpiTable {
> > +    const char *sig;
> > +    const uint8_t rev;
> > +    const char *oem_id;
> > +    const char *oem_table_id;
> > +    /* private vars tracking table state */
> > +    GArray *array;
> > +    unsigned table_offset;
> > +} AcpiTable;
> > +
> > +void acpi_init_table(AcpiTable *desc, GArray *array);
> > +void acpi_table_composed(BIOSLinker *linker, AcpiTable *table);
> > +
> >  void
> >  build_header(BIOSLinker *linker, GArray *table_data,
> >               AcpiTableHeader *h, const char *sig, int len, uint8_t rev,
> > diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c
> > index d5103e6d7b..c598010144 100644
> > --- a/hw/acpi/aml-build.c
> > +++ b/hw/acpi/aml-build.c
> > @@ -52,6 +52,19 @@ static void build_append_byte(GArray *array, uint8_t val)
> >      g_array_append_val(array, val);
> >  }
> >  
> > +static void build_append_padded_str(GArray *array, const char *str,
> > +                                    size_t maxlen, char pad)
> > +{
> > +    size_t i;
> > +    size_t len = strlen(str);
> > +
> > +    g_assert(len <= maxlen);
> > +    g_array_append_vals(array, str, len);
> > +    for (i = maxlen - len; i > 0; i--) {
> > +        g_array_append_val(array, pad);
> > +    }
> > +}
> > +
> >  static void build_append_array(GArray *array, GArray *val)
> >  {
> >      g_array_append_vals(array, val->data, val->len);
> > @@ -1692,6 +1705,51 @@ Aml *aml_object_type(Aml *object)
> >      return var;
> >  }
> >  
> > +void acpi_init_table(AcpiTable *desc, GArray *array)
> > +{
> > +
> > +    desc->array = array;
> > +    desc->table_offset = array->len;
> > +
> > +    /*
> > +     * ACPI spec 1.0b
> > +     * 5.2.3 System Description Table Header
> > +     */
> > +    g_assert(strlen(desc->sig) == 4);
> > +    g_array_append_vals(array, desc->sig, 4); /* Signature */  
> build_append_padded_str?

it will do the job even if it's a bit of overkill,
signature must be 4 characters long so there is nothing to pad here
(at least till this day).
Using padded variant may confuse reader in the future,
so I'd prefer to keep this line as is.


> > +    build_append_int_noprefix(array, 0, 4); /* Length */
> > +    build_append_int_noprefix(array, desc->rev, 1); /* Revision */
> > +    build_append_int_noprefix(array, 0, 1); /* Checksum */
> > +    build_append_padded_str(array, desc->oem_id, 6, ' '); /* OEMID */
> > +    /* OEM Table ID */
> > +    build_append_padded_str(array, desc->oem_table_id, 8, ' ');
> > +    build_append_int_noprefix(array, 1, 4); /* OEM Revision */
> > +    g_array_append_vals(array, ACPI_BUILD_APPNAME8, 4); /* Creator ID */

here we potentially can reuse build_append_padded_str() if we
remove padding in ACPI_BUILD_APPNAME8, but that should wait till
refactoring is complete (to avoid breaking incremental refactoring)

> > +    build_append_int_noprefix(array, 1, 4); /* Creator Revision */
> > +}
> > +
> > +void acpi_table_composed(BIOSLinker *linker, AcpiTable *desc)
> > +{
> > +    /*
> > +     * ACPI spec 1.0b
> > +     * 5.2.3 System Description Table Header
> > +     * Table 5-2 DESCRIPTION_HEADER Fields
> > +     */
> > +    const unsigned checksum_offset = 9;
> > +    uint32_t table_len = desc->array->len - desc->table_offset;
> > +    uint32_t table_len_le = cpu_to_le32(table_len);
> > +    gchar *len_ptr = &desc->array->data[desc->table_offset + 4];
> > +
> > +    /* patch "Length" field that has been reserved by acpi_init_table()
> > +     * to the actual length, i.e. accumulated table length from
> > +     * acpi_init_table() till acpi_table_composed()
> > +     */
> > +    memcpy(len_ptr, &table_len_le, sizeof table_len_le);  
> can't you use a garray/build_append function instead to be homogeneous
> with the rest of the code?
those are for inserting/adding _new_ values, and won't work here,
here we are patching value in place, comment above was supposed
to clarify that (I guess it wasn't sufficient),
Care to suggest a better comment?

> > +
> > +    bios_linker_loader_add_checksum(linker, ACPI_BUILD_TABLE_FILE,
> > +        desc->table_offset, table_len, desc->table_offset + 
> > checksum_offset);
> > +}
> > +
> >  void
> >  build_header(BIOSLinker *linker, GArray *table_data,
> >               AcpiTableHeader *h, const char *sig, int len, uint8_t rev,
> >   
> 
> Thanks
> 
> Eric
>