From: "Wu, Hao A" <hao.a.wu@intel.com>
To: "Kubacki, Michael A" <michael.a.kubacki@intel.com>,
"devel@edk2.groups.io" <devel@edk2.groups.io>
Cc: "Bi, Dandan" <dandan.bi@intel.com>,
Ard Biesheuvel <ard.biesheuvel@linaro.org>,
"Dong, Eric" <eric.dong@intel.com>,
Laszlo Ersek <lersek@redhat.com>,
"Gao, Liming" <liming.gao@intel.com>,
"Kinney, Michael D" <michael.d.kinney@intel.com>,
"Ni, Ray" <ray.ni@intel.com>,
"Wang, Jian J" <jian.j.wang@intel.com>,
"Yao, Jiewen" <jiewen.yao@intel.com>
Subject: Re: [PATCH V4 08/10] MdeModulePkg/Variable: Add RT GetNextVariableName() cache support
Date: Wed, 16 Oct 2019 07:56:56 +0000 [thread overview]
Message-ID: <B80AF82E9BFB8E4FBD8C89DA810C6A093C948058@SHSMSX104.ccr.corp.intel.com> (raw)
In-Reply-To: <20191014233001.33024-9-michael.a.kubacki@intel.com>
> -----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.wu@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.bi@intel.com>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Liming Gao <liming.gao@intel.com>
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Jian J Wang <jian.j.wang@intel.com>
> Cc: Hao A Wu <hao.a.wu@intel.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Signed-off-by: Michael Kubacki <michael.a.kubacki@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
next prev parent reply other threads:[~2019-10-16 7:57 UTC|newest]
Thread overview: 33+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-10-14 23:29 [PATCH V4 00/10] UEFI Variable SMI Reduction Kubacki, Michael A
2019-10-14 23:29 ` [PATCH V4 01/10] MdeModulePkg/Variable: Consolidate common parsing functions Kubacki, Michael A
2019-10-16 7:52 ` Wu, Hao A
2019-10-14 23:29 ` [PATCH V4 02/10] MdeModulePkg/Variable: Parameterize GetNextVariableInternal () stores Kubacki, Michael A
2019-10-16 7:53 ` Wu, Hao A
2019-10-14 23:29 ` [PATCH V4 03/10] MdeModulePkg/Variable: Parameterize VARIABLE_INFO_ENTRY buffer Kubacki, Michael A
2019-10-16 7:54 ` Wu, Hao A
2019-10-14 23:29 ` [PATCH V4 04/10] MdeModulePkg/Variable: Parameterize auth status in VariableParsing Kubacki, Michael A
2019-10-17 1:01 ` Wu, Hao A
2019-10-17 1:41 ` Kubacki, Michael A
2019-10-17 1:49 ` Wu, Hao A
2019-10-14 23:29 ` [PATCH V4 05/10] MdeModulePkg/Variable: Add a file for NV variable functions Kubacki, Michael A
2019-10-16 7:55 ` Wu, Hao A
2019-10-14 23:29 ` [PATCH V4 06/10] MdeModulePkg VariableInfo: Always consider RT DXE and SMM stats Kubacki, Michael A
2019-10-16 7:56 ` Wu, Hao A
2019-10-14 23:29 ` [PATCH V4 07/10] MdeModulePkg/Variable: Add RT GetVariable() cache support Kubacki, Michael A
2019-10-16 6:46 ` Wang, Jian J
[not found] ` <15CE0DB2DE3EB613.1607@groups.io>
2019-10-16 6:54 ` [edk2-devel] " Wang, Jian J
2019-10-17 1:24 ` Kubacki, Michael A
2019-10-17 1:47 ` Wang, Jian J
2019-10-16 7:56 ` Wu, Hao A
2019-10-16 16:44 ` Kubacki, Michael A
2019-10-17 14:23 ` Wang, Jian J
2019-10-17 17:44 ` Kubacki, Michael A
2019-10-14 23:29 ` [PATCH V4 08/10] MdeModulePkg/Variable: Add RT GetNextVariableName() " Kubacki, Michael A
2019-10-16 7:56 ` Wu, Hao A [this message]
2019-10-14 23:30 ` [PATCH V4 09/10] OvmfPkg: Disable variable runtime cache Kubacki, Michael A
2019-10-15 7:32 ` Laszlo Ersek
2019-10-14 23:30 ` [PATCH V4 10/10] MdeModulePkg: Enable variable runtime cache by default Kubacki, Michael A
2019-10-15 7:33 ` Laszlo Ersek
2019-10-16 7:57 ` Wu, Hao A
2019-10-15 0:49 ` [PATCH V4 00/10] UEFI Variable SMI Reduction Liming Gao
2019-10-15 16:15 ` Kubacki, Michael A
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=B80AF82E9BFB8E4FBD8C89DA810C6A093C948058@SHSMSX104.ccr.corp.intel.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