[PATCH v2 0/5] ArmPlatformPkg, ArmVirtPkg: discover NOR flash banks from DTB

[PATCH v2 0/5] ArmPlatformPkg, ArmVirtPkg: discover NOR flash banks from DTB

[Ard Biesheuvel]

This series fixes an issue reported by Hongbo and Philippe, where
ArmVirtQemuKernel will crash on an attempt to access flash bank #0,
which is secure-only when running QEMU with support for EL3.

So let's switch to discovering the NOR flash banks from the device tree
instead. This requires some preparatory changes in the NOR flash driver
to avoid having to invent GUIDs on the fly.

Changes since v1:
- split ArmPlatformPkg for clarity
- move DT node status check into FdtClientDxe where it belongs
- use correct UINT32* type for DT property values, and be pedantic about
  their potential misalignment when casting to UINT64*
- add patch to remove the 'Guid' member from NOR_FLASH_DESCRIPTION
- add some acks

Ard Biesheuvel (5):
  ArmPlatformPkg/NorFlashDxe: prepare for devicepath format change
  ArmPlatformPkg/NorFlashDxe: use one GUID plus index to identify flash
    banks
  ArmVirtPkg/FdtClientDxe: take DT node 'status' properties into account
  ArmVirtPkg/NorFlashQemuLib: discover NOR flash banks dynamically
  ArmPlatformPkg/NorFlashPlatformLib: remove unused Guid member from
    struct

 .../Drivers/NorFlashDxe/NorFlashDxe.c         | 15 ++--
 .../Drivers/NorFlashDxe/NorFlashDxe.h         |  3 +
 .../Include/Library/NorFlashPlatformLib.h     |  1 -
 ArmVirtPkg/FdtClientDxe/FdtClientDxe.c        | 38 +++++++--
 .../Library/NorFlashQemuLib/NorFlashQemuLib.c | 78 ++++++++++++++-----
 .../NorFlashQemuLib/NorFlashQemuLib.inf       | 12 +++
 6 files changed, 114 insertions(+), 33 deletions(-)

-- 
2.17.1

[PATCH v2 1/5] ArmPlatformPkg/NorFlashDxe: prepare for devicepath format change

[Ard Biesheuvel]

A subsequent patch will change the layout of devicepath nodes
produced by this driver. In preparation, make some tweaks to
the code to use a packed struct for the devicepath and to pass
the device index to NorFlashCreateInstance(). These are cosmetic
changes only, the resulting binaries should be identical.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
---
 ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.c | 9 ++++++---
 ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.h | 2 ++
 2 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.c b/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.c
index 46e815beb343..53753a4721ac 100644
--- a/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.c
+++ b/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.c
@@ -82,7 +82,10 @@ NOR_FLASH_INSTANCE  mNorFlashInstanceTemplate = {
       {
         HARDWARE_DEVICE_PATH,
         HW_VENDOR_DP,
-        { (UINT8)sizeof(VENDOR_DEVICE_PATH), (UINT8)((sizeof(VENDOR_DEVICE_PATH)) >> 8) }
+        {
+          (UINT8)(OFFSET_OF (NOR_FLASH_DEVICE_PATH, End)),
+          (UINT8)(OFFSET_OF (NOR_FLASH_DEVICE_PATH, End) >> 8)
+        }
       },
       { 0x0, 0x0, 0x0, { 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0 } }, // GUID ... NEED TO BE FILLED
     },
@@ -99,7 +102,7 @@ NorFlashCreateInstance (
   IN UINTN                  NorFlashDeviceBase,
   IN UINTN                  NorFlashRegionBase,
   IN UINTN                  NorFlashSize,
-  IN UINT32                 MediaId,
+  IN UINT32                 Index,
   IN UINT32                 BlockSize,
   IN BOOLEAN                SupportFvb,
   IN CONST GUID             *NorFlashGuid,
@@ -121,7 +124,7 @@ NorFlashCreateInstance (
   Instance->Size = NorFlashSize;
 
   Instance->BlockIoProtocol.Media = &Instance->Media;
-  Instance->Media.MediaId = MediaId;
+  Instance->Media.MediaId = Index;
   Instance->Media.BlockSize = BlockSize;
   Instance->Media.LastBlock = (NorFlashSize / BlockSize)-1;
 
diff --git a/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.h b/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.h
index 5c07694fbfaa..910681fd4412 100644
--- a/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.h
+++ b/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.h
@@ -122,10 +122,12 @@
 
 typedef struct _NOR_FLASH_INSTANCE                NOR_FLASH_INSTANCE;
 
+#pragma pack(1)
 typedef struct {
   VENDOR_DEVICE_PATH                  Vendor;
   EFI_DEVICE_PATH_PROTOCOL            End;
 } NOR_FLASH_DEVICE_PATH;
+#pragma pack()
 
 struct _NOR_FLASH_INSTANCE {
   UINT32                              Signature;
-- 
2.17.1

[PATCH v2 2/5] ArmPlatformPkg/NorFlashDxe: use one GUID plus index to identify flash banks

[Ard Biesheuvel]

Currently, each flash bank controlled by ArmPlatformPkg/NorFlashDxe
has its own VendorHw GUID, and instances of NorFlashPlatformLib
describe each bank to the driver, along with the GUID for each.

This works ok for bare metal platforms, but it would be useful for
virtual platforms if we could obtain this information from a
device tree, which would require us to invent GUIDs on the fly,
given that the 'cfi-flash' binding does not include a GUID.

So instead, let's switch to a single GUID for all flash banks,
and update the driver's device path handling to include an index
to identify each bank uniquely.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>
---
 ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.c | 6 +++---
 ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.h | 1 +
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.c b/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.c
index 53753a4721ac..af40a4c88412 100644
--- a/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.c
+++ b/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.c
@@ -89,6 +89,7 @@ NOR_FLASH_INSTANCE  mNorFlashInstanceTemplate = {
       },
       { 0x0, 0x0, 0x0, { 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0 } }, // GUID ... NEED TO BE FILLED
     },
+    0, // Index
     {
       END_DEVICE_PATH_TYPE,
       END_ENTIRE_DEVICE_PATH_SUBTYPE,
@@ -105,7 +106,6 @@ NorFlashCreateInstance (
   IN UINT32                 Index,
   IN UINT32                 BlockSize,
   IN BOOLEAN                SupportFvb,
-  IN CONST GUID             *NorFlashGuid,
   OUT NOR_FLASH_INSTANCE**  NorFlashInstance
   )
 {
@@ -128,7 +128,8 @@ NorFlashCreateInstance (
   Instance->Media.BlockSize = BlockSize;
   Instance->Media.LastBlock = (NorFlashSize / BlockSize)-1;
 
-  CopyGuid (&Instance->DevicePath.Vendor.Guid, NorFlashGuid);
+  CopyGuid (&Instance->DevicePath.Vendor.Guid, &gEfiCallerIdGuid);
+  Instance->DevicePath.Index = (UINT8)Index;
 
   Instance->ShadowBuffer = AllocateRuntimePool (BlockSize);;
   if (Instance->ShadowBuffer == NULL) {
@@ -1314,7 +1315,6 @@ NorFlashInitialise (
       Index,
       NorFlashDevices[Index].BlockSize,
       ContainVariableStorage,
-      &NorFlashDevices[Index].Guid,
       &mNorFlashInstances[Index]
     );
     if (EFI_ERROR(Status)) {
diff --git a/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.h b/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.h
index 910681fd4412..8886aa43d9f3 100644
--- a/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.h
+++ b/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.h
@@ -125,6 +125,7 @@ typedef struct _NOR_FLASH_INSTANCE                NOR_FLASH_INSTANCE;
 #pragma pack(1)
 typedef struct {
   VENDOR_DEVICE_PATH                  Vendor;
+  UINT8                               Index;
   EFI_DEVICE_PATH_PROTOCOL            End;
 } NOR_FLASH_DEVICE_PATH;
 #pragma pack()
-- 
2.17.1

[PATCH v2 3/5] ArmVirtPkg/FdtClientDxe: take DT node 'status' properties into account

[Ard Biesheuvel]

DT has a [pseudo-]standardized 'status' property that can be set on
any node, and which signifies that a node should be treated as
absent unless it is set to 'ok' or 'okay'. So take this into account
when iterating over nodes.

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

diff --git a/ArmVirtPkg/FdtClientDxe/FdtClientDxe.c b/ArmVirtPkg/FdtClientDxe/FdtClientDxe.c
index fb6e0aeb9215..5bfde381ecd0 100644
--- a/ArmVirtPkg/FdtClientDxe/FdtClientDxe.c
+++ b/ArmVirtPkg/FdtClientDxe/FdtClientDxe.c
@@ -78,6 +78,33 @@ SetNodeProperty (
   return EFI_SUCCESS;
 }
 
+STATIC
+BOOLEAN
+IsNodeEnabled (
+  INT32                       Node
+  )
+{
+  CONST CHAR8   *NodeStatus;
+  INT32         Len;
+
+  //
+  // A missing status property implies 'ok' so ignore any errors that
+  // may occur here. If the status property is present, check whether
+  // it is set to 'ok' or 'okay', anything else is treated as 'disabled'.
+  //
+  NodeStatus = fdt_getprop (mDeviceTreeBase, Node, "status", &Len);
+  if (NodeStatus == NULL) {
+    return TRUE;
+  }
+  if (Len >= 5 && AsciiStrCmp (NodeStatus, "okay") == 0) {
+    return TRUE;
+  }
+  if (Len >= 3 && AsciiStrCmp (NodeStatus, "ok") == 0) {
+    return TRUE;
+  }
+  return FALSE;
+}
+
 STATIC
 EFI_STATUS
 EFIAPI
@@ -101,6 +128,10 @@ FindNextCompatibleNode (
       break;
     }
 
+    if (!IsNodeEnabled (Next)) {
+      continue;
+    }
+
     Type = fdt_getprop (mDeviceTreeBase, Next, "compatible", &Len);
     if (Type == NULL) {
       continue;
@@ -210,7 +241,6 @@ FindNextMemoryNodeReg (
 {
   INT32          Prev, Next;
   CONST CHAR8    *DeviceType;
-  CONST CHAR8    *NodeStatus;
   INT32          Len;
   EFI_STATUS     Status;
 
@@ -223,10 +253,8 @@ 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));
+    if (!IsNodeEnabled (Next)) {
+      DEBUG ((DEBUG_WARN, "%a: ignoring disabled memory node\n", __FUNCTION__));
       continue;
     }
 
-- 
2.17.1

[PATCH v2 4/5] ArmVirtPkg/NorFlashQemuLib: discover NOR flash banks dynamically

[Ard Biesheuvel]

NorFlashQemuLib is one of the last remaining drivers in ArmVirtPkg
that are not based on the device tree received from QEMU.

For ArmVirtQemu, this does not really matter, given that the NOR
flash banks are always the same: the PEI code is linked to execute
in place from flash bank #0, and the fixed varstore PCDs refer to
flash bank #1 directly.

However, ArmVirtQemuKernel can execute at any offset, permitting it
to be used as an intermediary loader when running QEMU with secure
world emulation enabled, in which case NOR flash bank #0 is secure
only and contains the secure world firmware. In this case,
NorFlashQemuLib should not expose the first flash bank at all.

To prevent introducing too much internal knowledge about which flash
bank is accessible under which circumstances, let's switch to using
the DTB to decide which flash banks to expose to the NOR flash driver.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 ArmVirtPkg/Library/NorFlashQemuLib/NorFlashQemuLib.c   | 78 ++++++++++++++------
 ArmVirtPkg/Library/NorFlashQemuLib/NorFlashQemuLib.inf | 12 +++
 2 files changed, 69 insertions(+), 21 deletions(-)

diff --git a/ArmVirtPkg/Library/NorFlashQemuLib/NorFlashQemuLib.c b/ArmVirtPkg/Library/NorFlashQemuLib/NorFlashQemuLib.c
index e3bbae5b06c5..2678f57eaaad 100644
--- a/ArmVirtPkg/Library/NorFlashQemuLib/NorFlashQemuLib.c
+++ b/ArmVirtPkg/Library/NorFlashQemuLib/NorFlashQemuLib.c
@@ -1,6 +1,6 @@
 /** @file
 
- Copyright (c) 2014, Linaro Ltd. All rights reserved.<BR>
+ Copyright (c) 2014-2018, Linaro Ltd. All rights reserved.<BR>
 
  This program and the accompanying materials
  are licensed and made available under the terms and conditions of the BSD License
@@ -12,13 +12,16 @@
 
  **/
 
+#include <Library/BaseLib.h>
+#include <Library/DebugLib.h>
 #include <Library/NorFlashPlatformLib.h>
+#include <Library/UefiBootServicesTableLib.h>
+
+#include <Protocol/FdtClient.h>
 
 #define QEMU_NOR_BLOCK_SIZE    SIZE_256KB
-#define QEMU_NOR0_BASE         0x0
-#define QEMU_NOR0_SIZE         SIZE_64MB
-#define QEMU_NOR1_BASE         0x04000000
-#define QEMU_NOR1_SIZE         SIZE_64MB
+
+#define MAX_FLASH_BANKS        4
 
 EFI_STATUS
 NorFlashPlatformInitialization (
@@ -28,21 +31,7 @@ NorFlashPlatformInitialization (
   return EFI_SUCCESS;
 }
 
-NOR_FLASH_DESCRIPTION mNorFlashDevices[] = {
-  {
-    QEMU_NOR0_BASE,
-    QEMU_NOR0_BASE,
-    QEMU_NOR0_SIZE,
-    QEMU_NOR_BLOCK_SIZE,
-    {0xF9B94AE2, 0x8BA6, 0x409B, {0x9D, 0x56, 0xB9, 0xB4, 0x17, 0xF5, 0x3C, 0xB3}}
-  }, {
-    QEMU_NOR1_BASE,
-    QEMU_NOR1_BASE,
-    QEMU_NOR1_SIZE,
-    QEMU_NOR_BLOCK_SIZE,
-    {0x8047DB4B, 0x7E9C, 0x4C0C, {0x8E, 0xBC, 0xDF, 0xBB, 0xAA, 0xCA, 0xCE, 0x8F}}
-  }
-};
+NOR_FLASH_DESCRIPTION mNorFlashDevices[MAX_FLASH_BANKS];
 
 EFI_STATUS
 NorFlashPlatformGetDevices (
@@ -50,7 +39,54 @@ NorFlashPlatformGetDevices (
   OUT UINT32                  *Count
   )
 {
+  FDT_CLIENT_PROTOCOL         *FdtClient;
+  INT32                       Node;
+  EFI_STATUS                  Status;
+  EFI_STATUS                  FindNodeStatus;
+  CONST UINT32                *Reg;
+  UINT32                      PropSize;
+  UINT32                      Num;
+  UINT64                      Base;
+  UINT64                      Size;
+
+  Status = gBS->LocateProtocol (&gFdtClientProtocolGuid, NULL,
+                  (VOID **)&FdtClient);
+  ASSERT_EFI_ERROR (Status);
+
+  Num = 0;
+  for (FindNodeStatus = FdtClient->FindCompatibleNode (FdtClient,
+                                     "cfi-flash", &Node);
+       !EFI_ERROR (FindNodeStatus) && Num < MAX_FLASH_BANKS;
+       FindNodeStatus = FdtClient->FindNextCompatibleNode (FdtClient,
+                                     "cfi-flash", Node, &Node)) {
+
+    Status = FdtClient->GetNodeProperty (FdtClient, Node, "reg",
+                          (CONST VOID **)&Reg, &PropSize);
+    if (EFI_ERROR (Status)) {
+      DEBUG ((DEBUG_ERROR, "%a: GetNodeProperty () failed (Status == %r)\n",
+        __FUNCTION__, Status));
+      continue;
+    }
+
+    ASSERT ((PropSize % (4 * sizeof (UINT32))) == 0);
+
+    while (PropSize >= (4 * sizeof (UINT32)) && Num < MAX_FLASH_BANKS) {
+      Base = SwapBytes64 (ReadUnaligned64 ((VOID *)&Reg[0]));
+      Size = SwapBytes64 (ReadUnaligned64 ((VOID *)&Reg[2]));
+      Reg += 4;
+
+      mNorFlashDevices[Num].DeviceBaseAddress = (UINTN)Base;
+      mNorFlashDevices[Num].RegionBaseAddress = (UINTN)Base;
+      mNorFlashDevices[Num].Size              = (UINTN)Size;
+      mNorFlashDevices[Num].BlockSize         = QEMU_NOR_BLOCK_SIZE;
+      Num++;
+
+      PropSize -= 4 * sizeof (UINT32);
+    }
+  }
+
   *NorFlashDescriptions = mNorFlashDevices;
-  *Count = ARRAY_SIZE (mNorFlashDevices);
+  *Count = Num;
+
   return EFI_SUCCESS;
 }
diff --git a/ArmVirtPkg/Library/NorFlashQemuLib/NorFlashQemuLib.inf b/ArmVirtPkg/Library/NorFlashQemuLib/NorFlashQemuLib.inf
index 126d1671f544..d86ff36dbd58 100644
--- a/ArmVirtPkg/Library/NorFlashQemuLib/NorFlashQemuLib.inf
+++ b/ArmVirtPkg/Library/NorFlashQemuLib/NorFlashQemuLib.inf
@@ -28,3 +28,15 @@
 [Packages]
   MdePkg/MdePkg.dec
   ArmPlatformPkg/ArmPlatformPkg.dec
+  ArmVirtPkg/ArmVirtPkg.dec
+
+[LibraryClasses]
+  BaseLib
+  DebugLib
+  UefiBootServicesTableLib
+
+[Protocols]
+  gFdtClientProtocolGuid          ## CONSUMES
+
+[Depex]
+  gFdtClientProtocolGuid
-- 
2.17.1

[PATCH v2 5/5] ArmPlatformPkg/NorFlashPlatformLib: remove unused Guid member from struct

[Ard Biesheuvel]

We no longer use per-instance GUIDs to identify NOR flash banks so
there is no longer a need to define them. Drop the Guid member from
the NOR_FLASH_DESCRIPTION type.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
This patch should not be merged before users in edk2-platforms have been
updated.

 ArmPlatformPkg/Include/Library/NorFlashPlatformLib.h | 1 -
 1 file changed, 1 deletion(-)

diff --git a/ArmPlatformPkg/Include/Library/NorFlashPlatformLib.h b/ArmPlatformPkg/Include/Library/NorFlashPlatformLib.h
index e9e1c060787d..e99a217c3a51 100644
--- a/ArmPlatformPkg/Include/Library/NorFlashPlatformLib.h
+++ b/ArmPlatformPkg/Include/Library/NorFlashPlatformLib.h
@@ -20,7 +20,6 @@ typedef struct {
   UINTN       RegionBaseAddress;    // Start address of one single region
   UINTN       Size;
   UINTN       BlockSize;
-  EFI_GUID    Guid;
 } NOR_FLASH_DESCRIPTION;
 
 EFI_STATUS
-- 
2.17.1

Re: [PATCH v2 1/5] ArmPlatformPkg/NorFlashDxe: prepare for devicepath format change

[Laszlo Ersek]

On 11/21/18 12:58, Ard Biesheuvel wrote:
> A subsequent patch will change the layout of devicepath nodes
> produced by this driver. In preparation, make some tweaks to
> the code to use a packed struct for the devicepath and to pass
> the device index to NorFlashCreateInstance(). These are cosmetic
> changes only, the resulting binaries should be identical.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> ---
>  ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.c | 9 ++++++---
>  ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.h | 2 ++
>  2 files changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.c b/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.c
> index 46e815beb343..53753a4721ac 100644
> --- a/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.c
> +++ b/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.c
> @@ -82,7 +82,10 @@ NOR_FLASH_INSTANCE  mNorFlashInstanceTemplate = {
>        {
>          HARDWARE_DEVICE_PATH,
>          HW_VENDOR_DP,
> -        { (UINT8)sizeof(VENDOR_DEVICE_PATH), (UINT8)((sizeof(VENDOR_DEVICE_PATH)) >> 8) }
> +        {
> +          (UINT8)(OFFSET_OF (NOR_FLASH_DEVICE_PATH, End)),
> +          (UINT8)(OFFSET_OF (NOR_FLASH_DEVICE_PATH, End) >> 8)
> +        }
>        },
>        { 0x0, 0x0, 0x0, { 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0 } }, // GUID ... NEED TO BE FILLED
>      },
> @@ -99,7 +102,7 @@ NorFlashCreateInstance (
>    IN UINTN                  NorFlashDeviceBase,
>    IN UINTN                  NorFlashRegionBase,
>    IN UINTN                  NorFlashSize,
> -  IN UINT32                 MediaId,
> +  IN UINT32                 Index,
>    IN UINT32                 BlockSize,
>    IN BOOLEAN                SupportFvb,
>    IN CONST GUID             *NorFlashGuid,
> @@ -121,7 +124,7 @@ NorFlashCreateInstance (
>    Instance->Size = NorFlashSize;
>  
>    Instance->BlockIoProtocol.Media = &Instance->Media;
> -  Instance->Media.MediaId = MediaId;
> +  Instance->Media.MediaId = Index;
>    Instance->Media.BlockSize = BlockSize;
>    Instance->Media.LastBlock = (NorFlashSize / BlockSize)-1;
>  
> diff --git a/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.h b/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.h
> index 5c07694fbfaa..910681fd4412 100644
> --- a/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.h
> +++ b/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.h
> @@ -122,10 +122,12 @@
>  
>  typedef struct _NOR_FLASH_INSTANCE                NOR_FLASH_INSTANCE;
>  
> +#pragma pack(1)
>  typedef struct {
>    VENDOR_DEVICE_PATH                  Vendor;
>    EFI_DEVICE_PATH_PROTOCOL            End;
>  } NOR_FLASH_DEVICE_PATH;
> +#pragma pack()
>  
>  struct _NOR_FLASH_INSTANCE {
>    UINT32                              Signature;
> 

We tend to insert a space character between "pack" and "(1)"/"()" too.

Feel free to update the patch when you push it, or not at all. My R-b
stands either way.

Thanks!
Laszlo

Re: [PATCH v2 2/5] ArmPlatformPkg/NorFlashDxe: use one GUID plus index to identify flash banks

[Laszlo Ersek]

On 11/21/18 12:58, Ard Biesheuvel wrote:
> Currently, each flash bank controlled by ArmPlatformPkg/NorFlashDxe
> has its own VendorHw GUID, and instances of NorFlashPlatformLib
> describe each bank to the driver, along with the GUID for each.
> 
> This works ok for bare metal platforms, but it would be useful for
> virtual platforms if we could obtain this information from a
> device tree, which would require us to invent GUIDs on the fly,
> given that the 'cfi-flash' binding does not include a GUID.
> 
> So instead, let's switch to a single GUID for all flash banks,
> and update the driver's device path handling to include an index
> to identify each bank uniquely.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>
> ---
>  ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.c | 6 +++---
>  ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.h | 1 +
>  2 files changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.c b/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.c
> index 53753a4721ac..af40a4c88412 100644
> --- a/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.c
> +++ b/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.c
> @@ -89,6 +89,7 @@ NOR_FLASH_INSTANCE  mNorFlashInstanceTemplate = {
>        },
>        { 0x0, 0x0, 0x0, { 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0 } }, // GUID ... NEED TO BE FILLED
>      },
> +    0, // Index
>      {
>        END_DEVICE_PATH_TYPE,
>        END_ENTIRE_DEVICE_PATH_SUBTYPE,
> @@ -105,7 +106,6 @@ NorFlashCreateInstance (
>    IN UINT32                 Index,
>    IN UINT32                 BlockSize,
>    IN BOOLEAN                SupportFvb,
> -  IN CONST GUID             *NorFlashGuid,
>    OUT NOR_FLASH_INSTANCE**  NorFlashInstance
>    )
>  {
> @@ -128,7 +128,8 @@ NorFlashCreateInstance (
>    Instance->Media.BlockSize = BlockSize;
>    Instance->Media.LastBlock = (NorFlashSize / BlockSize)-1;
>  
> -  CopyGuid (&Instance->DevicePath.Vendor.Guid, NorFlashGuid);
> +  CopyGuid (&Instance->DevicePath.Vendor.Guid, &gEfiCallerIdGuid);
> +  Instance->DevicePath.Index = (UINT8)Index;
>  
>    Instance->ShadowBuffer = AllocateRuntimePool (BlockSize);;
>    if (Instance->ShadowBuffer == NULL) {
> @@ -1314,7 +1315,6 @@ NorFlashInitialise (
>        Index,
>        NorFlashDevices[Index].BlockSize,
>        ContainVariableStorage,
> -      &NorFlashDevices[Index].Guid,
>        &mNorFlashInstances[Index]
>      );
>      if (EFI_ERROR(Status)) {
> diff --git a/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.h b/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.h
> index 910681fd4412..8886aa43d9f3 100644
> --- a/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.h
> +++ b/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.h
> @@ -125,6 +125,7 @@ typedef struct _NOR_FLASH_INSTANCE                NOR_FLASH_INSTANCE;
>  #pragma pack(1)
>  typedef struct {
>    VENDOR_DEVICE_PATH                  Vendor;
> +  UINT8                               Index;
>    EFI_DEVICE_PATH_PROTOCOL            End;
>  } NOR_FLASH_DEVICE_PATH;
>  #pragma pack()
> 

looks good, thanks!

Re: [PATCH v2 3/5] ArmVirtPkg/FdtClientDxe: take DT node 'status' properties into account

[Laszlo Ersek]

On 11/21/18 12:58, Ard Biesheuvel wrote:
> DT has a [pseudo-]standardized 'status' property that can be set on
> any node, and which signifies that a node should be treated as
> absent unless it is set to 'ok' or 'okay'.

(I now really want "oktopus" to be [pseudo-]standardized too! ;) /jk)

> So take this into account
> when iterating over nodes.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  ArmVirtPkg/FdtClientDxe/FdtClientDxe.c | 38 +++++++++++++++++---
>  1 file changed, 33 insertions(+), 5 deletions(-)
> 
> diff --git a/ArmVirtPkg/FdtClientDxe/FdtClientDxe.c b/ArmVirtPkg/FdtClientDxe/FdtClientDxe.c
> index fb6e0aeb9215..5bfde381ecd0 100644
> --- a/ArmVirtPkg/FdtClientDxe/FdtClientDxe.c
> +++ b/ArmVirtPkg/FdtClientDxe/FdtClientDxe.c
> @@ -78,6 +78,33 @@ SetNodeProperty (
>    return EFI_SUCCESS;
>  }
>  
> +STATIC
> +BOOLEAN
> +IsNodeEnabled (
> +  INT32                       Node
> +  )
> +{
> +  CONST CHAR8   *NodeStatus;
> +  INT32         Len;
> +
> +  //
> +  // A missing status property implies 'ok' so ignore any errors that
> +  // may occur here. If the status property is present, check whether
> +  // it is set to 'ok' or 'okay', anything else is treated as 'disabled'.
> +  //
> +  NodeStatus = fdt_getprop (mDeviceTreeBase, Node, "status", &Len);
> +  if (NodeStatus == NULL) {
> +    return TRUE;
> +  }
> +  if (Len >= 5 && AsciiStrCmp (NodeStatus, "okay") == 0) {
> +    return TRUE;
> +  }
> +  if (Len >= 3 && AsciiStrCmp (NodeStatus, "ok") == 0) {
> +    return TRUE;
> +  }
> +  return FALSE;
> +}
> +
>  STATIC
>  EFI_STATUS
>  EFIAPI
> @@ -101,6 +128,10 @@ FindNextCompatibleNode (
>        break;
>      }
>  
> +    if (!IsNodeEnabled (Next)) {
> +      continue;
> +    }
> +
>      Type = fdt_getprop (mDeviceTreeBase, Next, "compatible", &Len);
>      if (Type == NULL) {
>        continue;
> @@ -210,7 +241,6 @@ FindNextMemoryNodeReg (
>  {
>    INT32          Prev, Next;
>    CONST CHAR8    *DeviceType;
> -  CONST CHAR8    *NodeStatus;
>    INT32          Len;
>    EFI_STATUS     Status;
>  
> @@ -223,10 +253,8 @@ 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));
> +    if (!IsNodeEnabled (Next)) {
> +      DEBUG ((DEBUG_WARN, "%a: ignoring disabled memory node\n", __FUNCTION__));
>        continue;
>      }
>  
> 

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

Thanks!
Laszlo

Re: [PATCH v2 4/5] ArmVirtPkg/NorFlashQemuLib: discover NOR flash banks dynamically

[Laszlo Ersek]

On 11/21/18 12:58, Ard Biesheuvel wrote:
> NorFlashQemuLib is one of the last remaining drivers in ArmVirtPkg
> that are not based on the device tree received from QEMU.
> 
> For ArmVirtQemu, this does not really matter, given that the NOR
> flash banks are always the same: the PEI code is linked to execute
> in place from flash bank #0, and the fixed varstore PCDs refer to
> flash bank #1 directly.
> 
> However, ArmVirtQemuKernel can execute at any offset, permitting it
> to be used as an intermediary loader when running QEMU with secure
> world emulation enabled, in which case NOR flash bank #0 is secure
> only and contains the secure world firmware. In this case,
> NorFlashQemuLib should not expose the first flash bank at all.
> 
> To prevent introducing too much internal knowledge about which flash
> bank is accessible under which circumstances, let's switch to using
> the DTB to decide which flash banks to expose to the NOR flash driver.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>  ArmVirtPkg/Library/NorFlashQemuLib/NorFlashQemuLib.c   | 78 ++++++++++++++------
>  ArmVirtPkg/Library/NorFlashQemuLib/NorFlashQemuLib.inf | 12 +++
>  2 files changed, 69 insertions(+), 21 deletions(-)
> 
> diff --git a/ArmVirtPkg/Library/NorFlashQemuLib/NorFlashQemuLib.c b/ArmVirtPkg/Library/NorFlashQemuLib/NorFlashQemuLib.c
> index e3bbae5b06c5..2678f57eaaad 100644
> --- a/ArmVirtPkg/Library/NorFlashQemuLib/NorFlashQemuLib.c
> +++ b/ArmVirtPkg/Library/NorFlashQemuLib/NorFlashQemuLib.c
> @@ -1,6 +1,6 @@
>  /** @file
>  
> - Copyright (c) 2014, Linaro Ltd. All rights reserved.<BR>
> + Copyright (c) 2014-2018, Linaro Ltd. All rights reserved.<BR>
>  
>   This program and the accompanying materials
>   are licensed and made available under the terms and conditions of the BSD License
> @@ -12,13 +12,16 @@
>  
>   **/
>  
> +#include <Library/BaseLib.h>
> +#include <Library/DebugLib.h>
>  #include <Library/NorFlashPlatformLib.h>
> +#include <Library/UefiBootServicesTableLib.h>
> +
> +#include <Protocol/FdtClient.h>
>  
>  #define QEMU_NOR_BLOCK_SIZE    SIZE_256KB
> -#define QEMU_NOR0_BASE         0x0
> -#define QEMU_NOR0_SIZE         SIZE_64MB
> -#define QEMU_NOR1_BASE         0x04000000
> -#define QEMU_NOR1_SIZE         SIZE_64MB
> +
> +#define MAX_FLASH_BANKS        4
>  
>  EFI_STATUS
>  NorFlashPlatformInitialization (
> @@ -28,21 +31,7 @@ NorFlashPlatformInitialization (
>    return EFI_SUCCESS;
>  }
>  
> -NOR_FLASH_DESCRIPTION mNorFlashDevices[] = {
> -  {
> -    QEMU_NOR0_BASE,
> -    QEMU_NOR0_BASE,
> -    QEMU_NOR0_SIZE,
> -    QEMU_NOR_BLOCK_SIZE,
> -    {0xF9B94AE2, 0x8BA6, 0x409B, {0x9D, 0x56, 0xB9, 0xB4, 0x17, 0xF5, 0x3C, 0xB3}}
> -  }, {
> -    QEMU_NOR1_BASE,
> -    QEMU_NOR1_BASE,
> -    QEMU_NOR1_SIZE,
> -    QEMU_NOR_BLOCK_SIZE,
> -    {0x8047DB4B, 0x7E9C, 0x4C0C, {0x8E, 0xBC, 0xDF, 0xBB, 0xAA, 0xCA, 0xCE, 0x8F}}
> -  }
> -};
> +NOR_FLASH_DESCRIPTION mNorFlashDevices[MAX_FLASH_BANKS];
>  
>  EFI_STATUS
>  NorFlashPlatformGetDevices (
> @@ -50,7 +39,54 @@ NorFlashPlatformGetDevices (
>    OUT UINT32                  *Count
>    )
>  {
> +  FDT_CLIENT_PROTOCOL         *FdtClient;
> +  INT32                       Node;
> +  EFI_STATUS                  Status;
> +  EFI_STATUS                  FindNodeStatus;
> +  CONST UINT32                *Reg;
> +  UINT32                      PropSize;
> +  UINT32                      Num;
> +  UINT64                      Base;
> +  UINT64                      Size;
> +
> +  Status = gBS->LocateProtocol (&gFdtClientProtocolGuid, NULL,
> +                  (VOID **)&FdtClient);
> +  ASSERT_EFI_ERROR (Status);
> +
> +  Num = 0;
> +  for (FindNodeStatus = FdtClient->FindCompatibleNode (FdtClient,
> +                                     "cfi-flash", &Node);
> +       !EFI_ERROR (FindNodeStatus) && Num < MAX_FLASH_BANKS;
> +       FindNodeStatus = FdtClient->FindNextCompatibleNode (FdtClient,
> +                                     "cfi-flash", Node, &Node)) {
> +
> +    Status = FdtClient->GetNodeProperty (FdtClient, Node, "reg",
> +                          (CONST VOID **)&Reg, &PropSize);
> +    if (EFI_ERROR (Status)) {
> +      DEBUG ((DEBUG_ERROR, "%a: GetNodeProperty () failed (Status == %r)\n",
> +        __FUNCTION__, Status));
> +      continue;
> +    }
> +
> +    ASSERT ((PropSize % (4 * sizeof (UINT32))) == 0);
> +
> +    while (PropSize >= (4 * sizeof (UINT32)) && Num < MAX_FLASH_BANKS) {
> +      Base = SwapBytes64 (ReadUnaligned64 ((VOID *)&Reg[0]));
> +      Size = SwapBytes64 (ReadUnaligned64 ((VOID *)&Reg[2]));
> +      Reg += 4;
> +
> +      mNorFlashDevices[Num].DeviceBaseAddress = (UINTN)Base;
> +      mNorFlashDevices[Num].RegionBaseAddress = (UINTN)Base;
> +      mNorFlashDevices[Num].Size              = (UINTN)Size;
> +      mNorFlashDevices[Num].BlockSize         = QEMU_NOR_BLOCK_SIZE;
> +      Num++;
> +
> +      PropSize -= 4 * sizeof (UINT32);
> +    }
> +  }
> +
>    *NorFlashDescriptions = mNorFlashDevices;
> -  *Count = ARRAY_SIZE (mNorFlashDevices);
> +  *Count = Num;
> +
>    return EFI_SUCCESS;
>  }
> diff --git a/ArmVirtPkg/Library/NorFlashQemuLib/NorFlashQemuLib.inf b/ArmVirtPkg/Library/NorFlashQemuLib/NorFlashQemuLib.inf
> index 126d1671f544..d86ff36dbd58 100644
> --- a/ArmVirtPkg/Library/NorFlashQemuLib/NorFlashQemuLib.inf
> +++ b/ArmVirtPkg/Library/NorFlashQemuLib/NorFlashQemuLib.inf
> @@ -28,3 +28,15 @@
>  [Packages]
>    MdePkg/MdePkg.dec
>    ArmPlatformPkg/ArmPlatformPkg.dec
> +  ArmVirtPkg/ArmVirtPkg.dec
> +
> +[LibraryClasses]
> +  BaseLib
> +  DebugLib
> +  UefiBootServicesTableLib
> +
> +[Protocols]
> +  gFdtClientProtocolGuid          ## CONSUMES
> +
> +[Depex]
> +  gFdtClientProtocolGuid
> 

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

Re: [PATCH v2 5/5] ArmPlatformPkg/NorFlashPlatformLib: remove unused Guid member from struct

[Laszlo Ersek]

On 11/21/18 12:58, Ard Biesheuvel wrote:
> We no longer use per-instance GUIDs to identify NOR flash banks so
> there is no longer a need to define them. Drop the Guid member from
> the NOR_FLASH_DESCRIPTION type.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
> This patch should not be merged before users in edk2-platforms have been
> updated.
> 
>  ArmPlatformPkg/Include/Library/NorFlashPlatformLib.h | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/ArmPlatformPkg/Include/Library/NorFlashPlatformLib.h b/ArmPlatformPkg/Include/Library/NorFlashPlatformLib.h
> index e9e1c060787d..e99a217c3a51 100644
> --- a/ArmPlatformPkg/Include/Library/NorFlashPlatformLib.h
> +++ b/ArmPlatformPkg/Include/Library/NorFlashPlatformLib.h
> @@ -20,7 +20,6 @@ typedef struct {
>    UINTN       RegionBaseAddress;    // Start address of one single region
>    UINTN       Size;
>    UINTN       BlockSize;
> -  EFI_GUID    Guid;
>  } NOR_FLASH_DESCRIPTION;
>  
>  EFI_STATUS
> 

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

Re: [PATCH v2 3/5] ArmVirtPkg/FdtClientDxe: take DT node 'status' properties into account

[Ard Biesheuvel]

On Wed, 21 Nov 2018 at 18:12, Laszlo Ersek <lersek@redhat.com> wrote:
>
> On 11/21/18 12:58, Ard Biesheuvel wrote:
> > DT has a [pseudo-]standardized 'status' property that can be set on
> > any node, and which signifies that a node should be treated as
> > absent unless it is set to 'ok' or 'okay'.
>
> (I now really want "oktopus" to be [pseudo-]standardized too! ;) /jk)
>

Be careful what you wish for :-)

> > So take this into account
> > when iterating over nodes.
> >
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > ---
> >  ArmVirtPkg/FdtClientDxe/FdtClientDxe.c | 38 +++++++++++++++++---
> >  1 file changed, 33 insertions(+), 5 deletions(-)
> >
> > diff --git a/ArmVirtPkg/FdtClientDxe/FdtClientDxe.c b/ArmVirtPkg/FdtClientDxe/FdtClientDxe.c
> > index fb6e0aeb9215..5bfde381ecd0 100644
> > --- a/ArmVirtPkg/FdtClientDxe/FdtClientDxe.c
> > +++ b/ArmVirtPkg/FdtClientDxe/FdtClientDxe.c
> > @@ -78,6 +78,33 @@ SetNodeProperty (
> >    return EFI_SUCCESS;
> >  }
> >
> > +STATIC
> > +BOOLEAN
> > +IsNodeEnabled (
> > +  INT32                       Node
> > +  )
> > +{
> > +  CONST CHAR8   *NodeStatus;
> > +  INT32         Len;
> > +
> > +  //
> > +  // A missing status property implies 'ok' so ignore any errors that
> > +  // may occur here. If the status property is present, check whether
> > +  // it is set to 'ok' or 'okay', anything else is treated as 'disabled'.
> > +  //
> > +  NodeStatus = fdt_getprop (mDeviceTreeBase, Node, "status", &Len);
> > +  if (NodeStatus == NULL) {
> > +    return TRUE;
> > +  }
> > +  if (Len >= 5 && AsciiStrCmp (NodeStatus, "okay") == 0) {
> > +    return TRUE;
> > +  }
> > +  if (Len >= 3 && AsciiStrCmp (NodeStatus, "ok") == 0) {
> > +    return TRUE;
> > +  }
> > +  return FALSE;
> > +}
> > +
> >  STATIC
> >  EFI_STATUS
> >  EFIAPI
> > @@ -101,6 +128,10 @@ FindNextCompatibleNode (
> >        break;
> >      }
> >
> > +    if (!IsNodeEnabled (Next)) {
> > +      continue;
> > +    }
> > +
> >      Type = fdt_getprop (mDeviceTreeBase, Next, "compatible", &Len);
> >      if (Type == NULL) {
> >        continue;
> > @@ -210,7 +241,6 @@ FindNextMemoryNodeReg (
> >  {
> >    INT32          Prev, Next;
> >    CONST CHAR8    *DeviceType;
> > -  CONST CHAR8    *NodeStatus;
> >    INT32          Len;
> >    EFI_STATUS     Status;
> >
> > @@ -223,10 +253,8 @@ 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));
> > +    if (!IsNodeEnabled (Next)) {
> > +      DEBUG ((DEBUG_WARN, "%a: ignoring disabled memory node\n", __FUNCTION__));
> >        continue;
> >      }
> >
> >
>
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
>

Thanks.

Re: [PATCH v2 2/5] ArmPlatformPkg/NorFlashDxe: use one GUID plus index to identify flash banks

[Philippe Mathieu-Daudé]

On 21/11/18 12:58, Ard Biesheuvel wrote:
> Currently, each flash bank controlled by ArmPlatformPkg/NorFlashDxe
> has its own VendorHw GUID, and instances of NorFlashPlatformLib
> describe each bank to the driver, along with the GUID for each.
> 
> This works ok for bare metal platforms, but it would be useful for
> virtual platforms if we could obtain this information from a
> device tree, which would require us to invent GUIDs on the fly,
> given that the 'cfi-flash' binding does not include a GUID.
> 
> So instead, let's switch to a single GUID for all flash banks,
> and update the driver's device path handling to include an index
> to identify each bank uniquely.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

> ---
>  ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.c | 6 +++---
>  ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.h | 1 +
>  2 files changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.c b/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.c
> index 53753a4721ac..af40a4c88412 100644
> --- a/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.c
> +++ b/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.c
> @@ -89,6 +89,7 @@ NOR_FLASH_INSTANCE  mNorFlashInstanceTemplate = {
>        },
>        { 0x0, 0x0, 0x0, { 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0 } }, // GUID ... NEED TO BE FILLED
>      },
> +    0, // Index
>      {
>        END_DEVICE_PATH_TYPE,
>        END_ENTIRE_DEVICE_PATH_SUBTYPE,
> @@ -105,7 +106,6 @@ NorFlashCreateInstance (
>    IN UINT32                 Index,
>    IN UINT32                 BlockSize,
>    IN BOOLEAN                SupportFvb,
> -  IN CONST GUID             *NorFlashGuid,
>    OUT NOR_FLASH_INSTANCE**  NorFlashInstance
>    )
>  {
> @@ -128,7 +128,8 @@ NorFlashCreateInstance (
>    Instance->Media.BlockSize = BlockSize;
>    Instance->Media.LastBlock = (NorFlashSize / BlockSize)-1;
>  
> -  CopyGuid (&Instance->DevicePath.Vendor.Guid, NorFlashGuid);
> +  CopyGuid (&Instance->DevicePath.Vendor.Guid, &gEfiCallerIdGuid);
> +  Instance->DevicePath.Index = (UINT8)Index;
>  
>    Instance->ShadowBuffer = AllocateRuntimePool (BlockSize);;
>    if (Instance->ShadowBuffer == NULL) {
> @@ -1314,7 +1315,6 @@ NorFlashInitialise (
>        Index,
>        NorFlashDevices[Index].BlockSize,
>        ContainVariableStorage,
> -      &NorFlashDevices[Index].Guid,
>        &mNorFlashInstances[Index]
>      );
>      if (EFI_ERROR(Status)) {
> diff --git a/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.h b/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.h
> index 910681fd4412..8886aa43d9f3 100644
> --- a/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.h
> +++ b/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.h
> @@ -125,6 +125,7 @@ typedef struct _NOR_FLASH_INSTANCE                NOR_FLASH_INSTANCE;
>  #pragma pack(1)
>  typedef struct {
>    VENDOR_DEVICE_PATH                  Vendor;
> +  UINT8                               Index;
>    EFI_DEVICE_PATH_PROTOCOL            End;
>  } NOR_FLASH_DEVICE_PATH;
>  #pragma pack()
> 

Re: [PATCH v2 3/5] ArmVirtPkg/FdtClientDxe: take DT node 'status' properties into account

[Philippe Mathieu-Daudé]

On 21/11/18 12:58, Ard Biesheuvel wrote:
> DT has a [pseudo-]standardized 'status' property that can be set on
> any node, and which signifies that a node should be treated as
> absent unless it is set to 'ok' or 'okay'. So take this into account
> when iterating over nodes.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

> ---
>  ArmVirtPkg/FdtClientDxe/FdtClientDxe.c | 38 +++++++++++++++++---
>  1 file changed, 33 insertions(+), 5 deletions(-)
> 
> diff --git a/ArmVirtPkg/FdtClientDxe/FdtClientDxe.c b/ArmVirtPkg/FdtClientDxe/FdtClientDxe.c
> index fb6e0aeb9215..5bfde381ecd0 100644
> --- a/ArmVirtPkg/FdtClientDxe/FdtClientDxe.c
> +++ b/ArmVirtPkg/FdtClientDxe/FdtClientDxe.c
> @@ -78,6 +78,33 @@ SetNodeProperty (
>    return EFI_SUCCESS;
>  }
>  
> +STATIC
> +BOOLEAN
> +IsNodeEnabled (
> +  INT32                       Node
> +  )
> +{
> +  CONST CHAR8   *NodeStatus;
> +  INT32         Len;
> +
> +  //
> +  // A missing status property implies 'ok' so ignore any errors that
> +  // may occur here. If the status property is present, check whether
> +  // it is set to 'ok' or 'okay', anything else is treated as 'disabled'.
> +  //
> +  NodeStatus = fdt_getprop (mDeviceTreeBase, Node, "status", &Len);
> +  if (NodeStatus == NULL) {
> +    return TRUE;
> +  }
> +  if (Len >= 5 && AsciiStrCmp (NodeStatus, "okay") == 0) {
> +    return TRUE;
> +  }
> +  if (Len >= 3 && AsciiStrCmp (NodeStatus, "ok") == 0) {
> +    return TRUE;
> +  }
> +  return FALSE;
> +}
> +
>  STATIC
>  EFI_STATUS
>  EFIAPI
> @@ -101,6 +128,10 @@ FindNextCompatibleNode (
>        break;
>      }
>  
> +    if (!IsNodeEnabled (Next)) {
> +      continue;
> +    }
> +
>      Type = fdt_getprop (mDeviceTreeBase, Next, "compatible", &Len);
>      if (Type == NULL) {
>        continue;
> @@ -210,7 +241,6 @@ FindNextMemoryNodeReg (
>  {
>    INT32          Prev, Next;
>    CONST CHAR8    *DeviceType;
> -  CONST CHAR8    *NodeStatus;
>    INT32          Len;
>    EFI_STATUS     Status;
>  
> @@ -223,10 +253,8 @@ 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));
> +    if (!IsNodeEnabled (Next)) {
> +      DEBUG ((DEBUG_WARN, "%a: ignoring disabled memory node\n", __FUNCTION__));
>        continue;
>      }
>  
> 

Re: [PATCH v2 1/5] ArmPlatformPkg/NorFlashDxe: prepare for devicepath format change

[Philippe Mathieu-Daudé]

On 21/11/18 12:58, Ard Biesheuvel wrote:
> A subsequent patch will change the layout of devicepath nodes
> produced by this driver. In preparation, make some tweaks to
> the code to use a packed struct for the devicepath and to pass
> the device index to NorFlashCreateInstance(). These are cosmetic
> changes only, the resulting binaries should be identical.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

> ---
>  ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.c | 9 ++++++---
>  ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.h | 2 ++
>  2 files changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.c b/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.c
> index 46e815beb343..53753a4721ac 100644
> --- a/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.c
> +++ b/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.c
> @@ -82,7 +82,10 @@ NOR_FLASH_INSTANCE  mNorFlashInstanceTemplate = {
>        {
>          HARDWARE_DEVICE_PATH,
>          HW_VENDOR_DP,
> -        { (UINT8)sizeof(VENDOR_DEVICE_PATH), (UINT8)((sizeof(VENDOR_DEVICE_PATH)) >> 8) }
> +        {
> +          (UINT8)(OFFSET_OF (NOR_FLASH_DEVICE_PATH, End)),
> +          (UINT8)(OFFSET_OF (NOR_FLASH_DEVICE_PATH, End) >> 8)
> +        }
>        },
>        { 0x0, 0x0, 0x0, { 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0 } }, // GUID ... NEED TO BE FILLED
>      },
> @@ -99,7 +102,7 @@ NorFlashCreateInstance (
>    IN UINTN                  NorFlashDeviceBase,
>    IN UINTN                  NorFlashRegionBase,
>    IN UINTN                  NorFlashSize,
> -  IN UINT32                 MediaId,
> +  IN UINT32                 Index,
>    IN UINT32                 BlockSize,
>    IN BOOLEAN                SupportFvb,
>    IN CONST GUID             *NorFlashGuid,
> @@ -121,7 +124,7 @@ NorFlashCreateInstance (
>    Instance->Size = NorFlashSize;
>  
>    Instance->BlockIoProtocol.Media = &Instance->Media;
> -  Instance->Media.MediaId = MediaId;
> +  Instance->Media.MediaId = Index;
>    Instance->Media.BlockSize = BlockSize;
>    Instance->Media.LastBlock = (NorFlashSize / BlockSize)-1;
>  
> diff --git a/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.h b/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.h
> index 5c07694fbfaa..910681fd4412 100644
> --- a/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.h
> +++ b/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.h
> @@ -122,10 +122,12 @@
>  
>  typedef struct _NOR_FLASH_INSTANCE                NOR_FLASH_INSTANCE;
>  
> +#pragma pack(1)
>  typedef struct {
>    VENDOR_DEVICE_PATH                  Vendor;
>    EFI_DEVICE_PATH_PROTOCOL            End;
>  } NOR_FLASH_DEVICE_PATH;
> +#pragma pack()
>  
>  struct _NOR_FLASH_INSTANCE {
>    UINT32                              Signature;
> 

Re: [PATCH v2 0/5] ArmPlatformPkg, ArmVirtPkg: discover NOR flash banks from DTB

[Ard Biesheuvel]

On Wed, 21 Nov 2018 at 12:58, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>
> This series fixes an issue reported by Hongbo and Philippe, where
> ArmVirtQemuKernel will crash on an attempt to access flash bank #0,
> which is secure-only when running QEMU with support for EL3.
>
> So let's switch to discovering the NOR flash banks from the device tree
> instead. This requires some preparatory changes in the NOR flash driver
> to avoid having to invent GUIDs on the fly.
>
> Changes since v1:
> - split ArmPlatformPkg for clarity
> - move DT node status check into FdtClientDxe where it belongs
> - use correct UINT32* type for DT property values, and be pedantic about
>   their potential misalignment when casting to UINT64*
> - add patch to remove the 'Guid' member from NOR_FLASH_DESCRIPTION
> - add some acks
>
> Ard Biesheuvel (5):
>   ArmPlatformPkg/NorFlashDxe: prepare for devicepath format change
>   ArmPlatformPkg/NorFlashDxe: use one GUID plus index to identify flash
>     banks
>   ArmVirtPkg/FdtClientDxe: take DT node 'status' properties into account
>   ArmVirtPkg/NorFlashQemuLib: discover NOR flash banks dynamically
>   ArmPlatformPkg/NorFlashPlatformLib: remove unused Guid member from
>     struct
>
>  .../Drivers/NorFlashDxe/NorFlashDxe.c         | 15 ++--
>  .../Drivers/NorFlashDxe/NorFlashDxe.h         |  3 +
>  .../Include/Library/NorFlashPlatformLib.h     |  1 -
>  ArmVirtPkg/FdtClientDxe/FdtClientDxe.c        | 38 +++++++--
>  .../Library/NorFlashQemuLib/NorFlashQemuLib.c | 78 ++++++++++++++-----
>  .../NorFlashQemuLib/NorFlashQemuLib.inf       | 12 +++
>  6 files changed, 114 insertions(+), 33 deletions(-)
>

Pushed as 1ec194b21c0e..72e514c90730

Thanks all

Re: [PATCH v2 0/5] ArmPlatformPkg, ArmVirtPkg: discover NOR flash banks from DTB

[Ard Biesheuvel]

On Mon, 26 Nov 2018 at 18:00, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>
> On Wed, 21 Nov 2018 at 12:58, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> >
> > This series fixes an issue reported by Hongbo and Philippe, where
> > ArmVirtQemuKernel will crash on an attempt to access flash bank #0,
> > which is secure-only when running QEMU with support for EL3.
> >
> > So let's switch to discovering the NOR flash banks from the device tree
> > instead. This requires some preparatory changes in the NOR flash driver
> > to avoid having to invent GUIDs on the fly.
> >
> > Changes since v1:
> > - split ArmPlatformPkg for clarity
> > - move DT node status check into FdtClientDxe where it belongs
> > - use correct UINT32* type for DT property values, and be pedantic about
> >   their potential misalignment when casting to UINT64*
> > - add patch to remove the 'Guid' member from NOR_FLASH_DESCRIPTION
> > - add some acks
> >
> > Ard Biesheuvel (5):
> >   ArmPlatformPkg/NorFlashDxe: prepare for devicepath format change
> >   ArmPlatformPkg/NorFlashDxe: use one GUID plus index to identify flash
> >     banks
> >   ArmVirtPkg/FdtClientDxe: take DT node 'status' properties into account
> >   ArmVirtPkg/NorFlashQemuLib: discover NOR flash banks dynamically
> >   ArmPlatformPkg/NorFlashPlatformLib: remove unused Guid member from
> >     struct
> >
> >  .../Drivers/NorFlashDxe/NorFlashDxe.c         | 15 ++--
> >  .../Drivers/NorFlashDxe/NorFlashDxe.h         |  3 +
> >  .../Include/Library/NorFlashPlatformLib.h     |  1 -
> >  ArmVirtPkg/FdtClientDxe/FdtClientDxe.c        | 38 +++++++--
> >  .../Library/NorFlashQemuLib/NorFlashQemuLib.c | 78 ++++++++++++++-----
> >  .../NorFlashQemuLib/NorFlashQemuLib.inf       | 12 +++
> >  6 files changed, 114 insertions(+), 33 deletions(-)
> >
>
> Pushed as 1ec194b21c0e..72e514c90730
>
> Thanks all

Forgot to add: minus the 5th patch, which I will push once the
dependent changes in edk2-platforms have been pushed.

Re: [PATCH v2 0/5] ArmPlatformPkg, ArmVirtPkg: discover NOR flash banks from DTB

[Ard Biesheuvel]

On Mon, 26 Nov 2018 at 18:02, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>
> On Mon, 26 Nov 2018 at 18:00, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> >
> > On Wed, 21 Nov 2018 at 12:58, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> > >
> > > This series fixes an issue reported by Hongbo and Philippe, where
> > > ArmVirtQemuKernel will crash on an attempt to access flash bank #0,
> > > which is secure-only when running QEMU with support for EL3.
> > >
> > > So let's switch to discovering the NOR flash banks from the device tree
> > > instead. This requires some preparatory changes in the NOR flash driver
> > > to avoid having to invent GUIDs on the fly.
> > >
> > > Changes since v1:
> > > - split ArmPlatformPkg for clarity
> > > - move DT node status check into FdtClientDxe where it belongs
> > > - use correct UINT32* type for DT property values, and be pedantic about
> > >   their potential misalignment when casting to UINT64*
> > > - add patch to remove the 'Guid' member from NOR_FLASH_DESCRIPTION
> > > - add some acks
> > >
> > > Ard Biesheuvel (5):
> > >   ArmPlatformPkg/NorFlashDxe: prepare for devicepath format change
> > >   ArmPlatformPkg/NorFlashDxe: use one GUID plus index to identify flash
> > >     banks
> > >   ArmVirtPkg/FdtClientDxe: take DT node 'status' properties into account
> > >   ArmVirtPkg/NorFlashQemuLib: discover NOR flash banks dynamically
> > >   ArmPlatformPkg/NorFlashPlatformLib: remove unused Guid member from
> > >     struct
> > >
> > >  .../Drivers/NorFlashDxe/NorFlashDxe.c         | 15 ++--
> > >  .../Drivers/NorFlashDxe/NorFlashDxe.h         |  3 +
> > >  .../Include/Library/NorFlashPlatformLib.h     |  1 -
> > >  ArmVirtPkg/FdtClientDxe/FdtClientDxe.c        | 38 +++++++--
> > >  .../Library/NorFlashQemuLib/NorFlashQemuLib.c | 78 ++++++++++++++-----
> > >  .../NorFlashQemuLib/NorFlashQemuLib.inf       | 12 +++
> > >  6 files changed, 114 insertions(+), 33 deletions(-)
> > >
> >
> > Pushed as 1ec194b21c0e..72e514c90730
> >
> > Thanks all
>
> Forgot to add: minus the 5th patch, which I will push once the
> dependent changes in edk2-platforms have been pushed.

#5 now pushed as 13d5d0a56e486be39f95c5e0883e64698ed02714