Hi Igor,
On 9/3/21 9:12 AM, Igor Mammedov wrote:
> On Thu, 2 Sep 2021 14:56:00 +0200
> Eric Auger <eau...@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 <imamm...@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
>>
>
