From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from linux.microsoft.com (linux.microsoft.com [13.77.154.182]) by mx.groups.io with SMTP id smtpd.web10.35843.1683645955985264823 for ; Tue, 09 May 2023 08:25:56 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@linux.microsoft.com header.s=default header.b=SqiXGi7x; spf=pass (domain: linux.microsoft.com, ip: 13.77.154.182, mailfrom: mikuback@linux.microsoft.com) Received: from [192.168.4.22] (unknown [47.201.8.94]) by linux.microsoft.com (Postfix) with ESMTPSA id 47D6A20B92A1; Tue, 9 May 2023 08:25:54 -0700 (PDT) DKIM-Filter: OpenDKIM Filter v2.11.0 linux.microsoft.com 47D6A20B92A1 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.microsoft.com; s=default; t=1683645955; bh=RKWsv0SO06JGw0RVRXHjp5SM9+puwx60WFvYZfpkjAU=; h=Date:Subject:To:Cc:References:From:In-Reply-To:From; b=SqiXGi7xpBL2POunONH+nalETibkiyqVWRxyu57xn4SXz6T4WdTHr4UJhboFzZOxA mou7hZZHMBbJGNwb8+sHNWpSqwgPms0t9nYU4rGjkApdL0m3fGfUEwPuBKyDN/SgCS f2iFENKADDyoBCXkpVUB+MVDa9bLoh12JhGMKAYs= Message-ID: Date: Tue, 9 May 2023 11:25:53 -0400 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101 Thunderbird/102.10.1 Subject: Re: [edk2-devel] [edk2-platforms:PATCH V1] MinPlatformPkg/SaveMemoryConfig: Support NVS Data compression. To: devel@edk2.groups.io, michael.d.kinney@intel.com, "Chiu, Chasel" , "Oram, Isaac W" , "Gudla, Raghava" Cc: "Desimone, Nathaniel L" , "Kubacki, Michael" , "Chaganty, Rangasai V" References: <20230506000300.389-1-raghava.gudla@intel.com> From: "Michael Kubacki" In-Reply-To: Content-Language: en-US Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit 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.inf 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 On Behalf Of Chiu, >> Chasel >> Sent: Monday, May 8, 2023 12:37 PM >> To: Oram, Isaac W ; Gudla, Raghava >> ; devel@edk2.groups.io >> Cc: Desimone, Nathaniel L ; Kubacki, >> Michael ; Chaganty, Rangasai V >> ; Chiu, Chasel >> 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 >>> Sent: Monday, May 8, 2023 12:15 PM >>> To: Gudla, Raghava ; devel@edk2.groups.io >>> Cc: Chiu, Chasel ; Desimone, Nathaniel L >>> ; Kubacki, Michael >>> ; Chaganty, Rangasai V >>> >>> 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 >>> Sent: Friday, May 5, 2023 5:03 PM >>> To: devel@edk2.groups.io >>> Cc: Gudla, Raghava ; Chiu, Chasel >>> ; Desimone, Nathaniel L >>> ; Oram, Isaac W >> >>> 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 >>> Cc: Nate DeSimone >>> Cc: Isaac Oram >>> >>> Signed-off-by: Raghava Gudla >>> --- >>> .../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 #include >>> #include >>> +#include +#include >>> /** 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 >> >> >> >> >> > > > > >