From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from linux.microsoft.com (linux.microsoft.com [13.77.154.182]) by mx.groups.io with SMTP id smtpd.web09.1532.1617746463328761522 for ; Tue, 06 Apr 2021 15:01:03 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@linux.microsoft.com header.s=default header.b=jjKYOjpY; spf=pass (domain: linux.microsoft.com, ip: 13.77.154.182, mailfrom: mikuback@linux.microsoft.com) Received: from [10.124.238.202] (unknown [167.220.2.74]) by linux.microsoft.com (Postfix) with ESMTPSA id CA7AF20B5681; Tue, 6 Apr 2021 15:01:02 -0700 (PDT) DKIM-Filter: OpenDKIM Filter v2.11.0 linux.microsoft.com CA7AF20B5681 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.microsoft.com; s=default; t=1617746462; bh=Iq/PRLtMR5EJp8WlMdwAtAoItUh62y40jULSwzwNRtA=; h=Subject:To:Cc:References:From:Date:In-Reply-To:From; b=jjKYOjpYJWLslvDA87YUSryedwlwk/05AVKz7433epFx1EzZ8a5/Ub2QlQyYYSd7E CU6iP9xT2OHXmG0tPoJiAFzb5d+4y7CU1SjhMhPyZhNK/CD/B0NMVNMe1z2//jSw7w 92MRXwSQo1jFRmTrvyDWtq23ZSGLxDNnNKZfyghU= Subject: Re: [edk2-devel] [edk2-platforms] [PATCH v3 3/4] MinPlatformPkg: Add LargeVariableReadLib To: devel@edk2.groups.io, nathaniel.l.desimone@intel.com Cc: Chasel Chiu , Liming Gao , Eric Dong , Michael Kubacki , Isaac Oram References: <20210406192411.6888-1-nathaniel.l.desimone@intel.com> <20210406192411.6888-4-nathaniel.l.desimone@intel.com> From: "Michael Kubacki" Message-ID: <88afacdc-4b7c-bf0f-dd88-591ef640330e@linux.microsoft.com> Date: Tue, 6 Apr 2021 15:01:03 -0700 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:78.0) Gecko/20100101 Thunderbird/78.9.0 MIME-Version: 1.0 In-Reply-To: <20210406192411.6888-4-nathaniel.l.desimone@intel.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit 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 > Cc: Liming Gao > Cc: Eric Dong > Cc: Michael Kubacki > Cc: Isaac Oram > Signed-off-by: Nate DeSimone > Reviewed-by: Isaac Oram > --- > .../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/LargeVariableReadLib.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/AuthVariableLibNull.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/VariablePolicyLib.inf > - VariablePolicyHelperLib|MdeModulePkg/Library/VariablePolicyHelperLib/VariablePolicyHelperLib.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/LargeVariableReadLib.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.
> + SPDX-License-Identifier: BSD-2-Clause-Patent > + > +**/ > + Similar comment to VariableReadLib/VariableWriteLib, header guard is missing. > +#include > + > +/** > + 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/BaseLargeVariableReadLib.inf b/Platform/Intel/MinPlatformPkg/Library/BaseLargeVariableReadLib/BaseLargeVariableReadLib.inf > new file mode 100644 > index 0000000000..822febd62b > --- /dev/null > +++ b/Platform/Intel/MinPlatformPkg/Library/BaseLargeVariableReadLib/BaseLargeVariableReadLib.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.
> +# > +# 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/LargeVariableReadLib.c b/Platform/Intel/MinPlatformPkg/Library/BaseLargeVariableReadLib/LargeVariableReadLib.c > new file mode 100644 > index 0000000000..115f3aeb17 > --- /dev/null > +++ b/Platform/Intel/MinPlatformPkg/Library/BaseLargeVariableReadLib/LargeVariableReadLib.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.
> + SPDX-License-Identifier: BSD-2-Clause-Patent > + > +**/ > + > +#include > +#include > +#include > +#include > +#include > + > +// > +// 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; > +} >