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://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.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/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 > >>> > >>> > >>> > >>> > >>> > >> > >> > >> > >> > >> > > > > > > > > > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#104426): https://edk2.groups.io/g/devel/message/104426 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] -=-=-=-=-=-=-=-=-=-=-=-
