-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Chiu,
Chasel
Sent: Monday, May 8, 2023 12:37 PM
To: Oram, Isaac W <isaac.w.o...@intel.com>; Gudla, Raghava
<raghava.gu...@intel.com>; devel@edk2.groups.io
Cc: Desimone, Nathaniel L <nathaniel.l.desim...@intel.com>; Kubacki,
Michael <michael.kuba...@microsoft.com>; Chaganty, Rangasai V
<rangasai.v.chaga...@intel.com>; Chiu, Chasel <chasel.c...@intel.com>
Subject: Re: [edk2-devel] [edk2-platforms:PATCH V1]
MinPlatformPkg/SaveMemoryConfig: Support NVS Data compression.
Hi Isaac,
Just my thoughts, I would vote for platform/bootloader to decide
compressing the variable data before saving to NVRAM or not.
It should be optional and when platform having big SPI flash they might not
want to do this.
Thanks,
Chasel
-----Original Message-----
From: Oram, Isaac W <isaac.w.o...@intel.com>
Sent: Monday, May 8, 2023 12:15 PM
To: Gudla, Raghava <raghava.gu...@intel.com>; devel@edk2.groups.io
Cc: Chiu, Chasel <chasel.c...@intel.com>; Desimone, Nathaniel L
<nathaniel.l.desim...@intel.com>; Kubacki, Michael
<michael.kuba...@microsoft.com>; Chaganty, Rangasai V
<rangasai.v.chaga...@intel.com>
Subject: RE: [edk2-platforms:PATCH V1] MinPlatformPkg/SaveMemoryConfig:
Support NVS Data compression.
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.gu...@intel.com>
Sent: Friday, May 5, 2023 5:03 PM
To: devel@edk2.groups.io
Cc: Gudla, Raghava <raghava.gu...@intel.com>; Chiu, Chasel
<chasel.c...@intel.com>; Desimone, Nathaniel L
<nathaniel.l.desim...@intel.com>; Oram, Isaac W
<isaac.w.o...@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.c...@intel.com>
Cc: Nate DeSimone <nathaniel.l.desim...@intel.com>
Cc: Isaac Oram <isaac.w.o...@intel.com>
Signed-off-by: Raghava Gudla <raghava.gu...@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/SaveMemo
r
yConfig.c
b/Platform/Intel/MinPlatformPkg/FspWrapper/SaveMemoryConfig/SaveMem
or
yConfig.c
index 0215e8eed..8aa935b54 100644
---
a/Platform/Intel/MinPlatformPkg/FspWrapper/SaveMemoryConfig/SaveMemo
r
yConfig.c
+++
b/Platform/Intel/MinPlatformPkg/FspWrapper/SaveMemoryConfig/SaveMem
o
+++ 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/SaveMemo
r
yConfig.inf
b/Platform/Intel/MinPlatformPkg/FspWrapper/SaveMemoryConfig/SaveMem
or
yConfig.inf
index 61e85a658..77920d031 100644
---
a/Platform/Intel/MinPlatformPkg/FspWrapper/SaveMemoryConfig/SaveMemo
r
yConfig.inf
+++
b/Platform/Intel/MinPlatformPkg/FspWrapper/SaveMemoryConfig/SaveMem
o
+++ 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/BaseBmpSuppor
tLi
b.inf
LargeVariableReadLib|MinPlatformPkg/Library/BaseLargeVariableLib/BaseLarg
eVariableReadLib.inf
LargeVariableWriteLib|MinPlatformPkg/Library/BaseLargeVariableLib/BaseLarg
eVariableWriteLib.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|0x90000
027
gMinPlatformPkgTokenSpaceGuid.PcdFadtMajorVersion|0x06|UINT8|0x90000
0
30
gMinPlatformPkgTokenSpaceGuid.PcdFadtMinorVersion|0x03|UINT8|0x90000
0
31+## 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|BOO
L
EAN|0x90000032 [PcdsFixedAtBuild] --
2.19.1.windows.1
