[edk2-devel] [PATCH v3] ArmPlatformPkg: Enable support for flash in 64-bit address space

Vijayenthiran Subramaniam posted 1 patch 4 months ago
Failed in applying to current master (apply log)
ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.inf |  5 +-
ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.c   | 22 +++++--
ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashFvb.c   | 61 +++++++++++++++++---
3 files changed, 72 insertions(+), 16 deletions(-)

[edk2-devel] [PATCH v3] ArmPlatformPkg: Enable support for flash in 64-bit address space

Posted by Vijayenthiran Subramaniam 4 months ago
The existing NOR Flash dxe driver supports NOR flash devices connected
in the 32-bit address space. Extend this driver to allow NOR flash
devices connected to 64-bit address space to be usable as well. Also,
convert the base address and size sanity check from ASSERT() to if
condition so that even if the firmware is build in release mode, it can
return error if the parameter(s) is/are invalid.

Signed-off-by: Vijayenthiran Subramaniam <vijayenthiran.subramaniam@arm.com>
Reviewed-by: Sami Mujawar <sami.mujawar@arm.com>
---

Changes since v2:
  - Rebased to latest edk2 master branch and update copyright year
  - Retaining Sami's R-by from
    https://edk2.groups.io/g/devel/message/69214

 ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.inf |  5 +-
 ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.c   | 22 +++++--
 ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashFvb.c   | 61 +++++++++++++++++---
 3 files changed, 72 insertions(+), 16 deletions(-)

diff --git a/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.inf b/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.inf
index 8b5078497fff..f8d4c2703143 100644
--- a/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.inf
+++ b/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.inf
@@ -2,7 +2,7 @@
 #
 #  Component description file for NorFlashDxe module
 #
-#  Copyright (c) 2011 - 2014, ARM Ltd. All rights reserved.<BR>
+#  Copyright (c) 2011 - 2021, Arm Limited. All rights reserved.<BR>
 #
 #  SPDX-License-Identifier: BSD-2-Clause-Patent
 #
@@ -55,10 +55,13 @@ [Protocols]
   gEfiDiskIoProtocolGuid
 
 [Pcd.common]
+  gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableBase64
   gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableBase
   gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableSize
+  gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwWorkingBase64
   gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwWorkingBase
   gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwWorkingSize
+  gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareBase64
   gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareBase
   gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareSize
 
diff --git a/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.c b/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.c
index 41cdd1cbd397..28dc8e125c78 100644
--- a/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.c
+++ b/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.c
@@ -1,6 +1,6 @@
 /** @file  NorFlashDxe.c
 
-  Copyright (c) 2011 - 2020, Arm Limited. All rights reserved.<BR>
+  Copyright (c) 2011 - 2021, Arm Limited. All rights reserved.<BR>
 
   SPDX-License-Identifier: BSD-2-Clause-Patent
 
@@ -343,9 +343,18 @@ NorFlashInitialise (
 
   for (Index = 0; Index < mNorFlashDeviceCount; Index++) {
     // Check if this NOR Flash device contain the variable storage region
-    ContainVariableStorage =
-        (NorFlashDevices[Index].RegionBaseAddress <= PcdGet32 (PcdFlashNvStorageVariableBase)) &&
-        (PcdGet32 (PcdFlashNvStorageVariableBase) + PcdGet32 (PcdFlashNvStorageVariableSize) <= NorFlashDevices[Index].RegionBaseAddress + NorFlashDevices[Index].Size);
+
+   if (PcdGet64 (PcdFlashNvStorageVariableBase64) != 0) {
+     ContainVariableStorage =
+       (NorFlashDevices[Index].RegionBaseAddress <= PcdGet64 (PcdFlashNvStorageVariableBase64)) &&
+       (PcdGet64 (PcdFlashNvStorageVariableBase64) + PcdGet32 (PcdFlashNvStorageVariableSize) <=
+        NorFlashDevices[Index].RegionBaseAddress + NorFlashDevices[Index].Size);
+   } else {
+     ContainVariableStorage =
+       (NorFlashDevices[Index].RegionBaseAddress <= PcdGet32 (PcdFlashNvStorageVariableBase)) &&
+       (PcdGet32 (PcdFlashNvStorageVariableBase) + PcdGet32 (PcdFlashNvStorageVariableSize) <=
+        NorFlashDevices[Index].RegionBaseAddress + NorFlashDevices[Index].Size);
+  }
 
     Status = NorFlashCreateInstance (
       NorFlashDevices[Index].DeviceBaseAddress,
@@ -413,10 +422,11 @@ NorFlashFvbInitialize (
       EFI_MEMORY_UC | EFI_MEMORY_RUNTIME);
   ASSERT_EFI_ERROR (Status);
 
-  mFlashNvStorageVariableBase = PcdGet32 (PcdFlashNvStorageVariableBase);
+  mFlashNvStorageVariableBase = (FixedPcdGet64 (PcdFlashNvStorageVariableBase64) != 0) ?
+    FixedPcdGet64 (PcdFlashNvStorageVariableBase64) : FixedPcdGet32 (PcdFlashNvStorageVariableBase);
 
   // Set the index of the first LBA for the FVB
-  Instance->StartLba = (PcdGet32 (PcdFlashNvStorageVariableBase) - Instance->RegionBaseAddress) / Instance->Media.BlockSize;
+  Instance->StartLba = (mFlashNvStorageVariableBase - Instance->RegionBaseAddress) / Instance->Media.BlockSize;
 
   BootMode = GetBootModeHob ();
   if (BootMode == BOOT_WITH_DEFAULT_SETTINGS) {
diff --git a/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashFvb.c b/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashFvb.c
index a332b5e98be7..db8eb595f4b8 100644
--- a/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashFvb.c
+++ b/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashFvb.c
@@ -1,6 +1,6 @@
 /*++ @file  NorFlashFvbDxe.c
 
- Copyright (c) 2011 - 2020, ARM Ltd. All rights reserved.<BR>
+ Copyright (c) 2011 - 2021, Arm Limited. All rights reserved.<BR>
 
  SPDX-License-Identifier: BSD-2-Clause-Patent
 
@@ -48,23 +48,66 @@ InitializeFvAndVariableStoreHeaders (
   UINTN                               HeadersLength;
   EFI_FIRMWARE_VOLUME_HEADER          *FirmwareVolumeHeader;
   VARIABLE_STORE_HEADER               *VariableStoreHeader;
+  UINT32                              NvStorageFtwSpareSize;
+  UINT32                              NvStorageFtwWorkingSize;
+  UINT32                              NvStorageVariableSize;
+  UINT64                              NvStorageFtwSpareBase;
+  UINT64                              NvStorageFtwWorkingBase;
+  UINT64                              NvStorageVariableBase;
 
   HeadersLength = sizeof(EFI_FIRMWARE_VOLUME_HEADER) + sizeof(EFI_FV_BLOCK_MAP_ENTRY) + sizeof(VARIABLE_STORE_HEADER);
   Headers = AllocateZeroPool(HeadersLength);
 
+  NvStorageFtwWorkingSize = PcdGet32 (PcdFlashNvStorageFtwWorkingSize);
+  NvStorageFtwSpareSize = PcdGet32 (PcdFlashNvStorageFtwSpareSize);
+  NvStorageVariableSize = PcdGet32 (PcdFlashNvStorageVariableSize);
+
+  NvStorageFtwSpareBase = (PcdGet64 (PcdFlashNvStorageFtwSpareBase64) != 0) ?
+    PcdGet64 (PcdFlashNvStorageFtwSpareBase64) : PcdGet32 (PcdFlashNvStorageFtwSpareBase);
+  NvStorageFtwWorkingBase = (PcdGet64 (PcdFlashNvStorageFtwWorkingBase64) != 0) ?
+    PcdGet64 (PcdFlashNvStorageFtwWorkingBase64) : PcdGet32 (PcdFlashNvStorageFtwWorkingBase);
+  NvStorageVariableBase = (PcdGet64 (PcdFlashNvStorageVariableBase64) != 0) ?
+    PcdGet64 (PcdFlashNvStorageVariableBase64) : PcdGet32 (PcdFlashNvStorageVariableBase);
+
   // FirmwareVolumeHeader->FvLength is declared to have the Variable area AND the FTW working area AND the FTW Spare contiguous.
-  ASSERT(PcdGet32(PcdFlashNvStorageVariableBase) + PcdGet32(PcdFlashNvStorageVariableSize) == PcdGet32(PcdFlashNvStorageFtwWorkingBase));
-  ASSERT(PcdGet32(PcdFlashNvStorageFtwWorkingBase) + PcdGet32(PcdFlashNvStorageFtwWorkingSize) == PcdGet32(PcdFlashNvStorageFtwSpareBase));
+  if ((NvStorageVariableBase + NvStorageVariableSize) != NvStorageFtwWorkingBase) {
+    DEBUG ((DEBUG_ERROR, "%a: NvStorageFtwWorkingBase is not contiguous with NvStorageVariableBase region\n",
+          __FUNCTION__));
+    return EFI_INVALID_PARAMETER;
+  }
+
+  if ((NvStorageFtwWorkingBase + NvStorageFtwWorkingSize) != NvStorageFtwSpareBase) {
+    DEBUG ((DEBUG_ERROR, "%a: NvStorageFtwSpareBase is not contiguous with NvStorageFtwWorkingBase region\n",
+        __FUNCTION__));
+    return EFI_INVALID_PARAMETER;
+  }
 
   // Check if the size of the area is at least one block size
-  ASSERT((PcdGet32(PcdFlashNvStorageVariableSize) > 0) && (PcdGet32(PcdFlashNvStorageVariableSize) / Instance->Media.BlockSize > 0));
-  ASSERT((PcdGet32(PcdFlashNvStorageFtwWorkingSize) > 0) && (PcdGet32(PcdFlashNvStorageFtwWorkingSize) / Instance->Media.BlockSize > 0));
-  ASSERT((PcdGet32(PcdFlashNvStorageFtwSpareSize) > 0) && (PcdGet32(PcdFlashNvStorageFtwSpareSize) / Instance->Media.BlockSize > 0));
+  if ((NvStorageVariableSize <= 0) || (NvStorageVariableSize / Instance->Media.BlockSize <= 0)) {
+    DEBUG ((DEBUG_ERROR, "%a: NvStorageVariableSize is 0x%x, should be atleast one block size\n", __FUNCTION__,
+          NvStorageVariableSize));
+    return EFI_INVALID_PARAMETER;
+  }
+
+  if ((NvStorageFtwWorkingSize <= 0) || (NvStorageFtwWorkingSize / Instance->Media.BlockSize <= 0)) {
+    DEBUG ((DEBUG_ERROR, "%a: NvStorageFtwWorkingSize is 0x%x, should be atleast one block size\n", __FUNCTION__,
+          NvStorageFtwWorkingSize));
+    return EFI_INVALID_PARAMETER;
+  }
+
+  if ((NvStorageFtwSpareSize <= 0) || (NvStorageFtwSpareSize / Instance->Media.BlockSize <= 0)) {
+    DEBUG ((DEBUG_ERROR, "%a: NvStorageFtwSpareSize is 0x%x, should be atleast one block size\n", __FUNCTION__,
+          NvStorageFtwSpareSize));
+    return EFI_INVALID_PARAMETER;
+  }
 
   // Ensure the Variable area Base Addresses are aligned on a block size boundaries
-  ASSERT(PcdGet32(PcdFlashNvStorageVariableBase) % Instance->Media.BlockSize == 0);
-  ASSERT(PcdGet32(PcdFlashNvStorageFtwWorkingBase) % Instance->Media.BlockSize == 0);
-  ASSERT(PcdGet32(PcdFlashNvStorageFtwSpareBase) % Instance->Media.BlockSize == 0);
+  if ((NvStorageVariableBase % Instance->Media.BlockSize != 0) ||
+      (NvStorageFtwWorkingBase % Instance->Media.BlockSize != 0) ||
+      (NvStorageFtwSpareBase % Instance->Media.BlockSize != 0)) {
+    DEBUG ((DEBUG_ERROR, "%a: NvStorage Base addresses must be aligned to block size boundaries", __FUNCTION__));
+    return EFI_INVALID_PARAMETER;
+  }
 
   //
   // EFI_FIRMWARE_VOLUME_HEADER
-- 
2.17.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#69677): https://edk2.groups.io/g/devel/message/69677
Mute This Topic: https://groups.io/mt/79445547/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-


Re: [edk2-devel] [PATCH v3] ArmPlatformPkg: Enable support for flash in 64-bit address space

Posted by Ard Biesheuvel 4 months ago
On 1/5/21 7:29 AM, Vijayenthiran Subramaniam wrote:
> The existing NOR Flash dxe driver supports NOR flash devices connected
> in the 32-bit address space. Extend this driver to allow NOR flash
> devices connected to 64-bit address space to be usable as well. Also,
> convert the base address and size sanity check from ASSERT() to if
> condition so that even if the firmware is build in release mode, it can
> return error if the parameter(s) is/are invalid.
> 
> Signed-off-by: Vijayenthiran Subramaniam <vijayenthiran.subramaniam@arm.com>
> Reviewed-by: Sami Mujawar <sami.mujawar@arm.com>
> ---
> 
> Changes since v2:
>   - Rebased to latest edk2 master branch and update copyright year
>   - Retaining Sami's R-by from
>     https://edk2.groups.io/g/devel/message/69214
> 

Thank you Vijay.

But your patch now breaks the build for the new standalone MM driver.
Please make sure ArmPlatformPkg/ArmPlatformPkg.dsc can be built
successfully with your patch applied.

-- 
Ard.



>  ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.inf |  5 +-
>  ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.c   | 22 +++++--
>  ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashFvb.c   | 61 +++++++++++++++++---
>  3 files changed, 72 insertions(+), 16 deletions(-)
> 
> diff --git a/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.inf b/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.inf
> index 8b5078497fff..f8d4c2703143 100644
> --- a/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.inf
> +++ b/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.inf
> @@ -2,7 +2,7 @@
>  #
>  #  Component description file for NorFlashDxe module
>  #
> -#  Copyright (c) 2011 - 2014, ARM Ltd. All rights reserved.<BR>
> +#  Copyright (c) 2011 - 2021, Arm Limited. All rights reserved.<BR>
>  #
>  #  SPDX-License-Identifier: BSD-2-Clause-Patent
>  #
> @@ -55,10 +55,13 @@ [Protocols]
>    gEfiDiskIoProtocolGuid
>  
>  [Pcd.common]
> +  gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableBase64
>    gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableBase
>    gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableSize
> +  gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwWorkingBase64
>    gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwWorkingBase
>    gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwWorkingSize
> +  gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareBase64
>    gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareBase
>    gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareSize
>  
> diff --git a/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.c b/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.c
> index 41cdd1cbd397..28dc8e125c78 100644
> --- a/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.c
> +++ b/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.c
> @@ -1,6 +1,6 @@
>  /** @file  NorFlashDxe.c
>  
> -  Copyright (c) 2011 - 2020, Arm Limited. All rights reserved.<BR>
> +  Copyright (c) 2011 - 2021, Arm Limited. All rights reserved.<BR>
>  
>    SPDX-License-Identifier: BSD-2-Clause-Patent
>  
> @@ -343,9 +343,18 @@ NorFlashInitialise (
>  
>    for (Index = 0; Index < mNorFlashDeviceCount; Index++) {
>      // Check if this NOR Flash device contain the variable storage region
> -    ContainVariableStorage =
> -        (NorFlashDevices[Index].RegionBaseAddress <= PcdGet32 (PcdFlashNvStorageVariableBase)) &&
> -        (PcdGet32 (PcdFlashNvStorageVariableBase) + PcdGet32 (PcdFlashNvStorageVariableSize) <= NorFlashDevices[Index].RegionBaseAddress + NorFlashDevices[Index].Size);
> +
> +   if (PcdGet64 (PcdFlashNvStorageVariableBase64) != 0) {
> +     ContainVariableStorage =
> +       (NorFlashDevices[Index].RegionBaseAddress <= PcdGet64 (PcdFlashNvStorageVariableBase64)) &&
> +       (PcdGet64 (PcdFlashNvStorageVariableBase64) + PcdGet32 (PcdFlashNvStorageVariableSize) <=
> +        NorFlashDevices[Index].RegionBaseAddress + NorFlashDevices[Index].Size);
> +   } else {
> +     ContainVariableStorage =
> +       (NorFlashDevices[Index].RegionBaseAddress <= PcdGet32 (PcdFlashNvStorageVariableBase)) &&
> +       (PcdGet32 (PcdFlashNvStorageVariableBase) + PcdGet32 (PcdFlashNvStorageVariableSize) <=
> +        NorFlashDevices[Index].RegionBaseAddress + NorFlashDevices[Index].Size);
> +  }
>  
>      Status = NorFlashCreateInstance (
>        NorFlashDevices[Index].DeviceBaseAddress,
> @@ -413,10 +422,11 @@ NorFlashFvbInitialize (
>        EFI_MEMORY_UC | EFI_MEMORY_RUNTIME);
>    ASSERT_EFI_ERROR (Status);
>  
> -  mFlashNvStorageVariableBase = PcdGet32 (PcdFlashNvStorageVariableBase);
> +  mFlashNvStorageVariableBase = (FixedPcdGet64 (PcdFlashNvStorageVariableBase64) != 0) ?
> +    FixedPcdGet64 (PcdFlashNvStorageVariableBase64) : FixedPcdGet32 (PcdFlashNvStorageVariableBase);
>  
>    // Set the index of the first LBA for the FVB
> -  Instance->StartLba = (PcdGet32 (PcdFlashNvStorageVariableBase) - Instance->RegionBaseAddress) / Instance->Media.BlockSize;
> +  Instance->StartLba = (mFlashNvStorageVariableBase - Instance->RegionBaseAddress) / Instance->Media.BlockSize;
>  
>    BootMode = GetBootModeHob ();
>    if (BootMode == BOOT_WITH_DEFAULT_SETTINGS) {
> diff --git a/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashFvb.c b/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashFvb.c
> index a332b5e98be7..db8eb595f4b8 100644
> --- a/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashFvb.c
> +++ b/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashFvb.c
> @@ -1,6 +1,6 @@
>  /*++ @file  NorFlashFvbDxe.c
>  
> - Copyright (c) 2011 - 2020, ARM Ltd. All rights reserved.<BR>
> + Copyright (c) 2011 - 2021, Arm Limited. All rights reserved.<BR>
>  
>   SPDX-License-Identifier: BSD-2-Clause-Patent
>  
> @@ -48,23 +48,66 @@ InitializeFvAndVariableStoreHeaders (
>    UINTN                               HeadersLength;
>    EFI_FIRMWARE_VOLUME_HEADER          *FirmwareVolumeHeader;
>    VARIABLE_STORE_HEADER               *VariableStoreHeader;
> +  UINT32                              NvStorageFtwSpareSize;
> +  UINT32                              NvStorageFtwWorkingSize;
> +  UINT32                              NvStorageVariableSize;
> +  UINT64                              NvStorageFtwSpareBase;
> +  UINT64                              NvStorageFtwWorkingBase;
> +  UINT64                              NvStorageVariableBase;
>  
>    HeadersLength = sizeof(EFI_FIRMWARE_VOLUME_HEADER) + sizeof(EFI_FV_BLOCK_MAP_ENTRY) + sizeof(VARIABLE_STORE_HEADER);
>    Headers = AllocateZeroPool(HeadersLength);
>  
> +  NvStorageFtwWorkingSize = PcdGet32 (PcdFlashNvStorageFtwWorkingSize);
> +  NvStorageFtwSpareSize = PcdGet32 (PcdFlashNvStorageFtwSpareSize);
> +  NvStorageVariableSize = PcdGet32 (PcdFlashNvStorageVariableSize);
> +
> +  NvStorageFtwSpareBase = (PcdGet64 (PcdFlashNvStorageFtwSpareBase64) != 0) ?
> +    PcdGet64 (PcdFlashNvStorageFtwSpareBase64) : PcdGet32 (PcdFlashNvStorageFtwSpareBase);
> +  NvStorageFtwWorkingBase = (PcdGet64 (PcdFlashNvStorageFtwWorkingBase64) != 0) ?
> +    PcdGet64 (PcdFlashNvStorageFtwWorkingBase64) : PcdGet32 (PcdFlashNvStorageFtwWorkingBase);
> +  NvStorageVariableBase = (PcdGet64 (PcdFlashNvStorageVariableBase64) != 0) ?
> +    PcdGet64 (PcdFlashNvStorageVariableBase64) : PcdGet32 (PcdFlashNvStorageVariableBase);
> +
>    // FirmwareVolumeHeader->FvLength is declared to have the Variable area AND the FTW working area AND the FTW Spare contiguous.
> -  ASSERT(PcdGet32(PcdFlashNvStorageVariableBase) + PcdGet32(PcdFlashNvStorageVariableSize) == PcdGet32(PcdFlashNvStorageFtwWorkingBase));
> -  ASSERT(PcdGet32(PcdFlashNvStorageFtwWorkingBase) + PcdGet32(PcdFlashNvStorageFtwWorkingSize) == PcdGet32(PcdFlashNvStorageFtwSpareBase));
> +  if ((NvStorageVariableBase + NvStorageVariableSize) != NvStorageFtwWorkingBase) {
> +    DEBUG ((DEBUG_ERROR, "%a: NvStorageFtwWorkingBase is not contiguous with NvStorageVariableBase region\n",
> +          __FUNCTION__));
> +    return EFI_INVALID_PARAMETER;
> +  }
> +
> +  if ((NvStorageFtwWorkingBase + NvStorageFtwWorkingSize) != NvStorageFtwSpareBase) {
> +    DEBUG ((DEBUG_ERROR, "%a: NvStorageFtwSpareBase is not contiguous with NvStorageFtwWorkingBase region\n",
> +        __FUNCTION__));
> +    return EFI_INVALID_PARAMETER;
> +  }
>  
>    // Check if the size of the area is at least one block size
> -  ASSERT((PcdGet32(PcdFlashNvStorageVariableSize) > 0) && (PcdGet32(PcdFlashNvStorageVariableSize) / Instance->Media.BlockSize > 0));
> -  ASSERT((PcdGet32(PcdFlashNvStorageFtwWorkingSize) > 0) && (PcdGet32(PcdFlashNvStorageFtwWorkingSize) / Instance->Media.BlockSize > 0));
> -  ASSERT((PcdGet32(PcdFlashNvStorageFtwSpareSize) > 0) && (PcdGet32(PcdFlashNvStorageFtwSpareSize) / Instance->Media.BlockSize > 0));
> +  if ((NvStorageVariableSize <= 0) || (NvStorageVariableSize / Instance->Media.BlockSize <= 0)) {
> +    DEBUG ((DEBUG_ERROR, "%a: NvStorageVariableSize is 0x%x, should be atleast one block size\n", __FUNCTION__,
> +          NvStorageVariableSize));
> +    return EFI_INVALID_PARAMETER;
> +  }
> +
> +  if ((NvStorageFtwWorkingSize <= 0) || (NvStorageFtwWorkingSize / Instance->Media.BlockSize <= 0)) {
> +    DEBUG ((DEBUG_ERROR, "%a: NvStorageFtwWorkingSize is 0x%x, should be atleast one block size\n", __FUNCTION__,
> +          NvStorageFtwWorkingSize));
> +    return EFI_INVALID_PARAMETER;
> +  }
> +
> +  if ((NvStorageFtwSpareSize <= 0) || (NvStorageFtwSpareSize / Instance->Media.BlockSize <= 0)) {
> +    DEBUG ((DEBUG_ERROR, "%a: NvStorageFtwSpareSize is 0x%x, should be atleast one block size\n", __FUNCTION__,
> +          NvStorageFtwSpareSize));
> +    return EFI_INVALID_PARAMETER;
> +  }
>  
>    // Ensure the Variable area Base Addresses are aligned on a block size boundaries
> -  ASSERT(PcdGet32(PcdFlashNvStorageVariableBase) % Instance->Media.BlockSize == 0);
> -  ASSERT(PcdGet32(PcdFlashNvStorageFtwWorkingBase) % Instance->Media.BlockSize == 0);
> -  ASSERT(PcdGet32(PcdFlashNvStorageFtwSpareBase) % Instance->Media.BlockSize == 0);
> +  if ((NvStorageVariableBase % Instance->Media.BlockSize != 0) ||
> +      (NvStorageFtwWorkingBase % Instance->Media.BlockSize != 0) ||
> +      (NvStorageFtwSpareBase % Instance->Media.BlockSize != 0)) {
> +    DEBUG ((DEBUG_ERROR, "%a: NvStorage Base addresses must be aligned to block size boundaries", __FUNCTION__));
> +    return EFI_INVALID_PARAMETER;
> +  }
>  
>    //
>    // EFI_FIRMWARE_VOLUME_HEADER
> 



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#69683): https://edk2.groups.io/g/devel/message/69683
Mute This Topic: https://groups.io/mt/79445547/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-


Re: [edk2-devel] [PATCH v3] ArmPlatformPkg: Enable support for flash in 64-bit address space

Posted by Vijayenthiran Subramaniam 4 months ago
Hi Ard,

On Tue, Jan 5, 2021 at 8:52 AM Ard Biesheuvel <ard.biesheuvel@arm.com> wrote:
>
> On 1/5/21 7:29 AM, Vijayenthiran Subramaniam wrote:
> > The existing NOR Flash dxe driver supports NOR flash devices connected
> > in the 32-bit address space. Extend this driver to allow NOR flash
> > devices connected to 64-bit address space to be usable as well. Also,
> > convert the base address and size sanity check from ASSERT() to if
> > condition so that even if the firmware is build in release mode, it can
> > return error if the parameter(s) is/are invalid.
> >
> > Signed-off-by: Vijayenthiran Subramaniam <vijayenthiran.subramaniam@arm.com>
> > Reviewed-by: Sami Mujawar <sami.mujawar@arm.com>
> > ---
> >
> > Changes since v2:
> >   - Rebased to latest edk2 master branch and update copyright year
> >   - Retaining Sami's R-by from
> >     https://edk2.groups.io/g/devel/message/69214
> >
>
> Thank you Vijay.
>
> But your patch now breaks the build for the new standalone MM driver.
> Please make sure ArmPlatformPkg/ArmPlatformPkg.dsc can be built
> successfully with your patch applied.
>
> --
> Ard.
>

My bad. Added the new PCDs to the standalonemm inf file and I can build
ArmPlatformPkg/ArmPlatformPkg.dsc successfully. Posted v4 here:
https://edk2.groups.io/g/devel/message/69691

Regards,
Vijay

> >  ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.inf |  5 +-
> >  ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.c   | 22 +++++--
> >  ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashFvb.c   | 61 +++++++++++++++++---
> >  3 files changed, 72 insertions(+), 16 deletions(-)
> >
> > diff --git a/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.inf b/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.inf
> > index 8b5078497fff..f8d4c2703143 100644
> > --- a/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.inf
> > +++ b/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.inf
> > @@ -2,7 +2,7 @@
> >  #
> >  #  Component description file for NorFlashDxe module
> >  #
> > -#  Copyright (c) 2011 - 2014, ARM Ltd. All rights reserved.<BR>
> > +#  Copyright (c) 2011 - 2021, Arm Limited. All rights reserved.<BR>
> >  #
> >  #  SPDX-License-Identifier: BSD-2-Clause-Patent
> >  #
> > @@ -55,10 +55,13 @@ [Protocols]
> >    gEfiDiskIoProtocolGuid
> >
> >  [Pcd.common]
> > +  gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableBase64
> >    gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableBase
> >    gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableSize
> > +  gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwWorkingBase64
> >    gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwWorkingBase
> >    gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwWorkingSize
> > +  gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareBase64
> >    gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareBase
> >    gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareSize
> >
> > diff --git a/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.c b/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.c
> > index 41cdd1cbd397..28dc8e125c78 100644
> > --- a/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.c
> > +++ b/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.c
> > @@ -1,6 +1,6 @@
> >  /** @file  NorFlashDxe.c
> >
> > -  Copyright (c) 2011 - 2020, Arm Limited. All rights reserved.<BR>
> > +  Copyright (c) 2011 - 2021, Arm Limited. All rights reserved.<BR>
> >
> >    SPDX-License-Identifier: BSD-2-Clause-Patent
> >
> > @@ -343,9 +343,18 @@ NorFlashInitialise (
> >
> >    for (Index = 0; Index < mNorFlashDeviceCount; Index++) {
> >      // Check if this NOR Flash device contain the variable storage region
> > -    ContainVariableStorage =
> > -        (NorFlashDevices[Index].RegionBaseAddress <= PcdGet32 (PcdFlashNvStorageVariableBase)) &&
> > -        (PcdGet32 (PcdFlashNvStorageVariableBase) + PcdGet32 (PcdFlashNvStorageVariableSize) <= NorFlashDevices[Index].RegionBaseAddress + NorFlashDevices[Index].Size);
> > +
> > +   if (PcdGet64 (PcdFlashNvStorageVariableBase64) != 0) {
> > +     ContainVariableStorage =
> > +       (NorFlashDevices[Index].RegionBaseAddress <= PcdGet64 (PcdFlashNvStorageVariableBase64)) &&
> > +       (PcdGet64 (PcdFlashNvStorageVariableBase64) + PcdGet32 (PcdFlashNvStorageVariableSize) <=
> > +        NorFlashDevices[Index].RegionBaseAddress + NorFlashDevices[Index].Size);
> > +   } else {
> > +     ContainVariableStorage =
> > +       (NorFlashDevices[Index].RegionBaseAddress <= PcdGet32 (PcdFlashNvStorageVariableBase)) &&
> > +       (PcdGet32 (PcdFlashNvStorageVariableBase) + PcdGet32 (PcdFlashNvStorageVariableSize) <=
> > +        NorFlashDevices[Index].RegionBaseAddress + NorFlashDevices[Index].Size);
> > +  }
> >
> >      Status = NorFlashCreateInstance (
> >        NorFlashDevices[Index].DeviceBaseAddress,
> > @@ -413,10 +422,11 @@ NorFlashFvbInitialize (
> >        EFI_MEMORY_UC | EFI_MEMORY_RUNTIME);
> >    ASSERT_EFI_ERROR (Status);
> >
> > -  mFlashNvStorageVariableBase = PcdGet32 (PcdFlashNvStorageVariableBase);
> > +  mFlashNvStorageVariableBase = (FixedPcdGet64 (PcdFlashNvStorageVariableBase64) != 0) ?
> > +    FixedPcdGet64 (PcdFlashNvStorageVariableBase64) : FixedPcdGet32 (PcdFlashNvStorageVariableBase);
> >
> >    // Set the index of the first LBA for the FVB
> > -  Instance->StartLba = (PcdGet32 (PcdFlashNvStorageVariableBase) - Instance->RegionBaseAddress) / Instance->Media.BlockSize;
> > +  Instance->StartLba = (mFlashNvStorageVariableBase - Instance->RegionBaseAddress) / Instance->Media.BlockSize;
> >
> >    BootMode = GetBootModeHob ();
> >    if (BootMode == BOOT_WITH_DEFAULT_SETTINGS) {
> > diff --git a/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashFvb.c b/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashFvb.c
> > index a332b5e98be7..db8eb595f4b8 100644
> > --- a/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashFvb.c
> > +++ b/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashFvb.c
> > @@ -1,6 +1,6 @@
> >  /*++ @file  NorFlashFvbDxe.c
> >
> > - Copyright (c) 2011 - 2020, ARM Ltd. All rights reserved.<BR>
> > + Copyright (c) 2011 - 2021, Arm Limited. All rights reserved.<BR>
> >
> >   SPDX-License-Identifier: BSD-2-Clause-Patent
> >
> > @@ -48,23 +48,66 @@ InitializeFvAndVariableStoreHeaders (
> >    UINTN                               HeadersLength;
> >    EFI_FIRMWARE_VOLUME_HEADER          *FirmwareVolumeHeader;
> >    VARIABLE_STORE_HEADER               *VariableStoreHeader;
> > +  UINT32                              NvStorageFtwSpareSize;
> > +  UINT32                              NvStorageFtwWorkingSize;
> > +  UINT32                              NvStorageVariableSize;
> > +  UINT64                              NvStorageFtwSpareBase;
> > +  UINT64                              NvStorageFtwWorkingBase;
> > +  UINT64                              NvStorageVariableBase;
> >
> >    HeadersLength = sizeof(EFI_FIRMWARE_VOLUME_HEADER) + sizeof(EFI_FV_BLOCK_MAP_ENTRY) + sizeof(VARIABLE_STORE_HEADER);
> >    Headers = AllocateZeroPool(HeadersLength);
> >
> > +  NvStorageFtwWorkingSize = PcdGet32 (PcdFlashNvStorageFtwWorkingSize);
> > +  NvStorageFtwSpareSize = PcdGet32 (PcdFlashNvStorageFtwSpareSize);
> > +  NvStorageVariableSize = PcdGet32 (PcdFlashNvStorageVariableSize);
> > +
> > +  NvStorageFtwSpareBase = (PcdGet64 (PcdFlashNvStorageFtwSpareBase64) != 0) ?
> > +    PcdGet64 (PcdFlashNvStorageFtwSpareBase64) : PcdGet32 (PcdFlashNvStorageFtwSpareBase);
> > +  NvStorageFtwWorkingBase = (PcdGet64 (PcdFlashNvStorageFtwWorkingBase64) != 0) ?
> > +    PcdGet64 (PcdFlashNvStorageFtwWorkingBase64) : PcdGet32 (PcdFlashNvStorageFtwWorkingBase);
> > +  NvStorageVariableBase = (PcdGet64 (PcdFlashNvStorageVariableBase64) != 0) ?
> > +    PcdGet64 (PcdFlashNvStorageVariableBase64) : PcdGet32 (PcdFlashNvStorageVariableBase);
> > +
> >    // FirmwareVolumeHeader->FvLength is declared to have the Variable area AND the FTW working area AND the FTW Spare contiguous.
> > -  ASSERT(PcdGet32(PcdFlashNvStorageVariableBase) + PcdGet32(PcdFlashNvStorageVariableSize) == PcdGet32(PcdFlashNvStorageFtwWorkingBase));
> > -  ASSERT(PcdGet32(PcdFlashNvStorageFtwWorkingBase) + PcdGet32(PcdFlashNvStorageFtwWorkingSize) == PcdGet32(PcdFlashNvStorageFtwSpareBase));
> > +  if ((NvStorageVariableBase + NvStorageVariableSize) != NvStorageFtwWorkingBase) {
> > +    DEBUG ((DEBUG_ERROR, "%a: NvStorageFtwWorkingBase is not contiguous with NvStorageVariableBase region\n",
> > +          __FUNCTION__));
> > +    return EFI_INVALID_PARAMETER;
> > +  }
> > +
> > +  if ((NvStorageFtwWorkingBase + NvStorageFtwWorkingSize) != NvStorageFtwSpareBase) {
> > +    DEBUG ((DEBUG_ERROR, "%a: NvStorageFtwSpareBase is not contiguous with NvStorageFtwWorkingBase region\n",
> > +        __FUNCTION__));
> > +    return EFI_INVALID_PARAMETER;
> > +  }
> >
> >    // Check if the size of the area is at least one block size
> > -  ASSERT((PcdGet32(PcdFlashNvStorageVariableSize) > 0) && (PcdGet32(PcdFlashNvStorageVariableSize) / Instance->Media.BlockSize > 0));
> > -  ASSERT((PcdGet32(PcdFlashNvStorageFtwWorkingSize) > 0) && (PcdGet32(PcdFlashNvStorageFtwWorkingSize) / Instance->Media.BlockSize > 0));
> > -  ASSERT((PcdGet32(PcdFlashNvStorageFtwSpareSize) > 0) && (PcdGet32(PcdFlashNvStorageFtwSpareSize) / Instance->Media.BlockSize > 0));
> > +  if ((NvStorageVariableSize <= 0) || (NvStorageVariableSize / Instance->Media.BlockSize <= 0)) {
> > +    DEBUG ((DEBUG_ERROR, "%a: NvStorageVariableSize is 0x%x, should be atleast one block size\n", __FUNCTION__,
> > +          NvStorageVariableSize));
> > +    return EFI_INVALID_PARAMETER;
> > +  }
> > +
> > +  if ((NvStorageFtwWorkingSize <= 0) || (NvStorageFtwWorkingSize / Instance->Media.BlockSize <= 0)) {
> > +    DEBUG ((DEBUG_ERROR, "%a: NvStorageFtwWorkingSize is 0x%x, should be atleast one block size\n", __FUNCTION__,
> > +          NvStorageFtwWorkingSize));
> > +    return EFI_INVALID_PARAMETER;
> > +  }
> > +
> > +  if ((NvStorageFtwSpareSize <= 0) || (NvStorageFtwSpareSize / Instance->Media.BlockSize <= 0)) {
> > +    DEBUG ((DEBUG_ERROR, "%a: NvStorageFtwSpareSize is 0x%x, should be atleast one block size\n", __FUNCTION__,
> > +          NvStorageFtwSpareSize));
> > +    return EFI_INVALID_PARAMETER;
> > +  }
> >
> >    // Ensure the Variable area Base Addresses are aligned on a block size boundaries
> > -  ASSERT(PcdGet32(PcdFlashNvStorageVariableBase) % Instance->Media.BlockSize == 0);
> > -  ASSERT(PcdGet32(PcdFlashNvStorageFtwWorkingBase) % Instance->Media.BlockSize == 0);
> > -  ASSERT(PcdGet32(PcdFlashNvStorageFtwSpareBase) % Instance->Media.BlockSize == 0);
> > +  if ((NvStorageVariableBase % Instance->Media.BlockSize != 0) ||
> > +      (NvStorageFtwWorkingBase % Instance->Media.BlockSize != 0) ||
> > +      (NvStorageFtwSpareBase % Instance->Media.BlockSize != 0)) {
> > +    DEBUG ((DEBUG_ERROR, "%a: NvStorage Base addresses must be aligned to block size boundaries", __FUNCTION__));
> > +    return EFI_INVALID_PARAMETER;
> > +  }
> >
> >    //
> >    // EFI_FIRMWARE_VOLUME_HEADER
> >
>
>
>
> 
>
>


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#69692): https://edk2.groups.io/g/devel/message/69692
Mute This Topic: https://groups.io/mt/79445547/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-