Re: [PATCH] hw/i386/acpi-build: Get NUMA information from struct NumaState

[Liu, Jingqi]

On 8/5/2021 5:26 PM, Igor Mammedov wrote:
> On Tue,  3 Aug 2021 14:30:05 +0800
> Jingqi Liu <jingqi.liu@intel.com> wrote:
>
> > The NUMA information in PCMachineState is copied from MachineState.
> > We get this information uniformly from struct NumaState in MachineState.
> Is there a another reason behind this patch?
>
> As cleanup it's not complete, why do you keep
> PCMachineState::numa_nodes & co around?
> I'd suggest to remove it completely and use data from
> MachineState everywhere.
Thanks for your confirmation.
I think so too.
We can get NUMA information completely from MachineState::numa_state.

Actually, MachineState::numa_state is used everywhere except for 
pc_guest_info_init(), which just copies NUMA information to 
PCMachineState::numa_nodes and PCMachineState::node_mem.

I'll remove them completely.

Thanks,
Jingqi

> > Signed-off-by: Jingqi Liu <jingqi.liu@intel.com>
> > ---
> >   hw/i386/acpi-build.c | 12 +++++++-----
> >   1 file changed, 7 insertions(+), 5 deletions(-)
> >
> > diff --git a/hw/i386/acpi-build.c b/hw/i386/acpi-build.c
> > index 17836149fe..e3c9ad011e 100644
> > --- a/hw/i386/acpi-build.c
> > +++ b/hw/i386/acpi-build.c
> > @@ -1902,6 +1902,8 @@ build_srat(GArray *table_data, BIOSLinker *linker, 
> > MachineState *machine)
> >       X86MachineState *x86ms = X86_MACHINE(machine);
> >       const CPUArchIdList *apic_ids = mc->possible_cpu_arch_ids(machine);
> >       PCMachineState *pcms = PC_MACHINE(machine);
> > +    int nb_numa_nodes = machine->numa_state->num_nodes;
> > +    NodeInfo *numa_info = machine->numa_state->nodes;
> >       ram_addr_t hotplugabble_address_space_size =
> >           object_property_get_int(OBJECT(pcms), PC_MACHINE_DEVMEM_REGION_SIZE,
> >                                   NULL);
> > @@ -1945,9 +1947,9 @@ build_srat(GArray *table_data, BIOSLinker *linker, 
> > MachineState *machine)
> >       next_base = 0;
> >       numa_start = table_data->len;
> >   
> > -    for (i = 1; i < pcms->numa_nodes + 1; ++i) {
> > +    for (i = 1; i < nb_numa_nodes + 1; ++i) {
> >           mem_base = next_base;
> > -        mem_len = pcms->node_mem[i - 1];
> > +        mem_len = numa_info[i - 1].node_mem;
> >           next_base = mem_base + mem_len;
> >   
> >           /* Cut out the 640K hole */
> > @@ -1995,7 +1997,7 @@ build_srat(GArray *table_data, BIOSLinker *linker, 
> > MachineState *machine)
> >       }
> >   
> >       slots = (table_data->len - numa_start) / sizeof *numamem;
> > -    for (; slots < pcms->numa_nodes + 2; slots++) {
> > +    for (; slots < nb_numa_nodes + 2; slots++) {
> >           numamem = acpi_data_push(table_data, sizeof *numamem);
> >           build_srat_memory(numamem, 0, 0, 0, MEM_AFFINITY_NOFLAGS);
> >       }
> > @@ -2011,7 +2013,7 @@ build_srat(GArray *table_data, BIOSLinker *linker, 
> > MachineState *machine)
> >       if (hotplugabble_address_space_size) {
> >           numamem = acpi_data_push(table_data, sizeof *numamem);
> >           build_srat_memory(numamem, machine->device_memory->base,
> > -                          hotplugabble_address_space_size, pcms->numa_nodes - 
> > 1,
> > +                          hotplugabble_address_space_size, nb_numa_nodes - 1,
> >                             MEM_AFFINITY_HOTPLUGGABLE | MEM_AFFINITY_ENABLED);
> >       }
> >   
> > @@ -2513,7 +2515,7 @@ void acpi_build(AcpiBuildTables *tables, MachineState *machine)
> >           }
> >       }
> >   #endif
> > -    if (pcms->numa_nodes) {
> > +    if (machine->numa_state->num_nodes) {
> >           acpi_add_table(table_offsets, tables_blob);
> >           build_srat(tables_blob, tables->linker, machine);
> >           if (machine->numa_state->have_numa_distance) {