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

[Gavin Shan <gshan@redhat.com>]

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