Re: [PATCH] hw/arm/virt: Expose empty NUMA nodes through ACPI

[Igor Mammedov <imammedo@redhat.com>]

On Wed, 27 Oct 2021 16:20:30 +1100
Gavin Shan <gshan@redhat.com> wrote:

> On 10/26/21 8:47 PM, Igor Mammedov wrote:
> > On Tue, 26 Oct 2021 07:41:01 +0800
> > Gavin Shan <gshan@redhat.com> wrote:
> >   
> >> The empty NUMA nodes, where no memory resides, aren't exposed
> >> through ACPI SRAT table. It's not user preferred behaviour because
> >> the corresponding memory node devices are missed from the guest
> >> kernel as the following example shows, and memory can't be hot
> >> added to these empty NUMA nodes at later point.  
> > 
> > a error message one gets would be useful here.
> > 
> > btw:
> > memory hotplug seems to work for x86 without adding empty nodes.
> > So it beg a question, if absence of empty nodes is the issue here.
> >   
> 
> Yes, the memory can be still hot added even the empty NUMA nodes
> aren't exposed. However, we still need to expose them so that
> the guest kernel has the information as the users specifies.

commit message says that memory can't be hotplugged though ...
so what doesn't work/is broken currently.

Question is why do we need to expose empty nodes
that aren't warranted by any present hardware (cpu/mem)?
(so far I see it as extra burden on qemu without any gain)

SRAT is typically used to describe startup configuration,
any changes to topology later during runtime are
made using _PXM objects.



> 
> I will make the commit log more precise in v2.
> 
> >>
> >>    /home/gavin/sandbox/qemu.main/build/qemu-system-aarch64 \
> >>    -accel kvm -machine virt,gic-version=host               \
> >>    -cpu host -smp 4,sockets=2,cores=2,threads=1            \
> >>    -m 1024M,slots=16,maxmem=64G                            \
> >>    -object memory-backend-ram,id=mem0,size=512M            \
> >>    -object memory-backend-ram,id=mem1,size=512M            \
> >>    -numa node,nodeid=0,cpus=0-1,memdev=mem0                \
> >>    -numa node,nodeid=1,cpus=2-3,memdev=mem1                \
> >>    -numa node,nodeid=2                                     \
> >>    -numa node,nodeid=3                                     \
> >>       :
> >>    guest# ls /sys/devices/system/node | grep node
> >>    node0
> >>    node1
> >>    node2
> >>
> >> This exposes these empty NUMA nodes through ACPI SRAT table. With
> >> this applied, the corresponding memory node devices can be found
> >> from the guest. Note that the hotpluggable capability is explicitly
> >> given to these empty NUMA nodes for sake of completeness.
> >>
> >>    guest# ls /sys/devices/system/node | grep node
> >>    node0
> >>    node1
> >>    node2
> >>    node3
> >>
> >> Signed-off-by: Gavin Shan <gshan@redhat.com>
> >> ---
> >>   hw/arm/virt-acpi-build.c | 14 +++++++++-----
> >>   1 file changed, 9 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/hw/arm/virt-acpi-build.c b/hw/arm/virt-acpi-build.c
> >> index 674f902652..a4c95b2f64 100644
> >> --- a/hw/arm/virt-acpi-build.c
> >> +++ b/hw/arm/virt-acpi-build.c
> >> @@ -526,6 +526,7 @@ build_srat(GArray *table_data, BIOSLinker *linker, VirtMachineState *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 };
> >> +    MemoryAffinityFlags flags;
> >>   
> >>       acpi_table_begin(&table, table_data);
> >>       build_append_int_noprefix(table_data, 1, 4); /* Reserved */
> >> @@ -547,12 +548,15 @@ build_srat(GArray *table_data, BIOSLinker *linker, VirtMachineState *vms)
> >>   
> >>       mem_base = vms->memmap[VIRT_MEM].base;
> >>       for (i = 0; i < ms->numa_state->num_nodes; ++i) {
> >> -        if (ms->numa_state->nodes[i].node_mem > 0) {
> >> -            build_srat_memory(table_data, mem_base,
> >> -                              ms->numa_state->nodes[i].node_mem, i,
> >> -                              MEM_AFFINITY_ENABLED);
> >> -            mem_base += ms->numa_state->nodes[i].node_mem;
> >> +        if (ms->numa_state->nodes[i].node_mem) {
> >> +            flags = MEM_AFFINITY_ENABLED;
> >> +        } else {
> >> +            flags = MEM_AFFINITY_ENABLED | MEM_AFFINITY_HOTPLUGGABLE;
> >>           }
> >> +
> >> +        build_srat_memory(table_data, mem_base,
> >> +                          ms->numa_state->nodes[i].node_mem, i, flags);
> >> +        mem_base += ms->numa_state->nodes[i].node_mem;
> >>       }
> >>   
> >>       if (ms->nvdimms_state->is_enabled) {  
> >   
> 
> Thanks,
> Gavin
>