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

[Igor Mammedov]

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>
>