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

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

On 1/7/21 4:00 AM, Masahisa Kojima wrote:
> Hi Vijay,
> 
> I have verified this patch on the StandaloneMM platform(SBSA-QEMU).
> Both 32bit(PcdFlashNvStorageVariableBase) and
> 64bit(PcdFlashNvStorageVariableBase64) cases work fine.
> 
> Tested-by: Masahisa Kojima <masahisa.kojima@linaro.org>
> 

Merged as #1315

Thanks all,

> 
> On Wed, 6 Jan 2021 at 18:04, Vijayenthiran Subramaniam
> <vijayenthiran.subramaniam@arm.com> wrote:
>>
>> The existing NOR Flash DXE and StandaloneMm driver supports NOR flash
>> devices connected in the 32-bit address space. Extend these drivers 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>
>> ---
>>
>> Changes since v4:
>>   - Update NorFlashStandaloneMm.c to be usable on 64-bit address space
>>
>> Changes since v3:
>>   - Add 64-bit PCDs to NorFlashStandaloneMm.inf to build for StandaloneMm
>>
>> 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/NorFlashStandaloneMm.inf |  5 +-
>>  ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.c            | 22 +++++--
>>  ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashFvb.c            | 61 +++++++++++++++++---
>>  ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashStandaloneMm.c   | 22 +++++--
>>  5 files changed, 92 insertions(+), 23 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/NorFlashStandaloneMm.inf b/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashStandaloneMm.inf
>> index f788472406b7..b2f72fb4de20 100644
>> --- a/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashStandaloneMm.inf
>> +++ b/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashStandaloneMm.inf
>> @@ -2,7 +2,7 @@
>>  #
>>  #  Component description file for NorFlashStandaloneMm module
>>  #
>> -#  Copyright (c) 2011 - 2014, ARM Ltd. All rights reserved.<BR>
>> +#  Copyright (c) 2011 - 2021, Arm Limited. All rights reserved.<BR>
>>  #  Copyright (c) 2020, Linaro, Ltd. All rights reserved.<BR>
>>  #
>>  #  SPDX-License-Identifier: BSD-2-Clause-Patent
>> @@ -50,10 +50,13 @@ [Protocols]
>>    gEfiSmmFirmwareVolumeBlockProtocolGuid
>>
>>  [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
>> diff --git a/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashStandaloneMm.c b/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashStandaloneMm.c
>> index 1ebf6d6ba70b..8a4fb395d286 100644
>> --- a/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashStandaloneMm.c
>> +++ b/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashStandaloneMm.c
>> @@ -1,6 +1,6 @@
>>  /** @file  NorFlashStandaloneMm.c
>>
>> -  Copyright (c) 2011 - 2020, Arm Limited. All rights reserved.<BR>
>> +  Copyright (c) 2011 - 2021, Arm Limited. All rights reserved.<BR>
>>    Copyright (c) 2020, Linaro, Ltd. All rights reserved.<BR>
>>
>>    SPDX-License-Identifier: BSD-2-Clause-Patent
>> @@ -298,9 +298,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,
>> @@ -330,10 +339,11 @@ NorFlashFvbInitialize (
>>
>>    ASSERT((Instance != NULL));
>>
>> -  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;
>>
>>    // Determine if there is a valid header at the beginning of the NorFlash
>>    Status = ValidateFvHeader (Instance);
>> --
>> 2.17.1
>>