From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by mx.groups.io with SMTP id smtpd.web11.1930.1609828497767329439 for ; Mon, 04 Jan 2021 22:34:57 -0800 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: arm.com, ip: 217.140.110.172, mailfrom: vijayenthiran.subramaniam@arm.com) Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 7AB721042 for ; Mon, 4 Jan 2021 22:34:57 -0800 (PST) Received: from mail-yb1-f182.google.com (unknown [172.31.20.19]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 62F133F73C for ; Mon, 4 Jan 2021 22:34:57 -0800 (PST) Received: by mail-yb1-f182.google.com with SMTP id u203so28286924ybb.2 for ; Mon, 04 Jan 2021 22:34:57 -0800 (PST) X-Gm-Message-State: AOAM5304pQzDDEIDXUe/glrqBglk5FG27eAViu3guqc3Pz0LRc8YetiB PZQrQb0Af4lVNPJPd+3C9+Sa2LYOwjahbtbT8QM= X-Google-Smtp-Source: ABdhPJzgISt8aa0wRcFa1VXUXLgvoB7T++wlnCBOOAOhA0z4MsdI6ErJtowFMY6dOp4IJ+YzLzY9ZnYX95V76NRtIJ4= X-Received: by 2002:a25:55c5:: with SMTP id j188mr90101780ybb.455.1609828492992; Mon, 04 Jan 2021 22:34:52 -0800 (PST) MIME-Version: 1.0 References: <1607608381-22625-1-git-send-email-vijayenthiran.subramaniam@arm.com> In-Reply-To: From: "Vijayenthiran Subramaniam" Date: Tue, 5 Jan 2021 06:34:38 +0000 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [edk2-devel] [PATCH v2] ArmPlatformPkg: Enable support for flash in 64-bit address space To: devel@edk2.groups.io, Ard Biesheuvel Cc: Sami Mujawar , Vijayenthiran Subramaniam , "leif@nuviainc.com" , Thomas Abraham Content-Type: text/plain; charset="UTF-8" Hi Ard, On Mon, Jan 4, 2021 at 6:36 PM Ard Biesheuvel wrote: > > On 12/18/20 7:13 PM, Sami Mujawar wrote: > > This patch looks good to me. > > > > Reviewed-by: Sami Mujawar > > > > 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 > > Sent: 10 December 2020 01:53 PM > > To: devel@edk2.groups.io; leif@nuviainc.com; Ard Biesheuvel ; Thomas Abraham ; Sami Mujawar > > 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 > > --- > > > > 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.
> > +# Copyright (c) 2011 - 2020, Arm Limited. All rights reserved.
> > # > > # 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.
> > + Copyright (c) 2011 - 2020, Arm Limited. All rights reserved.
> > > > 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 > > > > > > > >