Re: [edk2-platforms:PATCH V1] MinPlatformPkg/SaveMemoryConfig: Support NVS Data compression.

["Isaac Oram" <isaac.w.oram@intel.com>]

The proposed implementation is fine and I will reviewed-by and push if that is the desired direction.

My question is if we generally like the design of doing compression in common MinPlatform code, decompression in board specific FSP wrapper code.  The alternative design is to do compression and decompression inside the FSP.  This seems like a slightly simpler separation of responsibilities.
The board code is responsible for save/restore, the FSP code is responsible for data blob creation and use.  Data integrity, authentication, compression, etc all can be done based on more detailed knowledge of the silicon implementation requirements.

I can see another argument though that doing it inside FSP effectively forces both bootloader and FSP to carry compression/decompression.  The compression/decompression aren't likely to need to be silicon specific.  And bootloader may have more requirements to balance than just the silicon requirements.

Can I get some votes on preferred answer for compressing/decompressing FSP non-volatile data in bootloader or FSP?

Thanks,
Isaac

-----Original Message-----
From: Gudla, Raghava <raghava.gudla@intel.com> 
Sent: Friday, May 5, 2023 5:03 PM
To: devel@edk2.groups.io
Cc: Gudla, Raghava <raghava.gudla@intel.com>; Chiu, Chasel <chasel.chiu@intel.com>; Desimone, Nathaniel L <nathaniel.l.desimone@intel.com>; Oram, Isaac W <isaac.w.oram@intel.com>
Subject: [edk2-platforms:PATCH V1] MinPlatformPkg/SaveMemoryConfig: Support NVS Data compression.

Around 50KB "FspNonVolatileStorageHob" data can be compressed to approximately 3 KB which can save NVRAM space and enhance life of the SPI part by decreasing the number of reclaim cycles needed.

This patch added support to compress "FspNonVolatileStorageHob" data before saving to NVRAM.

A PcdEnableCompressFspNvsHob is introduced to enable/disable this feature per platform usage.

Cc: Chasel Chiu <chasel.chiu@intel.com>
Cc: Nate DeSimone <nathaniel.l.desimone@intel.com>
Cc: Isaac Oram <isaac.w.oram@intel.com>

Signed-off-by: Raghava Gudla <raghava.gudla@intel.com>
---
 .../SaveMemoryConfig/SaveMemoryConfig.c       | 34 +++++++++++++++++++
 .../SaveMemoryConfig/SaveMemoryConfig.inf     |  6 +++-
 .../Include/Dsc/CoreCommonLib.dsc             |  1 +
 .../Intel/MinPlatformPkg/MinPlatformPkg.dec   |  5 +++
 4 files changed, 45 insertions(+), 1 deletion(-)

diff --git a/Platform/Intel/MinPlatformPkg/FspWrapper/SaveMemoryConfig/SaveMemoryConfig.c b/Platform/Intel/MinPlatformPkg/FspWrapper/SaveMemoryConfig/SaveMemoryConfig.c
index 0215e8eed..8aa935b54 100644
--- a/Platform/Intel/MinPlatformPkg/FspWrapper/SaveMemoryConfig/SaveMemoryConfig.c
+++ b/Platform/Intel/MinPlatformPkg/FspWrapper/SaveMemoryConfig/SaveMemo
+++ ryConfig.c
@@ -21,6 +21,8 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
 #include <Library/LargeVariableWriteLib.h> #include <Library/VariableWriteLib.h> #include <Guid/FspNonVolatileStorageHob2.h>+#include <Library/PcdLib.h>+#include <Library/CompressLib.h>  /**   This is the standard EFI driver point that detects whether there is a@@ -45,6 +47,9 @@ SaveMemoryConfigEntryPoint (
   UINTN             DataSize;   UINTN             BufferSize;   BOOLEAN           DataIsIdentical;+  VOID              *CompressedData;+  UINT64            CompressedSize;+  UINTN             CompressedAllocationPages;    DataSize        = 0;   BufferSize      = 0;@@ -73,6 +78,35 @@ SaveMemoryConfigEntryPoint (
     }   } +  if (PcdGetBool (PcdEnableCompressFspNvsHob) == 1) {+    CompressedData            = NULL;+    CompressedSize            = 0;+    CompressedAllocationPages = 0;++    DEBUG ((DEBUG_INFO, "compressing mem config nvs variable\n"));+    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", __func__));+        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", __func__, Status));+        ASSERT_EFI_ERROR (Status);+        FreePool(CompressedData);+        return Status;+      } else {+        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));diff --git a/Platform/Intel/MinPlatformPkg/FspWrapper/SaveMemoryConfig/SaveMemoryConfig.inf b/Platform/Intel/MinPlatformPkg/FspWrapper/SaveMemoryConfig/SaveMemoryConfig.inf
index 61e85a658..77920d031 100644
--- a/Platform/Intel/MinPlatformPkg/FspWrapper/SaveMemoryConfig/SaveMemoryConfig.inf
+++ b/Platform/Intel/MinPlatformPkg/FspWrapper/SaveMemoryConfig/SaveMemo
+++ ryConfig.inf
@@ -26,6 +26,7 @@
   LargeVariableReadLib   LargeVariableWriteLib   BaseLib+  CompressLib  [Packages]   MdePkg/MdePkg.dec@@ -45,6 +46,9 @@
   gFspNonVolatileStorageHob2Guid                ## CONSUMES   gFspNvsBufferVariableGuid                     ## PRODUCES +[Pcd]+  gMinPlatformPkgTokenSpaceGuid.PcdEnableCompressFspNvsHob+ [Depex]   gEfiVariableArchProtocolGuid        AND-  gEfiVariableWriteArchProtocolGuid
\ No newline at end of file
+  gEfiVariableWriteArchProtocolGuiddiff --git 
+ a/Platform/Intel/MinPlatformPkg/Include/Dsc/CoreCommonLib.dsc 
+ b/Platform/Intel/MinPlatformPkg/Include/Dsc/CoreCommonLib.dsc
index 5ce21cf31..dfe7d836d 100644
--- a/Platform/Intel/MinPlatformPkg/Include/Dsc/CoreCommonLib.dsc
+++ b/Platform/Intel/MinPlatformPkg/Include/Dsc/CoreCommonLib.dsc
@@ -147,6 +147,7 @@
   BmpSupportLib|MdeModulePkg/Library/BaseBmpSupportLib/BaseBmpSupportLib.inf   LargeVariableReadLib|MinPlatformPkg/Library/BaseLargeVariableLib/BaseLargeVariableReadLib.inf   LargeVariableWriteLib|MinPlatformPkg/Library/BaseLargeVariableLib/BaseLargeVariableWriteLib.inf+  CompressLib|MinPlatformPkg/Library/CompressLib/CompressLib.inf    #   # CryptLibdiff --git a/Platform/Intel/MinPlatformPkg/MinPlatformPkg.dec b/Platform/Intel/MinPlatformPkg/MinPlatformPkg.dec
index 784abb828..e21d55fb3 100644
--- a/Platform/Intel/MinPlatformPkg/MinPlatformPkg.dec
+++ b/Platform/Intel/MinPlatformPkg/MinPlatformPkg.dec
@@ -348,6 +348,11 @@
   gMinPlatformPkgTokenSpaceGuid.PcdFadtFlags|0x000086A5|UINT32|0x90000027   gMinPlatformPkgTokenSpaceGuid.PcdFadtMajorVersion|0x06|UINT8|0x90000030   gMinPlatformPkgTokenSpaceGuid.PcdFadtMinorVersion|0x03|UINT8|0x90000031+## Controls whether the Memory Config UEFI Variable 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.PcdEnableCompressFspNvsHob|FALSE|BOOLEAN|0x90000032  [PcdsFixedAtBuild] -- 
2.19.1.windows.1