| Message ID | 20210907144814.741785-34-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:48 PM, Igor Mammedov wrote: > Drop usage of packed structures and explicit endian > conversions when building table and use endian agnostic > build_append_int_noprefix() API to build it. > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > --- > CC: marcel.apfelbaum@gmail.com > --- > include/hw/acpi/acpi-defs.h | 14 -------------- > include/hw/acpi/aml-build.h | 1 + > hw/acpi/aml-build.c | 2 +- > hw/i386/acpi-build.c | 18 ++++++++++++++---- > 4 files changed, 16 insertions(+), 19 deletions(-) > > diff --git a/include/hw/acpi/acpi-defs.h b/include/hw/acpi/acpi-defs.h > index 0b375e7589..1a0774edd6 100644 > --- a/include/hw/acpi/acpi-defs.h > +++ b/include/hw/acpi/acpi-defs.h > @@ -117,18 +117,4 @@ typedef struct AcpiFadtData { > #define ACPI_FADT_ARM_PSCI_COMPLIANT (1 << 0) > #define ACPI_FADT_ARM_PSCI_USE_HVC (1 << 1) > > -/* > - * ACPI 1.0 Firmware ACPI Control Structure (FACS) > - */ > -struct AcpiFacsDescriptorRev1 { > - uint32_t signature; /* ACPI Signature */ > - uint32_t length; /* Length of structure, in bytes */ > - uint32_t hardware_signature; /* Hardware configuration signature */ > - uint32_t firmware_waking_vector; /* ACPI OS waking vector */ > - uint32_t global_lock; /* Global Lock */ > - uint32_t flags; > - uint8_t resverved3 [40]; /* Reserved - must be zero */ > -} QEMU_PACKED; > -typedef struct AcpiFacsDescriptorRev1 AcpiFacsDescriptorRev1; > - > #endif > diff --git a/include/hw/acpi/aml-build.h b/include/hw/acpi/aml-build.h > index 6e1f42e119..6f3d1de1b1 100644 > --- a/include/hw/acpi/aml-build.h > +++ b/include/hw/acpi/aml-build.h > @@ -413,6 +413,7 @@ 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); > +void build_append_byte(GArray *array, uint8_t val); what's the rationale behind changes related to build_append_byte? > > typedef struct AcpiTable { > const char *sig; > diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c > index 050fdb3f37..4135b385e5 100644 > --- a/hw/acpi/aml-build.c > +++ b/hw/acpi/aml-build.c > @@ -47,7 +47,7 @@ static void build_prepend_byte(GArray *array, uint8_t val) > g_array_prepend_val(array, val); > } > > -static void build_append_byte(GArray *array, uint8_t val) > +void build_append_byte(GArray *array, uint8_t val) > { > g_array_append_val(array, val); > } > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c > index 9f888d5a2c..547cd4ed9d 100644 > --- a/hw/i386/acpi-build.c > +++ b/hw/i386/acpi-build.c > @@ -345,13 +345,23 @@ static void acpi_align_size(GArray *blob, unsigned align) > g_array_set_size(blob, ROUND_UP(acpi_data_len(blob), align)); > } > > -/* FACS */ > +/* > + * ACPI spec 1.0b, > + * 5.2.6 Firmware ACPI Control Structure > + */ > static void > build_facs(GArray *table_data) > { > - AcpiFacsDescriptorRev1 *facs = acpi_data_push(table_data, sizeof *facs); > - memcpy(&facs->signature, "FACS", 4); > - facs->length = cpu_to_le32(sizeof(*facs)); > + const char *sig = "FACS"; > + const uint8_t reserved[40] = {}; > + > + g_array_append_vals(table_data, sig, 4); /* Signature */ > + build_append_int_noprefix(table_data, 64, 4); /* Length */ > + build_append_int_noprefix(table_data, 0, 4); /* Hardware Signature */ > + build_append_int_noprefix(table_data, 0, 4); /* Firmware Waking Vector */ > + build_append_int_noprefix(table_data, 0, 4); /* Global Lock */ > + build_append_int_noprefix(table_data, 0, 4); /* Flags */ > + g_array_append_vals(table_data, reserved, 40); /* Reserved */ > } > > static void build_append_pcihp_notify_entry(Aml *method, int slot) > Without thoses changes Reviewed-by: Eric Auger <eric.auger@redhat.com>
On Wed, 22 Sep 2021 15:33:27 +0200 Eric Auger <eauger@redhat.com> wrote: > On 9/7/21 4:48 PM, Igor Mammedov wrote: > > Drop usage of packed structures and explicit endian > > conversions when building table and use endian agnostic > > build_append_int_noprefix() API to build it. > > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > > --- > > CC: marcel.apfelbaum@gmail.com > > --- > > include/hw/acpi/acpi-defs.h | 14 -------------- > > include/hw/acpi/aml-build.h | 1 + > > hw/acpi/aml-build.c | 2 +- > > hw/i386/acpi-build.c | 18 ++++++++++++++---- > > 4 files changed, 16 insertions(+), 19 deletions(-) > > > > diff --git a/include/hw/acpi/acpi-defs.h b/include/hw/acpi/acpi-defs.h > > index 0b375e7589..1a0774edd6 100644 > > --- a/include/hw/acpi/acpi-defs.h > > +++ b/include/hw/acpi/acpi-defs.h > > @@ -117,18 +117,4 @@ typedef struct AcpiFadtData { > > #define ACPI_FADT_ARM_PSCI_COMPLIANT (1 << 0) > > #define ACPI_FADT_ARM_PSCI_USE_HVC (1 << 1) > > > > -/* > > - * ACPI 1.0 Firmware ACPI Control Structure (FACS) > > - */ > > -struct AcpiFacsDescriptorRev1 { > > - uint32_t signature; /* ACPI Signature */ > > - uint32_t length; /* Length of structure, in bytes */ > > - uint32_t hardware_signature; /* Hardware configuration signature */ > > - uint32_t firmware_waking_vector; /* ACPI OS waking vector */ > > - uint32_t global_lock; /* Global Lock */ > > - uint32_t flags; > > - uint8_t resverved3 [40]; /* Reserved - must be zero */ > > -} QEMU_PACKED; > > -typedef struct AcpiFacsDescriptorRev1 AcpiFacsDescriptorRev1; > > - > > #endif > > diff --git a/include/hw/acpi/aml-build.h b/include/hw/acpi/aml-build.h > > index 6e1f42e119..6f3d1de1b1 100644 > > --- a/include/hw/acpi/aml-build.h > > +++ b/include/hw/acpi/aml-build.h > > @@ -413,6 +413,7 @@ 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); > > +void build_append_byte(GArray *array, uint8_t val); > what's the rationale behind changes related to build_append_byte? looks like stray remnants from previous revisions, not really needed. I'll drop it. > > > > typedef struct AcpiTable { > > const char *sig; > > diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c > > index 050fdb3f37..4135b385e5 100644 > > --- a/hw/acpi/aml-build.c > > +++ b/hw/acpi/aml-build.c > > @@ -47,7 +47,7 @@ static void build_prepend_byte(GArray *array, uint8_t val) > > g_array_prepend_val(array, val); > > } > > > > -static void build_append_byte(GArray *array, uint8_t val) > > +void build_append_byte(GArray *array, uint8_t val) > > { > > g_array_append_val(array, val); > > } > > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c > > index 9f888d5a2c..547cd4ed9d 100644 > > --- a/hw/i386/acpi-build.c > > +++ b/hw/i386/acpi-build.c > > @@ -345,13 +345,23 @@ static void acpi_align_size(GArray *blob, unsigned align) > > g_array_set_size(blob, ROUND_UP(acpi_data_len(blob), align)); > > } > > > > -/* FACS */ > > +/* > > + * ACPI spec 1.0b, > > + * 5.2.6 Firmware ACPI Control Structure > > + */ > > static void > > build_facs(GArray *table_data) > > { > > - AcpiFacsDescriptorRev1 *facs = acpi_data_push(table_data, sizeof *facs); > > - memcpy(&facs->signature, "FACS", 4); > > - facs->length = cpu_to_le32(sizeof(*facs)); > > + const char *sig = "FACS"; > > + const uint8_t reserved[40] = {}; > > + > > + g_array_append_vals(table_data, sig, 4); /* Signature */ > > + build_append_int_noprefix(table_data, 64, 4); /* Length */ > > + build_append_int_noprefix(table_data, 0, 4); /* Hardware Signature */ > > + build_append_int_noprefix(table_data, 0, 4); /* Firmware Waking Vector */ > > + build_append_int_noprefix(table_data, 0, 4); /* Global Lock */ > > + build_append_int_noprefix(table_data, 0, 4); /* Flags */ > > + g_array_append_vals(table_data, reserved, 40); /* Reserved */ > > } > > > > static void build_append_pcihp_notify_entry(Aml *method, int slot) > > > Without thoses changes > > Reviewed-by: Eric Auger <eric.auger@redhat.com> >
diff --git a/include/hw/acpi/acpi-defs.h b/include/hw/acpi/acpi-defs.h index 0b375e7589..1a0774edd6 100644 --- a/include/hw/acpi/acpi-defs.h +++ b/include/hw/acpi/acpi-defs.h @@ -117,18 +117,4 @@ typedef struct AcpiFadtData { #define ACPI_FADT_ARM_PSCI_COMPLIANT (1 << 0) #define ACPI_FADT_ARM_PSCI_USE_HVC (1 << 1) -/* - * ACPI 1.0 Firmware ACPI Control Structure (FACS) - */ -struct AcpiFacsDescriptorRev1 { - uint32_t signature; /* ACPI Signature */ - uint32_t length; /* Length of structure, in bytes */ - uint32_t hardware_signature; /* Hardware configuration signature */ - uint32_t firmware_waking_vector; /* ACPI OS waking vector */ - uint32_t global_lock; /* Global Lock */ - uint32_t flags; - uint8_t resverved3 [40]; /* Reserved - must be zero */ -} QEMU_PACKED; -typedef struct AcpiFacsDescriptorRev1 AcpiFacsDescriptorRev1; - #endif diff --git a/include/hw/acpi/aml-build.h b/include/hw/acpi/aml-build.h index 6e1f42e119..6f3d1de1b1 100644 --- a/include/hw/acpi/aml-build.h +++ b/include/hw/acpi/aml-build.h @@ -413,6 +413,7 @@ 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); +void build_append_byte(GArray *array, uint8_t val); typedef struct AcpiTable { const char *sig; diff --git a/hw/acpi/aml-build.c b/hw/acpi/aml-build.c index 050fdb3f37..4135b385e5 100644 --- a/hw/acpi/aml-build.c +++ b/hw/acpi/aml-build.c @@ -47,7 +47,7 @@ static void build_prepend_byte(GArray *array, uint8_t val) g_array_prepend_val(array, val); } -static void build_append_byte(GArray *array, uint8_t val) +void build_append_byte(GArray *array, uint8_t val) { g_array_append_val(array, val); } diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c index 9f888d5a2c..547cd4ed9d 100644 --- a/hw/i386/acpi-build.c +++ b/hw/i386/acpi-build.c @@ -345,13 +345,23 @@ static void acpi_align_size(GArray *blob, unsigned align) g_array_set_size(blob, ROUND_UP(acpi_data_len(blob), align)); } -/* FACS */ +/* + * ACPI spec 1.0b, + * 5.2.6 Firmware ACPI Control Structure + */ static void build_facs(GArray *table_data) { - AcpiFacsDescriptorRev1 *facs = acpi_data_push(table_data, sizeof *facs); - memcpy(&facs->signature, "FACS", 4); - facs->length = cpu_to_le32(sizeof(*facs)); + const char *sig = "FACS"; + const uint8_t reserved[40] = {}; + + g_array_append_vals(table_data, sig, 4); /* Signature */ + build_append_int_noprefix(table_data, 64, 4); /* Length */ + build_append_int_noprefix(table_data, 0, 4); /* Hardware Signature */ + build_append_int_noprefix(table_data, 0, 4); /* Firmware Waking Vector */ + build_append_int_noprefix(table_data, 0, 4); /* Global Lock */ + build_append_int_noprefix(table_data, 0, 4); /* Flags */ + g_array_append_vals(table_data, reserved, 40); /* Reserved */ } static void build_append_pcihp_notify_entry(Aml *method, int slot)
Drop usage of packed structures and explicit endian conversions when building table and use endian agnostic build_append_int_noprefix() API to build it. Signed-off-by: Igor Mammedov <imammedo@redhat.com> --- CC: marcel.apfelbaum@gmail.com --- include/hw/acpi/acpi-defs.h | 14 -------------- include/hw/acpi/aml-build.h | 1 + hw/acpi/aml-build.c | 2 +- hw/i386/acpi-build.c | 18 ++++++++++++++---- 4 files changed, 16 insertions(+), 19 deletions(-)