When reviewing the Variable feature that adds integrity and confidentiality,
I suggested that the interface between the Variable services and the NVStorage
could provide an abstraction to encode/decode the stored data that would
support encryption, compression, or both. Could also support a platform
policy for which variables the encode/decode operation is applied.
Wouldn't that be a better abstraction than piecemeal adding these
features?
Doesn't mean that this can't go in as-is. But would be an opportunity to
consolidate in the future.
Mike
> -----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
>
>
>
>
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#104293): https://edk2.groups.io/g/devel/message/104293
Mute This Topic: https://groups.io/mt/98719541/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe:
https://edk2.groups.io/g/devel/leave/9847357/21656/1706620634/xyzzy
[arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
