Re: [PATCH] hw/arm/virt: Validate memory size on the first NUMA node

[Igor Mammedov]

On Mon, 28 Feb 2022 15:52:03 +0800
Gavin Shan <gshan@redhat.com> wrote:

> When the memory size on the first NUMA node is less than 128MB, the
> guest hangs inside EDK2 as the following logs show.
> 
>   /home/gavin/sandbox/qemu.main/build/qemu-system-aarch64         \
>   -accel kvm -machine virt,gic-version=host                       \
>   -cpu host -smp 8,sockets=2,cores=2,threads=2                    \
>   -m 1024M,slots=16,maxmem=64G                                    \
>   -object memory-backend-ram,id=mem0,size=127M                    \
>   -object memory-backend-ram,id=mem1,size=897M                    \
>   -numa node,nodeid=0,memdev=mem0                                 \
>   -numa node,nodeid=1,memdev=mem1                                 \
>   -L /home/gavin/sandbox/qemu.main/build/pc-bios                  \
>    :
>   QemuVirtMemInfoPeiLibConstructor: System RAM @ 0x47F00000 - 0x7FFFFFFF
>   QemuVirtMemInfoPeiLibConstructor: System RAM @ 0x40000000 - 0x47EFFFFF
>   ASSERT [MemoryInit] 
> /home/lacos/src/upstream/qemu/roms/edk2/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoPeiLibConstructor.c(93):
>  NewSize >= 0x08000000
> 
> This adds MachineClass::validate_numa_nodes() to validate the memory
> size on the first NUMA node. The guest is stopped from booting and
> the reason is given for this specific case.

Unless it architecturally wrong thing i.e. (node size less than 128Mb)
,in which case limiting it in QEMU would be justified, I'd prefer
firmware being fixed or it reporting more useful for user error message.
 
> Signed-off-by: Gavin Shan <gshan@redhat.com>
> ---
>  hw/arm/virt.c       | 9 +++++++++
>  hw/core/numa.c      | 5 +++++
>  include/hw/boards.h | 1 +
>  3 files changed, 15 insertions(+)
> 
> diff --git a/hw/arm/virt.c b/hw/arm/virt.c
> index 46bf7ceddf..234e7fca28 100644
> --- a/hw/arm/virt.c
> +++ b/hw/arm/virt.c
> @@ -2491,6 +2491,14 @@ static int64_t virt_get_default_cpu_node_id(const 
> MachineState *ms, int idx)
>      return idx % ms->numa_state->num_nodes;
>  }
>  
> +static void virt_validate_numa_nodes(MachineState *ms)
> +{
> +    if (ms->numa_state->nodes[0].node_mem < 128 * MiB) {
> +        error_report("The first NUMA node should have at least 128MB 
> memory");
> +        exit(1);

perhaps error_fatal() would be better

> +    }
> +}
> +
>  static const CPUArchIdList *virt_possible_cpu_arch_ids(MachineState *ms)
>  {
>      int n;
> @@ -2836,6 +2844,7 @@ static void virt_machine_class_init(ObjectClass *oc, 
> void *data)
>      mc->cpu_index_to_instance_props = virt_cpu_index_to_props;
>      mc->default_cpu_type = ARM_CPU_TYPE_NAME("cortex-a15");
>      mc->get_default_cpu_node_id = virt_get_default_cpu_node_id;
> +    mc->validate_numa_nodes = virt_validate_numa_nodes;
>      mc->kvm_type = virt_kvm_type;
>      assert(!mc->get_hotplug_handler);
>      mc->get_hotplug_handler = virt_machine_get_hotplug_handler;
> diff --git a/hw/core/numa.c b/hw/core/numa.c
> index 1aa05dcf42..543a2eaf11 100644
> --- a/hw/core/numa.c
> +++ b/hw/core/numa.c
> @@ -724,6 +724,11 @@ void numa_complete_configuration(MachineState *ms)
>              /* Validation succeeded, now fill in any missing distances. */
>              complete_init_numa_distance(ms);
>          }
> +
> +        /* Validate NUMA nodes for the individual machine */
> +        if (mc->validate_numa_nodes) {
> +            mc->validate_numa_nodes(ms);
> +        }
>      }
>  }
>  
> diff --git a/include/hw/boards.h b/include/hw/boards.h
> index c92ac8815c..9709a35eeb 100644
> --- a/include/hw/boards.h
> +++ b/include/hw/boards.h
> @@ -282,6 +282,7 @@ struct MachineClass {
>                                                           unsigned cpu_index);
>      const CPUArchIdList *(*possible_cpu_arch_ids)(MachineState *machine);
>      int64_t (*get_default_cpu_node_id)(const MachineState *ms, int idx);
> +    void (*validate_numa_nodes)(MachineState *ms);
>      ram_addr_t (*fixup_ram_size)(ram_addr_t size);
>  };
>