From: "Masahisa Kojima" <masahisa.kojima@linaro.org>
To: Vijayenthiran Subramanian <vijayenthiran.subramaniam@arm.com>
Cc: edk2-devel-groups-io <devel@edk2.groups.io>,
Ard Biesheuvel <ard.biesheuvel@arm.com>,
Leif Lindholm <leif@nuviainc.com>,
Thomas Abraham <thomas.abraham@arm.com>,
Sami Mujawar <sami.mujawar@arm.com>
Subject: Re: [edk2-devel] [PATCH v4] ArmPlatformPkg: Enable support for flash in 64-bit address space
Date: Wed, 6 Jan 2021 21:13:29 +0900 [thread overview]
Message-ID: <CADQ0-X-cfZTkFFgXx_CHRoprZDMo=mu--X8oHxRWpEeRHfi9Ow@mail.gmail.com> (raw)
In-Reply-To: <CAC0BY-ef0Qp9ezxMc9q4vTLykCSrZA89vPBKTVccwnV3x05Wpw@mail.gmail.com>
Hi Vijay,
> Can you test the v5 of this patch on the StandaloneMm based platform?
> (posted here: https://edk2.groups.io/g/devel/message/69798)
OK.
I will test the patch tomorrow(JST).
Regards,
Masahisa
On Wed, 6 Jan 2021 at 18:07, Vijayenthiran Subramanian
<vijayenthiran.subramaniam@arm.com> wrote:
>
> Hi Masahisa,
>
> On Tue, Jan 5, 2021 at 11:17 AM Vijayenthiran Subramanian
> <vijayenthiran.subramaniam@arm.com> wrote:
> >
> > Hi Ard,
> >
> > On Tue, Jan 5, 2021 at 11:07 AM Ard Biesheuvel <ard.biesheuvel@arm.com> wrote:
> > >
> > > On 1/5/21 11:57 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>
> > > > ---
> > >
> > > Thanks Vijay.
> > >
> > > But adding the new PCDs to
> > > ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashStandaloneMm.inf only makes
> > > sense if the driver uses their values. So it appears to me its version
> > > of NorFlashInitialise needs to be modified as well.
> >
> > Right. I will do that and post a new version.
> >
> > > Also, please cc Masahisa (cc'ed) on your next revision - he should be
> > > able to test your patch on a standalone MM based platform.
> >
> > Sure :)
>
> Can you test the v5 of this patch on the StandaloneMm based platform?
> (posted here: https://edk2.groups.io/g/devel/message/69798)
>
> Regards,
> Vijay
>
>
>
> >
> > > Thanks,
> > > Ard.
> > >
> > >
> > > >
> > > > 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 +++++++++++++++++---
> > > > 4 files changed, 76 insertions(+), 17 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
> > > >
> > >
> > >
> > >
> > >
> > >
> > >
prev parent reply other threads:[~2021-01-06 12:13 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-01-05 10:57 [PATCH v4] ArmPlatformPkg: Enable support for flash in 64-bit address space Vijayenthiran Subramaniam
2021-01-05 11:07 ` Ard Biesheuvel
2021-01-05 11:17 ` [edk2-devel] " Vijayenthiran Subramaniam
2021-01-06 9:07 ` Vijayenthiran Subramaniam
2021-01-06 12:13 ` Masahisa Kojima [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-list from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CADQ0-X-cfZTkFFgXx_CHRoprZDMo=mu--X8oHxRWpEeRHfi9Ow@mail.gmail.com' \
--to=devel@edk2.groups.io \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox