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

[Gavin Shan]

Hi Igor,

On 2/28/22 5:08 PM, Igor Mammedov wrote:
> 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.
>  

[include EDK2 developers]

I don't think 128MB node memory size is architecturally required.
I also thought EDK2 would be better place to provide a precise error
mesage and discussed it through with EDK2 developers. Lets see what
are their thoughts this time.
 
> > 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

Yes, I think so :)

> > +    }
> > +}
> > +
> >   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);
> >   };
> >   

Thanks,
Gavin