| Message ID | 20210907144814.741785-17-imammedo@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | acpi: refactor error prone build_header() and packed structures usage in ACPI tables | expand |
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 > > 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) > - */ > - > -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
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 >
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 - */ - -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); } /*
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/ 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(-)