Re: [edk2-devel] [edk2-platforms][PATCH v1 1/1] MinPlatformPkg: Add FspNvsBuffer compression option

["Michael Kubacki" <mikuback@linux.microsoft.com>]

Hi Isaac,

Here's v2 rebased on 9769bf28d1fc.

   https://edk2.groups.io/g/devel/message/92644

I also pushed a copy to this branch on my fork in case it helps.
https://github.com/makubacki/edk2-platforms/tree/add_mrcdata_compression

Thanks,
Michael


On 8/16/2022 4:57 PM, Oram, Isaac W wrote:
> Michael,
> 
> I have not been able to apply this patch.  Can you rebase and resend a V2?
> 
> Thanks,
> Isaac
> 
> -----Original Message-----
> From: mikuback@linux.microsoft.com <mikuback@linux.microsoft.com>
> Sent: Friday, August 5, 2022 12:44 PM
> To: devel@edk2.groups.io
> Cc: Chiu, Chasel <chasel.chiu@intel.com>; Desimone, Nathaniel L <nathaniel.l.desimone@intel.com>; Oram, Isaac W <isaac.w.oram@intel.com>; Gao, Liming <gaoliming@byosoft.com.cn>; Dong, Eric <eric.dong@intel.com>
> Subject: [edk2-platforms][PATCH v1 1/1] MinPlatformPkg: Add FspNvsBuffer compression option
> 
> From: Michael Kubacki <michael.kubacki@microsoft.com>
> 
> Adds a PCD called "PcdEnableCompressedFspNvsBuffer" that allows the "FspNvsBuffer" UEFI variable data to be saved as compressed data.
> 
> Especially due to the nature of the data saved in this variable, it compresses well. For example, it has been found to reduce ~63KB of data to ~13KB. Boot time impact has been found to be negligible.
> 
> The default value is FALSE to keep default behavior consistent.
> 
> Decompression can be performed on the variable data using the standard UefiDecompressLib.
> 
> Cc: Chasel Chiu <chasel.chiu@intel.com>
> Cc: Nate DeSimone <nathaniel.l.desimone@intel.com>
> Cc: Isaac Oram <isaac.w.oram@intel.com>
> Cc: Liming Gao <gaoliming@byosoft.com.cn>
> Cc: Eric Dong <eric.dong@intel.com>
> Signed-off-by: Michael Kubacki <michael.kubacki@microsoft.com>
> ---
>   Platform/Intel/MinPlatformPkg/FspWrapper/SaveMemoryConfig/SaveMemoryConfig.c   | 62 ++++++++++++++++----
>   Platform/Intel/MinPlatformPkg/FspWrapper/SaveMemoryConfig/SaveMemoryConfig.inf |  4 ++
>   Platform/Intel/MinPlatformPkg/MinPlatformPkg.dec                               |  6 ++
>   Platform/Intel/MinPlatformPkg/MinPlatformPkg.dsc                               |  1 +
>   4 files changed, 60 insertions(+), 13 deletions(-)
> 
> diff --git a/Platform/Intel/MinPlatformPkg/FspWrapper/SaveMemoryConfig/SaveMemoryConfig.c b/Platform/Intel/MinPlatformPkg/FspWrapper/SaveMemoryConfig/SaveMemoryConfig.c
> index 0215e8eeddfb..95b8cef8b32b 100644
> --- a/Platform/Intel/MinPlatformPkg/FspWrapper/SaveMemoryConfig/SaveMemoryConfig.c
> +++ b/Platform/Intel/MinPlatformPkg/FspWrapper/SaveMemoryConfig/SaveMemo
> +++ ryConfig.c
> @@ -3,6 +3,7 @@
>     exists, and saves the data to nvRAM.
>   
>   Copyright (c) 2017 - 2022, Intel Corporation. All rights reserved.<BR>
> +Copyright (c) Microsoft Corporation.<BR>
>   SPDX-License-Identifier: BSD-2-Clause-Patent
>   
>   **/
> @@ -10,6 +11,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent  #include <Base.h>  #include <Uefi.h>  #include <Library/BaseLib.h>
> +#include <Library/CompressLib.h>
>   #include <Library/UefiBootServicesTableLib.h>
>   #include <Library/UefiRuntimeServicesTableLib.h>
>   #include <Library/HobLib.h>
> @@ -19,6 +21,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent  #include <Library/BaseMemoryLib.h>  #include <Library/LargeVariableReadLib.h>  #include <Library/LargeVariableWriteLib.h>
> +#include <Library/PcdLib.h>
>   #include <Library/VariableWriteLib.h>
>   #include <Guid/FspNonVolatileStorageHob2.h>
>   
> @@ -38,20 +41,26 @@ SaveMemoryConfigEntryPoint (
>     IN EFI_SYSTEM_TABLE   *SystemTable
>     )
>   {
> -  EFI_STATUS        Status;
> -  EFI_HOB_GUID_TYPE *GuidHob;
> -  VOID              *HobData;
> -  VOID              *VariableData;
> -  UINTN             DataSize;
> -  UINTN             BufferSize;
> -  BOOLEAN           DataIsIdentical;
> +  EFI_STATUS         Status;
> +  EFI_HOB_GUID_TYPE  *GuidHob;
> +  VOID               *HobData;
> +  VOID               *VariableData;
> +  UINTN              DataSize;
> +  UINTN              BufferSize;
> +  BOOLEAN            DataIsIdentical;
> +  VOID               *CompressedData;
> +  UINT64             CompressedSize;
> +  UINTN              CompressedAllocationPages;
>   
> -  DataSize        = 0;
> -  BufferSize      = 0;
> -  VariableData    = NULL;
> -  GuidHob         = NULL;
> -  HobData         = NULL;
> -  DataIsIdentical = FALSE;
> +  DataSize                  = 0;
> +  BufferSize                = 0;
> +  VariableData              = NULL;
> +  GuidHob                   = NULL;
> +  HobData                   = NULL;
> +  DataIsIdentical           = FALSE;
> +  CompressedData            = NULL;
> +  CompressedSize            = 0;
> +  CompressedAllocationPages = 0;
>   
>     //
>     // Search for the Memory Configuration GUID HOB.  If it is not present, then @@ -73,6 +82,29 @@ SaveMemoryConfigEntryPoint (
>       }
>     }
>   
> +  if (PcdGetBool (PcdEnableCompressedFspNvsBuffer)) {
> +    if (DataSize > 0) {
> +      CompressedAllocationPages = EFI_SIZE_TO_PAGES (DataSize);
> +      CompressedData            = AllocatePages (CompressedAllocationPages);
> +      if (CompressedData == NULL) {
> +        DEBUG ((DEBUG_ERROR, "[%a] - Failed to allocate compressed data buffer.\n", __FUNCTION__));
> +        ASSERT_EFI_ERROR (EFI_OUT_OF_RESOURCES);
> +        return EFI_OUT_OF_RESOURCES;
> +      }
> +
> +      CompressedSize = EFI_PAGES_TO_SIZE (CompressedAllocationPages);
> +      Status         = Compress (HobData, DataSize, CompressedData, &CompressedSize);
> +      if (EFI_ERROR (Status)) {
> +        DEBUG ((DEBUG_ERROR, "[%a] - failed to compress data. Status = %r\n", __FUNCTION__, Status));
> +        ASSERT_EFI_ERROR (Status);
> +        return Status;
> +      }
> +    }
> +
> +    HobData  = CompressedData;
> +    DataSize = (UINTN)CompressedSize;
> +  }
> +
>     if (HobData != NULL) {
>       DEBUG ((DEBUG_INFO, "FspNvsHob.NvsDataLength:%d\n", DataSize));
>       DEBUG ((DEBUG_INFO, "FspNvsHob.NvsDataPtr   : 0x%x\n", HobData));
> @@ -136,6 +168,10 @@ SaveMemoryConfigEntryPoint (
>       DEBUG((DEBUG_ERROR, "Memory S3 Data HOB was not found\n"));
>     }
>   
> +  if (CompressedData != NULL) {
> +    FreePages (CompressedData, CompressedAllocationPages);  }
> +
>     //
>     // This driver does not produce any protocol services, so always unload it.
>     //
> diff --git a/Platform/Intel/MinPlatformPkg/FspWrapper/SaveMemoryConfig/SaveMemoryConfig.inf b/Platform/Intel/MinPlatformPkg/FspWrapper/SaveMemoryConfig/SaveMemoryConfig.inf
> index 61e85a658693..0f12deb131ca 100644
> --- a/Platform/Intel/MinPlatformPkg/FspWrapper/SaveMemoryConfig/SaveMemoryConfig.inf
> +++ b/Platform/Intel/MinPlatformPkg/FspWrapper/SaveMemoryConfig/SaveMemo
> +++ ryConfig.inf
> @@ -26,6 +26,7 @@ [LibraryClasses]
>     LargeVariableReadLib
>     LargeVariableWriteLib
>     BaseLib
> +  CompressLib
>   
>   [Packages]
>     MdePkg/MdePkg.dec
> @@ -45,6 +46,9 @@ [Guids]
>     gFspNonVolatileStorageHob2Guid                ## CONSUMES
>     gFspNvsBufferVariableGuid                     ## PRODUCES
>   
> +[Pcd]
> +  gMinPlatformPkgTokenSpaceGuid.PcdEnableCompressedFspNvsBuffer
> +
>   [Depex]
>     gEfiVariableArchProtocolGuid        AND
>     gEfiVariableWriteArchProtocolGuid
> \ No newline at end of file
> diff --git a/Platform/Intel/MinPlatformPkg/MinPlatformPkg.dec b/Platform/Intel/MinPlatformPkg/MinPlatformPkg.dec
> index db0a19066f11..49e1f070f9ab 100644
> --- a/Platform/Intel/MinPlatformPkg/MinPlatformPkg.dec
> +++ b/Platform/Intel/MinPlatformPkg/MinPlatformPkg.dec
> @@ -305,6 +305,12 @@ [PcdsFixedAtBuild, PcdsPatchableInModule, PcdsDynamic, PcdsDynamicEx]
>   
>     gMinPlatformPkgTokenSpaceGuid.PcdPlatformMemoryCheckLevel|0|UINT32|0x30000009
>   
> +  ## Controls whether the FSP NVS buffer is saved as compressed data.
> +  # Data compression can significantly reduce variable storage usage for FSP NVS buffer data.
> +  # Platforms that choose to compress the data will need to decompress
> + the variable data upon  # extraction.
> +
> + gMinPlatformPkgTokenSpaceGuid.PcdEnableCompressedFspNvsBuffer|FALSE|BO
> + OLEAN|0x30000010
> +
>     ## This PCD is to control which device is the potential trusted console input device.<BR><BR>
>     # For example:<BR>
>     # USB Short Form: UsbHID(0xFFFF,0xFFFF,0x1,0x1)<BR> diff --git a/Platform/Intel/MinPlatformPkg/MinPlatformPkg.dsc b/Platform/Intel/MinPlatformPkg/MinPlatformPkg.dsc
> index 09aa6fe4d51c..ae170f87d548 100644
> --- a/Platform/Intel/MinPlatformPkg/MinPlatformPkg.dsc
> +++ b/Platform/Intel/MinPlatformPkg/MinPlatformPkg.dsc
> @@ -106,6 +106,7 @@ [LibraryClasses.common.DXE_DRIVER]
>     FspWrapperPlatformLib|MinPlatformPkg/FspWrapper/Library/DxeFspWrapperPlatformLib/DxeFspWrapperPlatformLib.inf
>     TestPointCheckLib|MinPlatformPkg/Test/Library/TestPointCheckLib/DxeTestPointCheckLib.inf
>     TestPointLib|MinPlatformPkg/Test/Library/TestPointLib/DxeTestPointLib.inf
> +  CompressLib|MinPlatformPkg/Library/CompressLib/CompressLib.inf
>   
>   [LibraryClasses.common.DXE_SMM_DRIVER]
>     TestPointCheckLib|MinPlatformPkg/Test/Library/TestPointCheckLib/SmmTestPointCheckLib.inf
> --
> 2.28.0.windows.1
> 
> 
> 
> 
>