public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
From: "Vijayenthiran Subramaniam" <vijayenthiran.subramaniam@arm.com>
To: devel@edk2.groups.io, Ard Biesheuvel <ard.biesheuvel@arm.com>
Cc: Sami Mujawar <Sami.Mujawar@arm.com>,
	 Vijayenthiran Subramaniam <Vijayenthiran.Subramaniam@arm.com>,
	"leif@nuviainc.com" <leif@nuviainc.com>,
	 Thomas Abraham <thomas.abraham@arm.com>
Subject: Re: [edk2-devel] [PATCH v2] ArmPlatformPkg: Enable support for flash in 64-bit address space
Date: Tue, 5 Jan 2021 06:34:38 +0000	[thread overview]
Message-ID: <CAC0BY-dX8BvjJ2oHq6pG7TEer6tPMKLwVv6Dq8cjGgtqW_mCaw@mail.gmail.com> (raw)
In-Reply-To: <f810dbe1-fb1f-5df3-d453-84c947c708ed@arm.com>

Hi Ard,

On Mon, Jan 4, 2021 at 6:36 PM Ard Biesheuvel <ard.biesheuvel@arm.com> wrote:
>
> On 12/18/20 7:13 PM, Sami Mujawar wrote:
> > This patch looks good to me.
> >
> > Reviewed-by: Sami Mujawar <sami.mujawar@arm.com>
> >
>
> Thanks Sami.
>
> Vijay, could you please rebase this onto the latest edk2? There have
> been some changes to the NorFlashDxe driver.
>
> Thanks,
> Ard.

Done. Posted here: https://edk2.groups.io/g/devel/message/69677

Happy New Year and Regards,
Vijay


>
> > -----Original Message-----
> > From: Vijayenthiran Subramaniam <vijayenthiran.subramaniam@arm.com>
> > Sent: 10 December 2020 01:53 PM
> > To: devel@edk2.groups.io; leif@nuviainc.com; Ard Biesheuvel <Ard.Biesheuvel@arm.com>; Thomas Abraham <thomas.abraham@arm.com>; Sami Mujawar <Sami.Mujawar@arm.com>
> > Subject: [PATCH v2] ArmPlatformPkg: Enable support for flash in 64-bit address space
> >
> > 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>
> > ---
> >
> > Changes since v1:
> >  - Address Sami Mujawar from edk2.groups.io/g/devel/message/67980
> >
> >  ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.inf  |  5 +-
> >  ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.c    | 15 ++++-
> >  ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashFvbDxe.c | 66 ++++++++++++++++----
> >  3 files changed, 71 insertions(+), 15 deletions(-)
> >
> > diff --git a/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.inf b/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.inf
> > index a647c016878d..992ae32f4c3b 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 - 2020, Arm Limited. All rights reserved.<BR>
> >  #
> >  #  SPDX-License-Identifier: BSD-2-Clause-Patent
> >  #
> > @@ -54,10 +54,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 20134094badc..de9f3d0d7edb 100644
> > --- a/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.c
> > +++ b/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashDxe.c
> > @@ -1300,9 +1300,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,
> > diff --git a/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashFvbDxe.c b/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashFvbDxe.c
> > index 9cdd85096a46..e2a1ad5ece50 100644
> > --- a/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashFvbDxe.c
> > +++ b/ArmPlatformPkg/Drivers/NorFlashDxe/NorFlashFvbDxe.c
> > @@ -1,6 +1,6 @@
> >  /*++ @file  NorFlashFvbDxe.c
> >
> > - Copyright (c) 2011 - 2020, ARM Ltd. All rights reserved.<BR>
> > + Copyright (c) 2011 - 2020, Arm Limited. All rights reserved.<BR>
> >
> >   SPDX-License-Identifier: BSD-2-Clause-Patent
> >
> > @@ -53,23 +53,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
> > @@ -736,10 +779,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) {
> > --
> > 2.17.1
> >
>
>
>
> 
>
>

      reply	other threads:[~2021-01-05  6:34 UTC|newest]

Thread overview: 4+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-12-10 13:53 [PATCH v2] ArmPlatformPkg: Enable support for flash in 64-bit address space Vijayenthiran Subramaniam
2020-12-18 18:13 ` Sami Mujawar
2021-01-04 18:36   ` Ard Biesheuvel
2021-01-05  6:34     ` Vijayenthiran Subramaniam [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=CAC0BY-dX8BvjJ2oHq6pG7TEer6tPMKLwVv6Dq8cjGgtqW_mCaw@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