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

[Ard Biesheuvel <ard.biesheuvel@linaro.org>]

On Mon, 19 Nov 2018 at 12:02, Laszlo Ersek <lersek@redhat.com> wrote:
>
> On 11/17/18 01:45, 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, and flash bank
> > 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   | 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;
> > +    }
>
> (1) Do you intend to silently continue if the "status" property is missing?
>

Yes. Absent implies 'ok'

> (2) Assuming the "status" property exists, I think we could improve the
> comparison against "ok". Can you allow GetNodeProperty() to output
> PropSize as well? And then,
>
>   if (!EFI_ERROR (Status) &&
>       AsciiStrnCmp (NodeStatus, "ok", PropSize) != 0) {
>     continue;
>   }
>
> Because, if the status property is guaranteed to be NUL-terminated, then
> we don't need AsciiStrnCmp(), we can use AsciiStrCmp() -- because both
> strings are NUL-terminated. Or else, if we want to be careful about the
> property (yes, we should be), then we should pass PropSize to
> AsciiStrnCmp(). (Or maybe PropSize-1, dependent on libfdt...)
>
> Either way it bothers me that in theory, the status property can be
> shorter than 2 chars, it may not be NUL-terminated, and we pass constant
> 2 to AsciiStrnCmp().
>
> I'm OK if we use an ASSERT() rather than an "if", in some of the above.
>

ard@mba13:~/linux-2.6$ git grep -E 'status = \"ok\"' *.dts |wc -l
239
ard@mba13:~/linux-2.6$ git grep -E 'status = \"okay\"' *.dts |wc -l
8776

IOW, we'll need to deal with both, and the spurious false positive on
'oktopus' didn't seem like a big deal to me, hence the truncated
comparison.

But indeed, we should not assume that the property value is guaranteed
to be at least 2 bytes in length. Let's be pedantic and permit 'ok'
and 'okay' only.

> > +
> > +    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;
> > +    }
>
> (3) We should say DEBUG_ERROR in new code.
>

Copy/paste error, will fix.

> > +
> > +    ASSERT ((RegSize % (2 * sizeof(UINT64))) == 0);
>
> (4) Please add a space after the sizeof operator.
>
> > +
> > +    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);
>
> (5) Same as (4).
>
> > +
> > +      if (Num >= MAX_FLASH_BANKS) {
> > +        goto Finished;
> > +      }
> > +    }
> > +  }
> > +
> > +Finished:
>
> (6) Can you replace the "goto" with an additional restriction, added to
> both loop's controlling expressions, namely (Num < MAX_FLASH_BANKS)?
>
> I understand the appeal of the "goto", but "Finished" is not an error
> handling label.
>
> If you disagree, I won't insist.
>

No, I'll change that - I wasn't entirely happy with it myself tbh.

> >    *NorFlashDescriptions = mNorFlashDevices;
> > -  *Count = ARRAY_SIZE (mNorFlashDevices);
> > +  *Count = Num;
>
> (7) *Count has type UINT32; I suggest changing Num to UINT32 as well.
>
> If you disagree, I won't insist; I do realize ARRAY_SIZE() used to
> produce an UINTN as well. :/
>

No I'll change that.

> >    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
> >
>
> Thanks for writing these patches!
> Laszlo