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

From: Gavin Shan <gshan@redhat.com>
To: Gerd Hoffmann <kraxel@redhat.com>
Cc: peter.maydell@linaro.org, drjones@redhat.com,
	richard.henderson@linaro.org, qemu-devel@nongnu.org,
	qemu-arm@nongnu.org, Shan Gavin <shan.gavin@gmail.com>,
	Igor Mammedov <imammedo@redhat.com>,
	Laszlo Ersek <lersek@redhat.com>
Subject: Re: [PATCH] hw/arm/virt: Validate memory size on the first NUMA node
Date: Thu, 3 Mar 2022 11:25:25 +0800	[thread overview]
Message-ID: <ee76dcac-8914-2f24-9920-9d93aa29c22d@redhat.com> (raw)
In-Reply-To: <20220301114257.2bppjnjqj7dgxztc@sirius.home.kraxel.org>

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 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