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

["Ard Biesheuvel" <ard.biesheuvel@arm.com>]

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
>