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 <[email protected]>
> ---
> 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?
(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.
> +
> + 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.
> +
> + 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.
> *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. :/
> 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
_______________________________________________
edk2-devel mailing list
[email protected]
https://lists.01.org/mailman/listinfo/edk2-devel
