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

["Chiu, Chasel" <chasel.chiu@intel.com>]

Hi Michael Kubacki,

Since Rahava has rebased the patch it might be easier to add " Signed-off-by: Michael Kubacki <michael.kubacki@microsoft.com>" to Rahave's patch commit message so both of you are authors.
What do you think?

By the way, I found a bug in SaveMemoryConfig.c: FreePool() should be FreePages (CompressedData, CompressedAllocationPages);
Raghava, please help to fix it by sending V2 patch  (adding -v2 to git format-patch command)

Thanks,
Chasel



> -----Original Message-----
> From: Michael Kubacki <mikuback@linux.microsoft.com>
> Sent: Tuesday, May 9, 2023 11:06 AM
> To: Gudla, Raghava <raghava.gudla@intel.com>; Oram, Isaac W
> <isaac.w.oram@intel.com>; devel@edk2.groups.io; Kinney, Michael D
> <michael.d.kinney@intel.com>; Chiu, Chasel <chasel.chiu@intel.com>
> Cc: Desimone, Nathaniel L <nathaniel.l.desimone@intel.com>; Kubacki, Michael
> <michael.kubacki@microsoft.com>; Chaganty, Rangasai V
> <rangasai.v.chaganty@intel.com>
> Subject: Re: [edk2-devel] [edk2-platforms:PATCH V1]
> MinPlatformPkg/SaveMemoryConfig: Support NVS Data compression.
> 
> Thanks for reraising this. If you need me to rebase or help in anyway let me
> know.
> 
> On 5/9/2023 1:41 PM, Gudla, Raghava wrote:
> > My patch is based on Michael's fix. I took his change to do a PoC a while back
> and totally forgot that his patch is still available. We can proceed to merge
> Michael's patch.
> >
> > Thanks,
> > Raghava
> >
> > -----Original Message-----
> > From: Oram, Isaac W <isaac.w.oram@intel.com>
> > Sent: Tuesday, May 9, 2023 10:35 AM
> > To: devel@edk2.groups.io; mikuback@linux.microsoft.com; Kinney,
> > Michael D <michael.d.kinney@intel.com>; Chiu, Chasel
> > <chasel.chiu@intel.com>; Gudla, Raghava <raghava.gudla@intel.com>
> > Cc: Desimone, Nathaniel L <nathaniel.l.desimone@intel.com>; Kubacki,
> > Michael <michael.kubacki@microsoft.com>; Chaganty, Rangasai V
> > <rangasai.v.chaganty@intel.com>
> > Subject: RE: [edk2-devel] [edk2-platforms:PATCH V1]
> MinPlatformPkg/SaveMemoryConfig: Support NVS Data compression.
> >
> > Rereading that thread and this request, other opinions, etc, I am convinced
> that the simpler interface and board responsibility is the correct short/medium
> term answer.  I don't have an opinion on Mike Kinney's question on
> encapsulated services.  I generally like that design, though I am sensitive to
> Michael Kubacki's feedback that variable services are too complex as it is.  I
> guess it probably depends a lot on the specifics of the proposal.  That said, it is
> pretty easy to migrate from a board specific solution to a more base layer
> solution in the future, so adopting this now doesn't seem harmful to me.
> >
> > Raghava, can you look at Michael's fix?  It looks nearly identical to yours and
> general convention is to accept the earlier one in case of collision I believe.  I
> like his PCD naming a little better, but both are fine to me.
> >
> > Regards,
> > Isaac
> >
> > -----Original Message-----
> > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Michael
> > Kubacki
> > Sent: Tuesday, May 9, 2023 8:26 AM
> > To: devel@edk2.groups.io; Kinney, Michael D
> > <michael.d.kinney@intel.com>; Chiu, Chasel <chasel.chiu@intel.com>;
> > Oram, Isaac W <isaac.w.oram@intel.com>; Gudla, Raghava
> > <raghava.gudla@intel.com>
> > Cc: Desimone, Nathaniel L <nathaniel.l.desimone@intel.com>; Kubacki,
> > Michael <michael.kubacki@microsoft.com>; Chaganty, Rangasai V
> > <rangasai.v.chaganty@intel.com>
> > Subject: Re: [edk2-devel] [edk2-platforms:PATCH V1]
> MinPlatformPkg/SaveMemoryConfig: Support NVS Data compression.
> >
> > At the surface, this looks similar to the following patch I sent a
> > while
> > back:
> >
> > https://edk2.groups.io/g/devel/message/92644
> >
> > That triggered a thread where we had a similar discussion about
> LargeVariableLib responsibilities, etc.
> >
> > We still have a fork of SaveMemoryConfig that uses the PCD I sent in
> > the
> > patch:
> >
> > https://github.com/microsoft/mu_common_intel_min_platform/blob/release
> >
> /202208/MinPlatformPkg/FspWrapper/SaveMemoryConfig/SaveMemoryConfig.
> in
> > f
> >
> > I believe a challenge that led to adding compression (in our fork code) to
> SaveMemoryConfig was the fact that the data was produced in pre-mem PEI
> and compression is expensive in CAR.
> >
> > In general though, I still believe that it is simpler to separate data mutation
> from service APIs. Platforms can manipulate data to achieve their goals whether
> size, security, and so on and the service APIs provide a simple interface to store
> and retrieve that data blob.
> >
> > I'd also like to see FSP reduce in size and eliminate operations that are not
> essential to its role and can be consolidated/reused in the wrapper.
> >
> > Thanks,
> > Michael
> >
> > On 5/8/2023 5:48 PM, Michael D Kinney wrote:
> >> 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.oram@intel.com>; Gudla, Raghava
> >>> <raghava.gudla@intel.com>; devel@edk2.groups.io
> >>> Cc: Desimone, Nathaniel L <nathaniel.l.desimone@intel.com>; Kubacki,
> >>> Michael <michael.kubacki@microsoft.com>; Chaganty, Rangasai V
> >>> <rangasai.v.chaganty@intel.com>; Chiu, Chasel
> >>> <chasel.chiu@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.oram@intel.com>
> >>>> Sent: Monday, May 8, 2023 12:15 PM
> >>>> To: Gudla, Raghava <raghava.gudla@intel.com>; devel@edk2.groups.io
> >>>> Cc: Chiu, Chasel <chasel.chiu@intel.com>; Desimone, Nathaniel L
> >>>> <nathaniel.l.desimone@intel.com>; Kubacki, Michael
> >>>> <michael.kubacki@microsoft.com>; Chaganty, Rangasai V
> >>>> <rangasai.v.chaganty@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.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/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);+



Bug: it should be FreePages (CompressedData, CompressedAllocationPages);



> >>>> 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/Bas
> >>> LargeVariableReadLib|e
> >>> LargeVariableReadLib|Larg
> >>>> eVariableReadLib.inf
> >>>>
> >>> LargeVariableWriteLib|MinPlatformPkg/Library/BaseLargeVariableLib/Ba
> >>> LargeVariableWriteLib|s
> >>> LargeVariableWriteLib|eLarg
> >>>> 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
> >>>
> >>>
> >>>
> >>>
> >>>
> >>
> >>
> >>
> >>
> >>
> >
> >
> > 
> >
> >