[edk2-devel] [edk2-platforms] [PATCH v3 3/4] MinPlatformPkg: Add LargeVariableReadLib
Nate DeSimone
nathaniel.l.desimone at intel.com
Wed Apr 7 03:05:31 UTC 2021
Hi Michael,
Thank you for the great feedback! I believe I have all of it addressed in V4.
Thanks,
Nate
-----Original Message-----
From: devel at edk2.groups.io <devel at edk2.groups.io> On Behalf Of Michael Kubacki
Sent: Tuesday, April 6, 2021 3:01 PM
To: devel at edk2.groups.io; Desimone, Nathaniel L <nathaniel.l.desimone at intel.com>
Cc: Chiu, Chasel <chasel.chiu at intel.com>; Liming Gao <gaoliming at byosoft.com.cn>; Dong, Eric <eric.dong at intel.com>; Michael Kubacki <michael.kubacki at microsoft.com>; Oram, Isaac W <isaac.w.oram at 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 at intel.com>
> Cc: Liming Gao <gaoliming at byosoft.com.cn>
> Cc: Eric Dong <eric.dong at intel.com>
> Cc: Michael Kubacki <michael.kubacki at microsoft.com>
> Cc: Isaac Oram <isaac.w.oram at intel.com>
> Signed-off-by: Nate DeSimone <nathaniel.l.desimone at intel.com>
> Reviewed-by: Isaac Oram <isaac.w.oram at 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;
> +}
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#73754): https://edk2.groups.io/g/devel/message/73754
Mute This Topic: https://groups.io/mt/81898861/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list