diff mbox

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

Message ID 1473946233-10547-4-git-send-email-ard.biesheuvel@linaro.org
State Accepted
Commit 969d2eb3875a700a223840d7ea415e78de4f8cbe
Headers show

Commit Message

Ard Biesheuvel Sept. 15, 2016, 1:30 p.m. UTC 
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(+)

-- 
2.7.4

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Comments

Laszlo Ersek Sept. 15, 2016, 1:57 p.m. UTC | #1 
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>

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Ard Biesheuvel Sept. 15, 2016, 2:04 p.m. UTC | #2 
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>

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
diff mbox

Patch

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