Re: [PATCH v3 16/35] acpi: arm/x86: build_srat: use acpi_table_begin()/acpi_table_end() instead of build_header()

[Igor Mammedov]

On Wed, 22 Sep 2021 09:38:14 +0200
Eric Auger <eauger@redhat.com> wrote:

> Hi Igor,
> 
> 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.  

> nit: also removes AcpiSystemResourceAffinityTable and use
> build_append_int_noprefix for reserved fields

I didn't find it as worth of mentioning, the same is done
in some other patches where header wasn't standard one.

> > Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> > ---
> > v3:
> >   * s/acpi_init_table|acpi_table_composed/acpi_table_begin|acpi_table_end/
> > 
> > CC: shannon.zhaosl@gmail.com
> > CC: peter.maydell@linaro.org
> > CC: marcel.apfelbaum@gmail.com
> > CC: qemu-arm@nongnu.org
> > CC: drjones@redhat.com
> > CC: eauger@redhat.com
> > ---
> >  include/hw/acpi/acpi-defs.h | 11 -----------
> >  hw/arm/virt-acpi-build.c    | 15 +++++++--------
> >  hw/i386/acpi-build.c        | 18 +++++++-----------
> >  3 files changed, 14 insertions(+), 30 deletions(-)
> > 
> > diff --git a/include/hw/acpi/acpi-defs.h b/include/hw/acpi/acpi-defs.h
> > index 3b42b138f0..5826ee04b6 100644
> > --- a/include/hw/acpi/acpi-defs.h
> > +++ b/include/hw/acpi/acpi-defs.h
> > @@ -358,17 +358,6 @@ struct AcpiGenericTimerTable {
> >  } QEMU_PACKED;
> >  typedef struct AcpiGenericTimerTable AcpiGenericTimerTable;
> >  
> > -/*
> > - * SRAT (NUMA topology description) table  
> Missing version reference if any
> https://uefi.org/specs/ACPI/6.4/05_ACPI_Software_Programming_Model/ACPI_Software_Programming_Model.html#system-resource-affinity-table-srat
> 5.2.16. System Resource Affinity Table (SRAT)

next patch adds missing doc comment(s) when converting
to build_append_int_noprefix()


> > - */
> > -
> > -struct AcpiSystemResourceAffinityTable {
> > -    ACPI_TABLE_HEADER_DEF
> > -    uint32_t    reserved1;
> > -    uint32_t    reserved2[2];
> > -} QEMU_PACKED;
> > -typedef struct AcpiSystemResourceAffinityTable 
> > AcpiSystemResourceAffinityTable;
> > -
> >  #define ACPI_SRAT_PROCESSOR_APIC     0
> >  #define ACPI_SRAT_MEMORY             1
> >  #define ACPI_SRAT_PROCESSOR_x2APIC   2
> > diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> > index 037cc1fd82..21efe7fe34 100644
> > --- a/hw/arm/virt-acpi-build.c
> > +++ b/hw/arm/virt-acpi-build.c
> > @@ -477,18 +477,19 @@ build_spcr(GArray *table_data, BIOSLinker *linker, 
> > VirtMachineState *vms)
> >  static void
> >  build_srat(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
> >  {
> > -    AcpiSystemResourceAffinityTable *srat;
> >      AcpiSratProcessorGiccAffinity *core;
> >      AcpiSratMemoryAffinity *numamem;
> > -    int i, srat_start;
> > +    int i;
> >      uint64_t mem_base;
> >      MachineClass *mc = MACHINE_GET_CLASS(vms);
> >      MachineState *ms = MACHINE(vms);
> >      const CPUArchIdList *cpu_list = mc->possible_cpu_arch_ids(ms);
> > +    AcpiTable table = { .sig = "SRAT", .rev = 3, .oem_id = vms->oem_id,
> > +                        .oem_table_id = vms->oem_table_id };
> >  
> > -    srat_start = table_data->len;
> > -    srat = acpi_data_push(table_data, sizeof(*srat));
> > -    srat->reserved1 = cpu_to_le32(1);
> > +    acpi_table_begin(&table, table_data);
> > +    build_append_int_noprefix(table_data, 1, 4); /* Reserved */
> > +    build_append_int_noprefix(table_data, 0, 8); /* Reserved */
> >  
> >      for (i = 0; i < cpu_list->len; ++i) {
> >          core = acpi_data_push(table_data, sizeof(*core));
> > @@ -522,9 +523,7 @@ build_srat(GArray *table_data, BIOSLinker *linker, 
> > VirtMachineState *vms)
> >                            MEM_AFFINITY_HOTPLUGGABLE | 
> > MEM_AFFINITY_ENABLED);
> >      }
> >  
> > -    build_header(linker, table_data, (void *)(table_data->data + 
> > srat_start),
> > -                 "SRAT", table_data->len - srat_start, 3, vms->oem_id,
> > -                 vms->oem_table_id);
> > +    acpi_table_end(linker, &table);
> >  }
> >  
> >  /* GTDT */
> > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> > index c329580cff..41c0a63b30 100644
> > --- a/hw/i386/acpi-build.c
> > +++ b/hw/i386/acpi-build.c
> > @@ -1920,11 +1920,10 @@ build_tpm_tcpa(GArray *table_data, BIOSLinker 
> > *linker, GArray *tcpalog,
> >  static void
> >  build_srat(GArray *table_data, BIOSLinker *linker, MachineState *machine)
> >  {
> > -    AcpiSystemResourceAffinityTable *srat;
> >      AcpiSratMemoryAffinity *numamem;
> >  
> >      int i;
> > -    int srat_start, numa_start, slots;
> > +    int numa_start, slots;
> >      uint64_t mem_len, mem_base, next_base;
> >      MachineClass *mc = MACHINE_GET_CLASS(machine);
> >      X86MachineState *x86ms = X86_MACHINE(machine);
> > @@ -1935,11 +1934,12 @@ build_srat(GArray *table_data, BIOSLinker *linker, 
> > MachineState *machine)
> >      ram_addr_t hotplugabble_address_space_size =
> >          object_property_get_int(OBJECT(pcms), 
> > PC_MACHINE_DEVMEM_REGION_SIZE,
> >                                  NULL);
> > +    AcpiTable table = { .sig = "SRAT", .rev = 1, .oem_id = x86ms->oem_id,
> > +                        .oem_table_id = x86ms->oem_table_id };
> >  
> > -    srat_start = table_data->len;
> > -
> > -    srat = acpi_data_push(table_data, sizeof *srat);
> > -    srat->reserved1 = cpu_to_le32(1);
> > +    acpi_table_begin(&table, table_data);
> > +    build_append_int_noprefix(table_data, 1, 4); /* Reserved */
> > +    build_append_int_noprefix(table_data, 0, 8); /* Reserved */
> >  
> >      for (i = 0; i < apic_ids->len; i++) {
> >          int node_id = apic_ids->cpus[i].props.node_id;
> > @@ -2045,11 +2045,7 @@ build_srat(GArray *table_data, BIOSLinker *linker, 
> > MachineState *machine)
> >                            MEM_AFFINITY_HOTPLUGGABLE | 
> > MEM_AFFINITY_ENABLED);
> >      }
> >  
> > -    build_header(linker, table_data,
> > -                 (void *)(table_data->data + srat_start),
> > -                 "SRAT",
> > -                 table_data->len - srat_start, 1, x86ms->oem_id,
> > -                 x86ms->oem_table_id);
> > +    acpi_table_end(linker, &table);
> >  }
> >  
> >  /*
> >   
> Besides
> 
> Reviewed-by: Eric Auger <eric.auger@redhat.com>
> 
> 
> Eric
>