[PATCH] ArmVirtPkg: Fix boot fail on numa system.

[PATCH] ArmVirtPkg: Fix boot fail on numa system.

[mark-pk.tsai]

If "numa-node-id" is specified in a memory node,
take node 0 as system memory instead of taking
the first memory node.

Cc: YJ Chiang <yj.chiang@medaitek.com>
Signed-off-by: Mark-PK Tsai <mark-pk.tsai@mediatek.com>
---
 ArmVirtPkg/PrePi/FdtParser.c | 32 ++++++++++++++++++++++++++++----
 1 file changed, 28 insertions(+), 4 deletions(-)

diff --git a/ArmVirtPkg/PrePi/FdtParser.c b/ArmVirtPkg/PrePi/FdtParser.c
index 5a91f7e62d..5c7de3bc31 100644
--- a/ArmVirtPkg/PrePi/FdtParser.c
+++ b/ArmVirtPkg/PrePi/FdtParser.c
@@ -19,19 +19,43 @@ FindMemnode (
   INT32        SizeCells;
   INT32        Length;
   CONST INT32  *Prop;
+  INT32        NumaId;
+  INT32        Node, Prev;
+  CONST CHAR8  *Type;
 
   if (fdt_check_header (DeviceTreeBlob) != 0) {
     return FALSE;
   }
 
   //
-  // Look for a node called "memory" at the lowest level of the tree
+  // Look for the lowest memory node.
+  // On Numa system, use node 0 as system memory.
   //
-  MemoryNode = fdt_path_offset (DeviceTreeBlob, "/memory");
-  if (MemoryNode <= 0) {
-    return FALSE;
+  MemoryNode = -1;
+  NumaId = -1;
+
+  for (Prev = 0; ; Prev = Node) {
+    Node = fdt_next_node (DeviceTreeBlob, Prev, NULL);
+    if (Node < 0)
+      break;
+
+    Type = fdt_getprop (DeviceTreeBlob, Node, "device_type", &Length);
+    if (Type && (AsciiStrnCmp (Type, "memory", Length) == 0)) {
+      Prop = fdt_getprop (DeviceTreeBlob, Node, "numa-node-id", &Length);
+      if (Prop && Length == 4) {
+        NumaId = fdt32_to_cpu (*Prop);
+      }
+
+      if (!Prop || (Prop && NumaId == 0)) {
+        MemoryNode = Node;
+        break;
+      }
+    }
   }
 
+  if (MemoryNode < 0)
+    return FALSE;
+
   //
   // Retrieve the #address-cells and #size-cells properties
   // from the root node, or use the default if not provided.
-- 
2.32.0

Re: [PATCH] ArmVirtPkg: Fix boot fail on numa system.

[Ard Biesheuvel]

On Tue, 5 Jul 2022 at 08:55, Mark-PK Tsai <mark-pk.tsai@mediatek.com> wrote:
>
> If "numa-node-id" is specified in a memory node,
> take node 0 as system memory instead of taking
> the first memory node.
>
> Cc: YJ Chiang <yj.chiang@medaitek.com>

Typo in email address

> Signed-off-by: Mark-PK Tsai <mark-pk.tsai@mediatek.com>


> ---
>  ArmVirtPkg/PrePi/FdtParser.c | 32 ++++++++++++++++++++++++++++----
>  1 file changed, 28 insertions(+), 4 deletions(-)
>
> diff --git a/ArmVirtPkg/PrePi/FdtParser.c b/ArmVirtPkg/PrePi/FdtParser.c
> index 5a91f7e62d..5c7de3bc31 100644
> --- a/ArmVirtPkg/PrePi/FdtParser.c
> +++ b/ArmVirtPkg/PrePi/FdtParser.c
> @@ -19,19 +19,43 @@ FindMemnode (
>    INT32        SizeCells;
>    INT32        Length;
>    CONST INT32  *Prop;
> +  INT32        NumaId;
> +  INT32        Node, Prev;
> +  CONST CHAR8  *Type;
>
>    if (fdt_check_header (DeviceTreeBlob) != 0) {
>      return FALSE;
>    }
>
>    //
> -  // Look for a node called "memory" at the lowest level of the tree
> +  // Look for the lowest memory node.
> +  // On Numa system, use node 0 as system memory.

There could be other reasons there are multiple memory nodes, right?
So wouldn't it be better if we cross reference the memory node with
some other information, for instance, where the firmware image itself
was loaded in memory?

>    //
> -  MemoryNode = fdt_path_offset (DeviceTreeBlob, "/memory");
> -  if (MemoryNode <= 0) {
> -    return FALSE;
> +  MemoryNode = -1;
> +  NumaId = -1;
> +
> +  for (Prev = 0; ; Prev = Node) {
> +    Node = fdt_next_node (DeviceTreeBlob, Prev, NULL);
> +    if (Node < 0)
> +      break;
> +
> +    Type = fdt_getprop (DeviceTreeBlob, Node, "device_type", &Length);
> +    if (Type && (AsciiStrnCmp (Type, "memory", Length) == 0)) {
> +      Prop = fdt_getprop (DeviceTreeBlob, Node, "numa-node-id", &Length);
> +      if (Prop && Length == 4) {
> +        NumaId = fdt32_to_cpu (*Prop);
> +      }
> +
> +      if (!Prop || (Prop && NumaId == 0)) {
> +        MemoryNode = Node;
> +        break;
> +      }
> +    }
>    }
>
> +  if (MemoryNode < 0)
> +    return FALSE;
> +
>    //
>    // Retrieve the #address-cells and #size-cells properties
>    // from the root node, or use the default if not provided.
> --
> 2.32.0
>