On Tue, 29 Jun 2021 15:16:42 -0400
"Michael S. Tsirkin" <m...@redhat.com> wrote:
> On Fri, Jun 25, 2021 at 05:17:44AM -0400, Igor Mammedov wrote:
> > 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 it's for later initialization on guest side.
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.
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 <imamm...@redhat.com>
> > ---
> > include/hw/acpi/aml-build.h | 14 ++++++++++
> > hw/acpi/aml-build.c | 53 +++++++++++++++++++++++++++++++++++++
> > 2 files changed, 67 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..576fc0238c 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,46 @@ 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_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 */
> > + 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];
> > +
> > + memcpy(len_ptr, &table_len_le, sizeof table_len_le);
>
> ok so this patches in the correct length.
> Not sure how to make it prettier ... at least please
> add comments explaining what's going on here.
it's not prettier, combo of acpi_init_table()/acpi_table_composed()
intention is to hide offsets manipulation from callers
in the most usecases. (that's where we used to have bugs).
I have this explained in cover letter, but I lost it when
writing down this commit message (see updated commit message above)
how about:
/* patch table length reserved by acpi_init_table()
* to the actual length, i.e. accumulated table length from
* acpi_init_table() till acpi_table_composed()
*/
>
> > + 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,
> > --
> > 2.27.0
>
