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

[Igor Mammedov]

On Thu, 3 Mar 2022 11:25:25 +0800
Gavin Shan <gshan@redhat.com> wrote:

> Hi Gerd,
> 
> On 3/1/22 7:42 PM, Gerd Hoffmann wrote:
> >>> 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.  
> > 
> > Useful error reporting that early in the firmware initialization is a
> > rather hard problem, it's much easier for qemu to catch those problems
> > and print a useful error message.
> > 
> > Fixing the firmware would be possible.  The firmware simply uses the
> > memory of the first numa note in the early initialization phase, which
> > could be changed to look for additional numa nodes.  It's IMHO simply
> > not worth the trouble though.  numa nodes with less memory than 128M
> > simply doesn't happen in practice, except when QE does questionable
> > scaleability testing (scale up the number of numa nodes without also
> > scaling up the total amount of memory, ending up with rather tiny
> > numa nodes and a configuration nobody actually uses in practice).
> >   
> 
> I still don't think QEMU is best place to have this kind of limitation,
> which the first NUMA node should have 128MB memory at least. For example,
> the limitation exists in EDK2-version-A.0 and the limitation is removed
> in EDK2-version-A.1. The QEMU can't boot the guest using EDK2-version-A.1,
> but we should succeed.

if firmware is not an option, I wouldn't opposed to printing warning
message from QEMU if you can detect that you are running broken edk2
and under condition that no new infa/hooks are invented for this.
(assuming it's worth all the effort at all)

Maybe put it in virt-arm specific machine_done.

> If it's not worthwhile to be fixed in EDK2, it might be doable to improve
> the error message printed by EDK2. Otherwise, we would ignore this issue
> because 128MB node memory size isn't used in practice. If the EDK2 error
> message can be improved, we might have something like below:
> 
> ASSERT [MemoryInit] 
> /home/lacos/src/upstream/qemu/roms/edk2/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoPeiLibConstructor.c(93):
>  NewSize >= 0x08000000
> 
> to
> 
> The first NUMA node should have more than 128MB memory
> 
> Thanks,
> Gavin
>