[PATCH 0/3] ArmVirtPkg: ignore DT nodes with a status != 'okay'

[PATCH 0/3] ArmVirtPkg: ignore DT nodes with a status != 'okay'

[Ard Biesheuvel]

The DT passed to us by QEMU may contain nodes that are supposed to be
consumed by the secure world only. In some case, their status may be
set to 'secure', but 'disabled' occurs as well, if the secure OS has
actually modified the DT.

Since as a general rule, DT nodes should only be consumed if they lack a
status or if their status equals 'okay', update our DT node parsing
routines accordingly.

Ard Biesheuvel (3):
  ArmVirtPkg/FdtClientDxe: take DT memory node 'status' property into
    account
  ArmVirtPkg/FdtPL011SerialPortLib: take DT node 'status' property into
    account
  ArmVirtPkg/PlatformPeiLib: take DT node 'status' property into account

 ArmVirtPkg/FdtClientDxe/FdtClientDxe.c                                | 8 ++++++++
 ArmVirtPkg/Library/FdtPL011SerialPortLib/EarlyFdtPL011SerialPortLib.c | 6 ++++++
 ArmVirtPkg/Library/PlatformPeiLib/PlatformPeiLib.c                    | 7 +++++++
 3 files changed, 21 insertions(+)

-- 
2.9.3

[PATCH 1/3] ArmVirtPkg/FdtClientDxe: take DT memory node 'status' property into account

[Ard Biesheuvel]

In some cases, (e.g., when running QEMU with TrustZone emulation), the
DT may contain memory nodes whose status is set to 'secure'. Similarly,
the status may be set to 'disabled' if the consumer of the DT image is
expected to treat it as if it weren't there.

So check whether a 'status' property is present, and if so, ignore the
node if the status is not 'okay'.

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

diff --git a/ArmVirtPkg/FdtClientDxe/FdtClientDxe.c b/ArmVirtPkg/FdtClientDxe/FdtClientDxe.c
index 2d867b16fda8..fb6e0aeb9215 100644
--- a/ArmVirtPkg/FdtClientDxe/FdtClientDxe.c
+++ b/ArmVirtPkg/FdtClientDxe/FdtClientDxe.c
@@ -210,6 +210,7 @@ FindNextMemoryNodeReg (
 {
   INT32          Prev, Next;
   CONST CHAR8    *DeviceType;
+  CONST CHAR8    *NodeStatus;
   INT32          Len;
   EFI_STATUS     Status;
 
@@ -222,6 +223,13 @@ FindNextMemoryNodeReg (
       break;
     }
 
+    NodeStatus = fdt_getprop (mDeviceTreeBase, Next, "status", &Len);
+    if (NodeStatus != NULL && AsciiStrCmp (NodeStatus, "okay") != 0) {
+      DEBUG ((DEBUG_WARN, "%a: ignoring memory node with status \"%a\"\n",
+        __FUNCTION__, NodeStatus));
+      continue;
+    }
+
     DeviceType = fdt_getprop (mDeviceTreeBase, Next, "device_type", &Len);
     if (DeviceType != NULL && AsciiStrCmp (DeviceType, "memory") == 0) {
       //
-- 
2.9.3

[PATCH 2/3] ArmVirtPkg/FdtPL011SerialPortLib: take DT node 'status' property into account

[Ard Biesheuvel]

In some cases, (e.g., when running QEMU with TrustZone emulation), the
DT may contain DT nodes whose status is set to 'secure'. Similarly, the
status may be set to 'disabled' if the consumer of the DT image is expected
to treat it as if it weren't there.

So check whether a 'status' property is present, and if so, ignore the node
if the status is not 'okay'.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 ArmVirtPkg/Library/FdtPL011SerialPortLib/EarlyFdtPL011SerialPortLib.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/ArmVirtPkg/Library/FdtPL011SerialPortLib/EarlyFdtPL011SerialPortLib.c b/ArmVirtPkg/Library/FdtPL011SerialPortLib/EarlyFdtPL011SerialPortLib.c
index c458abb622d9..e28750f3b4c4 100644
--- a/ArmVirtPkg/Library/FdtPL011SerialPortLib/EarlyFdtPL011SerialPortLib.c
+++ b/ArmVirtPkg/Library/FdtPL011SerialPortLib/EarlyFdtPL011SerialPortLib.c
@@ -66,6 +66,7 @@ SerialPortGetBaseAddress (
   INT32               Node, Prev;
   INT32               Len;
   CONST CHAR8         *Compatible;
+  CONST CHAR8         *NodeStatus;
   CONST CHAR8         *CompatibleItem;
   CONST UINT64        *RegProperty;
   UINTN               UartBase;
@@ -98,6 +99,11 @@ SerialPortGetBaseAddress (
       CompatibleItem += 1 + AsciiStrLen (CompatibleItem)) {
 
       if (AsciiStrCmp (CompatibleItem, "arm,pl011") == 0) {
+        NodeStatus = fdt_getprop (DeviceTreeBase, Node, "status", &Len);
+        if (NodeStatus != NULL && AsciiStrCmp (NodeStatus, "okay") != 0) {
+          continue;
+        }
+
         RegProperty = fdt_getprop (DeviceTreeBase, Node, "reg", &Len);
         if (Len != 16) {
           return 0;
-- 
2.9.3

[PATCH 3/3] ArmVirtPkg/PlatformPeiLib: take DT node 'status' property into account

[Ard Biesheuvel]

In some cases, (e.g., when running QEMU with TrustZone emulation), the
DT may contain DT nodes whose status is set to 'secure'. Similarly, the
status may be set to 'disabled' if the consumer of the DT image is expected
to treat it as if it weren't there.

So check whether a 'status' property is present, and if so, ignore the node
if the status is not 'okay'.

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

diff --git a/ArmVirtPkg/Library/PlatformPeiLib/PlatformPeiLib.c b/ArmVirtPkg/Library/PlatformPeiLib/PlatformPeiLib.c
index bdf2b57fcb1e..df52d3653360 100644
--- a/ArmVirtPkg/Library/PlatformPeiLib/PlatformPeiLib.c
+++ b/ArmVirtPkg/Library/PlatformPeiLib/PlatformPeiLib.c
@@ -39,7 +39,9 @@ PlatformPeim (
   INT32              Node, Prev;
   CONST CHAR8        *Compatible;
   CONST CHAR8        *CompItem;
+  CONST CHAR8        *NodeStatus;
   INT32              Len;
+  INT32              StatusLen;
   CONST UINT64       *RegProp;
   UINT64             UartBase;
 
@@ -83,6 +85,11 @@ PlatformPeim (
       CompItem += 1 + AsciiStrLen (CompItem)) {
 
       if (AsciiStrCmp (CompItem, "arm,pl011") == 0) {
+        NodeStatus = fdt_getprop (Base, Node, "status", &StatusLen);
+        if (NodeStatus != NULL && AsciiStrCmp (NodeStatus, "okay") != 0) {
+          continue;
+        }
+
         RegProp = fdt_getprop (Base, Node, "reg", &Len);
         ASSERT (Len == 16);
 
-- 
2.9.3

Re: [PATCH 0/3] ArmVirtPkg: ignore DT nodes with a status != 'okay'

[Laszlo Ersek]

On 04/04/17 15:24, Ard Biesheuvel wrote:
> The DT passed to us by QEMU may contain nodes that are supposed to be
> consumed by the secure world only. In some case, their status may be
> set to 'secure', but 'disabled' occurs as well, if the secure OS has
> actually modified the DT.
> 
> Since as a general rule, DT nodes should only be consumed if they lack a
> status or if their status equals 'okay', update our DT node parsing
> routines accordingly.
> 
> Ard Biesheuvel (3):
>   ArmVirtPkg/FdtClientDxe: take DT memory node 'status' property into
>     account
>   ArmVirtPkg/FdtPL011SerialPortLib: take DT node 'status' property into
>     account
>   ArmVirtPkg/PlatformPeiLib: take DT node 'status' property into account
> 
>  ArmVirtPkg/FdtClientDxe/FdtClientDxe.c                                | 8 ++++++++
>  ArmVirtPkg/Library/FdtPL011SerialPortLib/EarlyFdtPL011SerialPortLib.c | 6 ++++++
>  ArmVirtPkg/Library/PlatformPeiLib/PlatformPeiLib.c                    | 7 +++++++
>  3 files changed, 21 insertions(+)
> 

Please re-wrap commit messages #2 and #3 to a width of 74 characters.

The subject lines are also too long (on patches #1 and #2); please come
up with alternatives that fit into 74 characters. For example, replace

  take DT xxx node 'status' property into account

with

  honor DT xxx node 'status' property

With that, series
Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Thanks,
Laszlo

Re: [PATCH 0/3] ArmVirtPkg: ignore DT nodes with a status != 'okay'

[Ard Biesheuvel]

On 4 April 2017 at 15:16, Laszlo Ersek <lersek@redhat.com> wrote:
> On 04/04/17 15:24, Ard Biesheuvel wrote:
>> The DT passed to us by QEMU may contain nodes that are supposed to be
>> consumed by the secure world only. In some case, their status may be
>> set to 'secure', but 'disabled' occurs as well, if the secure OS has
>> actually modified the DT.
>>
>> Since as a general rule, DT nodes should only be consumed if they lack a
>> status or if their status equals 'okay', update our DT node parsing
>> routines accordingly.
>>
>> Ard Biesheuvel (3):
>>   ArmVirtPkg/FdtClientDxe: take DT memory node 'status' property into
>>     account
>>   ArmVirtPkg/FdtPL011SerialPortLib: take DT node 'status' property into
>>     account
>>   ArmVirtPkg/PlatformPeiLib: take DT node 'status' property into account
>>
>>  ArmVirtPkg/FdtClientDxe/FdtClientDxe.c                                | 8 ++++++++
>>  ArmVirtPkg/Library/FdtPL011SerialPortLib/EarlyFdtPL011SerialPortLib.c | 6 ++++++
>>  ArmVirtPkg/Library/PlatformPeiLib/PlatformPeiLib.c                    | 7 +++++++
>>  3 files changed, 21 insertions(+)
>>
>
> Please re-wrap commit messages #2 and #3 to a width of 74 characters.
>
> The subject lines are also too long (on patches #1 and #2); please come
> up with alternatives that fit into 74 characters. For example, replace
>
>   take DT xxx node 'status' property into account
>
> with
>
>   honor DT xxx node 'status' property
>
> With that, series
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
>

Thanks

Pushed as

d014044395b5 ArmVirtPkg/FdtClientDxe: honor memory DT node 'status' property
b1f3e48ed8f9 ArmVirtPkg/FdtPL011SerialPortLib: honor DT node 'status' property
83ae7589b08a ArmVirtPkg/PlatformPeiLib: honor DT node 'status' property