[PATCH 0/4] ArmVirtPkg: FdtClientDxe & HighMemDxe updates

[PATCH 0/4] ArmVirtPkg: FdtClientDxe & HighMemDxe updates

[Ard Biesheuvel]

Patch #4 moves HighMemDxe to the FDT client protocol, and updates it to
handle #address-cells/#size-cells values of <1> as well as <2>, and lets
it deal with memory nodes whose 'reg' properties describe multiple disjoint
regions.

Patches #1 to #3 are somewhat preparatory in nature:
- Patch #1 is a small thinko fix
- Patch #2 drops the 'regelemsize' output parameter for 'reg' related methods
  of the FDT client protocol in favour of addresscells/sizecells.
- Patch #3 adds methods to iterate over all 'reg' properties of all memory nodes

Ard Biesheuvel (4):
  ArmVirtPkg/FdtClientDxe: fix check for size of "reg" properties
  ArmVirtPkg/FdtClientDxe: report address and size cell count directly
  ArmVirtPkg/FdtClient: add methods to iterate over memory nodes
  ArmVirtPkg/HighMemDxe: move to FDT client protocol

 ArmVirtPkg/FdtClientDxe/FdtClientDxe.c                   |  84 +++++++++++++-
 ArmVirtPkg/HighMemDxe/HighMemDxe.c                       | 120 +++++++++-----------
 ArmVirtPkg/HighMemDxe/HighMemDxe.inf                     |  16 ++-
 ArmVirtPkg/Include/Protocol/FdtClient.h                  |  29 ++++-
 ArmVirtPkg/Library/ArmVirtGicArchLib/ArmVirtGicArchLib.c |   9 +-
 ArmVirtPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c           |   9 +-
 ArmVirtPkg/XenAcpiPlatformDxe/XenAcpiPlatformDxe.c       |   8 +-
 ArmVirtPkg/XenioFdtDxe/XenioFdtDxe.c                     |  10 +-
 8 files changed, 196 insertions(+), 89 deletions(-)

-- 
2.7.4

[PATCH 1/4] ArmVirtPkg/FdtClientDxe: fix check for size of "reg" properties

[Ard Biesheuvel]

Currently, the code in FdtClientDxe assumes #address-cells and
of <base, size> tuples, this means the size of the entire property
should always be a multiple of 16 bytes (i.e, 4 * sizeof(UINT32),
not 8. So fix this.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 ArmVirtPkg/FdtClientDxe/FdtClientDxe.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ArmVirtPkg/FdtClientDxe/FdtClientDxe.c b/ArmVirtPkg/FdtClientDxe/FdtClientDxe.c
index c336e2410033..2063a597323b 100644
--- a/ArmVirtPkg/FdtClientDxe/FdtClientDxe.c
+++ b/ArmVirtPkg/FdtClientDxe/FdtClientDxe.c
@@ -178,7 +178,7 @@ FindCompatibleNodeReg (
     return Status;
   }
 
-  if ((*RegSize % 8) != 0) {
+  if ((*RegSize % 16) != 0) {
     DEBUG ((EFI_D_ERROR,
       "%a: '%a' compatible node has invalid 'reg' property (size == 0x%x)\n",
       __FUNCTION__, CompatibleString, *RegSize));
-- 
2.7.4

[PATCH 2/4] ArmVirtPkg/FdtClientDxe: report address and size cell count directly

[Ard Biesheuvel]

The FDT client protocol methods dealing with "reg" properties return
the size of a 'reg' element. Currently, we have hardcoded this as '8',
since #address-cells == #size-cells == 2 in most cases. However, for
different values, have a single 'reg' element size is not unambiguous,
since - however unlikely - if #address-cells != #size-cells, we do not
know which is which.

So before adding more methods to the protocol, fix up this oversight.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 ArmVirtPkg/FdtClientDxe/FdtClientDxe.c                   |  6 ++++--
 ArmVirtPkg/Include/Protocol/FdtClient.h                  |  3 ++-
 ArmVirtPkg/Library/ArmVirtGicArchLib/ArmVirtGicArchLib.c |  9 ++++++---
 ArmVirtPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c           |  9 ++++++---
 ArmVirtPkg/XenAcpiPlatformDxe/XenAcpiPlatformDxe.c       |  8 ++++++--
 ArmVirtPkg/XenioFdtDxe/XenioFdtDxe.c                     | 10 +++++++---
 6 files changed, 31 insertions(+), 14 deletions(-)

diff --git a/ArmVirtPkg/FdtClientDxe/FdtClientDxe.c b/ArmVirtPkg/FdtClientDxe/FdtClientDxe.c
index 2063a597323b..382e9af6bf84 100644
--- a/ArmVirtPkg/FdtClientDxe/FdtClientDxe.c
+++ b/ArmVirtPkg/FdtClientDxe/FdtClientDxe.c
@@ -159,7 +159,8 @@ FindCompatibleNodeReg (
   IN  FDT_CLIENT_PROTOCOL     *This,
   IN  CONST CHAR8             *CompatibleString,
   OUT CONST VOID              **Reg,
-  OUT UINT32                  *RegElemSize,
+  OUT UINTN                   *AddressCells,
+  OUT UINTN                   *SizeCells,
   OUT UINT32                  *RegSize
   )
 {
@@ -185,7 +186,8 @@ FindCompatibleNodeReg (
     return EFI_NOT_FOUND;
   }
 
-  *RegElemSize = 8;
+  *AddressCells = 2;
+  *SizeCells = 2;
 
   return EFI_SUCCESS;
 }
diff --git a/ArmVirtPkg/Include/Protocol/FdtClient.h b/ArmVirtPkg/Include/Protocol/FdtClient.h
index 79a8f3ce1b5d..b593c7441426 100644
--- a/ArmVirtPkg/Include/Protocol/FdtClient.h
+++ b/ArmVirtPkg/Include/Protocol/FdtClient.h
@@ -80,7 +80,8 @@ EFI_STATUS
   IN  FDT_CLIENT_PROTOCOL     *This,
   IN  CONST CHAR8             *CompatibleString,
   OUT CONST VOID              **Reg,
-  OUT UINT32                  *RegElemSize,
+  OUT UINTN                   *AddressCells,
+  OUT UINTN                   *SizeCells,
   OUT UINT32                  *RegSize
   );
 
diff --git a/ArmVirtPkg/Library/ArmVirtGicArchLib/ArmVirtGicArchLib.c b/ArmVirtPkg/Library/ArmVirtGicArchLib/ArmVirtGicArchLib.c
index a1cd2da2d43a..64afc4de6b4d 100644
--- a/ArmVirtPkg/Library/ArmVirtGicArchLib/ArmVirtGicArchLib.c
+++ b/ArmVirtPkg/Library/ArmVirtGicArchLib/ArmVirtGicArchLib.c
@@ -36,7 +36,8 @@ ArmVirtGicArchLibConstructor (
   UINT32                IccSre;
   FDT_CLIENT_PROTOCOL   *FdtClient;
   CONST UINT64          *Reg;
-  UINT32                RegElemSize, RegSize;
+  UINT32                RegSize;
+  UINTN                 AddressCells, SizeCells;
   UINTN                 GicRevision;
   EFI_STATUS            Status;
   UINT64                DistBase, CpuBase, RedistBase;
@@ -47,11 +48,13 @@ ArmVirtGicArchLibConstructor (
 
   GicRevision = 2;
   Status = FdtClient->FindCompatibleNodeReg (FdtClient, "arm,cortex-a15-gic",
-                        (CONST VOID **)&Reg, &RegElemSize, &RegSize);
+                        (CONST VOID **)&Reg, &AddressCells, &SizeCells,
+                        &RegSize);
   if (Status == EFI_NOT_FOUND) {
     GicRevision = 3;
     Status = FdtClient->FindCompatibleNodeReg (FdtClient, "arm,gic-v3",
-                          (CONST VOID **)&Reg, &RegElemSize, &RegSize);
+                          (CONST VOID **)&Reg, &AddressCells, &SizeCells,
+                          &RegSize);
   }
   if (EFI_ERROR (Status)) {
     return Status;
diff --git a/ArmVirtPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c b/ArmVirtPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c
index 377262563e3e..8ecbe3fb5fe6 100644
--- a/ArmVirtPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c
+++ b/ArmVirtPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c
@@ -122,7 +122,8 @@ QemuFwCfgInitialize (
   EFI_STATUS                    Status;
   FDT_CLIENT_PROTOCOL           *FdtClient;
   CONST UINT64                  *Reg;
-  UINT32                        RegElemSize, RegSize;
+  UINT32                        RegSize;
+  UINTN                         AddressCells, SizeCells;
   UINT64                        FwCfgSelectorAddress;
   UINT64                        FwCfgSelectorSize;
   UINT64                        FwCfgDataAddress;
@@ -135,7 +136,8 @@ QemuFwCfgInitialize (
   ASSERT_EFI_ERROR (Status);
 
   Status = FdtClient->FindCompatibleNodeReg (FdtClient, "qemu,fw-cfg-mmio",
-                         (CONST VOID **)&Reg, &RegElemSize, &RegSize);
+                         (CONST VOID **)&Reg, &AddressCells, &SizeCells,
+                         &RegSize);
   if (EFI_ERROR (Status)) {
     DEBUG ((EFI_D_WARN,
       "%a: No 'qemu,fw-cfg-mmio' compatible DT node found (Status == %r)\n",
@@ -143,7 +145,8 @@ QemuFwCfgInitialize (
     return EFI_SUCCESS;
   }
 
-  ASSERT (RegElemSize == sizeof (UINT64));
+  ASSERT (AddressCells == 2);
+  ASSERT (SizeCells == 2);
   ASSERT (RegSize == 2 * sizeof (UINT64));
 
   FwCfgDataAddress     = SwapBytes64 (Reg[0]);
diff --git a/ArmVirtPkg/XenAcpiPlatformDxe/XenAcpiPlatformDxe.c b/ArmVirtPkg/XenAcpiPlatformDxe/XenAcpiPlatformDxe.c
index 37aea806283e..203946f97bf8 100644
--- a/ArmVirtPkg/XenAcpiPlatformDxe/XenAcpiPlatformDxe.c
+++ b/ArmVirtPkg/XenAcpiPlatformDxe/XenAcpiPlatformDxe.c
@@ -45,7 +45,8 @@ GetXenArmAcpiRsdp (
   EFI_STATUS                                     Status;
   FDT_CLIENT_PROTOCOL                            *FdtClient;
   CONST UINT64                                   *Reg;
-  UINT32                                         RegElemSize, RegSize;
+  UINT32                                         RegSize;
+  UINTN                                          AddressCells, SizeCells;
   UINT64                                         RegBase;
   UINT8                                          Sum;
 
@@ -59,13 +60,16 @@ GetXenArmAcpiRsdp (
   ASSERT_EFI_ERROR (Status);
 
   Status = FdtClient->FindCompatibleNodeReg (FdtClient, "xen,guest-acpi",
-                        (CONST VOID **)&Reg, &RegElemSize, &RegSize);
+                        (CONST VOID **)&Reg, &AddressCells, &SizeCells,
+                        &RegSize);
   if (EFI_ERROR (Status)) {
     DEBUG ((EFI_D_WARN, "%a: No 'xen,guest-acpi' compatible DT node found\n",
       __FUNCTION__));
     return EFI_NOT_FOUND;
   }
 
+  ASSERT (AddressCells == 2);
+  ASSERT (SizeCells == 2);
   ASSERT (RegSize == 2 * sizeof (UINT64));
 
   RegBase = SwapBytes64(Reg[0]);
diff --git a/ArmVirtPkg/XenioFdtDxe/XenioFdtDxe.c b/ArmVirtPkg/XenioFdtDxe/XenioFdtDxe.c
index 4a88db32170c..ae012a76f5e0 100644
--- a/ArmVirtPkg/XenioFdtDxe/XenioFdtDxe.c
+++ b/ArmVirtPkg/XenioFdtDxe/XenioFdtDxe.c
@@ -31,7 +31,8 @@ InitializeXenioFdtDxe (
   EFI_STATUS            Status;
   FDT_CLIENT_PROTOCOL   *FdtClient;
   CONST UINT64          *Reg;
-  UINT32                RegElemSize, RegSize;
+  UINT32                RegSize;
+  UINTN                 AddressCells, SizeCells;
   EFI_HANDLE            Handle;
   UINT64                RegBase;
 
@@ -40,14 +41,17 @@ InitializeXenioFdtDxe (
   ASSERT_EFI_ERROR (Status);
 
   Status = FdtClient->FindCompatibleNodeReg (FdtClient, "xen,xen",
-                        (CONST VOID **)&Reg, &RegElemSize, &RegSize);
+                        (CONST VOID **)&Reg, &AddressCells, &SizeCells,
+                        &RegSize);
   if (EFI_ERROR (Status)) {
     DEBUG ((EFI_D_WARN, "%a: No 'xen,xen' compatible DT node found\n",
       __FUNCTION__));
     return EFI_UNSUPPORTED;
   }
 
-  ASSERT (RegSize == 16);
+  ASSERT (AddressCells == 2);
+  ASSERT (SizeCells == 2);
+  ASSERT (RegSize == 2 * sizeof (UINT64));
 
   //
   // Retrieve the reg base from this node and wire it up to the
-- 
2.7.4

[PATCH 3/4] ArmVirtPkg/FdtClient: add methods to iterate over memory nodes

[Ard Biesheuvel]

Add high level methods to iterate over all 'reg' properties of all DT
nodes whose device_type properties have the value "memory". Since we are
modifying the FdtClient protocol, update the protocol and the only existing
implementation at the same time.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 ArmVirtPkg/FdtClientDxe/FdtClientDxe.c  | 76 ++++++++++++++++++++
 ArmVirtPkg/Include/Protocol/FdtClient.h | 26 +++++++
 2 files changed, 102 insertions(+)

diff --git a/ArmVirtPkg/FdtClientDxe/FdtClientDxe.c b/ArmVirtPkg/FdtClientDxe/FdtClientDxe.c
index 382e9af6bf84..099cce7821d6 100644
--- a/ArmVirtPkg/FdtClientDxe/FdtClientDxe.c
+++ b/ArmVirtPkg/FdtClientDxe/FdtClientDxe.c
@@ -194,6 +194,80 @@ FindCompatibleNodeReg (
 
 STATIC
 EFI_STATUS
+EFIAPI
+FindNextMemoryNodeReg (
+  IN  FDT_CLIENT_PROTOCOL     *This,
+  IN  INT32                   PrevNode,
+  OUT INT32                   *Node,
+  OUT CONST VOID              **Reg,
+  OUT UINTN                   *AddressCells,
+  OUT UINTN                   *SizeCells,
+  OUT UINT32                  *RegSize
+  )
+{
+  INT32          Prev, Next;
+  CONST CHAR8    *DeviceType;
+  INT32          Len;
+  EFI_STATUS     Status;
+
+  ASSERT (mDeviceTreeBase != NULL);
+  ASSERT (Node != NULL);
+
+  for (Prev = PrevNode;; Prev = Next) {
+    Next = fdt_next_node (mDeviceTreeBase, Prev, NULL);
+    if (Next < 0) {
+      break;
+    }
+
+    DeviceType = fdt_getprop (mDeviceTreeBase, Next, "device_type", &Len);
+    if (DeviceType != NULL && AsciiStrCmp (DeviceType, "memory") == 0) {
+
+      //
+      // Get the 'reg' property of this memory node. For now, we will assume
+      // 8 byte quantities for base and size, respectively.
+      // TODO use #cells root properties instead
+      //
+      Status = GetNodeProperty (This, Next, "reg", Reg, RegSize);
+      if (EFI_ERROR (Status)) {
+        DEBUG ((EFI_D_WARN,
+          "%a: ignoring memory node with no 'reg' property\n",
+          __FUNCTION__));
+        continue;
+      }
+      if ((*RegSize % 16) != 0) {
+        DEBUG ((EFI_D_WARN,
+          "%a: ignoring memory node with invalid 'reg' property (size == 0x%x)\n",
+          __FUNCTION__, RegSize));
+        continue;
+      }
+
+      *Node = Next;
+      *AddressCells = 2;
+      *SizeCells = 2;
+      return EFI_SUCCESS;
+    }
+  }
+  return EFI_NOT_FOUND;
+}
+
+STATIC
+EFI_STATUS
+EFIAPI
+FindMemoryNodeReg (
+  IN  FDT_CLIENT_PROTOCOL     *This,
+  OUT INT32                   *Node,
+  OUT CONST VOID              **Reg,
+  OUT UINTN                   *AddressCells,
+  OUT UINTN                   *SizeCells,
+  OUT UINT32                  *RegSize
+  )
+{
+  return FindNextMemoryNodeReg (This, 0, Node, Reg, AddressCells, SizeCells,
+           RegSize);
+}
+
+STATIC
+EFI_STATUS
 GetOrInsertChosenNode (
   IN  FDT_CLIENT_PROTOCOL     *This,
   OUT INT32                   *Node
@@ -225,6 +299,8 @@ STATIC FDT_CLIENT_PROTOCOL mFdtClientProtocol = {
   FindNextCompatibleNode,
   FindCompatibleNodeProperty,
   FindCompatibleNodeReg,
+  FindMemoryNodeReg,
+  FindNextMemoryNodeReg,
   GetOrInsertChosenNode,
 };
 
diff --git a/ArmVirtPkg/Include/Protocol/FdtClient.h b/ArmVirtPkg/Include/Protocol/FdtClient.h
index b593c7441426..aad76db388be 100644
--- a/ArmVirtPkg/Include/Protocol/FdtClient.h
+++ b/ArmVirtPkg/Include/Protocol/FdtClient.h
@@ -87,6 +87,29 @@ EFI_STATUS
 
 typedef
 EFI_STATUS
+(EFIAPI *FDT_CLIENT_FIND_NEXT_MEMORY_NODE_REG) (
+  IN  FDT_CLIENT_PROTOCOL     *This,
+  IN  INT32                   PrevNode,
+  OUT INT32                   *Node,
+  OUT CONST VOID              **Reg,
+  OUT UINTN                   *AddressCells,
+  OUT UINTN                   *SizeCells,
+  OUT UINT32                  *RegSize
+  );
+
+typedef
+EFI_STATUS
+(EFIAPI *FDT_CLIENT_FIND_MEMORY_NODE_REG) (
+  IN  FDT_CLIENT_PROTOCOL     *This,
+  OUT INT32                   *Node,
+  OUT CONST VOID              **Reg,
+  OUT UINTN                   *AddressCells,
+  OUT UINTN                   *SizeCells,
+  OUT UINT32                  *RegSize
+  );
+
+typedef
+EFI_STATUS
 (EFIAPI *FDT_CLIENT_GET_OR_INSERT_CHOSEN_NODE) (
   IN  FDT_CLIENT_PROTOCOL     *This,
   OUT INT32                   *Node
@@ -101,6 +124,9 @@ struct _FDT_CLIENT_PROTOCOL {
   FDT_CLIENT_FIND_COMPATIBLE_NODE_PROPERTY FindCompatibleNodeProperty;
   FDT_CLIENT_FIND_COMPATIBLE_NODE_REG      FindCompatibleNodeReg;
 
+  FDT_CLIENT_FIND_MEMORY_NODE_REG          FindMemoryNodeReg;
+  FDT_CLIENT_FIND_NEXT_MEMORY_NODE_REG     FindNextMemoryNodeReg;
+
   FDT_CLIENT_GET_OR_INSERT_CHOSEN_NODE     GetOrInsertChosenNode;
 };
 
-- 
2.7.4

[PATCH 4/4] ArmVirtPkg/HighMemDxe: move to FDT client protocol

[Ard Biesheuvel]

Use the FDT client protocol rather than parsing the DT directly using
fdtlib. While we're at it, update the code so it deals correctly with
memory nodes that describe multiple disjoint regions in their "reg"
properties, and make the code work with #address-cells/#size-cells
properties of <1> as well as <2>.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 ArmVirtPkg/HighMemDxe/HighMemDxe.c   | 120 +++++++++-----------
 ArmVirtPkg/HighMemDxe/HighMemDxe.inf |  16 ++-
 2 files changed, 62 insertions(+), 74 deletions(-)

diff --git a/ArmVirtPkg/HighMemDxe/HighMemDxe.c b/ArmVirtPkg/HighMemDxe/HighMemDxe.c
index 4d56e6236b54..08de3cbb7e9c 100644
--- a/ArmVirtPkg/HighMemDxe/HighMemDxe.c
+++ b/ArmVirtPkg/HighMemDxe/HighMemDxe.c
@@ -1,7 +1,7 @@
 /** @file
 *  High memory node enumeration DXE driver for ARM Virtual Machines
 *
-*  Copyright (c) 2015, Linaro Ltd. All rights reserved.
+*  Copyright (c) 2015-2016, Linaro Ltd. All rights reserved.
 *
 *  This program and the accompanying materials are licensed and made available
 *  under the terms and conditions of the BSD License which accompanies this
@@ -15,12 +15,12 @@
 **/
 
 #include <Library/BaseLib.h>
-#include <Library/UefiDriverEntryPoint.h>
 #include <Library/DebugLib.h>
-#include <Library/PcdLib.h>
-#include <Library/HobLib.h>
-#include <libfdt.h>
 #include <Library/DxeServicesTableLib.h>
+#include <Library/PcdLib.h>
+#include <Library/UefiBootServicesTableLib.h>
+
+#include <Protocol/FdtClient.h>
 
 EFI_STATUS
 EFIAPI
@@ -29,76 +29,66 @@ InitializeHighMemDxe (
   IN EFI_SYSTEM_TABLE     *SystemTable
   )
 {
-  VOID             *Hob;
-  VOID             *DeviceTreeBase;
-  INT32            Node, Prev;
-  EFI_STATUS       Status;
-  CONST CHAR8      *Type;
-  INT32            Len;
-  CONST VOID       *RegProp;
-  UINT64           CurBase;
-  UINT64           CurSize;
-
-  Hob = GetFirstGuidHob(&gFdtHobGuid);
-  if (Hob == NULL || GET_GUID_HOB_DATA_SIZE (Hob) != sizeof (UINT64)) {
-    return EFI_NOT_FOUND;
-  }
-  DeviceTreeBase = (VOID *)(UINTN)*(UINT64 *)GET_GUID_HOB_DATA (Hob);
-
-  if (fdt_check_header (DeviceTreeBase) != 0) {
-    DEBUG ((EFI_D_ERROR, "%a: No DTB found @ 0x%p\n", __FUNCTION__,
-      DeviceTreeBase));
-    return EFI_NOT_FOUND;
-  }
+  FDT_CLIENT_PROTOCOL   *FdtClient;
+  EFI_STATUS            Status, FindNodeStatus;
+  INT32                 Node;
+  CONST UINT32          *Reg;
+  UINT32                RegSize;
+  UINTN                 AddressCells, SizeCells;
+  UINT64                CurBase;
+  UINT64                CurSize;
 
-  DEBUG ((EFI_D_INFO, "%a: DTB @ 0x%p\n", __FUNCTION__, DeviceTreeBase));
+  Status = gBS->LocateProtocol (&gFdtClientProtocolGuid, NULL,
+                  (VOID **)&FdtClient);
+  ASSERT_EFI_ERROR (Status);
 
   //
-  // Check for memory node and add the memory spaces expect the lowest one
+  // Check for memory node and add the memory spaces except the lowest one
   //
-  for (Prev = 0;; Prev = Node) {
-    Node = fdt_next_node (DeviceTreeBase, Prev, NULL);
-    if (Node < 0) {
-      break;
-    }
+  for (FindNodeStatus = FdtClient->FindMemoryNodeReg (FdtClient, &Node,
+                                     (CONST VOID **) &Reg, &AddressCells,
+                                     &SizeCells, &RegSize);
+       !EFI_ERROR (FindNodeStatus);
+       FindNodeStatus = FdtClient->FindNextMemoryNodeReg (FdtClient, Node,
+                                     &Node, (CONST VOID **) &Reg, &AddressCells,
+                                     &SizeCells, &RegSize)) {
+
+    ASSERT (AddressCells <= 2);
+    ASSERT (SizeCells <= 2);
 
-    Type = fdt_getprop (DeviceTreeBase, Node, "device_type", &Len);
-    if (Type && AsciiStrnCmp (Type, "memory", Len) == 0) {
-      //
-      // Get the 'reg' property of this node. For now, we will assume
-      // two 8 byte quantities for base and size, respectively.
-      //
-      RegProp = fdt_getprop (DeviceTreeBase, Node, "reg", &Len);
-      if (RegProp != NULL && Len == (2 * sizeof (UINT64))) {
+    while (RegSize > 0) {
 
-        CurBase = fdt64_to_cpu (((UINT64 *)RegProp)[0]);
-        CurSize = fdt64_to_cpu (((UINT64 *)RegProp)[1]);
+      CurBase = SwapBytes32 (*Reg++);
+      if (AddressCells > 1) {
+        CurBase |= (CurBase << 32) | SwapBytes32 (*Reg++);
+      }
+      CurSize = SwapBytes32 (*Reg++);
+      if (SizeCells > 1) {
+        CurSize |= (CurSize << 32) | SwapBytes32 (*Reg++);
+      }
+      RegSize -= (AddressCells + SizeCells) * sizeof (UINT32);
 
-        if (PcdGet64 (PcdSystemMemoryBase) != CurBase) {
-          Status = gDS->AddMemorySpace (
-                          EfiGcdMemoryTypeSystemMemory,
-                          CurBase, CurSize,
-                          EFI_MEMORY_WB);
+      if (PcdGet64 (PcdSystemMemoryBase) != CurBase) {
+        Status = gDS->AddMemorySpace (EfiGcdMemoryTypeSystemMemory, CurBase,
+                        CurSize, EFI_MEMORY_WB);
 
-          if (EFI_ERROR (Status)) {
-            DEBUG ((EFI_D_ERROR,
-              "%a: Failed to add System RAM @ 0x%lx - 0x%lx (%r)\n",
-              __FUNCTION__, CurBase, CurBase + CurSize - 1, Status));
-            continue;
-          }
+        if (EFI_ERROR (Status)) {
+          DEBUG ((EFI_D_ERROR,
+            "%a: Failed to add System RAM @ 0x%lx - 0x%lx (%r)\n",
+            __FUNCTION__, CurBase, CurBase + CurSize - 1, Status));
+          continue;
+        }
 
-          Status = gDS->SetMemorySpaceAttributes (
-                          CurBase, CurSize,
-                          EFI_MEMORY_WB);
+        Status = gDS->SetMemorySpaceAttributes (CurBase, CurSize,
+                        EFI_MEMORY_WB);
 
-          if (EFI_ERROR (Status)) {
-            DEBUG ((EFI_D_ERROR,
-              "%a: Failed to set System RAM @ 0x%lx - 0x%lx attribute (%r)\n",
-              __FUNCTION__, CurBase, CurBase + CurSize - 1, Status));
-          } else {
-            DEBUG ((EFI_D_INFO, "%a: Add System RAM @ 0x%lx - 0x%lx\n",
-              __FUNCTION__, CurBase, CurBase + CurSize - 1));
-          }
+        if (EFI_ERROR (Status)) {
+          DEBUG ((EFI_D_ERROR,
+            "%a: Failed to set System RAM @ 0x%lx - 0x%lx attribute (%r)\n",
+            __FUNCTION__, CurBase, CurBase + CurSize - 1, Status));
+        } else {
+          DEBUG ((EFI_D_INFO, "%a: Add System RAM @ 0x%lx - 0x%lx\n",
+            __FUNCTION__, CurBase, CurBase + CurSize - 1));
         }
       }
     }
diff --git a/ArmVirtPkg/HighMemDxe/HighMemDxe.inf b/ArmVirtPkg/HighMemDxe/HighMemDxe.inf
index ae632a8bee93..3661cfd8c80c 100644
--- a/ArmVirtPkg/HighMemDxe/HighMemDxe.inf
+++ b/ArmVirtPkg/HighMemDxe/HighMemDxe.inf
@@ -1,7 +1,7 @@
 ## @file
 #  High memory node enumeration DXE driver for ARM Virtual Machines
 #
-#  Copyright (c) 2015, Linaro Ltd. All rights reserved.
+#  Copyright (c) 2015-2016, Linaro Ltd. All rights reserved.
 #
 #  This program and the accompanying materials are licensed and made available
 #  under the terms and conditions of the BSD License which accompanies this
@@ -30,23 +30,21 @@ [Packages]
   MdePkg/MdePkg.dec
   MdeModulePkg/MdeModulePkg.dec
   ArmPkg/ArmPkg.dec
-  ArmPlatformPkg/ArmPlatformPkg.dec
   ArmVirtPkg/ArmVirtPkg.dec
-  EmbeddedPkg/EmbeddedPkg.dec
 
 [LibraryClasses]
   BaseLib
+  DebugLib
+  DxeServicesTableLib
   PcdLib
+  UefiBootServicesTableLib
   UefiDriverEntryPoint
-  FdtLib
-  HobLib
-  DxeServicesTableLib
 
-[Guids]
-  gFdtHobGuid
+[Protocols]
+  gFdtClientProtocolGuid                  ## CONSUMES
 
 [Pcd]
   gArmTokenSpaceGuid.PcdSystemMemoryBase
 
 [Depex]
-  gEfiCpuArchProtocolGuid
+  gEfiCpuArchProtocolGuid AND gFdtClientProtocolGuid
-- 
2.7.4

Re: [PATCH 0/4] ArmVirtPkg: FdtClientDxe & HighMemDxe updates

[Ard Biesheuvel]

On 15 September 2016 at 14:30, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> Patch #4 moves HighMemDxe to the FDT client protocol, and updates it to
> handle #address-cells/#size-cells values of <1> as well as <2>, and lets
> it deal with memory nodes whose 'reg' properties describe multiple disjoint
> regions.
>
> Patches #1 to #3 are somewhat preparatory in nature:
> - Patch #1 is a small thinko fix
> - Patch #2 drops the 'regelemsize' output parameter for 'reg' related methods
>   of the FDT client protocol in favour of addresscells/sizecells.
> - Patch #3 adds methods to iterate over all 'reg' properties of all memory nodes
>

Branch here
https://git.linaro.org/people/ard.biesheuvel/uefi-next.git/shortlog/refs/heads/virt-highmem-fdt

> Ard Biesheuvel (4):
>   ArmVirtPkg/FdtClientDxe: fix check for size of "reg" properties
>   ArmVirtPkg/FdtClientDxe: report address and size cell count directly
>   ArmVirtPkg/FdtClient: add methods to iterate over memory nodes
>   ArmVirtPkg/HighMemDxe: move to FDT client protocol
>
>  ArmVirtPkg/FdtClientDxe/FdtClientDxe.c                   |  84 +++++++++++++-
>  ArmVirtPkg/HighMemDxe/HighMemDxe.c                       | 120 +++++++++-----------
>  ArmVirtPkg/HighMemDxe/HighMemDxe.inf                     |  16 ++-
>  ArmVirtPkg/Include/Protocol/FdtClient.h                  |  29 ++++-
>  ArmVirtPkg/Library/ArmVirtGicArchLib/ArmVirtGicArchLib.c |   9 +-
>  ArmVirtPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c           |   9 +-
>  ArmVirtPkg/XenAcpiPlatformDxe/XenAcpiPlatformDxe.c       |   8 +-
>  ArmVirtPkg/XenioFdtDxe/XenioFdtDxe.c                     |  10 +-
>  8 files changed, 196 insertions(+), 89 deletions(-)
>
> --
> 2.7.4
>

Re: [PATCH 1/4] ArmVirtPkg/FdtClientDxe: fix check for size of "reg" properties

[Laszlo Ersek]

On 09/15/16 15:30, Ard Biesheuvel wrote:
> Currently, the code in FdtClientDxe assumes #address-cells and

s/and/are/?

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

> of <base, size> tuples, this means the size of the entire property
> should always be a multiple of 16 bytes (i.e, 4 * sizeof(UINT32),
> not 8. So fix this.
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  ArmVirtPkg/FdtClientDxe/FdtClientDxe.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/ArmVirtPkg/FdtClientDxe/FdtClientDxe.c b/ArmVirtPkg/FdtClientDxe/FdtClientDxe.c
> index c336e2410033..2063a597323b 100644
> --- a/ArmVirtPkg/FdtClientDxe/FdtClientDxe.c
> +++ b/ArmVirtPkg/FdtClientDxe/FdtClientDxe.c
> @@ -178,7 +178,7 @@ FindCompatibleNodeReg (
>      return Status;
>    }
>  
> -  if ((*RegSize % 8) != 0) {
> +  if ((*RegSize % 16) != 0) {
>      DEBUG ((EFI_D_ERROR,
>        "%a: '%a' compatible node has invalid 'reg' property (size == 0x%x)\n",
>        __FUNCTION__, CompatibleString, *RegSize));
> 

Re: [PATCH 1/4] ArmVirtPkg/FdtClientDxe: fix check for size of "reg" properties

[Ard Biesheuvel]

On 15 September 2016 at 14:38, Laszlo Ersek <lersek@redhat.com> wrote:
> On 09/15/16 15:30, Ard Biesheuvel wrote:
>> Currently, the code in FdtClientDxe assumes #address-cells and
>
> s/and/are/?
>

No, amusingly, the second sentence started with #size-cells, and was
dropped completely by git commit due to the leading #. I will try to
remember what I put there, and fix it up when committing.

> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
>

Thanks,
Ard.

Re: [PATCH 2/4] ArmVirtPkg/FdtClientDxe: report address and size cell count directly

[Laszlo Ersek]

On 09/15/16 15:30, Ard Biesheuvel wrote:
> The FDT client protocol methods dealing with "reg" properties return
> the size of a 'reg' element. Currently, we have hardcoded this as '8',
> since #address-cells == #size-cells == 2 in most cases. However, for
> different values, have a single 'reg' element size is not unambiguous,
> since - however unlikely - if #address-cells != #size-cells, we do not
> know which is which.
> 
> So before adding more methods to the protocol, fix up this oversight.
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  ArmVirtPkg/FdtClientDxe/FdtClientDxe.c                   |  6 ++++--
>  ArmVirtPkg/Include/Protocol/FdtClient.h                  |  3 ++-
>  ArmVirtPkg/Library/ArmVirtGicArchLib/ArmVirtGicArchLib.c |  9 ++++++---
>  ArmVirtPkg/Library/QemuFwCfgLib/QemuFwCfgLib.c           |  9 ++++++---
>  ArmVirtPkg/XenAcpiPlatformDxe/XenAcpiPlatformDxe.c       |  8 ++++++--
>  ArmVirtPkg/XenioFdtDxe/XenioFdtDxe.c                     | 10 +++++++---
>  6 files changed, 31 insertions(+), 14 deletions(-)

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Re: [PATCH 3/4] ArmVirtPkg/FdtClient: add methods to iterate over memory nodes

[Laszlo Ersek]

On 09/15/16 15:30, Ard Biesheuvel wrote:
> Add high level methods to iterate over all 'reg' properties of all DT
> nodes whose device_type properties have the value "memory". Since we are
> modifying the FdtClient protocol, update the protocol and the only existing
> implementation at the same time.
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  ArmVirtPkg/FdtClientDxe/FdtClientDxe.c  | 76 ++++++++++++++++++++
>  ArmVirtPkg/Include/Protocol/FdtClient.h | 26 +++++++
>  2 files changed, 102 insertions(+)
> 
> diff --git a/ArmVirtPkg/FdtClientDxe/FdtClientDxe.c b/ArmVirtPkg/FdtClientDxe/FdtClientDxe.c
> index 382e9af6bf84..099cce7821d6 100644
> --- a/ArmVirtPkg/FdtClientDxe/FdtClientDxe.c
> +++ b/ArmVirtPkg/FdtClientDxe/FdtClientDxe.c
> @@ -194,6 +194,80 @@ FindCompatibleNodeReg (
>  
>  STATIC
>  EFI_STATUS
> +EFIAPI
> +FindNextMemoryNodeReg (
> +  IN  FDT_CLIENT_PROTOCOL     *This,
> +  IN  INT32                   PrevNode,
> +  OUT INT32                   *Node,
> +  OUT CONST VOID              **Reg,
> +  OUT UINTN                   *AddressCells,
> +  OUT UINTN                   *SizeCells,
> +  OUT UINT32                  *RegSize
> +  )
> +{
> +  INT32          Prev, Next;
> +  CONST CHAR8    *DeviceType;
> +  INT32          Len;
> +  EFI_STATUS     Status;
> +
> +  ASSERT (mDeviceTreeBase != NULL);
> +  ASSERT (Node != NULL);
> +
> +  for (Prev = PrevNode;; Prev = Next) {
> +    Next = fdt_next_node (mDeviceTreeBase, Prev, NULL);
> +    if (Next < 0) {
> +      break;
> +    }
> +
> +    DeviceType = fdt_getprop (mDeviceTreeBase, Next, "device_type", &Len);
> +    if (DeviceType != NULL && AsciiStrCmp (DeviceType, "memory") == 0) {

HighMemDxe uses AsciiStrnCmp (Type, "memory", Len) here.

If we're sure that we're not looking "memory*" types here, then the
change is fine. Are we sure?

> +

The empty line is likely unintended.

> +      //
> +      // Get the 'reg' property of this memory node. For now, we will assume
> +      // 8 byte quantities for base and size, respectively.
> +      // TODO use #cells root properties instead
> +      //
> +      Status = GetNodeProperty (This, Next, "reg", Reg, RegSize);
> +      if (EFI_ERROR (Status)) {
> +        DEBUG ((EFI_D_WARN,
> +          "%a: ignoring memory node with no 'reg' property\n",
> +          __FUNCTION__));
> +        continue;
> +      }
> +      if ((*RegSize % 16) != 0) {
> +        DEBUG ((EFI_D_WARN,
> +          "%a: ignoring memory node with invalid 'reg' property (size == 0x%x)\n",
> +          __FUNCTION__, RegSize));

This should be *RegSize.

With the above confirmed / fixed:

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Re: [PATCH 3/4] ArmVirtPkg/FdtClient: add methods to iterate over memory nodes

[Ard Biesheuvel]

On 15 September 2016 at 14:57, Laszlo Ersek <lersek@redhat.com> wrote:
> On 09/15/16 15:30, Ard Biesheuvel wrote:
>> Add high level methods to iterate over all 'reg' properties of all DT
>> nodes whose device_type properties have the value "memory". Since we are
>> modifying the FdtClient protocol, update the protocol and the only existing
>> implementation at the same time.
>>
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> ---
>>  ArmVirtPkg/FdtClientDxe/FdtClientDxe.c  | 76 ++++++++++++++++++++
>>  ArmVirtPkg/Include/Protocol/FdtClient.h | 26 +++++++
>>  2 files changed, 102 insertions(+)
>>
>> diff --git a/ArmVirtPkg/FdtClientDxe/FdtClientDxe.c b/ArmVirtPkg/FdtClientDxe/FdtClientDxe.c
>> index 382e9af6bf84..099cce7821d6 100644
>> --- a/ArmVirtPkg/FdtClientDxe/FdtClientDxe.c
>> +++ b/ArmVirtPkg/FdtClientDxe/FdtClientDxe.c
>> @@ -194,6 +194,80 @@ FindCompatibleNodeReg (
>>
>>  STATIC
>>  EFI_STATUS
>> +EFIAPI
>> +FindNextMemoryNodeReg (
>> +  IN  FDT_CLIENT_PROTOCOL     *This,
>> +  IN  INT32                   PrevNode,
>> +  OUT INT32                   *Node,
>> +  OUT CONST VOID              **Reg,
>> +  OUT UINTN                   *AddressCells,
>> +  OUT UINTN                   *SizeCells,
>> +  OUT UINT32                  *RegSize
>> +  )
>> +{
>> +  INT32          Prev, Next;
>> +  CONST CHAR8    *DeviceType;
>> +  INT32          Len;
>> +  EFI_STATUS     Status;
>> +
>> +  ASSERT (mDeviceTreeBase != NULL);
>> +  ASSERT (Node != NULL);
>> +
>> +  for (Prev = PrevNode;; Prev = Next) {
>> +    Next = fdt_next_node (mDeviceTreeBase, Prev, NULL);
>> +    if (Next < 0) {
>> +      break;
>> +    }
>> +
>> +    DeviceType = fdt_getprop (mDeviceTreeBase, Next, "device_type", &Len);
>> +    if (DeviceType != NULL && AsciiStrCmp (DeviceType, "memory") == 0) {
>
> HighMemDxe uses AsciiStrnCmp (Type, "memory", Len) here.
>
> If we're sure that we're not looking "memory*" types here, then the
> change is fine. Are we sure?
>

Actually, that is a bug. AsciiStrCmp () is guaranteed to check the NUL
terminator, and so it will only match on "memory\0". Using
AsciiStrnCmp() with the length of the property value is guaranteed
*not* to check the NUL terminator, so it may match on prefixes of
"memory\0"




>> +
>
> The empty line is likely unintended.
>

indeed.

>> +      //
>> +      // Get the 'reg' property of this memory node. For now, we will assume
>> +      // 8 byte quantities for base and size, respectively.
>> +      // TODO use #cells root properties instead
>> +      //
>> +      Status = GetNodeProperty (This, Next, "reg", Reg, RegSize);
>> +      if (EFI_ERROR (Status)) {
>> +        DEBUG ((EFI_D_WARN,
>> +          "%a: ignoring memory node with no 'reg' property\n",
>> +          __FUNCTION__));
>> +        continue;
>> +      }
>> +      if ((*RegSize % 16) != 0) {
>> +        DEBUG ((EFI_D_WARN,
>> +          "%a: ignoring memory node with invalid 'reg' property (size == 0x%x)\n",
>> +          __FUNCTION__, RegSize));
>
> This should be *RegSize.
>

OK

> With the above confirmed / fixed:
>
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Re: [PATCH 4/4] ArmVirtPkg/HighMemDxe: move to FDT client protocol

[Laszlo Ersek]

On 09/15/16 15:30, Ard Biesheuvel wrote:
> Use the FDT client protocol rather than parsing the DT directly using
> fdtlib. While we're at it, update the code so it deals correctly with
> memory nodes that describe multiple disjoint regions in their "reg"
> properties, and make the code work with #address-cells/#size-cells
> properties of <1> as well as <2>.
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  ArmVirtPkg/HighMemDxe/HighMemDxe.c   | 120 +++++++++-----------
>  ArmVirtPkg/HighMemDxe/HighMemDxe.inf |  16 ++-
>  2 files changed, 62 insertions(+), 74 deletions(-)
> 
> diff --git a/ArmVirtPkg/HighMemDxe/HighMemDxe.c b/ArmVirtPkg/HighMemDxe/HighMemDxe.c
> index 4d56e6236b54..08de3cbb7e9c 100644
> --- a/ArmVirtPkg/HighMemDxe/HighMemDxe.c
> +++ b/ArmVirtPkg/HighMemDxe/HighMemDxe.c
> @@ -1,7 +1,7 @@
>  /** @file
>  *  High memory node enumeration DXE driver for ARM Virtual Machines
>  *
> -*  Copyright (c) 2015, Linaro Ltd. All rights reserved.
> +*  Copyright (c) 2015-2016, Linaro Ltd. All rights reserved.
>  *
>  *  This program and the accompanying materials are licensed and made available
>  *  under the terms and conditions of the BSD License which accompanies this
> @@ -15,12 +15,12 @@
>  **/
>  
>  #include <Library/BaseLib.h>
> -#include <Library/UefiDriverEntryPoint.h>
>  #include <Library/DebugLib.h>
> -#include <Library/PcdLib.h>
> -#include <Library/HobLib.h>
> -#include <libfdt.h>
>  #include <Library/DxeServicesTableLib.h>
> +#include <Library/PcdLib.h>
> +#include <Library/UefiBootServicesTableLib.h>
> +
> +#include <Protocol/FdtClient.h>
>  
>  EFI_STATUS
>  EFIAPI
> @@ -29,76 +29,66 @@ InitializeHighMemDxe (
>    IN EFI_SYSTEM_TABLE     *SystemTable
>    )
>  {
> -  VOID             *Hob;
> -  VOID             *DeviceTreeBase;
> -  INT32            Node, Prev;
> -  EFI_STATUS       Status;
> -  CONST CHAR8      *Type;
> -  INT32            Len;
> -  CONST VOID       *RegProp;
> -  UINT64           CurBase;
> -  UINT64           CurSize;
> -
> -  Hob = GetFirstGuidHob(&gFdtHobGuid);
> -  if (Hob == NULL || GET_GUID_HOB_DATA_SIZE (Hob) != sizeof (UINT64)) {
> -    return EFI_NOT_FOUND;
> -  }
> -  DeviceTreeBase = (VOID *)(UINTN)*(UINT64 *)GET_GUID_HOB_DATA (Hob);
> -
> -  if (fdt_check_header (DeviceTreeBase) != 0) {
> -    DEBUG ((EFI_D_ERROR, "%a: No DTB found @ 0x%p\n", __FUNCTION__,
> -      DeviceTreeBase));
> -    return EFI_NOT_FOUND;
> -  }
> +  FDT_CLIENT_PROTOCOL   *FdtClient;
> +  EFI_STATUS            Status, FindNodeStatus;
> +  INT32                 Node;
> +  CONST UINT32          *Reg;
> +  UINT32                RegSize;
> +  UINTN                 AddressCells, SizeCells;
> +  UINT64                CurBase;
> +  UINT64                CurSize;
>  
> -  DEBUG ((EFI_D_INFO, "%a: DTB @ 0x%p\n", __FUNCTION__, DeviceTreeBase));
> +  Status = gBS->LocateProtocol (&gFdtClientProtocolGuid, NULL,
> +                  (VOID **)&FdtClient);
> +  ASSERT_EFI_ERROR (Status);
>  
>    //
> -  // Check for memory node and add the memory spaces expect the lowest one
> +  // Check for memory node and add the memory spaces except the lowest one
>    //
> -  for (Prev = 0;; Prev = Node) {
> -    Node = fdt_next_node (DeviceTreeBase, Prev, NULL);
> -    if (Node < 0) {
> -      break;
> -    }
> +  for (FindNodeStatus = FdtClient->FindMemoryNodeReg (FdtClient, &Node,
> +                                     (CONST VOID **) &Reg, &AddressCells,
> +                                     &SizeCells, &RegSize);
> +       !EFI_ERROR (FindNodeStatus);
> +       FindNodeStatus = FdtClient->FindNextMemoryNodeReg (FdtClient, Node,
> +                                     &Node, (CONST VOID **) &Reg, &AddressCells,
> +                                     &SizeCells, &RegSize)) {
> +

I think we can remove this empty line. Not too important of course.

> +    ASSERT (AddressCells <= 2);
> +    ASSERT (SizeCells <= 2);
>  
> -    Type = fdt_getprop (DeviceTreeBase, Node, "device_type", &Len);
> -    if (Type && AsciiStrnCmp (Type, "memory", Len) == 0) {
> -      //
> -      // Get the 'reg' property of this node. For now, we will assume
> -      // two 8 byte quantities for base and size, respectively.
> -      //
> -      RegProp = fdt_getprop (DeviceTreeBase, Node, "reg", &Len);
> -      if (RegProp != NULL && Len == (2 * sizeof (UINT64))) {
> +    while (RegSize > 0) {
>  

This one too. (Also just a matter of taste.)

> -        CurBase = fdt64_to_cpu (((UINT64 *)RegProp)[0]);
> -        CurSize = fdt64_to_cpu (((UINT64 *)RegProp)[1]);
> +      CurBase = SwapBytes32 (*Reg++);
> +      if (AddressCells > 1) {
> +        CurBase |= (CurBase << 32) | SwapBytes32 (*Reg++);

The operator |= seems wrong; it should be just =.

(You could do { CurBase <<= 32; CurBase |= SwapBytes32 (*Reg++); }. The
above is more compact, but the operator should be fixed.)

> +      }
> +      CurSize = SwapBytes32 (*Reg++);
> +      if (SizeCells > 1) {
> +        CurSize |= (CurSize << 32) | SwapBytes32 (*Reg++);

Same here.

With these fixed:

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Re: [PATCH 4/4] ArmVirtPkg/HighMemDxe: move to FDT client protocol

[Ard Biesheuvel]

On 15 September 2016 at 15:15, Laszlo Ersek <lersek@redhat.com> wrote:
> On 09/15/16 15:30, Ard Biesheuvel wrote:
>> Use the FDT client protocol rather than parsing the DT directly using
>> fdtlib. While we're at it, update the code so it deals correctly with
>> memory nodes that describe multiple disjoint regions in their "reg"
>> properties, and make the code work with #address-cells/#size-cells
>> properties of <1> as well as <2>.
>>
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> ---
>>  ArmVirtPkg/HighMemDxe/HighMemDxe.c   | 120 +++++++++-----------
>>  ArmVirtPkg/HighMemDxe/HighMemDxe.inf |  16 ++-
>>  2 files changed, 62 insertions(+), 74 deletions(-)
>>
>> diff --git a/ArmVirtPkg/HighMemDxe/HighMemDxe.c b/ArmVirtPkg/HighMemDxe/HighMemDxe.c
>> index 4d56e6236b54..08de3cbb7e9c 100644
>> --- a/ArmVirtPkg/HighMemDxe/HighMemDxe.c
>> +++ b/ArmVirtPkg/HighMemDxe/HighMemDxe.c
>> @@ -1,7 +1,7 @@
>>  /** @file
>>  *  High memory node enumeration DXE driver for ARM Virtual Machines
>>  *
>> -*  Copyright (c) 2015, Linaro Ltd. All rights reserved.
>> +*  Copyright (c) 2015-2016, Linaro Ltd. All rights reserved.
>>  *
>>  *  This program and the accompanying materials are licensed and made available
>>  *  under the terms and conditions of the BSD License which accompanies this
>> @@ -15,12 +15,12 @@
>>  **/
>>
>>  #include <Library/BaseLib.h>
>> -#include <Library/UefiDriverEntryPoint.h>
>>  #include <Library/DebugLib.h>
>> -#include <Library/PcdLib.h>
>> -#include <Library/HobLib.h>
>> -#include <libfdt.h>
>>  #include <Library/DxeServicesTableLib.h>
>> +#include <Library/PcdLib.h>
>> +#include <Library/UefiBootServicesTableLib.h>
>> +
>> +#include <Protocol/FdtClient.h>
>>
>>  EFI_STATUS
>>  EFIAPI
>> @@ -29,76 +29,66 @@ InitializeHighMemDxe (
>>    IN EFI_SYSTEM_TABLE     *SystemTable
>>    )
>>  {
>> -  VOID             *Hob;
>> -  VOID             *DeviceTreeBase;
>> -  INT32            Node, Prev;
>> -  EFI_STATUS       Status;
>> -  CONST CHAR8      *Type;
>> -  INT32            Len;
>> -  CONST VOID       *RegProp;
>> -  UINT64           CurBase;
>> -  UINT64           CurSize;
>> -
>> -  Hob = GetFirstGuidHob(&gFdtHobGuid);
>> -  if (Hob == NULL || GET_GUID_HOB_DATA_SIZE (Hob) != sizeof (UINT64)) {
>> -    return EFI_NOT_FOUND;
>> -  }
>> -  DeviceTreeBase = (VOID *)(UINTN)*(UINT64 *)GET_GUID_HOB_DATA (Hob);
>> -
>> -  if (fdt_check_header (DeviceTreeBase) != 0) {
>> -    DEBUG ((EFI_D_ERROR, "%a: No DTB found @ 0x%p\n", __FUNCTION__,
>> -      DeviceTreeBase));
>> -    return EFI_NOT_FOUND;
>> -  }
>> +  FDT_CLIENT_PROTOCOL   *FdtClient;
>> +  EFI_STATUS            Status, FindNodeStatus;
>> +  INT32                 Node;
>> +  CONST UINT32          *Reg;
>> +  UINT32                RegSize;
>> +  UINTN                 AddressCells, SizeCells;
>> +  UINT64                CurBase;
>> +  UINT64                CurSize;
>>
>> -  DEBUG ((EFI_D_INFO, "%a: DTB @ 0x%p\n", __FUNCTION__, DeviceTreeBase));
>> +  Status = gBS->LocateProtocol (&gFdtClientProtocolGuid, NULL,
>> +                  (VOID **)&FdtClient);
>> +  ASSERT_EFI_ERROR (Status);
>>
>>    //
>> -  // Check for memory node and add the memory spaces expect the lowest one
>> +  // Check for memory node and add the memory spaces except the lowest one
>>    //
>> -  for (Prev = 0;; Prev = Node) {
>> -    Node = fdt_next_node (DeviceTreeBase, Prev, NULL);
>> -    if (Node < 0) {
>> -      break;
>> -    }
>> +  for (FindNodeStatus = FdtClient->FindMemoryNodeReg (FdtClient, &Node,
>> +                                     (CONST VOID **) &Reg, &AddressCells,
>> +                                     &SizeCells, &RegSize);
>> +       !EFI_ERROR (FindNodeStatus);
>> +       FindNodeStatus = FdtClient->FindNextMemoryNodeReg (FdtClient, Node,
>> +                                     &Node, (CONST VOID **) &Reg, &AddressCells,
>> +                                     &SizeCells, &RegSize)) {
>> +
>
> I think we can remove this empty line. Not too important of course.
>

Sure

>> +    ASSERT (AddressCells <= 2);
>> +    ASSERT (SizeCells <= 2);
>>
>> -    Type = fdt_getprop (DeviceTreeBase, Node, "device_type", &Len);
>> -    if (Type && AsciiStrnCmp (Type, "memory", Len) == 0) {
>> -      //
>> -      // Get the 'reg' property of this node. For now, we will assume
>> -      // two 8 byte quantities for base and size, respectively.
>> -      //
>> -      RegProp = fdt_getprop (DeviceTreeBase, Node, "reg", &Len);
>> -      if (RegProp != NULL && Len == (2 * sizeof (UINT64))) {
>> +    while (RegSize > 0) {
>>
>
> This one too. (Also just a matter of taste.)
>

If you prefer, I don't care either way.

>> -        CurBase = fdt64_to_cpu (((UINT64 *)RegProp)[0]);
>> -        CurSize = fdt64_to_cpu (((UINT64 *)RegProp)[1]);
>> +      CurBase = SwapBytes32 (*Reg++);
>> +      if (AddressCells > 1) {
>> +        CurBase |= (CurBase << 32) | SwapBytes32 (*Reg++);
>
> The operator |= seems wrong; it should be just =.
>
> (You could do { CurBase <<= 32; CurBase |= SwapBytes32 (*Reg++); }. The
> above is more compact, but the operator should be fixed.)
>

You are right. I got away with it due to the fact that I tested with
base/size pairs <= 4GB, and so the leading cell == 0 in all cases.
Thanks for spotting that!

>> +      }
>> +      CurSize = SwapBytes32 (*Reg++);
>> +      if (SizeCells > 1) {
>> +        CurSize |= (CurSize << 32) | SwapBytes32 (*Reg++);
>
> Same here.
>
> With these fixed:
>
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
>

OK, thanks.

Re: [PATCH 0/4] ArmVirtPkg: FdtClientDxe & HighMemDxe updates

[Ard Biesheuvel]

On 15 September 2016 at 14:34, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> On 15 September 2016 at 14:30, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>> Patch #4 moves HighMemDxe to the FDT client protocol, and updates it to
>> handle #address-cells/#size-cells values of <1> as well as <2>, and lets
>> it deal with memory nodes whose 'reg' properties describe multiple disjoint
>> regions.
>>
>> Patches #1 to #3 are somewhat preparatory in nature:
>> - Patch #1 is a small thinko fix
>> - Patch #2 drops the 'regelemsize' output parameter for 'reg' related methods
>>   of the FDT client protocol in favour of addresscells/sizecells.
>> - Patch #3 adds methods to iterate over all 'reg' properties of all memory nodes
>>

Committed as

38ed4a9e3a65 ArmVirtPkg/FdtClientDxe: fix check for size of "reg" properties
cfc8d51c0cbf ArmVirtPkg/FdtClientDxe: report address and size cell
count directly
969d2eb3875a ArmVirtPkg/FdtClientDxe: add methods to iterate over memory nodes
490acf8908f7 ArmVirtPkg/HighMemDxe: move to FDT client protocol

with your comments addressed.

Thanks,
Ard,