Re: [PATCH 2/2] ArmVirtPkg/NorFlashQemuLib: discover NOR flash banks dynamically

[Leif Lindholm <leif.lindholm@linaro.org>]

On Fri, Nov 16, 2018 at 05:29:05PM -0800, Ard Biesheuvel wrote:
> On Fri, 16 Nov 2018 at 16:45, Ard Biesheuvel <ard.biesheuvel@linaro.org> 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, and flash bank
> 
> #0 is configured as secure-only when running with support for EL3 enabled.

Never gets old :)

> > 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.

Does this mean we as a side effect get rid of the limitation that the
pflash image needs to be exactly 64MB. Or does that require further
changes on the QEMU side?

If it does, please add a comment to the commit message.
Either way:
Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>

/
    Leif

> >
> > Contributed-under: TianoCore Contribution Agreement 1.1
> > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > ---
> >  ArmVirtPkg/Library/NorFlashQemuLib/NorFlashQemuLib.c   | 84 +++++++++++++++-----
> >  ArmVirtPkg/Library/NorFlashQemuLib/NorFlashQemuLib.inf | 12 +++
> >  2 files changed, 75 insertions(+), 21 deletions(-)
> >
> > diff --git a/ArmVirtPkg/Library/NorFlashQemuLib/NorFlashQemuLib.c b/ArmVirtPkg/Library/NorFlashQemuLib/NorFlashQemuLib.c
> > index e3bbae5b06c5..dc0a15e77170 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,60 @@ NorFlashPlatformGetDevices (
> >    OUT UINT32                  *Count
> >    )
> >  {
> > +  FDT_CLIENT_PROTOCOL         *FdtClient;
> > +  INT32                       Node;
> > +  EFI_STATUS                  Status;
> > +  EFI_STATUS                  FindNodeStatus;
> > +  CONST UINT64                *Reg;
> > +  UINT32                      RegSize;
> > +  CONST CHAR8                 *NodeStatus;
> > +  UINTN                       Num;
> > +
> > +  Status = gBS->LocateProtocol (&gFdtClientProtocolGuid, NULL,
> > +                  (VOID **)&FdtClient);
> > +  ASSERT_EFI_ERROR (Status);
> > +
> > +  Num = 0;
> > +  for (FindNodeStatus = FdtClient->FindCompatibleNode (FdtClient,
> > +                                     "cfi-flash", &Node);
> > +       !EFI_ERROR (FindNodeStatus);
> > +       FindNodeStatus = FdtClient->FindNextCompatibleNode (FdtClient,
> > +                                     "cfi-flash", Node, &Node)) {
> > +
> > +    Status = FdtClient->GetNodeProperty (FdtClient, Node, "status",
> > +                          (CONST VOID **)&NodeStatus, NULL);
> > +    if (!EFI_ERROR (Status) && AsciiStrnCmp (NodeStatus, "ok", 2) != 0) {
> > +      continue;
> > +    }
> > +
> > +    Status = FdtClient->GetNodeProperty (FdtClient, Node, "reg",
> > +                          (CONST VOID **)&Reg, &RegSize);
> > +    if (EFI_ERROR (Status)) {
> > +      DEBUG ((EFI_D_ERROR, "%a: GetNodeProperty () failed (Status == %r)\n",
> > +        __FUNCTION__, Status));
> > +      continue;
> > +    }
> > +
> > +    ASSERT ((RegSize % (2 * sizeof(UINT64))) == 0);
> > +
> > +    while (RegSize > 0) {
> > +      mNorFlashDevices[Num].DeviceBaseAddress = (UINTN)SwapBytes64 (Reg[0]);
> > +      mNorFlashDevices[Num].RegionBaseAddress = (UINTN)SwapBytes64 (Reg[0]);
> > +      mNorFlashDevices[Num].Size              = (UINTN)SwapBytes64 (Reg[1]);
> > +      mNorFlashDevices[Num].BlockSize         = QEMU_NOR_BLOCK_SIZE;
> > +
> > +      Num++;
> > +      Reg += 2;
> > +      RegSize -= 2 * sizeof(UINT64);
> > +
> > +      if (Num >= MAX_FLASH_BANKS) {
> > +        goto Finished;
> > +      }
> > +    }
> > +  }
> > +
> > +Finished:
> >    *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
> >