From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: intel.com, ip: 134.134.136.31, mailfrom: hao.a.wu@intel.com) Received: from mga06.intel.com (mga06.intel.com [134.134.136.31]) by groups.io with SMTP; Thu, 03 Oct 2019 01:03:28 -0700 X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga001.jf.intel.com ([10.7.209.18]) by orsmga104.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 03 Oct 2019 01:03:27 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.67,251,1566889200"; d="scan'208";a="275632471" Received: from fmsmsx107.amr.corp.intel.com ([10.18.124.205]) by orsmga001.jf.intel.com with ESMTP; 03 Oct 2019 01:03:27 -0700 Received: from shsmsx107.ccr.corp.intel.com (10.239.4.96) by fmsmsx107.amr.corp.intel.com (10.18.124.205) with Microsoft SMTP Server (TLS) id 14.3.439.0; Thu, 3 Oct 2019 01:03:27 -0700 Received: from shsmsx104.ccr.corp.intel.com ([169.254.5.166]) by SHSMSX107.ccr.corp.intel.com ([169.254.9.33]) with mapi id 14.03.0439.000; Thu, 3 Oct 2019 16:03:24 +0800 From: "Wu, Hao A" To: "Kubacki, Michael A" , "devel@edk2.groups.io" CC: "Bi, Dandan" , Ard Biesheuvel , "Dong, Eric" , Laszlo Ersek , "Gao, Liming" , "Kinney, Michael D" , "Ni, Ray" , "Wang, Jian J" , "Yao, Jiewen" Subject: Re: [PATCH V2 2/9] MdeModulePkg/Variable: Parameterize GetNextVariableEx() store list Thread-Topic: [PATCH V2 2/9] MdeModulePkg/Variable: Parameterize GetNextVariableEx() store list Thread-Index: AQHVdZ61whsGybAqqEC3YhFUf6CSOadCKeKg Date: Thu, 3 Oct 2019 08:03:24 +0000 Message-ID: References: <20190928014717.31372-1-michael.a.kubacki@intel.com> <20190928014717.31372-3-michael.a.kubacki@intel.com> In-Reply-To: <20190928014717.31372-3-michael.a.kubacki@intel.com> Accept-Language: zh-CN, en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.239.127.40] MIME-Version: 1.0 Return-Path: hao.a.wu@intel.com Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable 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; Ki= nney, > Michael D; Ni, Ray; Wang, Jian J; Wu, Hao A; Yao, Jiewen > Subject: [PATCH V2 2/9] MdeModulePkg/Variable: Parameterize > GetNextVariableEx() store list >=20 > 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 "E= x" means here. 2. The origin function VariableServiceGetNextVariableInternal() does get us= ed in multiple source files (Variable.c & VariableExLib.c). I am not sure w= hat is the intention for such renaming. >=20 > Cc: Dandan Bi > Cc: Ard Biesheuvel > Cc: Eric Dong > Cc: Laszlo Ersek > Cc: Liming Gao > Cc: Michael D Kinney > Cc: Ray Ni > Cc: Jian J Wang > Cc: Hao A Wu > Cc: Jiewen Yao > Signed-off-by: Michael Kubacki > --- > 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(-) >=20 > 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 ( > ); >=20 > /** > - This code Finds the Next available variable. > + This code finds the next available variable. >=20 > 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. >=20 > - @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. >=20 > @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. >=20 > **/ > EFI_STATUS > EFIAPI > -VariableServiceGetNextVariableInternal ( > +GetNextVariableEx ( > IN CHAR16 *VariableName, > IN EFI_GUID *VendorGuid, > + IN VARIABLE_STORE_HEADER **VariableStoreList, > OUT VARIABLE_HEADER **VariablePtr > ); >=20 > 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]; >=20 > if (VariableNameSize =3D=3D NULL || VariableName =3D=3D NULL || Vendor= Guid =3D=3D > NULL) { > return EFI_INVALID_PARAMETER; > @@ -2377,7 +2378,16 @@ VariableServiceGetNextVariableName ( >=20 > AcquireLockOnlyAtBootTime(&mVariableModuleGlobal- > >VariableGlobal.VariableServicesLock); >=20 > - Status =3D 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] =3D > (VARIABLE_STORE_HEADER *) (UINTN) mVariableModuleGlobal- > >VariableGlobal.VolatileVariableBase; > + VariableStoreHeader[VariableStoreTypeHob] =3D > (VARIABLE_STORE_HEADER *) (UINTN) mVariableModuleGlobal- > >VariableGlobal.HobVariableBase; > + VariableStoreHeader[VariableStoreTypeNv] =3D mNvVariableCache; > + > + Status =3D GetNextVariableEx (VariableName, VendorGuid, > VariableStoreHeader, &VariablePtr); > if (!EFI_ERROR (Status)) { > VarNameSize =3D NameSizeOfVariable (VariablePtr); > ASSERT (VarNameSize !=3D 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]; >=20 > - Status =3D VariableServiceGetNextVariableInternal ( > + VariableStoreHeader[VariableStoreTypeVolatile] =3D > (VARIABLE_STORE_HEADER *) (UINTN) mVariableModuleGlobal- > >VariableGlobal.VolatileVariableBase; > + VariableStoreHeader[VariableStoreTypeHob] =3D > (VARIABLE_STORE_HEADER *) (UINTN) mVariableModuleGlobal- > >VariableGlobal.HobVariableBase; > + VariableStoreHeader[VariableStoreTypeNv] =3D mNvVariableCache; > + > + Status =3D 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 ( > } >=20 > /** > - This code Finds the Next available variable. > + This code finds the next available variable. >=20 > 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. >=20 > - @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. >=20 > @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]; >=20 > - Status =3D FindVariable (VariableName, VendorGuid, &Variable, > &mVariableModuleGlobal->VariableGlobal, FALSE); > + Status =3D EFI_NOT_FOUND; > + > + if (VariableStoreList =3D=3D NULL) { > + return EFI_INVALID_PARAMETER; > + } > + > + // Check if the variable exists in the given variable store list > + for (StoreType =3D (VARIABLE_STORE_TYPE) 0; StoreType < > VariableStoreTypeMax; StoreType++) { > + if (VariableStoreList[StoreType] =3D=3D NULL) { > + continue; > + } > + > + Variable.StartPtr =3D GetStartPointer (VariableStoreList[StoreType])= ; > + Variable.EndPtr =3D GetEndPointer (VariableStoreList[StoreType])= ; > + Variable.Volatile =3D (BOOLEAN) (StoreType =3D=3D VariableStoreTypeV= olatile); > + > + Status =3D FindVariableEx (VariableName, VendorGuid, FALSE, &Variabl= e); > + if (!EFI_ERROR (Status)) { > + break; > + } > + } > + > if (Variable.CurrPtr =3D=3D NULL || EFI_ERROR (Status)) { > // > // For VariableName is an empty string, FindVariable() will try to f= ind and Some description comments within this function that mention "FindVariable()= " can be updated. Since the calling of the FindVariable() has been replaced b= y the above 'for' loop. Best Regards, Hao Wu > return > @@ -527,39 +550,30 @@ VariableServiceGetNextVariableInternal ( >=20 > if (VariableName[0] !=3D 0) { > // > - // If variable name is not NULL, get next variable. > + // If variable name is not empty, get next variable. > // > Variable.CurrPtr =3D GetNextVariablePtr (Variable.CurrPtr); > } >=20 > - // > - // 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] =3D > (VARIABLE_STORE_HEADER *) (UINTN) mVariableModuleGlobal- > >VariableGlobal.VolatileVariableBase; > - VariableStoreHeader[VariableStoreTypeHob] =3D > (VARIABLE_STORE_HEADER *) (UINTN) mVariableModuleGlobal- > >VariableGlobal.HobVariableBase; > - VariableStoreHeader[VariableStoreTypeNv] =3D 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 =3D (VARIABLE_STORE_TYPE) 0; Type < VariableStoreTypeMax= ; > Type++) { > - if ((VariableStoreHeader[Type] !=3D NULL) && (Variable.StartPtr = =3D=3D > GetStartPointer (VariableStoreHeader[Type]))) { > + for (StoreType =3D (VARIABLE_STORE_TYPE) 0; StoreType < > VariableStoreTypeMax; StoreType++) { > + if ((VariableStoreList[StoreType] !=3D NULL) && (Variable.StartP= tr =3D=3D > GetStartPointer (VariableStoreList[StoreType]))) { > break; > } > } > - ASSERT (Type < VariableStoreTypeMax); > + ASSERT (StoreType < VariableStoreTypeMax); > // > // Switch to next storage > // > - for (Type++; Type < VariableStoreTypeMax; Type++) { > - if (VariableStoreHeader[Type] !=3D NULL) { > + for (StoreType++; StoreType < VariableStoreTypeMax; StoreType++) { > + if (VariableStoreList[StoreType] !=3D NULL) { > break; > } > } > @@ -568,12 +582,12 @@ VariableServiceGetNextVariableInternal ( > // 1. current storage is the last one, or > // 2. no further storage > // > - if (Type =3D=3D VariableStoreTypeMax) { > + if (StoreType =3D=3D VariableStoreTypeMax) { > Status =3D EFI_NOT_FOUND; > goto Done; > } > - Variable.StartPtr =3D GetStartPointer (VariableStoreHeader[Type]); > - Variable.EndPtr =3D GetEndPointer (VariableStoreHeader[Type]); > + Variable.StartPtr =3D GetStartPointer (VariableStoreList[StoreType= ]); > + Variable.EndPtr =3D GetEndPointer (VariableStoreList[StoreType= ]); > Variable.CurrPtr =3D Variable.StartPtr; > } >=20 > @@ -605,11 +619,11 @@ VariableServiceGetNextVariableInternal ( > // > // Don't return NV variable when HOB overrides it > // > - if ((VariableStoreHeader[VariableStoreTypeHob] !=3D NULL) && > (VariableStoreHeader[VariableStoreTypeNv] !=3D NULL) && > - (Variable.StartPtr =3D=3D GetStartPointer > (VariableStoreHeader[VariableStoreTypeNv])) > + if ((VariableStoreList[VariableStoreTypeHob] !=3D NULL) && > (VariableStoreList[VariableStoreTypeNv] !=3D NULL) && > + (Variable.StartPtr =3D=3D GetStartPointer > (VariableStoreList[VariableStoreTypeNv])) > ) { > - VariableInHob.StartPtr =3D GetStartPointer > (VariableStoreHeader[VariableStoreTypeHob]); > - VariableInHob.EndPtr =3D GetEndPointer > (VariableStoreHeader[VariableStoreTypeHob]); > + VariableInHob.StartPtr =3D GetStartPointer > (VariableStoreList[VariableStoreTypeHob]); > + VariableInHob.EndPtr =3D GetEndPointer > (VariableStoreList[VariableStoreTypeHob]); > Status =3D FindVariableEx ( > GetVariableNamePtr (Variable.CurrPtr), > GetVendorGuidPtr (Variable.CurrPtr), > -- > 2.16.2.windows.1