Re: [edk2-devel] [edk2-platforms] [PATCH v3 3/4] MinPlatformPkg: Add LargeVariableReadLib

["Nate DeSimone" <nathaniel.l.desimone@intel.com>]

Hi Michael,

Thank you for the great feedback! I believe I have all of it addressed in V4.

Thanks,
Nate

-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Michael Kubacki
Sent: Tuesday, April 6, 2021 3:01 PM
To: devel@edk2.groups.io; Desimone, Nathaniel L <nathaniel.l.desimone@intel.com>
Cc: Chiu, Chasel <chasel.chiu@intel.com>; Liming Gao <gaoliming@byosoft.com.cn>; Dong, Eric <eric.dong@intel.com>; Michael Kubacki <michael.kubacki@microsoft.com>; Oram, Isaac W <isaac.w.oram@intel.com>
Subject: Re: [edk2-devel] [edk2-platforms] [PATCH v3 3/4] MinPlatformPkg: Add LargeVariableReadLib

Hi Nate,

Feedback is inline.

Please carry over applicable feedback to LargeVariableWriteLib, I did not duplicate the response there.

Thanks,
Michael

On 4/6/2021 12:24 PM, Nate DeSimone wrote:
> LargeVariableReadLib is used to retrieve large data sets using the 
> UEFI Variable Services. At time of writting, most UEFI Variable 
> Services implementations to not allow more than 64KB of data to be 
> stored in a single UEFI variable. This library will split data sets 
> across multiple variables as needed.
> 
> It adds the GetLargeVariable() API to provide this service.
> 
> The primary use for this library is to create binary compatible 
> drivers and OpROMs which need to work both with TianoCore and other 
> UEFI PI implementations. When customizing and recompiling the platform 
> firmware image is possible, adjusting the value of PcdMaxVariableSize 
> may provide a simpler solution to this problem.
> 
> Cc: Chasel Chiu <chasel.chiu@intel.com>
> Cc: Liming Gao <gaoliming@byosoft.com.cn>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Michael Kubacki <michael.kubacki@microsoft.com>
> Cc: Isaac Oram <isaac.w.oram@intel.com>
> Signed-off-by: Nate DeSimone <nathaniel.l.desimone@intel.com>
> Reviewed-by: Isaac Oram <isaac.w.oram@intel.com>
> ---
>   .../Include/Dsc/CoreCommonLib.dsc             |   5 +-
>   .../Include/Library/LargeVariableReadLib.h    |  56 +++++
>   .../BaseLargeVariableReadLib.inf              |  50 +++++
>   .../LargeVariableReadLib.c                    | 199 ++++++++++++++++++
>   4 files changed, 308 insertions(+), 2 deletions(-)
>   create mode 100644 Platform/Intel/MinPlatformPkg/Include/Library/LargeVariableReadLib.h
>   create mode 100644 Platform/Intel/MinPlatformPkg/Library/BaseLargeVariableReadLib/BaseLargeVariableReadLib.inf
>   create mode 100644 
> Platform/Intel/MinPlatformPkg/Library/BaseLargeVariableReadLib/LargeVa
> riableReadLib.c
> 
> diff --git 
> a/Platform/Intel/MinPlatformPkg/Include/Dsc/CoreCommonLib.dsc 
> b/Platform/Intel/MinPlatformPkg/Include/Dsc/CoreCommonLib.dsc
> index cf2940cf02..5f2ad3f0f0 100644
> --- a/Platform/Intel/MinPlatformPkg/Include/Dsc/CoreCommonLib.dsc
> +++ b/Platform/Intel/MinPlatformPkg/Include/Dsc/CoreCommonLib.dsc
> @@ -135,13 +135,14 @@
>     VarCheckLib|MdeModulePkg/Library/VarCheckLib/VarCheckLib.inf
>     PlatformSecureLib|SecurityPkg/Library/PlatformSecureLibNull/PlatformSecureLibNull.inf
>     
> AuthVariableLib|MdeModulePkg/Library/AuthVariableLibNull/AuthVariableL
> ibNull.inf
> -
> +
>   !if gMinPlatformPkgTokenSpaceGuid.PcdUefiSecureBootEnable == TRUE
>     AuthVariableLib|SecurityPkg/Library/AuthVariableLib/AuthVariableLib.inf
>   !endif
>   
>     SafeIntLib|MdePkg/Library/BaseSafeIntLib/BaseSafeIntLib.inf
>     
> BmpSupportLib|MdeModulePkg/Library/BaseBmpSupportLib/BaseBmpSupportLib
> .inf
> +  
> + LargeVariableReadLib|MinPlatformPkg/Library/BaseLargeVariableReadLib
> + /BaseLargeVariableReadLib.inf
>   
>     #
>     # CryptLib
> @@ -165,4 +166,4 @@
>   
>     SmbusLib|MdePkg/Library/BaseSmbusLibNull/BaseSmbusLibNull.inf
>     
> VariablePolicyLib|MdeModulePkg/Library/VariablePolicyLib/VariablePolic
> yLib.inf
> -  
> VariablePolicyHelperLib|MdeModulePkg/Library/VariablePolicyHelperLib/V
> ariablePolicyHelperLib.inf
> \ No newline at end of file
> +  
> + VariablePolicyHelperLib|MdeModulePkg/Library/VariablePolicyHelperLib
> + /VariablePolicyHelperLib.inf
> diff --git 
> a/Platform/Intel/MinPlatformPkg/Include/Library/LargeVariableReadLib.h 
> b/Platform/Intel/MinPlatformPkg/Include/Library/LargeVariableReadLib.h
> new file mode 100644
> index 0000000000..5579492727
> --- /dev/null
> +++ b/Platform/Intel/MinPlatformPkg/Include/Library/LargeVariableReadL
> +++ ib.h
> @@ -0,0 +1,56 @@
> +/** @file
> +  Large Variable Read Lib
> +
> +  This library is used to retrieve large data sets using the UEFI 
> + Variable  Services. At time of writing, most UEFI Variable Services 
> + implementations to  not allow more than 64KB of data to be stored in 
> + a single UEFI variable. This  library will split data sets across multiple variables as needed.
> +
> +  In the case where more than one variable is needed to store the 
> + data, an  integer number will be added to the end of the variable 
> + name. This number  will be incremented for each variable as needed 
> + to retrieve the entire data  set.
> +
> +  The primary use for this library is to create binary compatible 
> + drivers  and OpROMs which need to work both with TianoCore and other 
> + UEFI PI  implementations. When customizing and recompiling the 
> + platform firmware image  is possible, adjusting the value of 
> + PcdMaxVariableSize may provide a simpler  solution to this problem.
> +
> +  Copyright (c) 2021, Intel Corporation. All rights reserved.<BR>
> +  SPDX-License-Identifier: BSD-2-Clause-Patent
> +
> +**/
> +

Similar comment to VariableReadLib/VariableWriteLib, header guard is missing.

> +#include <Uefi/UefiBaseType.h>
> +
> +/**
> +  Returns the value of a large variable.
> +
> +  @param[in]       VariableName  A Null-terminated string that is the name of the vendor's
> +                                 variable.
> +  @param[in]       VendorGuid    A unique identifier for the vendor.
> +  @param[in, out]  DataSize      On input, the size in bytes of the return Data buffer.
> +                                 On output the size of data returned in Data.
> +  @param[out]      Data          The buffer to return the contents of the variable. May be NULL
> +                                 with a zero DataSize in order to determine the size buffer needed.
> +
> +  @retval EFI_SUCCESS            The function completed successfully.
> +  @retval EFI_NOT_FOUND          The variable was not found.
> +  @retval EFI_BUFFER_TOO_SMALL   The DataSize is too small for the result.
> +  @retval EFI_INVALID_PARAMETER  VariableName is NULL.
> +  @retval EFI_INVALID_PARAMETER  VendorGuid is NULL.
> +  @retval EFI_INVALID_PARAMETER  DataSize is NULL.
> +  @retval EFI_INVALID_PARAMETER  The DataSize is not too small and Data is NULL.
> +  @retval EFI_DEVICE_ERROR       The variable could not be retrieved due to a hardware error.
> +  @retval EFI_SECURITY_VIOLATION The variable could not be retrieved due to an authentication failure.
> +
> +**/
> +EFI_STATUS
> +EFIAPI
> +GetLargeVariable (
> +  IN     CHAR16                      *VariableName,
> +  IN     EFI_GUID                    *VendorGuid,
> +  IN OUT UINTN                       *DataSize,
> +  OUT    VOID                        *Data           OPTIONAL
> +  );
> diff --git 
> a/Platform/Intel/MinPlatformPkg/Library/BaseLargeVariableReadLib/BaseL
> argeVariableReadLib.inf 
> b/Platform/Intel/MinPlatformPkg/Library/BaseLargeVariableReadLib/BaseL
> argeVariableReadLib.inf
> new file mode 100644
> index 0000000000..822febd62b
> --- /dev/null
> +++ b/Platform/Intel/MinPlatformPkg/Library/BaseLargeVariableReadLib/B
> +++ aseLargeVariableReadLib.inf
> @@ -0,0 +1,50 @@
> +## @file
> +# Component description file for Large Variable Read Library # # This 
> +library is used to retrieve large data sets using the UEFI Variable # 
> +Services. At time of writing, most UEFI Variable Services 
> +implementations to # not allow more than 64KB of data to be stored in 
> +a single UEFI variable. This # library will split data sets across multiple variables as needed.
> +#
> +# In the case where more than one variable is needed to store the 
> +data, an # integer number will be added to the end of the variable 
> +name. This number # will be incremented for each variable as needed 
> +to retrieve the entire data # set.
> +#
> +# The primary use for this library is to create binary compatible 
> +drivers # and OpROMs which need to work both with TianoCore and other 
> +UEFI PI # implementations. When customizing and recompiling the 
> +platform firmware image # is possible, adjusting the value of 
> +PcdMaxVariableSize may provide a simpler # solution to this problem.
> +#
> +# Copyright (c) 2021, Intel Corporation. All rights reserved.<BR> # # 
> +SPDX-License-Identifier: BSD-2-Clause-Patent # ##
> +
> +[Defines]
> +  INF_VERSION                    = 0x00010005
> +  BASE_NAME                      = BaseLargeVariableReadLib
> +  FILE_GUID                      = 4E9D7D31-A7A0-4004-AE93-D12F1AB08730
> +  MODULE_TYPE                    = BASE
> +  VERSION_STRING                 = 1.0
> +  LIBRARY_CLASS                  = LargeVariableReadLib
> +
> +#
> +#  VALID_ARCHITECTURES           = IA32 X64 EBC
> +#
> +
> +[Sources]
> +  LargeVariableReadLib.c
> +
> +[Packages]
> +  MdePkg/MdePkg.dec
> +  MinPlatformPkg/MinPlatformPkg.dec
> +
> +[LibraryClasses]
> +  BaseLib
> +  BaseMemoryLib
> +  DebugLib
> +  PrintLib
> +  VariableReadLib
> diff --git 
> a/Platform/Intel/MinPlatformPkg/Library/BaseLargeVariableReadLib/Large
> VariableReadLib.c 
> b/Platform/Intel/MinPlatformPkg/Library/BaseLargeVariableReadLib/Large
> VariableReadLib.c
> new file mode 100644
> index 0000000000..115f3aeb17
> --- /dev/null
> +++ b/Platform/Intel/MinPlatformPkg/Library/BaseLargeVariableReadLib/L
> +++ argeVariableReadLib.c
> @@ -0,0 +1,199 @@
> +/** @file
> +  Large Variable Read Lib
> +
> +  This library is used to retrieve large data sets using the UEFI 
> + Variable  Services. At time of writing, most UEFI Variable Services 
> + implementations to  not allow more than 64KB of data to be stored in 
> + a single UEFI variable. This  library will split data sets across multiple variables as needed.
> +
> +  In the case where more than one variable is needed to store the 
> + data, an  integer number will be added to the end of the variable 
> + name. This number  will be incremented for each variable as needed 
> + to retrieve the entire data  set.
> +
> +  Copyright (c) 2021, Intel Corporation. All rights reserved.<BR>
> +  SPDX-License-Identifier: BSD-2-Clause-Patent
> +
> +**/
> +
> +#include <Library/BaseLib.h>
> +#include <Library/BaseMemoryLib.h>
> +#include <Library/DebugLib.h>
> +#include <Library/PrintLib.h>
> +#include <Library/VariableReadLib.h>
> +
> +//
> +// 1024 was choosen because this is the size of the SMM communication 
> +buffer // used by VariableDxeSmm to transfer the VariableName from 
> +DXE to SMM. Choosing // the same size will prevent this library from 
> +limiting variable names any // more than the MdeModulePkg implementation of UEFI Variable Services does.
> +//
> +#define MAX_VARIABLE_NAME_SIZE      1024
> +

It would be nice to share a definition of these values with LargeVariableWriteLib to prevent the two from potentially getting out of sync.

> +//
> +// The 2012 Windows Hardware Requirements specified a minimum 
> +variable size of // 32KB. By setting the maximum allowed number of 
> +variables to 0x20000, this // allows up to 4GB of data to be stored 
> +on most UEFI implementations in // existence. Older UEFI 
> +implementations were known to only provide 8KB per // variable. In 
> +this case, up to 1GB can be stored. Since 1GB vastly exceeds the // 
> +size of any known NvStorage FV, choosing this number should effectively // enable all available NvStorage space to be used to store the given data.
> +//
> +#define MAX_VARIABLE_SPLIT          131072  // 0x20000
> +
> +//
> +// There are 6 digits in the number 131072, which means the length of 
> +the string // representation of this number will be at most 6 characters long.
> +//
> +#define MAX_VARIABLE_SPLIT_DIGITS   6
> +
> +/**
> +  Returns the value of a large variable.
> +
> +  @param[in]       VariableName  A Null-terminated string that is the name of the vendor's
> +                                 variable.
> +  @param[in]       VendorGuid    A unique identifier for the vendor.
> +  @param[in, out]  DataSize      On input, the size in bytes of the return Data buffer.
> +                                 On output the size of data returned in Data.
> +  @param[out]      Data          The buffer to return the contents of the variable. May be NULL
> +                                 with a zero DataSize in order to determine the size buffer needed.
> +
> +  @retval EFI_SUCCESS            The function completed successfully.
> +  @retval EFI_NOT_FOUND          The variable was not found.
> +  @retval EFI_BUFFER_TOO_SMALL   The DataSize is too small for the result.
> +  @retval EFI_INVALID_PARAMETER  VariableName is NULL.
> +  @retval EFI_INVALID_PARAMETER  VendorGuid is NULL.
> +  @retval EFI_INVALID_PARAMETER  DataSize is NULL.
> +  @retval EFI_INVALID_PARAMETER  The DataSize is not too small and Data is NULL.
> +  @retval EFI_DEVICE_ERROR       The variable could not be retrieved due to a hardware error.
> +  @retval EFI_SECURITY_VIOLATION The variable could not be retrieved due to an authentication failure.
> +
> +**/
> +EFI_STATUS
> +EFIAPI
> +GetLargeVariable (
> +  IN     CHAR16                      *VariableName,
> +  IN     EFI_GUID                    *VendorGuid,
> +  IN OUT UINTN                       *DataSize,
> +  OUT    VOID                        *Data           OPTIONAL
> +  )
> +{
> +  CHAR16        TempVariableName[MAX_VARIABLE_NAME_SIZE];
> +  EFI_STATUS    Status;
> +  UINTN         TotalSize;
> +  UINTN         VarDataSize;
> +  UINTN         Index;
> +  UINTN         VariableSize;
> +  UINTN         BytesRemaining;
> +  UINT8         *OffsetPtr;
> +
> +  VarDataSize = 0;
> +
> +  //
> +  // First check if a variable with the given name exists  //  Status 
> + = VarLibGetVariable (VariableName, VendorGuid, NULL, &VarDataSize, 
> + NULL);  if (Status == EFI_BUFFER_TOO_SMALL) {
> +    if (*DataSize >= VarDataSize) {
> +      if (Data == NULL) {
> +        Status = EFI_INVALID_PARAMETER;
> +        goto Done;
> +      }
> +      DEBUG ((DEBUG_INFO, "GetLargeVariable: Single Variable 
> + Found\n"));

This is somewhat subjective but do you think the message above should remain DEBUG_INFO? It seems like DEBUG_VERBOSE might be appropriate (same to counterpart messages in other places).

> +      Status = VarLibGetVariable (VariableName, VendorGuid, NULL, DataSize, Data);
> +      goto Done;
> +    } else {
> +      *DataSize = VarDataSize;
> +      Status = EFI_BUFFER_TOO_SMALL;
> +      goto Done;
> +    }
> +
> +  } else if (Status == EFI_NOT_FOUND) {
> +    //
> +    // Check if the first variable of a multi-variable set exists
> +    //
> +    if (StrLen (VariableName) >= (MAX_VARIABLE_NAME_SIZE - MAX_VARIABLE_SPLIT_DIGITS)) {
> +      DEBUG ((DEBUG_ERROR, "GetLargeVariable: Variable name too long\n"));
> +      Status = EFI_OUT_OF_RESOURCES;
> +      goto Done;
> +    }
> +
> +    VarDataSize = 0;
> +    Index       = 0;
> +    ZeroMem (TempVariableName, MAX_VARIABLE_NAME_SIZE);
> +    UnicodeSPrint (TempVariableName, MAX_VARIABLE_NAME_SIZE, L"%s%d", VariableName, Index);
> +    Status = VarLibGetVariable (TempVariableName, VendorGuid, NULL, 
> + &VarDataSize, NULL);
> +
> +    if (Status == EFI_BUFFER_TOO_SMALL) {
> +      //
> +      // The first variable exists. Calculate the total size of all the variables.
> +      //
> +      DEBUG ((DEBUG_INFO, "GetLargeVariable: Multiple Variables Found\n"));
> +      TotalSize = 0;
> +      for (Index = 0; Index < MAX_VARIABLE_SPLIT; Index++) {
> +        VarDataSize = 0;
> +        ZeroMem (TempVariableName, MAX_VARIABLE_NAME_SIZE);
> +        UnicodeSPrint (TempVariableName, MAX_VARIABLE_NAME_SIZE, L"%s%d", VariableName, Index);
> +        Status = VarLibGetVariable (TempVariableName, VendorGuid, NULL, &VarDataSize, NULL);
> +        if (Status != EFI_BUFFER_TOO_SMALL) {
> +          break;
> +        }
> +        TotalSize += VarDataSize;
> +      }
> +      DEBUG ((DEBUG_INFO, "TotalSize = %d, NumVariables = %d\n", 
> + TotalSize, Index));
> +
> +      //
> +      // Check if the user provided a large enough buffer
> +      //
> +      if (*DataSize >= TotalSize) {
> +        if (Data == NULL) {
> +          Status = EFI_INVALID_PARAMETER;
> +          goto Done;
> +        }
> +
> +        //
> +        // Read the data from all variables
> +        //
> +        OffsetPtr       = (UINT8 *) Data;
> +        BytesRemaining  = *DataSize;
> +        for (Index = 0; Index < MAX_VARIABLE_SPLIT; Index++) {
> +          VarDataSize = 0;
> +          ZeroMem (TempVariableName, MAX_VARIABLE_NAME_SIZE);
> +          UnicodeSPrint (TempVariableName, MAX_VARIABLE_NAME_SIZE, L"%s%d", VariableName, Index);
> +          VariableSize = BytesRemaining;
> +          DEBUG ((DEBUG_INFO, "Reading %s, Guid = %g,", TempVariableName, VendorGuid));
> +          Status = VarLibGetVariable (TempVariableName, VendorGuid, NULL, &VariableSize, (VOID *) OffsetPtr);
> +          DEBUG ((DEBUG_INFO, " Size %d\n", VariableSize));
> +          if (EFI_ERROR (Status)) {
> +            if (Status == EFI_NOT_FOUND) {
> +              DEBUG ((DEBUG_INFO, "No more variables found\n"));
> +              Status = EFI_SUCCESS;   // The end has been reached
> +            }
> +            goto Done;
> +          }
> +
> +          if (VariableSize < BytesRemaining) {
> +            BytesRemaining -= VariableSize;
> +            OffsetPtr += VariableSize;
> +          } else {
> +            DEBUG ((DEBUG_INFO, "All data has been read\n"));
> +            BytesRemaining = 0;
> +            break;
> +          }
> +        }   //End of for loop
> +
> +        goto Done;
> +      } else {
> +        *DataSize = TotalSize;
> +        Status = EFI_BUFFER_TOO_SMALL;
> +        goto Done;
> +      }
> +    } else {
> +      Status = EFI_NOT_FOUND;
> +    }
> +  }
> +
> +Done:
> +  DEBUG ((DEBUG_ERROR, "GetLargeVariable: Status = %r\n", Status));
> +  return Status;
> +}
>