* [PATCH 0/2] ArmPlatformPkg, ArmVirtPkg: discover NOR flash banks from DTB
@ 2018-11-17 0:45 Ard Biesheuvel
2018-11-17 0:45 ` [PATCH 1/2] ArmPlatformPkg/NorFlashDxe: use one GUID plus index to identify flash banks Ard Biesheuvel
2018-11-17 0:45 ` [PATCH 2/2] ArmVirtPkg/NorFlashQemuLib: discover NOR flash banks dynamically Ard Biesheuvel
0 siblings, 2 replies; 15+ messages in thread
From: Ard Biesheuvel @ 2018-11-17 0:45 UTC (permalink / raw)
To: edk2-devel; +Cc: lersek, leif.lindholm, philmd, hongbo.zhang, 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.
Ard Biesheuvel (2):
ArmPlatformPkg/NorFlashDxe: use one GUID plus index to identify flash
banks
ArmVirtPkg/NorFlashQemuLib: discover NOR flash banks dynamically
.../Drivers/NorFlashDxe/NorFlashDxe.c | 15 ++--
.../Drivers/NorFlashDxe/NorFlashDxe.h | 3 +
.../Library/NorFlashQemuLib/NorFlashQemuLib.c | 84 ++++++++++++++-----
.../NorFlashQemuLib/NorFlashQemuLib.inf | 12 +++
4 files changed, 87 insertions(+), 27 deletions(-)
--
2.17.1
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH 1/2] ArmPlatformPkg/NorFlashDxe: use one GUID plus index to identify flash banks
2018-11-17 0:45 [PATCH 0/2] ArmPlatformPkg, ArmVirtPkg: discover NOR flash banks from DTB Ard Biesheuvel
@ 2018-11-17 0:45 ` Ard Biesheuvel
2018-11-19 19:05 ` Leif Lindholm
2018-11-19 19:16 ` Laszlo Ersek
2018-11-17 0:45 ` [PATCH 2/2] ArmVirtPkg/NorFlashQemuLib: discover NOR flash banks dynamically Ard Biesheuvel
1 sibling, 2 replies; 15+ messages in thread
From: Ard Biesheuvel @ 2018-11-17 0:45 UTC (permalink / raw)
To: edk2-devel; +Cc: lersek, leif.lindholm, philmd, hongbo.zhang, 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>
---
ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.c | 15 +++++++++------
ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.h | 3 +++
2 files changed, 12 insertions(+), 6 deletions(-)
diff --git a/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.c b/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.c
index 46e815beb343..60a06e4a5058 100644
--- a/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.c
+++ b/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.c
@@ -82,10 +82,14 @@ 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
},
+ 0, // Index
{
END_DEVICE_PATH_TYPE,
END_ENTIRE_DEVICE_PATH_SUBTYPE,
@@ -99,10 +103,9 @@ 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,
OUT NOR_FLASH_INSTANCE** NorFlashInstance
)
{
@@ -121,11 +124,12 @@ 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;
- CopyGuid (&Instance->DevicePath.Vendor.Guid, NorFlashGuid);
+ CopyGuid (&Instance->DevicePath.Vendor.Guid, &gEfiCallerIdGuid);
+ Instance->DevicePath.Index = Index;
Instance->ShadowBuffer = AllocateRuntimePool (BlockSize);;
if (Instance->ShadowBuffer == NULL) {
@@ -1311,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 5c07694fbfaa..8886aa43d9f3 100644
--- a/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.h
+++ b/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.h
@@ -122,10 +122,13 @@
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()
struct _NOR_FLASH_INSTANCE {
UINT32 Signature;
--
2.17.1
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH 2/2] ArmVirtPkg/NorFlashQemuLib: discover NOR flash banks dynamically
2018-11-17 0:45 [PATCH 0/2] ArmPlatformPkg, ArmVirtPkg: discover NOR flash banks from DTB Ard Biesheuvel
2018-11-17 0:45 ` [PATCH 1/2] ArmPlatformPkg/NorFlashDxe: use one GUID plus index to identify flash banks Ard Biesheuvel
@ 2018-11-17 0:45 ` Ard Biesheuvel
2018-11-17 1:29 ` Ard Biesheuvel
2018-11-19 20:02 ` Laszlo Ersek
1 sibling, 2 replies; 15+ messages in thread
From: Ard Biesheuvel @ 2018-11-17 0:45 UTC (permalink / raw)
To: edk2-devel; +Cc: lersek, leif.lindholm, philmd, hongbo.zhang, 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, 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;
+ }
+
+ 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
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH 2/2] ArmVirtPkg/NorFlashQemuLib: discover NOR flash banks dynamically
2018-11-17 0:45 ` [PATCH 2/2] ArmVirtPkg/NorFlashQemuLib: discover NOR flash banks dynamically Ard Biesheuvel
@ 2018-11-17 1:29 ` Ard Biesheuvel
2018-11-19 19:10 ` Leif Lindholm
2018-11-19 20:02 ` Laszlo Ersek
1 sibling, 1 reply; 15+ messages in thread
From: Ard Biesheuvel @ 2018-11-17 1:29 UTC (permalink / raw)
To: edk2-devel@lists.01.org
Cc: Laszlo Ersek, Leif Lindholm, Philippe Mathieu-Daudé,
Hongbo Zhang
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.
> 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;
> + }
> +
> + 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
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/2] ArmPlatformPkg/NorFlashDxe: use one GUID plus index to identify flash banks
2018-11-17 0:45 ` [PATCH 1/2] ArmPlatformPkg/NorFlashDxe: use one GUID plus index to identify flash banks Ard Biesheuvel
@ 2018-11-19 19:05 ` Leif Lindholm
2018-11-19 19:09 ` Ard Biesheuvel
2018-11-19 19:16 ` Laszlo Ersek
1 sibling, 1 reply; 15+ messages in thread
From: Leif Lindholm @ 2018-11-19 19:05 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: edk2-devel, lersek, philmd, hongbo.zhang,
Thomas Panakamattam Abraham, Nariman Poushin
On Fri, Nov 16, 2018 at 04:45:23PM -0800, 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>
> ---
> ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.c | 15 +++++++++------
> ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.h | 3 +++
> 2 files changed, 12 insertions(+), 6 deletions(-)
>
> diff --git a/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.c b/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.c
> index 46e815beb343..60a06e4a5058 100644
> --- a/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.c
> +++ b/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.c
> @@ -82,10 +82,14 @@ 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
> },
> + 0, // Index
> {
> END_DEVICE_PATH_TYPE,
> END_ENTIRE_DEVICE_PATH_SUBTYPE,
> @@ -99,10 +103,9 @@ 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,
> OUT NOR_FLASH_INSTANCE** NorFlashInstance
> )
> {
> @@ -121,11 +124,12 @@ 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;
>
> - CopyGuid (&Instance->DevicePath.Vendor.Guid, NorFlashGuid);
> + CopyGuid (&Instance->DevicePath.Vendor.Guid, &gEfiCallerIdGuid);
Just sanity checking: this sets the VendorGuid to the NorFlashDxe
GUID? (93E34C7E-B50E-11DF-9223-2443DFD72085)
If not, can you explain it to me slowly? :)
Thomas, Nariman: would this change cause any transient issues for
anything that has set Boot#### options in any of your configurations?
And if it would, is that a big deal?
(Ard has a separate patch that fixes up any default values.)
/
Leif
> + Instance->DevicePath.Index = Index;
>
> Instance->ShadowBuffer = AllocateRuntimePool (BlockSize);;
> if (Instance->ShadowBuffer == NULL) {
> @@ -1311,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 5c07694fbfaa..8886aa43d9f3 100644
> --- a/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.h
> +++ b/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.h
> @@ -122,10 +122,13 @@
>
> 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()
>
> struct _NOR_FLASH_INSTANCE {
> UINT32 Signature;
> --
> 2.17.1
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/2] ArmPlatformPkg/NorFlashDxe: use one GUID plus index to identify flash banks
2018-11-19 19:05 ` Leif Lindholm
@ 2018-11-19 19:09 ` Ard Biesheuvel
2018-11-19 19:23 ` Leif Lindholm
0 siblings, 1 reply; 15+ messages in thread
From: Ard Biesheuvel @ 2018-11-19 19:09 UTC (permalink / raw)
To: Leif Lindholm
Cc: edk2-devel@lists.01.org, Laszlo Ersek,
Philippe Mathieu-Daudé, Hongbo Zhang,
Thomas Panakamattam Abraham, Nariman Poushin
On Mon, 19 Nov 2018 at 11:05, Leif Lindholm <leif.lindholm@linaro.org> wrote:
>
> On Fri, Nov 16, 2018 at 04:45:23PM -0800, 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>
> > ---
> > ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.c | 15 +++++++++------
> > ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.h | 3 +++
> > 2 files changed, 12 insertions(+), 6 deletions(-)
> >
> > diff --git a/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.c b/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.c
> > index 46e815beb343..60a06e4a5058 100644
> > --- a/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.c
> > +++ b/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.c
> > @@ -82,10 +82,14 @@ 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
> > },
> > + 0, // Index
> > {
> > END_DEVICE_PATH_TYPE,
> > END_ENTIRE_DEVICE_PATH_SUBTYPE,
> > @@ -99,10 +103,9 @@ 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,
> > OUT NOR_FLASH_INSTANCE** NorFlashInstance
> > )
> > {
> > @@ -121,11 +124,12 @@ 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;
> >
> > - CopyGuid (&Instance->DevicePath.Vendor.Guid, NorFlashGuid);
> > + CopyGuid (&Instance->DevicePath.Vendor.Guid, &gEfiCallerIdGuid);
>
> Just sanity checking: this sets the VendorGuid to the NorFlashDxe
> GUID? (93E34C7E-B50E-11DF-9223-2443DFD72085)
>
Yes.
Before:
Mapping table
BLK1: Alias(s):
VenHw(F9B94AE2-8BA6-409B-9D56-B9B417F53CB3)
BLK0: Alias(s):
VenHw(8047DB4B-7E9C-4C0C-8EBC-DFBBAACACE8F)
After:
Mapping table
BLK0: Alias(s):
VenHw(93E34C7E-B50E-11DF-9223-2443DFD72085,00)
BLK1: Alias(s):
VenHw(93E34C7E-B50E-11DF-9223-2443DFD72085,01)
> If not, can you explain it to me slowly? :)
>
> Thomas, Nariman: would this change cause any transient issues for
> anything that has set Boot#### options in any of your configurations?
> And if it would, is that a big deal?
> (Ard has a separate patch that fixes up any default values.)
>
> /
> Leif
>
> > + Instance->DevicePath.Index = Index;
> >
> > Instance->ShadowBuffer = AllocateRuntimePool (BlockSize);;
> > if (Instance->ShadowBuffer == NULL) {
> > @@ -1311,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 5c07694fbfaa..8886aa43d9f3 100644
> > --- a/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.h
> > +++ b/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.h
> > @@ -122,10 +122,13 @@
> >
> > 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()
> >
> > struct _NOR_FLASH_INSTANCE {
> > UINT32 Signature;
> > --
> > 2.17.1
> >
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/2] ArmVirtPkg/NorFlashQemuLib: discover NOR flash banks dynamically
2018-11-17 1:29 ` Ard Biesheuvel
@ 2018-11-19 19:10 ` Leif Lindholm
2018-11-19 19:16 ` Ard Biesheuvel
0 siblings, 1 reply; 15+ messages in thread
From: Leif Lindholm @ 2018-11-19 19:10 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: edk2-devel@lists.01.org, Laszlo Ersek,
Philippe Mathieu-Daudé, Hongbo Zhang
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
> >
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/2] ArmVirtPkg/NorFlashQemuLib: discover NOR flash banks dynamically
2018-11-19 19:10 ` Leif Lindholm
@ 2018-11-19 19:16 ` Ard Biesheuvel
2018-11-19 19:24 ` Leif Lindholm
0 siblings, 1 reply; 15+ messages in thread
From: Ard Biesheuvel @ 2018-11-19 19:16 UTC (permalink / raw)
To: Leif Lindholm
Cc: edk2-devel@lists.01.org, Laszlo Ersek,
Philippe Mathieu-Daudé, Hongbo Zhang
On Mon, 19 Nov 2018 at 11:10, Leif Lindholm <leif.lindholm@linaro.org> wrote:
>
> 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 :)
>
No this is entirely reasonable: it makes perfect sense for a NOR flash
at address 0x0 to be secure only on a system that implements EL3,
since mach-virt's default reset vector is 0x0.
> > > 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?
>
No that is a QEMU thing.
> If it does, please add a comment to the commit message.
> Either way:
> Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>
>
Thanks
> > >
> > > 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
> > >
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/2] ArmPlatformPkg/NorFlashDxe: use one GUID plus index to identify flash banks
2018-11-17 0:45 ` [PATCH 1/2] ArmPlatformPkg/NorFlashDxe: use one GUID plus index to identify flash banks Ard Biesheuvel
2018-11-19 19:05 ` Leif Lindholm
@ 2018-11-19 19:16 ` Laszlo Ersek
1 sibling, 0 replies; 15+ messages in thread
From: Laszlo Ersek @ 2018-11-19 19:16 UTC (permalink / raw)
To: Ard Biesheuvel, edk2-devel
On 11/17/18 01:45, 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>
> ---
> ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.c | 15 +++++++++------
> ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.h | 3 +++
> 2 files changed, 12 insertions(+), 6 deletions(-)
>
> diff --git a/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.c b/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.c
> index 46e815beb343..60a06e4a5058 100644
> --- a/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.c
> +++ b/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.c
> @@ -82,10 +82,14 @@ 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)
> + }
(1) Please add a space character after OFFSET_OF (both instances).
(2) OFFSET_OF suggests a now-missing #pragma pack (1)... added at the
bottom, great.
(3) Normally I would prefer to split this hunk to a prior patch, as a
no-op refactoring. Up to you.
> },
> { 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,
> @@ -99,10 +103,9 @@ NorFlashCreateInstance (
> IN UINTN NorFlashDeviceBase,
> IN UINTN NorFlashRegionBase,
> IN UINTN NorFlashSize,
> - IN UINT32 MediaId,
> + IN UINT32 Index,
(4) Same as (3), also up to you.
> IN UINT32 BlockSize,
> IN BOOLEAN SupportFvb,
> - IN CONST GUID *NorFlashGuid,
> OUT NOR_FLASH_INSTANCE** NorFlashInstance
> )
> {
> @@ -121,11 +124,12 @@ NorFlashCreateInstance (
> Instance->Size = NorFlashSize;
>
> Instance->BlockIoProtocol.Media = &Instance->Media;
> - Instance->Media.MediaId = MediaId;
> + Instance->Media.MediaId = Index;
(5) Ditto.
> 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 = Index;
>
> Instance->ShadowBuffer = AllocateRuntimePool (BlockSize);;
> if (Instance->ShadowBuffer == NULL) {
> @@ -1311,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 5c07694fbfaa..8886aa43d9f3 100644
> --- a/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.h
> +++ b/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.h
> @@ -122,10 +122,13 @@
>
> 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()
>
> struct _NOR_FLASH_INSTANCE {
> UINT32 Signature;
>
(6) Given that you introduce this field as UINT8, the
"Instance->DevicePath.Index = Index" assignment in
NorFlashCreateInstance() is liable to trigger UINT32-->UINT8
"truncation" warnings under at least some toolchains, IMO. Can you add
an explicit cast to that assignment (assuming you deem the assignment
safe otherwise)?
If you decide to ignore (3) through (5), I think (1) and (6) can be
fixed up before pushing. In that case:
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Thanks
Laszlo
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 1/2] ArmPlatformPkg/NorFlashDxe: use one GUID plus index to identify flash banks
2018-11-19 19:09 ` Ard Biesheuvel
@ 2018-11-19 19:23 ` Leif Lindholm
0 siblings, 0 replies; 15+ messages in thread
From: Leif Lindholm @ 2018-11-19 19:23 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: edk2-devel@lists.01.org, Laszlo Ersek,
Philippe Mathieu-Daudé, Hongbo Zhang,
Thomas Panakamattam Abraham, Nariman Poushin
On Mon, Nov 19, 2018 at 11:09:32AM -0800, Ard Biesheuvel wrote:
> > > @@ -121,11 +124,12 @@ 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;
> > >
> > > - CopyGuid (&Instance->DevicePath.Vendor.Guid, NorFlashGuid);
> > > + CopyGuid (&Instance->DevicePath.Vendor.Guid, &gEfiCallerIdGuid);
> >
> > Just sanity checking: this sets the VendorGuid to the NorFlashDxe
> > GUID? (93E34C7E-B50E-11DF-9223-2443DFD72085)
> >
>
> Yes.
>
> Before:
>
> Mapping table
> BLK1: Alias(s):
> VenHw(F9B94AE2-8BA6-409B-9D56-B9B417F53CB3)
> BLK0: Alias(s):
> VenHw(8047DB4B-7E9C-4C0C-8EBC-DFBBAACACE8F)
>
> After:
>
> Mapping table
> BLK0: Alias(s):
> VenHw(93E34C7E-B50E-11DF-9223-2443DFD72085,00)
> BLK1: Alias(s):
> VenHw(93E34C7E-B50E-11DF-9223-2443DFD72085,01)
OK, I'm happy with that.
If Thomas/Nariman don't object:
Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>
/
Leif
> > If not, can you explain it to me slowly? :)
> >
> > Thomas, Nariman: would this change cause any transient issues for
> > anything that has set Boot#### options in any of your configurations?
> > And if it would, is that a big deal?
> > (Ard has a separate patch that fixes up any default values.)
> >
> > /
> > Leif
> >
> > > + Instance->DevicePath.Index = Index;
> > >
> > > Instance->ShadowBuffer = AllocateRuntimePool (BlockSize);;
> > > if (Instance->ShadowBuffer == NULL) {
> > > @@ -1311,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 5c07694fbfaa..8886aa43d9f3 100644
> > > --- a/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.h
> > > +++ b/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.h
> > > @@ -122,10 +122,13 @@
> > >
> > > 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()
> > >
> > > struct _NOR_FLASH_INSTANCE {
> > > UINT32 Signature;
> > > --
> > > 2.17.1
> > >
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/2] ArmVirtPkg/NorFlashQemuLib: discover NOR flash banks dynamically
2018-11-19 19:16 ` Ard Biesheuvel
@ 2018-11-19 19:24 ` Leif Lindholm
2018-11-19 19:27 ` Ard Biesheuvel
0 siblings, 1 reply; 15+ messages in thread
From: Leif Lindholm @ 2018-11-19 19:24 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: edk2-devel@lists.01.org, Laszlo Ersek,
Philippe Mathieu-Daudé, Hongbo Zhang
On Mon, Nov 19, 2018 at 11:16:09AM -0800, Ard Biesheuvel wrote:
> On Mon, 19 Nov 2018 at 11:10, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> >
> > 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 :)
>
> No this is entirely reasonable: it makes perfect sense for a NOR flash
> at address 0x0 to be secure only on a system that implements EL3,
> since mach-virt's default reset vector is 0x0.
*cough* sorry, I was referring to the leading #.
> > > > 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?
>
> No that is a QEMU thing.
OK, thanks for confirming.
But this should mean that we don't need any changes on the guest sides
if/when we do fix this in QEMU?
/
Leif
> > If it does, please add a comment to the commit message.
> > Either way:
> > Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>
> >
>
> Thanks
>
> > > >
> > > > 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
> > > >
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/2] ArmVirtPkg/NorFlashQemuLib: discover NOR flash banks dynamically
2018-11-19 19:24 ` Leif Lindholm
@ 2018-11-19 19:27 ` Ard Biesheuvel
2018-11-19 19:31 ` Leif Lindholm
0 siblings, 1 reply; 15+ messages in thread
From: Ard Biesheuvel @ 2018-11-19 19:27 UTC (permalink / raw)
To: Leif Lindholm
Cc: edk2-devel@lists.01.org, Laszlo Ersek,
Philippe Mathieu-Daudé, Hongbo Zhang
On Mon, 19 Nov 2018 at 11:24, Leif Lindholm <leif.lindholm@linaro.org> wrote:
>
>
> On Mon, Nov 19, 2018 at 11:16:09AM -0800, Ard Biesheuvel wrote:
> > On Mon, 19 Nov 2018 at 11:10, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> > >
> > > 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 :)
> >
> > No this is entirely reasonable: it makes perfect sense for a NOR flash
> > at address 0x0 to be secure only on a system that implements EL3,
> > since mach-virt's default reset vector is 0x0.
>
> *cough* sorry, I was referring to the leading #.
>
Ah yes :-) Been caught by that a couple of times already.
> > > > > 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?
> >
> > No that is a QEMU thing.
>
> OK, thanks for confirming.
> But this should mean that we don't need any changes on the guest sides
> if/when we do fix this in QEMU?
>
This would indeed get rid of any discrepancies between what QEMU
exposes and what the firmware assumes, so yes, it's an improvement in
that sense. However, I don't think the QEMU side of this is likely to
change any time soon.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/2] ArmVirtPkg/NorFlashQemuLib: discover NOR flash banks dynamically
2018-11-19 19:27 ` Ard Biesheuvel
@ 2018-11-19 19:31 ` Leif Lindholm
0 siblings, 0 replies; 15+ messages in thread
From: Leif Lindholm @ 2018-11-19 19:31 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: edk2-devel@lists.01.org, Laszlo Ersek,
Philippe Mathieu-Daudé, Hongbo Zhang
On Mon, Nov 19, 2018 at 11:27:22AM -0800, Ard Biesheuvel wrote:
> > > > > > 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?
> > >
> > > No that is a QEMU thing.
> >
> > OK, thanks for confirming.
> > But this should mean that we don't need any changes on the guest sides
> > if/when we do fix this in QEMU?
>
> This would indeed get rid of any discrepancies between what QEMU
> exposes and what the firmware assumes, so yes, it's an improvement in
> that sense. However, I don't think the QEMU side of this is likely to
> change any time soon.
Sure. But it does decrease the amount of reward delay required for
someone to consider having a look at it :)
Thanks!
/
Leif
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/2] ArmVirtPkg/NorFlashQemuLib: discover NOR flash banks dynamically
2018-11-17 0:45 ` [PATCH 2/2] ArmVirtPkg/NorFlashQemuLib: discover NOR flash banks dynamically Ard Biesheuvel
2018-11-17 1:29 ` Ard Biesheuvel
@ 2018-11-19 20:02 ` Laszlo Ersek
2018-11-19 20:24 ` Ard Biesheuvel
1 sibling, 1 reply; 15+ messages in thread
From: Laszlo Ersek @ 2018-11-19 20:02 UTC (permalink / raw)
To: Ard Biesheuvel, edk2-devel
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?
(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
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH 2/2] ArmVirtPkg/NorFlashQemuLib: discover NOR flash banks dynamically
2018-11-19 20:02 ` Laszlo Ersek
@ 2018-11-19 20:24 ` Ard Biesheuvel
0 siblings, 0 replies; 15+ messages in thread
From: Ard Biesheuvel @ 2018-11-19 20:24 UTC (permalink / raw)
To: Laszlo Ersek
Cc: edk2-devel@lists.01.org, Leif Lindholm,
Philippe Mathieu-Daudé, Hongbo Zhang
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
^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2018-11-19 20:24 UTC | newest]
Thread overview: 15+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-11-17 0:45 [PATCH 0/2] ArmPlatformPkg, ArmVirtPkg: discover NOR flash banks from DTB Ard Biesheuvel
2018-11-17 0:45 ` [PATCH 1/2] ArmPlatformPkg/NorFlashDxe: use one GUID plus index to identify flash banks Ard Biesheuvel
2018-11-19 19:05 ` Leif Lindholm
2018-11-19 19:09 ` Ard Biesheuvel
2018-11-19 19:23 ` Leif Lindholm
2018-11-19 19:16 ` Laszlo Ersek
2018-11-17 0:45 ` [PATCH 2/2] ArmVirtPkg/NorFlashQemuLib: discover NOR flash banks dynamically Ard Biesheuvel
2018-11-17 1:29 ` Ard Biesheuvel
2018-11-19 19:10 ` Leif Lindholm
2018-11-19 19:16 ` Ard Biesheuvel
2018-11-19 19:24 ` Leif Lindholm
2018-11-19 19:27 ` Ard Biesheuvel
2018-11-19 19:31 ` Leif Lindholm
2018-11-19 20:02 ` Laszlo Ersek
2018-11-19 20:24 ` Ard Biesheuvel
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox