| Message ID | 20210708154617.1538485-2-imammedo@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | acpi: refactor error prone build_header() and packed structures usage in ACPI tables | expand |
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? > > 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? > + 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]; > + > + /* 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? > + > + 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
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 >
Hi Igor, On 9/3/21 9:12 AM, 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? acpi_table_build_init()/_complete() or somethink like that giving an idea of what it does actually. But you would be obliged to revisit all the patches. I think it is good enough if you add the comment. > > >>> 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. sure it was just a nit > > >>> + 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) OK > >>> + 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? I thought g_array_insert_vals() could do the job. No the comment was clear to me actually. Maybe in acpi_init_table, along with Length comment you add a comment saying actual value will be set in composed(). Eric > >>> + >>> + 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 >> >
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? Then one of the classical pairs will work well, e.g. acpi_table_begin / acpi_table_end or maybe 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 > >
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 > > > >
On Fri, 3 Sep 2021 09:22:10 +0200 Eric Auger <eauger@redhat.com> wrote: > Hi Igor, > > On 9/3/21 9:12 AM, 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? > acpi_table_build_init()/_complete() or somethink like that giving an > idea of what it does actually. But you would be obliged to revisit all > the patches. I think it is good enough if you add the comment. it's fine to rewrite before it's merged, one of the points of refactoring is to make current code more clear and concise. I'll use Michael's suggestion acpi_table_begin / acpi_table_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. > > sure it was just a nit > > > > > >>> + 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) > > OK > > > >>> + 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? > > I thought g_array_insert_vals() could do the job. No the comment was > clear to me actually. Maybe in acpi_init_table, along with Length > comment you add a comment saying actual value will be set in composed(). sure, I'll add that. > > Eric > > > >>> + > >>> + 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 > >> > > >
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_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]; + + /* 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); + + 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,
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 <imammedo@redhat.com> --- include/hw/acpi/aml-build.h | 14 +++++++++ hw/acpi/aml-build.c | 58 +++++++++++++++++++++++++++++++++++++ 2 files changed, 72 insertions(+)