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

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

[Gavin Shan]

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.

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

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

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

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

[Gerd Hoffmann]

  Hi,

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

take care,
  Gerd

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

[Gavin Shan]

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

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

[Gerd Hoffmann]

  Hi,

> 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

Well, except the firmware doesn't even know yet at that point
whenever it runs on a NUMA machine or not ...

take care,
  Gerd

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
> 

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

[Peter Maydell]

On Fri, 4 Mar 2022 at 10:52, Igor Mammedov <imammedo@redhat.com> wrote:
> 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)

I am definitely not in favour of that. QEMU should provide the
emulated hardware and let the firmware deal with detecting if
it's missing important stuff. It should as far as is possible
not have special-case detection-of-broken-guests handling.

thanks
-- PMM

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

[Laszlo Ersek]

[-- Attachment #1: Type: text/plain, Size: 865 bytes --]

Gerd,

On 03/04/22 11:58, Peter Maydell wrote:
> On Fri, 4 Mar 2022 at 10:52, Igor Mammedov <imammedo@redhat.com> wrote:
>> 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)
> 
> I am definitely not in favour of that. QEMU should provide the
> emulated hardware and let the firmware deal with detecting if
> it's missing important stuff. It should as far as is possible
> not have special-case detection-of-broken-guests handling.
> 
> thanks
> -- PMM
> 

It's probably simplest if you replace the ASSERT()s in question; please
see the attachment. (Only smoke tested.) Gavin indicated up-thread he'd
be OK with an easier-to-understand error message.

Laszlo

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-ArmVirtPkg-QemuVirtMemInfoLib-improve-error-messages-for-MemoryInitPei.patch --]
[-- Type: text/x-patch; name="0001-ArmVirtPkg-QemuVirtMemInfoLib-improve-error-messages-for-MemoryInitPei.patch", Size: 3620 bytes --]

From 0ab4569c45ab23c12ca9eab42016c0e844689b7c Mon Sep 17 00:00:00 2001
From: Laszlo Ersek <lersek@redhat.com>
Date: Fri, 4 Mar 2022 15:14:25 +0100
Subject: [PATCH 1/1] ArmVirtPkg/QemuVirtMemInfoLib: improve error messages for
 MemoryInitPei

QEMU makes "virt" board configurations possible where the lowest memory
node is smaller than 128 MB. Currently we catch that with an ASSERT()
only; turn it into a hard CpuDeadLoop(), and log a more user-friendly
error message.

Convert some other ASSERT()s as well.

While at it, fix up the disorder in the [LibraryClasses] section of
"QemuVirtMemInfoPeiLib.inf".

Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=2041323
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---
 .../QemuVirtMemInfoPeiLib.inf                 |  3 +-
 .../QemuVirtMemInfoPeiLibConstructor.c        | 36 +++++++++++++++----
 2 files changed, 31 insertions(+), 8 deletions(-)

diff --git a/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoPeiLib.inf b/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoPeiLib.inf
index 7ecf6dfbb786..076ee24a4961 100644
--- a/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoPeiLib.inf
+++ b/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoPeiLib.inf
@@ -29,11 +29,12 @@ [Packages]
 
 [LibraryClasses]
   ArmLib
+  BaseLib
   BaseMemoryLib
   DebugLib
   FdtLib
-  PcdLib
   MemoryAllocationLib
+  PcdLib
 
 [Pcd]
   gArmTokenSpaceGuid.PcdFdBaseAddress
diff --git a/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoPeiLibConstructor.c b/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoPeiLibConstructor.c
index 33d3597d57ab..43fb38bde436 100644
--- a/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoPeiLibConstructor.c
+++ b/ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoPeiLibConstructor.c
@@ -7,6 +7,7 @@
 **/
 
 #include <Base.h>
+#include <Library/BaseLib.h>
 #include <Library/DebugLib.h>
 #include <Library/PcdLib.h>
 #include <libfdt.h>
@@ -85,7 +86,15 @@ QemuVirtMemInfoPeiLibConstructor (
   //
   // Make sure the start of DRAM matches our expectation
   //
-  ASSERT (FixedPcdGet64 (PcdSystemMemoryBase) == NewBase);
+  if (FixedPcdGet64 (PcdSystemMemoryBase) != NewBase) {
+    DEBUG ((
+      DEBUG_ERROR,
+      "%a: the lowest memory node must start at 0x%Lx\n",
+      __FUNCTION__,
+      FixedPcdGet64 (PcdSystemMemoryBase)
+      ));
+    CpuDeadLoop ();
+  }
   PcdStatus = PcdSet64S (PcdSystemMemorySize, NewSize);
   ASSERT_RETURN_ERROR (PcdStatus);
 
@@ -97,12 +106,25 @@ QemuVirtMemInfoPeiLibConstructor (
   // chance of marking its location as reserved or copy it to a freshly
   // allocated block in the permanent PEI RAM in the platform PEIM.
   //
-  ASSERT (NewSize >= SIZE_128MB);
-  ASSERT (
-    (((UINT64)PcdGet64 (PcdFdBaseAddress) +
-      (UINT64)PcdGet32 (PcdFdSize)) <= NewBase) ||
-    ((UINT64)PcdGet64 (PcdFdBaseAddress) >= (NewBase + NewSize))
-    );
+  if (NewSize < SIZE_128MB) {
+    DEBUG ((
+      DEBUG_ERROR,
+      "%a: the lowest memory node must be at least 0x%x bytes\n",
+      __FUNCTION__,
+      (UINT32)SIZE_128MB
+      ));
+    CpuDeadLoop ();
+  }
+  if ((((UINT64)PcdGet64 (PcdFdBaseAddress) +
+        (UINT64)PcdGet32 (PcdFdSize)) > NewBase) &&
+      ((UINT64)PcdGet64 (PcdFdBaseAddress) < (NewBase + NewSize))) {
+    DEBUG ((
+      DEBUG_ERROR,
+      "%a: the lowest memory node must not overlap the flash device\n",
+      __FUNCTION__
+      ));
+    CpuDeadLoop ();
+  }
 
   return RETURN_SUCCESS;
 }

base-commit: 589d51df260465e2561979b8a988e77b0f32a6e8
-- 
2.19.1.3.g30247aa5d201

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

[Shan Gavin]

Hi Laszlo,

Yes, I think it's enough to provide the user-friendly error message in EDK2.
The command lines to start the VM/guest can be adjusted to have more than
128MB memory associated for NUMA node#0 when it's seen by users.

Thanks,
Gavin

Laszlo Ersek <lersek@redhat.com> 于2022年3月4日周五 22:24写道：
>
> Gerd,
>
> On 03/04/22 11:58, Peter Maydell wrote:
> > On Fri, 4 Mar 2022 at 10:52, Igor Mammedov <imammedo@redhat.com> wrote:
> >> 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)
> >
> > I am definitely not in favour of that. QEMU should provide the
> > emulated hardware and let the firmware deal with detecting if
> > it's missing important stuff. It should as far as is possible
> > not have special-case detection-of-broken-guests handling.
> >
> > thanks
> > -- PMM
> >
>
> It's probably simplest if you replace the ASSERT()s in question; please
> see the attachment. (Only smoke tested.) Gavin indicated up-thread he'd
> be OK with an easier-to-understand error message.
>
> Laszlo