From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-lf1-f44.google.com (mail-lf1-f44.google.com [209.85.167.44]) by mx.groups.io with SMTP id smtpd.web08.3316.1609988455732578463 for ; Wed, 06 Jan 2021 19:00:56 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@linaro.org header.s=google header.b=Ztf1nDPi; spf=pass (domain: linaro.org, ip: 209.85.167.44, mailfrom: masahisa.kojima@linaro.org) Received: by mail-lf1-f44.google.com with SMTP id a12so11328098lfl.6 for ; Wed, 06 Jan 2021 19:00:55 -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=N2PdRPowtl80bLjHSJcp1VFw9xaL6H8laCfuiz115fk=; b=Ztf1nDPiV4MlNEgSp1OzesWAP/kI5F18q5wkhqBNGIO8TrprYrd6v/2oYsPoX/44ZL gg38xv+5p//B1J+7kKFwcvZbqepOFCsz0KEF3F4LVG0Lyg5s02bF1qzd/OHu+C+q4zu1 QHTHx9M+cM2HLF0PJRfV/7Hsh0BV50GI7vxebi0xmQN1cg6EPBuaNZIv00Ppneau/mWB yy73Auh6JQVbYtUrCCJdbvlKqkrJ9xLpmSceyRr7UFwMAco1vPjkyGHHjQfoYLavP1nj SI0SXGA1f130uxBYCpdDA1aJR9j5Tv9LG6wDbKcTebSz6rcOlvlfVaRzKOa2l/oDi++t e8ow== 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=N2PdRPowtl80bLjHSJcp1VFw9xaL6H8laCfuiz115fk=; b=VmnTGj9tSxVzTuUo2/xm3bgd5tKqma/rxRVWNANYolJkYdtqnkbcTqOu/DjZGMwei6 X+cj/WYYzpvpKHva+AsjJ/pNQmD/GhD9WVMeWC8OTC08pJ89xacrAUbqFE1nqTBneuaL Zs9K7ImIf7wKotLIek/+nHXRrnsr+z0KMJtvx/Q6m25tt9XfPFD900CpzUXRtrjJFGM3 p55TULQa3mcu7JOmnl1Go/PQ4lPFtV6ZNIDgxdURfNc7+vChcxtmjf9DSvLKQFxS88Qh qCCmz8LclK7ZAHVghhtDBe77gveFTugLzU1L1P17YP0cMSDj74phSSS+rIQXuLEBvAak 4/gw== X-Gm-Message-State: AOAM531Mp68hv+PqgJA9J7QEQOovPzH02ZHVAnW0zYlLf9WX6Yr/QbMS YwiI5vVcFeibUnXlgpHvmWUza3/IeIFXP2g7zgaeWg== X-Google-Smtp-Source: ABdhPJxuFEDMQ0L/1ZbW3aMJ9uRJJ1MgwVE4I275SNKsSmby/L4uqrjxutfRnPH2GRQq7mdFJSfePy6k2axR2x1apoM= X-Received: by 2002:a2e:b4f1:: with SMTP id s17mr3091781ljm.228.1609988453771; Wed, 06 Jan 2021 19:00:53 -0800 (PST) MIME-Version: 1.0 References: <1609923840-26526-1-git-send-email-vijayenthiran.subramaniam@arm.com> In-Reply-To: <1609923840-26526-1-git-send-email-vijayenthiran.subramaniam@arm.com> From: "Masahisa Kojima" Date: Thu, 7 Jan 2021 12:00:42 +0900 Message-ID: Subject: Re: [PATCH v5] ArmPlatformPkg: Enable support for flash in 64-bit address space To: Vijayenthiran Subramaniam Cc: edk2-devel-groups-io , Ard Biesheuvel , Sami Mujawar , Leif Lindholm , Thomas Abraham Content-Type: text/plain; charset="UTF-8" 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 On Wed, 6 Jan 2021 at 18:04, Vijayenthiran Subramaniam 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 > --- > > 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.
> +# 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 > 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.
> + Copyright (c) 2011 - 2021, Arm Limited. All rights reserved.
> Copyright (c) 2020, Linaro, Ltd. All rights reserved.
> > 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 >