Re: [PATCH V2 2/9] MdeModulePkg/Variable: Parameterize GetNextVariableEx() store list

["Kubacki, Michael A" <michael.a.kubacki@intel.com>]

Thanks for the feedback, I'll rename it back to VariableServiceGetNextVariableInternal ()
 and update the comments to refer to FindVariableEx () instead of FindVariable () in V3.

Thanks,
Michael

> -----Original Message-----
> From: Wu, Hao A <hao.a.wu@intel.com>
> Sent: Thursday, October 3, 2019 1:03 AM
> To: Kubacki, Michael A <michael.a.kubacki@intel.com>;
> 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
> 
> 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
>