From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-lf1-f41.google.com (mail-lf1-f41.google.com [209.85.167.41]) by mx.groups.io with SMTP id smtpd.web11.7103.1609935223180344829 for ; Wed, 06 Jan 2021 04:13:43 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@linaro.org header.s=google header.b=ssu/eZzx; spf=pass (domain: linaro.org, ip: 209.85.167.41, mailfrom: masahisa.kojima@linaro.org) Received: by mail-lf1-f41.google.com with SMTP id o13so6085426lfr.3 for ; Wed, 06 Jan 2021 04:13:42 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=P9RzOkV+5YRqDZmR1/Bs32824X0np1dncoLCa6dDtak=; b=ssu/eZzxYGrrpNJNRXInQsfHXjZhoa2iBLn9bDdcdJ1tBaCNJRKId9ZRU5pu8gK069 02Pj1hFN4mLib/idCc0YOEU2SKYkssKZXNSmyJixOZvotw6fkm9I5Y74LV+mTrzUXrhO Iky7tzaHj8lu8zFZZRcqsQ6EP9SpFmbxuJcTDTzzjFMwO066VlR4jVATYGvTzHzgbTXn Nl/vEHzzagGPkPoGeLk2AS0VMZHApN7BZbxXEexajjK5jRYY/FPRFdqYT9quWeeBZnW/ 2Nh/m2jObN1XXNlDvEPe43qh55N7HNyKFB85k8AwsjPlRwvhGMe7uPoLc8q1MB4JAJrv vutw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=P9RzOkV+5YRqDZmR1/Bs32824X0np1dncoLCa6dDtak=; b=T1gSlZqAfvoohYQwjycn/2McdZ464mpu07fv6U5ObUO7qWayCld0vKAbCZ0jzheLJG Nz1fT2qeh0169a/XYV6Xper+/GzezrZH9BoNEo0nytjLoZmgzqmHw/Td7h/u6A7AArFb j57IyMiVGjsQO4c1nQ86hhejOAr7wtc286Q4gUXfwW4pGDlUubOCvdVv+45TBQVigh/p W4Ku9kWVFlADpIwpemvpZ87Lw3/OgiETwlZMdndp0S/SlnuLMGbctywBXf5Fwx0Hc1jk 0avUuf7fEAdV7zqlvqP6GgQ96zoa/AXSnA1IpqrEd3keo8rTI+I8Et9/1/PusPKRAP+t kmkA== X-Gm-Message-State: AOAM530UvrNVSnwUUQWbt1jcLPBviK9NYNCxHqpsQhQK1Ee6arvDtxTK xYXsOxEnzQ4+txalMF4ZpE0SgqfxBov99R+yTNDUmA== X-Google-Smtp-Source: ABdhPJy+hkEP3fgUKKEZMUde7RF69crwEKfDt8+G2Y7GA7U/nU7O2aX/hlWVr3o0IHjKcPx5txyiizmEUAD8pkdzPGs= X-Received: by 2002:a2e:6a04:: with SMTP id f4mr2032688ljc.255.1609935221170; Wed, 06 Jan 2021 04:13:41 -0800 (PST) MIME-Version: 1.0 References: <1609844255-25068-1-git-send-email-vijayenthiran.subramaniam@arm.com> <3be65049-b1bf-ea4d-94f9-105124e81cb9@arm.com> In-Reply-To: From: "Masahisa Kojima" Date: Wed, 6 Jan 2021 21:13:29 +0900 Message-ID: Subject: Re: [edk2-devel] [PATCH v4] ArmPlatformPkg: Enable support for flash in 64-bit address space To: Vijayenthiran Subramanian Cc: edk2-devel-groups-io , Ard Biesheuvel , Leif Lindholm , Thomas Abraham , Sami Mujawar Content-Type: text/plain; charset="UTF-8" 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 wrote: > > Hi Masahisa, > > On Tue, Jan 5, 2021 at 11:17 AM Vijayenthiran Subramanian > wrote: > > > > Hi Ard, > > > > On Tue, Jan 5, 2021 at 11:07 AM Ard Biesheuvel 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 > > > > Reviewed-by: Sami Mujawar > > > > --- > > > > > > 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.
> > > > +# Copyright (c) 2011 - 2021, Arm Limited. All rights reserved.
> > > > # > > > > # 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.
> > > > +# Copyright (c) 2011 - 2021, Arm Limited. All rights reserved.
> > > > # Copyright (c) 2020, Linaro, Ltd. All rights reserved.
> > > > # > > > > # 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.
> > > > + Copyright (c) 2011 - 2021, Arm Limited. All rights reserved.
> > > > > > > > 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.
> > > > + Copyright (c) 2011 - 2021, Arm Limited. All rights reserved.
> > > > > > > > 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 > > > > > > > > > > > > > > > > > > > > > >