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

[Igor Mammedov]

On Sat, 4 Sep 2021 15:57:52 -0400
"Michael S. Tsirkin" <mst@redhat.com> wrote:

> On Fri, Sep 03, 2021 at 09:12:21AM +0200, Igor Mammedov wrote:
> > 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?  
> 
> Aren't these a pair? acpi_init_table is called before you
> start composing it, acpi_table_composed after it's composed?
yep, that's the basic idea.

 
> Then one of the classical pairs will work well, e.g.
> acpi_table_begin / acpi_table_end or maybe

I'll use this on respin,
 as more concise compared to _compose_

> acpi_table_compose_begin / acpi_table_compose_end .
> 
> 
> >   
> > > > 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
> > >   
>