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 V2 2/9] MdeModulePkg/Variable: Parameterize GetNextVariableEx() store list
Date: Thu, 3 Oct 2019 08:03:24 +0000 [thread overview]
Message-ID: <B80AF82E9BFB8E4FBD8C89DA810C6A093C940D21@SHSMSX104.ccr.corp.intel.com> (raw)
In-Reply-To: <20190928014717.31372-3-michael.a.kubacki@intel.com>
A couple of inline comments below:
> -----Original Message-----
> From: Kubacki, Michael A
> Sent: Saturday, September 28, 2019 9:47 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 V2 2/9] MdeModulePkg/Variable: Parameterize
> GetNextVariableEx() store list
>
> The majority of logic related to GetNextVariableName () is currently
> implemented in VariableServiceGetNextVariableInternal (). The list
> of variable stores to search for the given variable name and variable
> GUID is defined in the function body. This change renames the function
> to GetNextVariableEx () since the function is no longer internal to
> a specific source file and adds a new parameter so that the caller
> must pass in the list of variable stores to be used in the variable
> search.
I am not sure if 'GetNextVariableEx' is a good name for the function, since:
1. There is no function named GetNextVariable(), so it is not clear what "Ex"
means here.
2. The origin function VariableServiceGetNextVariableInternal() does get used
in multiple source files (Variable.c & VariableExLib.c). I am not sure what
is the intention for such renaming.
>
> 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/VariableParsing.h | 15 ++-
> -
> MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c | 12 ++-
> MdeModulePkg/Universal/Variable/RuntimeDxe/VariableExLib.c | 8 +-
> MdeModulePkg/Universal/Variable/RuntimeDxe/VariableParsing.c | 78
> ++++++++++++--------
> 4 files changed, 73 insertions(+), 40 deletions(-)
>
> diff --git
> a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableParsing.h
> b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableParsing.h
> index 9d77c4916c..0d231511ea 100644
> --- a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableParsing.h
> +++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableParsing.h
> @@ -248,27 +248,30 @@ FindVariableEx (
> );
>
> /**
> - This code Finds the Next available variable.
> + This code finds the next available variable.
>
> Caution: This function may receive untrusted input.
> This function may be invoked in SMM mode. This function will do basic
> validation, before parse the data.
>
> - @param[in] VariableName Pointer to variable name.
> - @param[in] VendorGuid Variable Vendor Guid.
> - @param[out] VariablePtr Pointer to variable header address.
> + @param[in] VariableName Pointer to variable name.
> + @param[in] VendorGuid Variable Vendor Guid.
> + @param[in] VariableStoreList A list of variable stores that should be used
> to get the next variable.
> + The maximum number of entries is the max value of
> VARIABLE_STORE_TYPE.
> + @param[out] VariablePtr Pointer to variable header address.
>
> @retval EFI_SUCCESS The function completed successfully.
> @retval EFI_NOT_FOUND The next variable was not found.
> - @retval EFI_INVALID_PARAMETER If VariableName is not an empty string,
> while VendorGuid is NULL.
> + @retval EFI_INVALID_PARAMETER If VariableName is nt an empty string,
> while VendorGuid is NULL.
> @retval EFI_INVALID_PARAMETER The input values of VariableName and
> VendorGuid are not a name and
> GUID of an existing variable.
>
> **/
> EFI_STATUS
> EFIAPI
> -VariableServiceGetNextVariableInternal (
> +GetNextVariableEx (
> IN CHAR16 *VariableName,
> IN EFI_GUID *VendorGuid,
> + IN VARIABLE_STORE_HEADER **VariableStoreList,
> OUT VARIABLE_HEADER **VariablePtr
> );
>
> diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
> b/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
> index 76536308e6..816e8f7b8f 100644
> --- a/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
> +++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/Variable.c
> @@ -2358,6 +2358,7 @@ VariableServiceGetNextVariableName (
> UINTN MaxLen;
> UINTN VarNameSize;
> VARIABLE_HEADER *VariablePtr;
> + VARIABLE_STORE_HEADER
> *VariableStoreHeader[VariableStoreTypeMax];
>
> if (VariableNameSize == NULL || VariableName == NULL || VendorGuid ==
> NULL) {
> return EFI_INVALID_PARAMETER;
> @@ -2377,7 +2378,16 @@ VariableServiceGetNextVariableName (
>
> AcquireLockOnlyAtBootTime(&mVariableModuleGlobal-
> >VariableGlobal.VariableServicesLock);
>
> - Status = VariableServiceGetNextVariableInternal (VariableName,
> VendorGuid, &VariablePtr);
> + //
> + // 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] =
> (VARIABLE_STORE_HEADER *) (UINTN) mVariableModuleGlobal-
> >VariableGlobal.VolatileVariableBase;
> + VariableStoreHeader[VariableStoreTypeHob] =
> (VARIABLE_STORE_HEADER *) (UINTN) mVariableModuleGlobal-
> >VariableGlobal.HobVariableBase;
> + VariableStoreHeader[VariableStoreTypeNv] = mNvVariableCache;
> +
> + Status = GetNextVariableEx (VariableName, VendorGuid,
> VariableStoreHeader, &VariablePtr);
> if (!EFI_ERROR (Status)) {
> VarNameSize = NameSizeOfVariable (VariablePtr);
> ASSERT (VarNameSize != 0);
> diff --git a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableExLib.c
> b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableExLib.c
> index dc78f68fa9..232d9ffe25 100644
> --- a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableExLib.c
> +++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableExLib.c
> @@ -98,10 +98,16 @@ VariableExLibFindNextVariable (
> EFI_STATUS Status;
> VARIABLE_HEADER *VariablePtr;
> AUTHENTICATED_VARIABLE_HEADER *AuthVariablePtr;
> + VARIABLE_STORE_HEADER
> *VariableStoreHeader[VariableStoreTypeMax];
>
> - Status = VariableServiceGetNextVariableInternal (
> + VariableStoreHeader[VariableStoreTypeVolatile] =
> (VARIABLE_STORE_HEADER *) (UINTN) mVariableModuleGlobal-
> >VariableGlobal.VolatileVariableBase;
> + VariableStoreHeader[VariableStoreTypeHob] =
> (VARIABLE_STORE_HEADER *) (UINTN) mVariableModuleGlobal-
> >VariableGlobal.HobVariableBase;
> + VariableStoreHeader[VariableStoreTypeNv] = mNvVariableCache;
> +
> + Status = GetNextVariableEx (
> VariableName,
> VendorGuid,
> + VariableStoreHeader,
> &VariablePtr
> );
> if (EFI_ERROR (Status)) {
> diff --git
> a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableParsing.c
> b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableParsing.c
> index 7de0a90772..9bc5369a90 100644
> --- a/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableParsing.c
> +++ b/MdeModulePkg/Universal/Variable/RuntimeDxe/VariableParsing.c
> @@ -476,14 +476,16 @@ FindVariableEx (
> }
>
> /**
> - This code Finds the Next available variable.
> + This code finds the next available variable.
>
> Caution: This function may receive untrusted input.
> This function may be invoked in SMM mode. This function will do basic
> validation, before parse the data.
>
> - @param[in] VariableName Pointer to variable name.
> - @param[in] VendorGuid Variable Vendor Guid.
> - @param[out] VariablePtr Pointer to variable header address.
> + @param[in] VariableName Pointer to variable name.
> + @param[in] VendorGuid Variable Vendor Guid.
> + @param[in] VariableStoreList A list of variable stores that should be used
> to get the next variable.
> + The maximum number of entries is the max value of
> VARIABLE_STORE_TYPE.
> + @param[out] VariablePtr Pointer to variable header address.
>
> @retval EFI_SUCCESS The function completed successfully.
> @retval EFI_NOT_FOUND The next variable was not found.
> @@ -494,20 +496,41 @@ FindVariableEx (
> **/
> EFI_STATUS
> EFIAPI
> -VariableServiceGetNextVariableInternal (
> +GetNextVariableEx (
> IN CHAR16 *VariableName,
> IN EFI_GUID *VendorGuid,
> + IN VARIABLE_STORE_HEADER **VariableStoreList,
> OUT VARIABLE_HEADER **VariablePtr
> )
> {
> - VARIABLE_STORE_TYPE Type;
> + EFI_STATUS Status;
> + VARIABLE_STORE_TYPE StoreType;
> VARIABLE_POINTER_TRACK Variable;
> VARIABLE_POINTER_TRACK VariableInHob;
> VARIABLE_POINTER_TRACK VariablePtrTrack;
> - EFI_STATUS Status;
> - VARIABLE_STORE_HEADER *VariableStoreHeader[VariableStoreTypeMax];
>
> - Status = FindVariable (VariableName, VendorGuid, &Variable,
> &mVariableModuleGlobal->VariableGlobal, FALSE);
> + Status = EFI_NOT_FOUND;
> +
> + if (VariableStoreList == NULL) {
> + return EFI_INVALID_PARAMETER;
> + }
> +
> + // Check if the variable exists in the given variable store list
> + for (StoreType = (VARIABLE_STORE_TYPE) 0; StoreType <
> VariableStoreTypeMax; StoreType++) {
> + if (VariableStoreList[StoreType] == NULL) {
> + continue;
> + }
> +
> + Variable.StartPtr = GetStartPointer (VariableStoreList[StoreType]);
> + Variable.EndPtr = GetEndPointer (VariableStoreList[StoreType]);
> + Variable.Volatile = (BOOLEAN) (StoreType == VariableStoreTypeVolatile);
> +
> + Status = FindVariableEx (VariableName, VendorGuid, FALSE, &Variable);
> + if (!EFI_ERROR (Status)) {
> + break;
> + }
> + }
> +
> if (Variable.CurrPtr == NULL || EFI_ERROR (Status)) {
> //
> // For VariableName is an empty string, FindVariable() will try to find and
Some description comments within this function that mention "FindVariable()"
can be updated. Since the calling of the FindVariable() has been replaced by
the above 'for' loop.
Best Regards,
Hao Wu
> return
> @@ -527,39 +550,30 @@ VariableServiceGetNextVariableInternal (
>
> if (VariableName[0] != 0) {
> //
> - // If variable name is not NULL, get next variable.
> + // If variable name is not empty, get next variable.
> //
> Variable.CurrPtr = GetNextVariablePtr (Variable.CurrPtr);
> }
>
> - //
> - // 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] =
> (VARIABLE_STORE_HEADER *) (UINTN) mVariableModuleGlobal-
> >VariableGlobal.VolatileVariableBase;
> - VariableStoreHeader[VariableStoreTypeHob] =
> (VARIABLE_STORE_HEADER *) (UINTN) mVariableModuleGlobal-
> >VariableGlobal.HobVariableBase;
> - VariableStoreHeader[VariableStoreTypeNv] = mNvVariableCache;
> -
> while (TRUE) {
> //
> - // Switch from Volatile to HOB, to Non-Volatile.
> + // Switch to the next variable store if needed
> //
> while (!IsValidVariableHeader (Variable.CurrPtr, Variable.EndPtr)) {
> //
> // Find current storage index
> //
> - for (Type = (VARIABLE_STORE_TYPE) 0; Type < VariableStoreTypeMax;
> Type++) {
> - if ((VariableStoreHeader[Type] != NULL) && (Variable.StartPtr ==
> GetStartPointer (VariableStoreHeader[Type]))) {
> + for (StoreType = (VARIABLE_STORE_TYPE) 0; StoreType <
> VariableStoreTypeMax; StoreType++) {
> + if ((VariableStoreList[StoreType] != NULL) && (Variable.StartPtr ==
> GetStartPointer (VariableStoreList[StoreType]))) {
> break;
> }
> }
> - ASSERT (Type < VariableStoreTypeMax);
> + ASSERT (StoreType < VariableStoreTypeMax);
> //
> // Switch to next storage
> //
> - for (Type++; Type < VariableStoreTypeMax; Type++) {
> - if (VariableStoreHeader[Type] != NULL) {
> + for (StoreType++; StoreType < VariableStoreTypeMax; StoreType++) {
> + if (VariableStoreList[StoreType] != NULL) {
> break;
> }
> }
> @@ -568,12 +582,12 @@ VariableServiceGetNextVariableInternal (
> // 1. current storage is the last one, or
> // 2. no further storage
> //
> - if (Type == VariableStoreTypeMax) {
> + if (StoreType == VariableStoreTypeMax) {
> Status = EFI_NOT_FOUND;
> goto Done;
> }
> - Variable.StartPtr = GetStartPointer (VariableStoreHeader[Type]);
> - Variable.EndPtr = GetEndPointer (VariableStoreHeader[Type]);
> + Variable.StartPtr = GetStartPointer (VariableStoreList[StoreType]);
> + Variable.EndPtr = GetEndPointer (VariableStoreList[StoreType]);
> Variable.CurrPtr = Variable.StartPtr;
> }
>
> @@ -605,11 +619,11 @@ VariableServiceGetNextVariableInternal (
> //
> // Don't return NV variable when HOB overrides it
> //
> - if ((VariableStoreHeader[VariableStoreTypeHob] != NULL) &&
> (VariableStoreHeader[VariableStoreTypeNv] != NULL) &&
> - (Variable.StartPtr == GetStartPointer
> (VariableStoreHeader[VariableStoreTypeNv]))
> + if ((VariableStoreList[VariableStoreTypeHob] != NULL) &&
> (VariableStoreList[VariableStoreTypeNv] != NULL) &&
> + (Variable.StartPtr == GetStartPointer
> (VariableStoreList[VariableStoreTypeNv]))
> ) {
> - VariableInHob.StartPtr = GetStartPointer
> (VariableStoreHeader[VariableStoreTypeHob]);
> - VariableInHob.EndPtr = GetEndPointer
> (VariableStoreHeader[VariableStoreTypeHob]);
> + VariableInHob.StartPtr = GetStartPointer
> (VariableStoreList[VariableStoreTypeHob]);
> + VariableInHob.EndPtr = GetEndPointer
> (VariableStoreList[VariableStoreTypeHob]);
> Status = FindVariableEx (
> GetVariableNamePtr (Variable.CurrPtr),
> GetVendorGuidPtr (Variable.CurrPtr),
> --
> 2.16.2.windows.1
next prev parent reply other threads:[~2019-10-03 8:03 UTC|newest]
Thread overview: 45+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-09-28 1:47 [PATCH V2 0/9] UEFI Variable SMI Reduction Kubacki, Michael A
2019-09-28 1:47 ` [PATCH V2 1/9] MdeModulePkg/Variable: Consolidate common parsing functions Kubacki, Michael A
2019-10-03 8:03 ` Wu, Hao A
2019-10-03 17:35 ` Kubacki, Michael A
2019-10-08 2:11 ` Wu, Hao A
2019-10-08 21:53 ` Kubacki, Michael A
2019-10-08 6:07 ` Wang, Jian J
2019-10-08 22:00 ` Kubacki, Michael A
2019-09-28 1:47 ` [PATCH V2 2/9] MdeModulePkg/Variable: Parameterize GetNextVariableEx() store list Kubacki, Michael A
2019-10-03 8:03 ` Wu, Hao A [this message]
2019-10-03 18:04 ` Kubacki, Michael A
2019-09-28 1:47 ` [PATCH V2 3/9] MdeModulePkg/Variable: Parameterize VARIABLE_INFO_ENTRY buffer Kubacki, Michael A
2019-10-03 8:03 ` Wu, Hao A
2019-10-03 18:05 ` Kubacki, Michael A
2019-10-08 2:11 ` [edk2-devel] " Wu, Hao A
2019-10-08 21:49 ` Kubacki, Michael A
2019-09-28 1:47 ` [PATCH V2 4/9] MdeModulePkg/Variable: Add local auth status in VariableParsing Kubacki, Michael A
2019-10-03 8:04 ` [edk2-devel] " Wu, Hao A
2019-10-03 18:35 ` Kubacki, Michael A
2019-10-16 7:55 ` Wu, Hao A
2019-10-16 16:37 ` Kubacki, Michael A
2019-10-17 1:00 ` Wu, Hao A
2019-09-28 1:47 ` [PATCH V2 5/9] MdeModulePkg/Variable: Add a file for NV variable functions Kubacki, Michael A
2019-10-03 8:04 ` Wu, Hao A
2019-10-03 18:43 ` Kubacki, Michael A
2019-09-28 1:47 ` [PATCH V2 6/9] MdeModulePkg VariableInfo: Always consider RT DXE and SMM stats Kubacki, Michael A
2019-10-03 8:04 ` Wu, Hao A
2019-09-28 1:47 ` [PATCH V2 7/9] MdeModulePkg/Variable: Add RT GetVariable() cache support Kubacki, Michael A
2019-10-03 8:04 ` Wu, Hao A
2019-10-03 11:00 ` Laszlo Ersek
2019-10-03 20:53 ` Kubacki, Michael A
2019-10-03 21:53 ` Kubacki, Michael A
2019-10-03 22:01 ` Michael D Kinney
2019-10-03 23:31 ` Kubacki, Michael A
2019-10-04 6:50 ` Laszlo Ersek
2019-10-04 16:48 ` Kubacki, Michael A
2019-10-04 6:38 ` Laszlo Ersek
2019-10-04 16:48 ` Kubacki, Michael A
2019-10-08 2:12 ` Wu, Hao A
2019-09-28 1:47 ` [PATCH V2 8/9] MdeModulePkg/Variable: Add RT GetNextVariableName() " Kubacki, Michael A
2019-10-03 8:04 ` Wu, Hao A
2019-10-03 18:52 ` Kubacki, Michael A
2019-10-03 18:59 ` [edk2-devel] " Andrew Fish
2019-10-03 20:12 ` Kubacki, Michael A
2019-09-28 1:47 ` [PATCH V2 9/9] MdeModulePkg/VariableSmm: Remove unused SMI handler functions 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=B80AF82E9BFB8E4FBD8C89DA810C6A093C940D21@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