> -----Original Message-----
> From: Kubacki, Michael A
> Sent: Tuesday, October 15, 2019 7:30 AM
> To: devel@edk2.groups.io
> Cc: Bi, Dandan; Ard Biesheuvel; Dong, Eric; Laszlo Ersek; Gao, Liming; Kinney,
> Michael D; Ni, Ray; Wang, Jian J; Wu, Hao A; Yao, Jiewen
> Subject: [PATCH V4 08/10] MdeModulePkg/Variable: Add RT
> GetNextVariableName() cache support
>
> https://bugzilla.tianocore.org/show_bug.cgi?id=2220
>
> This change implements the Runtime Service GetNextVariableName()
> using the runtime cache in VariableSmmRuntimeDxe. Runtime Service
> calls to GetNextVariableName() will no longer trigger a SW SMI
> when gEfiMdeModulePkgTokenSpaceGuid.PcdEnableVariableRuntimeCache
> is set to TRUE (default value).
I think from the perspective of this patch, the default value for the feature
PCD is still FALSE, and the last patch of the series will actually switch
the default value to TRUE, right?
If my take is correct, I think we can drop the mention of "(default value)"
here. Other than this, the patch looks good to me:
Reviewed-by: Hao A Wu <hao.a...@intel.com>
Best Regards,
Hao Wu
>
> Overall system performance and stability will be improved by
> eliminating an SMI for these calls as they typically result in a
> relatively large number of invocations to retrieve all variable
> names in all variable stores present.
>
> Cc: Dandan Bi <dandan...@intel.com>
> Cc: Ard Biesheuvel <ard.biesheu...@linaro.org>
> Cc: Eric Dong <eric.d...@intel.com>
> Cc: Laszlo Ersek <ler...@redhat.com>
> Cc: Liming Gao <liming....@intel.com>
> Cc: Michael D Kinney <michael.d.kin...@intel.com>
> Cc: Ray Ni <ray...@intel.com>
> Cc: Jian J Wang <jian.j.w...@intel.com>
> Cc: Hao A Wu <hao.a...@intel.com>
> Cc: Jiewen Yao <jiewen....@intel.com>
> Signed-off-by: Michael Kubacki <michael.a.kuba...@intel.com>
> ---
>
> MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmmRuntimeDxe.
> c | 137 ++++++++++++++++++--
> 1 file changed, 128 insertions(+), 9 deletions(-)
>
> diff --git
> a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmmRuntimeDx
> e.c
> b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmmRuntimeDx
> e.c
> index e236ddff33..a795b9fcab 100644
> ---
> a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmmRuntimeDx
> e.c
> +++
> b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmmRuntimeDx
> e.c
> @@ -823,7 +823,7 @@ RuntimeServiceGetVariable (
> }
>
> /**
> - This code Finds the Next available variable.
> + Finds the next available variable in a runtime cache variable store.
>
> @param[in, out] VariableNameSize Size of the variable name.
> @param[in, out] VariableName Pointer to variable name.
> @@ -836,8 +836,81 @@ RuntimeServiceGetVariable (
>
> **/
> EFI_STATUS
> -EFIAPI
> -RuntimeServiceGetNextVariableName (
> +GetNextVariableNameInRuntimeCache (
> + IN OUT UINTN *VariableNameSize,
> + IN OUT CHAR16 *VariableName,
> + IN OUT EFI_GUID *VendorGuid
> + )
> +{
> + EFI_STATUS Status;
> + UINTN VarNameSize;
> + VARIABLE_HEADER *VariablePtr;
> + VARIABLE_STORE_HEADER
> *VariableStoreHeader[VariableStoreTypeMax];
> +
> + Status = EFI_NOT_FOUND;
> +
> + //
> + // The UEFI specification restricts Runtime Services callers from invoking
> the same or certain other Runtime Service
> + // functions prior to completion and return from a previous Runtime
> Service call. These restrictions prevent
> + // a GetVariable () or GetNextVariable () call from being issued until a
> prior
> call has returned. The runtime
> + // cache read lock should always be free when entering this function.
> + //
> + ASSERT (!mVariableRuntimeCacheReadLock);
> +
> + CheckForRuntimeCacheSync ();
> +
> + mVariableRuntimeCacheReadLock = TRUE;
> + if (!mVariableRuntimeCachePendingUpdate) {
> + //
> + // 0: Volatile, 1: HOB, 2: Non-Volatile.
> + // The index and attributes mapping must be kept in this order as
> FindVariable
> + // makes use of this mapping to implement search algorithm.
> + //
> + VariableStoreHeader[VariableStoreTypeVolatile] =
> mVariableRuntimeVolatileCacheBuffer;
> + VariableStoreHeader[VariableStoreTypeHob] =
> mVariableRuntimeHobCacheBuffer;
> + VariableStoreHeader[VariableStoreTypeNv] =
> mVariableRuntimeNvCacheBuffer;
> +
> + Status = VariableServiceGetNextVariableInternal (
> + VariableName,
> + VendorGuid,
> + VariableStoreHeader,
> + &VariablePtr,
> + mVariableAuthFormat
> + );
> + if (!EFI_ERROR (Status)) {
> + VarNameSize = NameSizeOfVariable (VariablePtr,
> mVariableAuthFormat);
> + ASSERT (VarNameSize != 0);
> + if (VarNameSize <= *VariableNameSize) {
> + CopyMem (VariableName, GetVariableNamePtr (VariablePtr,
> mVariableAuthFormat), VarNameSize);
> + CopyMem (VendorGuid, GetVendorGuidPtr (VariablePtr,
> mVariableAuthFormat), sizeof (EFI_GUID));
> + Status = EFI_SUCCESS;
> + } else {
> + Status = EFI_BUFFER_TOO_SMALL;
> + }
> +
> + *VariableNameSize = VarNameSize;
> + }
> + }
> + mVariableRuntimeCacheReadLock = FALSE;
> +
> + return Status;
> +}
> +
> +/**
> + Finds the next available variable in a SMM variable store.
> +
> + @param[in, out] VariableNameSize Size of the variable name.
> + @param[in, out] VariableName Pointer to variable name.
> + @param[in, out] VendorGuid Variable Vendor Guid.
> +
> + @retval EFI_INVALID_PARAMETER Invalid parameter.
> + @retval EFI_SUCCESS Find the specified variable.
> + @retval EFI_NOT_FOUND Not found.
> + @retval EFI_BUFFER_TO_SMALL DataSize is too small for the result.
> +
> +**/
> +EFI_STATUS
> +GetNextVariableNameInSmm (
> IN OUT UINTN *VariableNameSize,
> IN OUT CHAR16 *VariableName,
> IN OUT EFI_GUID *VendorGuid
> @@ -849,10 +922,6 @@ RuntimeServiceGetNextVariableName (
> UINTN OutVariableNameSize;
> UINTN InVariableNameSize;
>
> - if (VariableNameSize == NULL || VariableName == NULL || VendorGuid ==
> NULL) {
> - return EFI_INVALID_PARAMETER;
> - }
> -
> OutVariableNameSize = *VariableNameSize;
> InVariableNameSize = StrSize (VariableName);
> SmmGetNextVariableName = NULL;
> @@ -864,8 +933,6 @@ RuntimeServiceGetNextVariableName (
> return EFI_INVALID_PARAMETER;
> }
>
> - AcquireLockOnlyAtBootTime(&mVariableServicesLock);
> -
> //
> // Init the communicate buffer. The buffer data size is:
> // SMM_COMMUNICATE_HEADER_SIZE +
> SMM_VARIABLE_COMMUNICATE_HEADER_SIZE + PayloadSize.
> @@ -924,7 +991,59 @@ RuntimeServiceGetNextVariableName (
> CopyMem (VariableName, SmmGetNextVariableName->Name,
> SmmGetNextVariableName->NameSize);
>
> Done:
> + return Status;
> +}
> +
> +/**
> + This code Finds the Next available variable.
> +
> + @param[in, out] VariableNameSize Size of the variable name.
> + @param[in, out] VariableName Pointer to variable name.
> + @param[in, out] VendorGuid Variable Vendor Guid.
> +
> + @retval EFI_INVALID_PARAMETER Invalid parameter.
> + @retval EFI_SUCCESS Find the specified variable.
> + @retval EFI_NOT_FOUND Not found.
> + @retval EFI_BUFFER_TO_SMALL DataSize is too small for the result.
> +
> +**/
> +EFI_STATUS
> +EFIAPI
> +RuntimeServiceGetNextVariableName (
> + IN OUT UINTN *VariableNameSize,
> + IN OUT CHAR16 *VariableName,
> + IN OUT EFI_GUID *VendorGuid
> + )
> +{
> + EFI_STATUS Status;
> + UINTN MaxLen;
> +
> + Status = EFI_NOT_FOUND;
> +
> + if (VariableNameSize == NULL || VariableName == NULL || VendorGuid ==
> NULL) {
> + return EFI_INVALID_PARAMETER;
> + }
> +
> + //
> + // Calculate the possible maximum length of name string, including the Null
> terminator.
> + //
> + MaxLen = *VariableNameSize / sizeof (CHAR16);
> + if ((MaxLen == 0) || (StrnLenS (VariableName, MaxLen) == MaxLen)) {
> + //
> + // Null-terminator is not found in the first VariableNameSize bytes of
> the
> input VariableName buffer,
> + // follow spec to return EFI_INVALID_PARAMETER.
> + //
> + return EFI_INVALID_PARAMETER;
> + }
> +
> + AcquireLockOnlyAtBootTime (&mVariableServicesLock);
> + if (FeaturePcdGet (PcdEnableVariableRuntimeCache)) {
> + Status = GetNextVariableNameInRuntimeCache (VariableNameSize,
> VariableName, VendorGuid);
> + } else {
> + Status = GetNextVariableNameInSmm (VariableNameSize, VariableName,
> VendorGuid);
> + }
> ReleaseLockOnlyAtBootTime (&mVariableServicesLock);
> +
> return Status;
> }
>
> --
> 2.16.2.windows.1
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#49072): https://edk2.groups.io/g/devel/message/49072
Mute This Topic: https://groups.io/mt/34540093/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
