From: "Sami Mujawar" <sami.mujawar@arm.com>
To: devel@edk2.groups.io, mikuback@linux.microsoft.com
Cc: Jian J Wang <jian.j.wang@intel.com>,
Hao A Wu <hao.a.wu@intel.com>,
Liming Gao <gaoliming@byosoft.com.cn>,
nd@arm.com
Subject: Re: [edk2-devel] [PATCH v4 3/8] MdeModulePkg/Variable: Consume Variable Flash Info
Date: Mon, 25 Apr 2022 17:05:36 +0100 [thread overview]
Message-ID: <04f4a048-4e39-c754-a4f7-10b3db5812bc@arm.com> (raw)
In-Reply-To: <20220412162940.4978-4-mikuback@linux.microsoft.com>
Hi Michael,
Thank you for this patch.
These changes look good to me.
Reviewed-by: Sami Mujawar <sami.mujawar@arm.com>
Regards,
Sami Mujawar
On 12/04/2022 05:29 pm, Michael Kubacki via groups.io wrote:
> From: Michael Kubacki <michael.kubacki@microsoft.com>
>
> REF:https://bugzilla.tianocore.org/show_bug.cgi?id=3479
>
> Updates VariableRuntimeDxe, VariableSmm, and VariableStandaloneMm
> to acquire variable flash information from the Variable Flash
> Information library.
>
> Cc: Jian J Wang <jian.j.wang@intel.com>
> Cc: Hao A Wu <hao.a.wu@intel.com>
> Cc: Liming Gao <gaoliming@byosoft.com.cn>
> Signed-off-by: Michael Kubacki <michael.kubacki@microsoft.com>
> ---
> MdeModulePkg/Universal/Variable/Pei/Variable.c | 14 +++++++++-----
> MdeModulePkg/Universal/Variable/RuntimeDxe/VariableDxe.c | 16 ++++++++++++----
> MdeModulePkg/Universal/Variable/RuntimeDxe/VariableNonVolatile.c | 14 ++++++++++----
> MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.c | 17 +++++++++++++----
> MdeModulePkg/Universal/Variable/Pei/Variable.h | 2 ++
> MdeModulePkg/Universal/Variable/Pei/VariablePei.inf | 5 ++---
> MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.h | 7 ++-----
> MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf | 5 ++---
> MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.inf | 5 ++---
> MdeModulePkg/Universal/Variable/RuntimeDxe/VariableStandaloneMm.inf | 5 ++---
> 10 files changed, 56 insertions(+), 34 deletions(-)
>
> diff --git a/MdeModulePkg/Universal/Variable/Pei/Variable.c b/MdeModulePkg/Universal/Variable/Pei/Variable.c
> index b36dd0de67b2..26a4c73b45a5 100644
> --- a/MdeModulePkg/Universal/Variable/Pei/Variable.c
> +++ b/MdeModulePkg/Universal/Variable/Pei/Variable.c
> @@ -567,11 +567,13 @@ GetVariableStore (
> OUT VARIABLE_STORE_INFO *StoreInfo
> )
> {
> + EFI_STATUS Status;
> EFI_HOB_GUID_TYPE *GuidHob;
> EFI_FIRMWARE_VOLUME_HEADER *FvHeader;
> VARIABLE_STORE_HEADER *VariableStoreHeader;
> EFI_PHYSICAL_ADDRESS NvStorageBase;
> UINT32 NvStorageSize;
> + UINT64 NvStorageSize64;
> FAULT_TOLERANT_WRITE_LAST_WRITE_DATA *FtwLastWriteData;
> UINT32 BackUpOffset;
>
> @@ -591,11 +593,13 @@ GetVariableStore (
> // Emulated non-volatile variable mode is not enabled.
> //
>
> - NvStorageSize = PcdGet32 (PcdFlashNvStorageVariableSize);
> - NvStorageBase = (EFI_PHYSICAL_ADDRESS)(PcdGet64 (PcdFlashNvStorageVariableBase64) != 0 ?
> - PcdGet64 (PcdFlashNvStorageVariableBase64) :
> - PcdGet32 (PcdFlashNvStorageVariableBase)
> - );
> + Status = GetVariableFlashNvStorageInfo (&NvStorageBase, &NvStorageSize64);
> + ASSERT_EFI_ERROR (Status);
> +
> + Status = SafeUint64ToUint32 (NvStorageSize64, &NvStorageSize);
> + // This driver currently assumes the size will be UINT32 so assert the value is safe for now.
> + ASSERT_EFI_ERROR (Status);
> +
> ASSERT (NvStorageBase != 0);
>
> //
> diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableDxe.c b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableDxe.c
> index 03fec3048dc4..d5c409c914d1 100644
> --- a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableDxe.c
> +++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableDxe.c
> @@ -423,6 +423,8 @@ FtwNotificationEvent (
> EFI_PHYSICAL_ADDRESS VariableStoreBase;
> UINT64 VariableStoreLength;
> UINTN FtwMaxBlockSize;
> + UINT32 NvStorageVariableSize;
> + UINT64 NvStorageVariableSize64;
>
> //
> // Ensure FTW protocol is installed.
> @@ -432,14 +434,20 @@ FtwNotificationEvent (
> return;
> }
>
> + Status = GetVariableFlashNvStorageInfo (&NvStorageVariableBase, &NvStorageVariableSize64);
> + ASSERT_EFI_ERROR (Status);
> +
> + Status = SafeUint64ToUint32 (NvStorageVariableSize64, &NvStorageVariableSize);
> + // This driver currently assumes the size will be UINT32 so assert the value is safe for now.
> + ASSERT_EFI_ERROR (Status);
> +
> + VariableStoreBase = NvStorageVariableBase + mNvFvHeaderCache->HeaderLength;
> +
> Status = FtwProtocol->GetMaxBlockSize (FtwProtocol, &FtwMaxBlockSize);
> if (!EFI_ERROR (Status)) {
> - ASSERT (PcdGet32 (PcdFlashNvStorageVariableSize) <= FtwMaxBlockSize);
> + ASSERT (NvStorageVariableSize <= FtwMaxBlockSize);
> }
>
> - NvStorageVariableBase = NV_STORAGE_VARIABLE_BASE;
> - VariableStoreBase = NvStorageVariableBase + mNvFvHeaderCache->HeaderLength;
> -
> //
> // Let NonVolatileVariableBase point to flash variable store base directly after FTW ready.
> //
> diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableNonVolatile.c b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableNonVolatile.c
> index 5e9d40b67ac2..9e2d8fe0fe0c 100644
> --- a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableNonVolatile.c
> +++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableNonVolatile.c
> @@ -142,6 +142,7 @@ InitRealNonVolatileVariableStore (
> EFI_PHYSICAL_ADDRESS NvStorageBase;
> UINT8 *NvStorageData;
> UINT32 NvStorageSize;
> + UINT64 NvStorageSize64;
> FAULT_TOLERANT_WRITE_LAST_WRITE_DATA *FtwLastWriteData;
> UINT32 BackUpOffset;
> UINT32 BackUpSize;
> @@ -153,19 +154,24 @@ InitRealNonVolatileVariableStore (
>
> mVariableModuleGlobal->FvbInstance = NULL;
>
> + Status = GetVariableFlashNvStorageInfo (&NvStorageBase, &NvStorageSize64);
> + ASSERT_EFI_ERROR (Status);
> +
> + Status = SafeUint64ToUint32 (NvStorageSize64, &NvStorageSize);
> + // This driver currently assumes the size will be UINT32 so assert the value is safe for now.
> + ASSERT_EFI_ERROR (Status);
> +
> + ASSERT (NvStorageBase != 0);
> +
> //
> // Allocate runtime memory used for a memory copy of the FLASH region.
> // Keep the memory and the FLASH in sync as updates occur.
> //
> - NvStorageSize = PcdGet32 (PcdFlashNvStorageVariableSize);
> NvStorageData = AllocateRuntimeZeroPool (NvStorageSize);
> if (NvStorageData == NULL) {
> return EFI_OUT_OF_RESOURCES;
> }
>
> - NvStorageBase = NV_STORAGE_VARIABLE_BASE;
> - ASSERT (NvStorageBase != 0);
> -
> //
> // Copy NV storage data to the memory buffer.
> //
> diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.c b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.c
> index 517cae7b00f8..5253c328dcd9 100644
> --- a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.c
> +++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.c
> @@ -1084,6 +1084,8 @@ SmmFtwNotificationEvent (
> EFI_SMM_FAULT_TOLERANT_WRITE_PROTOCOL *FtwProtocol;
> EFI_PHYSICAL_ADDRESS NvStorageVariableBase;
> UINTN FtwMaxBlockSize;
> + UINT32 NvStorageVariableSize;
> + UINT64 NvStorageVariableSize64;
>
> if (mVariableModuleGlobal->FvbInstance != NULL) {
> return EFI_SUCCESS;
> @@ -1097,14 +1099,21 @@ SmmFtwNotificationEvent (
> return Status;
> }
>
> + Status = GetVariableFlashNvStorageInfo (&NvStorageVariableBase, &NvStorageVariableSize64);
> + ASSERT_EFI_ERROR (Status);
> +
> + Status = SafeUint64ToUint32 (NvStorageVariableSize64, &NvStorageVariableSize);
> + // This driver currently assumes the size will be UINT32 so assert the value is safe for now.
> + ASSERT_EFI_ERROR (Status);
> +
> + ASSERT (NvStorageVariableBase != 0);
> + VariableStoreBase = NvStorageVariableBase + mNvFvHeaderCache->HeaderLength;
> +
> Status = FtwProtocol->GetMaxBlockSize (FtwProtocol, &FtwMaxBlockSize);
> if (!EFI_ERROR (Status)) {
> - ASSERT (PcdGet32 (PcdFlashNvStorageVariableSize) <= FtwMaxBlockSize);
> + ASSERT (NvStorageVariableSize <= FtwMaxBlockSize);
> }
>
> - NvStorageVariableBase = NV_STORAGE_VARIABLE_BASE;
> - VariableStoreBase = NvStorageVariableBase + mNvFvHeaderCache->HeaderLength;
> -
> //
> // Let NonVolatileVariableBase point to flash variable store base directly after FTW ready.
> //
> diff --git a/MdeModulePkg/Universal/Variable/Pei/Variable.h b/MdeModulePkg/Universal/Variable/Pei/Variable.h
> index 7f9ad5bfc357..51effbf79987 100644
> --- a/MdeModulePkg/Universal/Variable/Pei/Variable.h
> +++ b/MdeModulePkg/Universal/Variable/Pei/Variable.h
> @@ -20,6 +20,8 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
> #include <Library/BaseMemoryLib.h>
> #include <Library/PeiServicesTablePointerLib.h>
> #include <Library/PeiServicesLib.h>
> +#include <Library/SafeIntLib.h>
> +#include <Library/VariableFlashInfoLib.h>
>
> #include <Guid/VariableFormat.h>
> #include <Guid/VariableIndexTable.h>
> diff --git a/MdeModulePkg/Universal/Variable/Pei/VariablePei.inf b/MdeModulePkg/Universal/Variable/Pei/VariablePei.inf
> index 7cbdd2385e8f..7264a24bdf71 100644
> --- a/MdeModulePkg/Universal/Variable/Pei/VariablePei.inf
> +++ b/MdeModulePkg/Universal/Variable/Pei/VariablePei.inf
> @@ -39,6 +39,8 @@ [LibraryClasses]
> DebugLib
> PeiServicesTablePointerLib
> PeiServicesLib
> + SafeIntLib
> + VariableFlashInfoLib
>
> [Guids]
> ## CONSUMES ## GUID # Variable store header
> @@ -59,9 +61,6 @@ [Ppis]
> gEfiPeiReadOnlyVariable2PpiGuid ## PRODUCES
>
> [Pcd]
> - gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableBase ## SOMETIMES_CONSUMES
> - gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableBase64 ## CONSUMES
> - gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableSize ## CONSUMES
> gEfiMdeModulePkgTokenSpaceGuid.PcdEmuVariableNvModeEnable ## SOMETIMES_CONSUMES
>
> [Depex]
> diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.h b/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.h
> index 31e408976a35..a668abb82b15 100644
> --- a/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.h
> +++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.h
> @@ -31,6 +31,8 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
> #include <Library/MemoryAllocationLib.h>
> #include <Library/AuthVariableLib.h>
> #include <Library/VarCheckLib.h>
> +#include <Library/VariableFlashInfoLib.h>
> +#include <Library/SafeIntLib.h>
> #include <Guid/GlobalVariable.h>
> #include <Guid/EventGroup.h>
> #include <Guid/VariableFormat.h>
> @@ -40,11 +42,6 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
>
> #include "PrivilegePolymorphic.h"
>
> -#define NV_STORAGE_VARIABLE_BASE (EFI_PHYSICAL_ADDRESS)\
> - (PcdGet64 (PcdFlashNvStorageVariableBase64) != 0 ? \
> - PcdGet64 (PcdFlashNvStorageVariableBase64) : \
> - PcdGet32 (PcdFlashNvStorageVariableBase))
> -
> #define EFI_VARIABLE_ATTRIBUTES_MASK (EFI_VARIABLE_NON_VOLATILE |\
> EFI_VARIABLE_BOOTSERVICE_ACCESS | \
> EFI_VARIABLE_RUNTIME_ACCESS | \
> diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf
> index c9434df631ee..3858adf6739d 100644
> --- a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf
> +++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableRuntimeDxe.inf
> @@ -71,8 +71,10 @@ [LibraryClasses]
> TpmMeasurementLib
> AuthVariableLib
> VarCheckLib
> + VariableFlashInfoLib
> VariablePolicyLib
> VariablePolicyHelperLib
> + SafeIntLib
>
> [Protocols]
> gEfiFirmwareVolumeBlockProtocolGuid ## CONSUMES
> @@ -125,9 +127,6 @@ [Guids]
> gEfiImageSecurityDatabaseGuid
>
> [Pcd]
> - gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableSize ## CONSUMES
> - gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableBase ## SOMETIMES_CONSUMES
> - gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableBase64 ## CONSUMES
> gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVariableSize ## CONSUMES
> gEfiMdeModulePkgTokenSpaceGuid.PcdMaxAuthVariableSize ## CONSUMES
> gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVolatileVariableSize ## CONSUMES
> diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.inf b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.inf
> index eaa97a01c6e5..8c552b87e080 100644
> --- a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.inf
> +++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.inf
> @@ -80,8 +80,10 @@ [LibraryClasses]
> AuthVariableLib
> VarCheckLib
> UefiBootServicesTableLib
> + VariableFlashInfoLib
> VariablePolicyLib
> VariablePolicyHelperLib
> + SafeIntLib
>
> [Protocols]
> gEfiSmmFirmwareVolumeBlockProtocolGuid ## CONSUMES
> @@ -127,9 +129,6 @@ [Guids]
> gEdkiiVarErrorFlagGuid
>
> [Pcd]
> - gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableSize ## CONSUMES
> - gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableBase ## SOMETIMES_CONSUMES
> - gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableBase64 ## CONSUMES
> gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVariableSize ## CONSUMES
> gEfiMdeModulePkgTokenSpaceGuid.PcdMaxAuthVariableSize ## CONSUMES
> gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVolatileVariableSize ## CONSUMES
> diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableStandaloneMm.inf b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableStandaloneMm.inf
> index d8c4f77e7f1f..f09bed40cf51 100644
> --- a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableStandaloneMm.inf
> +++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableStandaloneMm.inf
> @@ -73,9 +73,11 @@ [LibraryClasses]
> HobLib
> MemoryAllocationLib
> MmServicesTableLib
> + SafeIntLib
> StandaloneMmDriverEntryPoint
> SynchronizationLib
> VarCheckLib
> + VariableFlashInfoLib
> VariablePolicyLib
> VariablePolicyHelperLib
>
> @@ -120,9 +122,6 @@ [Guids]
> gEdkiiVarErrorFlagGuid
>
> [Pcd]
> - gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableBase ## SOMETIMES_CONSUMES
> - gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableBase64 ## CONSUMES
> - gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageVariableSize ## CONSUMES
> gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVariableSize ## CONSUMES
> gEfiMdeModulePkgTokenSpaceGuid.PcdMaxAuthVariableSize ## CONSUMES
> gEfiMdeModulePkgTokenSpaceGuid.PcdMaxVolatileVariableSize ## CONSUMES
next prev parent reply other threads:[~2022-04-25 16:05 UTC|newest]
Thread overview: 20+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-04-12 16:29 [PATCH v4 0/8] Add Variable Flash Info HOB Michael Kubacki
2022-04-12 16:29 ` [PATCH v4 1/8] MdeModulePkg: " Michael Kubacki
2022-04-25 16:05 ` [edk2-devel] " Sami Mujawar
2022-04-12 16:29 ` [PATCH v4 2/8] MdeModulePkg/VariableFlashInfoLib: Add initial library Michael Kubacki
2022-04-25 16:05 ` [edk2-devel] " Sami Mujawar
2022-04-25 22:21 ` Michael Kubacki
2022-04-12 16:29 ` [PATCH v4 3/8] MdeModulePkg/Variable: Consume Variable Flash Info Michael Kubacki
2022-04-25 16:05 ` Sami Mujawar [this message]
2022-04-12 16:29 ` [PATCH v4 4/8] MdeModulePkg/FaultTolerantWrite: " Michael Kubacki
2022-04-25 16:05 ` [edk2-devel] " Sami Mujawar
2022-04-12 16:29 ` [PATCH v4 5/8] ArmVirtPkg/ArmVirt.dsc.inc: Add VariableFlashInfoLib Michael Kubacki
2022-04-25 16:06 ` Sami Mujawar
2022-04-12 16:29 ` [PATCH v4 6/8] EmulatorPkg: " Michael Kubacki
2022-04-12 16:29 ` [PATCH v4 7/8] OvmfPkg: " Michael Kubacki
2022-04-19 19:26 ` Rebecca Cran
2022-04-12 16:29 ` [PATCH v4 8/8] UefiPayloadPkg: " Michael Kubacki
2022-04-14 3:02 ` [edk2-devel] " Guo Dong
2022-04-20 7:43 ` [PATCH v4 0/8] Add Variable Flash Info HOB Ard Biesheuvel
[not found] ` <16E5331AC6E8EA6F.9041@groups.io>
2022-04-20 22:51 ` [edk2-devel] [PATCH v4 6/8] EmulatorPkg: Add VariableFlashInfoLib Michael Kubacki
2022-04-21 1:51 ` Abner Chang
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=04f4a048-4e39-c754-a4f7-10b3db5812bc@arm.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