Re: [PATCH v3 33/35] acpi: build_facs: use build_append_int_noprefix() API to compose table

[Eric Auger <eauger@redhat.com>]

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>