| Message ID | 20210907144814.741785-13-imammedo@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | acpi: refactor error prone build_header() and packed structures usage in ACPI tables | expand |
On 9/7/21 4:47 PM, Igor Mammedov wrote: > it replaces error-prone pointer arithmetic for build_header() API, > with 2 calls to start and finish table creation, > which hides offsets magic from API user. > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > --- > v3: > * s/acpi_init_table|acpi_table_composed/acpi_table_begin|acpi_table_end/ > --- > hw/acpi/vmgenid.c | 16 +++++++--------- > 1 file changed, 7 insertions(+), 9 deletions(-) > > diff --git a/hw/acpi/vmgenid.c b/hw/acpi/vmgenid.c > index 4f41a13ea0..95f94a2510 100644 > --- a/hw/acpi/vmgenid.c > +++ b/hw/acpi/vmgenid.c > @@ -29,6 +29,8 @@ void vmgenid_build_acpi(VmGenIdState *vms, GArray *table_data, GArray *guid, > Aml *ssdt, *dev, *scope, *method, *addr, *if_ctx; > uint32_t vgia_offset; > QemuUUID guid_le; > + AcpiTable table = { .sig = "SSDT", .rev = 1, > + .oem_id = oem_id, .oem_table_id = "VMGENID" }; > > /* Fill in the GUID values. These need to be converted to little-endian > * first, since that's what the guest expects > @@ -42,15 +44,12 @@ void vmgenid_build_acpi(VmGenIdState *vms, GArray *table_data, GArray *guid, > g_array_insert_vals(guid, VMGENID_GUID_OFFSET, guid_le.data, > ARRAY_SIZE(guid_le.data)); > > - /* Put this in a separate SSDT table */ > + /* Put VMGNEID into a separate SSDT table */ > + acpi_table_begin(&table, table_data); > ssdt = init_aml_allocator(); > > - /* Reserve space for header */ > - acpi_data_push(ssdt->buf, sizeof(AcpiTableHeader)); > - > /* Storage for the GUID address */ > - vgia_offset = table_data->len + > - build_append_named_dword(ssdt->buf, "VGIA"); > + vgia_offset = table_data->len + build_append_named_dword(ssdt->buf, "VGIA"); not mandated but well > scope = aml_scope("\\_SB"); > dev = aml_device("VGEN"); > aml_append(dev, aml_name_decl("_HID", aml_string("QEMUVGID"))); > @@ -116,9 +115,8 @@ void vmgenid_build_acpi(VmGenIdState *vms, GArray *table_data, GArray *guid, > ACPI_BUILD_TABLE_FILE, vgia_offset, sizeof(uint32_t), > VMGENID_GUID_FW_CFG_FILE, 0); > > - build_header(linker, table_data, > - (void *)(table_data->data + table_data->len - ssdt->buf->len), > - "SSDT", ssdt->buf->len, 1, oem_id, "VMGENID"); > + /* must be called after above command to ensure correct table checksum */ ditto > + acpi_table_end(linker, &table); > free_aml_allocator(); > } > > Reviewed-by: Eric Auger <eric.auger@redhat.com> Eric
On Wed, 22 Sep 2021 09:23:16 +0200 Eric Auger <eauger@redhat.com> wrote: > On 9/7/21 4:47 PM, Igor Mammedov wrote: > > it replaces error-prone pointer arithmetic for build_header() API, > > with 2 calls to start and finish table creation, > > which hides offsets magic from API user. > > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > > --- > > v3: > > * s/acpi_init_table|acpi_table_composed/acpi_table_begin|acpi_table_end/ > > --- > > hw/acpi/vmgenid.c | 16 +++++++--------- > > 1 file changed, 7 insertions(+), 9 deletions(-) > > > > diff --git a/hw/acpi/vmgenid.c b/hw/acpi/vmgenid.c > > index 4f41a13ea0..95f94a2510 100644 > > --- a/hw/acpi/vmgenid.c > > +++ b/hw/acpi/vmgenid.c > > @@ -29,6 +29,8 @@ void vmgenid_build_acpi(VmGenIdState *vms, GArray *table_data, GArray *guid, > > Aml *ssdt, *dev, *scope, *method, *addr, *if_ctx; > > uint32_t vgia_offset; > > QemuUUID guid_le; > > + AcpiTable table = { .sig = "SSDT", .rev = 1, > > + .oem_id = oem_id, .oem_table_id = "VMGENID" }; > > > > /* Fill in the GUID values. These need to be converted to little-endian > > * first, since that's what the guest expects > > @@ -42,15 +44,12 @@ void vmgenid_build_acpi(VmGenIdState *vms, GArray *table_data, GArray *guid, > > g_array_insert_vals(guid, VMGENID_GUID_OFFSET, guid_le.data, > > ARRAY_SIZE(guid_le.data)); > > > > - /* Put this in a separate SSDT table */ > > + /* Put VMGNEID into a separate SSDT table */ > > + acpi_table_begin(&table, table_data); > > ssdt = init_aml_allocator(); > > > > - /* Reserve space for header */ > > - acpi_data_push(ssdt->buf, sizeof(AcpiTableHeader)); > > - > > /* Storage for the GUID address */ > > - vgia_offset = table_data->len + > > - build_append_named_dword(ssdt->buf, "VGIA"); > > + vgia_offset = table_data->len + build_append_named_dword(ssdt->buf, "VGIA"); > not mandated but well dropped > > > scope = aml_scope("\\_SB"); > > dev = aml_device("VGEN"); > > aml_append(dev, aml_name_decl("_HID", aml_string("QEMUVGID"))); > > @@ -116,9 +115,8 @@ void vmgenid_build_acpi(VmGenIdState *vms, GArray *table_data, GArray *guid, > > ACPI_BUILD_TABLE_FILE, vgia_offset, sizeof(uint32_t), > > VMGENID_GUID_FW_CFG_FILE, 0); > > > > - build_header(linker, table_data, > > - (void *)(table_data->data + table_data->len - ssdt->buf->len), > > - "SSDT", ssdt->buf->len, 1, oem_id, "VMGENID"); > > + /* must be called after above command to ensure correct table checksum */ > ditto I'd prefer to keep this reminder until there is a way to enforce valid order. > > + acpi_table_end(linker, &table); > > free_aml_allocator(); > > } > > > > > Reviewed-by: Eric Auger <eric.auger@redhat.com> > > Eric >
diff --git a/hw/acpi/vmgenid.c b/hw/acpi/vmgenid.c index 4f41a13ea0..95f94a2510 100644 --- a/hw/acpi/vmgenid.c +++ b/hw/acpi/vmgenid.c @@ -29,6 +29,8 @@ void vmgenid_build_acpi(VmGenIdState *vms, GArray *table_data, GArray *guid, Aml *ssdt, *dev, *scope, *method, *addr, *if_ctx; uint32_t vgia_offset; QemuUUID guid_le; + AcpiTable table = { .sig = "SSDT", .rev = 1, + .oem_id = oem_id, .oem_table_id = "VMGENID" }; /* Fill in the GUID values. These need to be converted to little-endian * first, since that's what the guest expects @@ -42,15 +44,12 @@ void vmgenid_build_acpi(VmGenIdState *vms, GArray *table_data, GArray *guid, g_array_insert_vals(guid, VMGENID_GUID_OFFSET, guid_le.data, ARRAY_SIZE(guid_le.data)); - /* Put this in a separate SSDT table */ + /* Put VMGNEID into a separate SSDT table */ + acpi_table_begin(&table, table_data); ssdt = init_aml_allocator(); - /* Reserve space for header */ - acpi_data_push(ssdt->buf, sizeof(AcpiTableHeader)); - /* Storage for the GUID address */ - vgia_offset = table_data->len + - build_append_named_dword(ssdt->buf, "VGIA"); + vgia_offset = table_data->len + build_append_named_dword(ssdt->buf, "VGIA"); scope = aml_scope("\\_SB"); dev = aml_device("VGEN"); aml_append(dev, aml_name_decl("_HID", aml_string("QEMUVGID"))); @@ -116,9 +115,8 @@ void vmgenid_build_acpi(VmGenIdState *vms, GArray *table_data, GArray *guid, ACPI_BUILD_TABLE_FILE, vgia_offset, sizeof(uint32_t), VMGENID_GUID_FW_CFG_FILE, 0); - build_header(linker, table_data, - (void *)(table_data->data + table_data->len - ssdt->buf->len), - "SSDT", ssdt->buf->len, 1, oem_id, "VMGENID"); + /* must be called after above command to ensure correct table checksum */ + acpi_table_end(linker, &table); free_aml_allocator(); }
it replaces error-prone pointer arithmetic for build_header() API, with 2 calls to start and finish table creation, which hides offsets magic from API user. Signed-off-by: Igor Mammedov <imammedo@redhat.com> --- v3: * s/acpi_init_table|acpi_table_composed/acpi_table_begin|acpi_table_end/ --- hw/acpi/vmgenid.c | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-)