I haven't done a deep comparison of the changes, but I'd lean toward using what 
is in the branch that I referenced earlier since it has been used for a while 
now.

Is there anything of value in the new set of changes?

Thanks,
Michael

-----Original Message-----
From: Chiu, Chasel <chasel.c...@intel.com> 
Sent: Tuesday, May 9, 2023 2:56 PM
To: Michael Kubacki <mikub...@linux.microsoft.com>; Gudla, Raghava 
<raghava.gu...@intel.com>; Oram, Isaac W <isaac.w.o...@intel.com>; 
devel@edk2.groups.io; Kinney, Michael D <michael.d.kin...@intel.com>
Cc: Desimone, Nathaniel L <nathaniel.l.desim...@intel.com>; Michael Kubacki 
<michael.kuba...@microsoft.com>; Chaganty, Rangasai V 
<rangasai.v.chaga...@intel.com>
Subject: [EXTERNAL] RE: [edk2-devel] [edk2-platforms:PATCH V1] 
MinPlatformPkg/SaveMemoryConfig: Support NVS Data compression.


Hi Michael Kubacki,

Since Rahava has rebased the patch it might be easier to add " Signed-off-by: 
Michael Kubacki <michael.kuba...@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 <mikub...@linux.microsoft.com>
> Sent: Tuesday, May 9, 2023 11:06 AM
> To: Gudla, Raghava <raghava.gu...@intel.com>; Oram, Isaac W 
> <isaac.w.o...@intel.com>; devel@edk2.groups.io; Kinney, Michael D 
> <michael.d.kin...@intel.com>; Chiu, Chasel <chasel.c...@intel.com>
> Cc: 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-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.o...@intel.com>
> > Sent: Tuesday, May 9, 2023 10:35 AM
> > To: devel@edk2.groups.io; mikub...@linux.microsoft.com; Kinney, 
> > Michael D <michael.d.kin...@intel.com>; Chiu, Chasel 
> > <chasel.c...@intel.com>; Gudla, Raghava <raghava.gu...@intel.com>
> > Cc: 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-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.kin...@intel.com>; Chiu, Chasel <chasel.c...@intel.com>; 
> > Oram, Isaac W <isaac.w.o...@intel.com>; Gudla, Raghava 
> > <raghava.gu...@intel.com>
> > Cc: 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-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://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fed
> > k2.groups.io%2Fg%2Fdevel%2Fmessage%2F92644&data=05%7C01%7Cmichael.ku
> > backi%40microsoft.com%7C05c93413c875400acef708db50bf197d%7C72f988bf8
> > 6f141af91ab2d7cd011db47%7C1%7C0%7C638192553936149621%7CUnknown%7CTWF
> > pbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI
> > 6Mn0%3D%7C3000%7C%7C%7C&sdata=E4hpQLMgLy0BKHngjpec88AI0v3TZ8xgxebjU0
> > SS0FM%3D&reserved=0
> >
> > 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://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgi
> > thub.com%2Fmicrosoft%2Fmu_common_intel_min_platform%2Fblob%2Frelease
> > &data=05%7C01%7Cmichael.kubacki%40microsoft.com%7C05c93413c875400ace
> > f708db50bf197d%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C63819255
> > 3936305834%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luM
> > zIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=lXFLZV78CPev
> > UL7De1%2B6RtHaKWQcjnXx6IFABcmWJ54%3D&reserved=0
> >
> /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.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);+



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/B
> >>> LargeVariableReadLib|as
> >>> LargeVariableReadLib|e
> >>> LargeVariableReadLib|Larg
> >>>> eVariableReadLib.inf
> >>>>
> >>> LargeVariableWriteLib|MinPlatformPkg/Library/BaseLargeVariableLib/
> >>> LargeVariableWriteLib|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 
> >>>> 31+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 (#104428): https://edk2.groups.io/g/devel/message/104428
Mute This Topic: https://groups.io/mt/98719541/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-


Reply via email to