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.web11.39178.1661531784672163662 for ; Fri, 26 Aug 2022 09:36:24 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@linux.microsoft.com header.s=default header.b=HRbZI8PC; spf=pass (domain: linux.microsoft.com, ip: 13.77.154.182, mailfrom: mikuback@linux.microsoft.com) Received: from [192.168.4.22] (unknown [47.195.228.134]) by linux.microsoft.com (Postfix) with ESMTPSA id 20A13236D987; Fri, 26 Aug 2022 09:36:23 -0700 (PDT) DKIM-Filter: OpenDKIM Filter v2.11.0 linux.microsoft.com 20A13236D987 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.microsoft.com; s=default; t=1661531783; bh=6yTsk3Q3f4xLAxP8zn1YE5UkWn87NRSC0mBAMM3gi1E=; h=Date:Subject:To:Cc:References:From:In-Reply-To:From; b=HRbZI8PC2tG3ha9tInMXFUgIMLWMOOKJc8CiL5JE3KrTM5faNItOpqh41FoH15GGy t8achaDo0gWB3PgV45C1IrKg/ZZqpvAA0abi328erQuVaQ0BsHgQWiPbXk7pNJJLOB v2E21HckDPJTpySi141gsU+PGJAqOtic1u6qw/Vc= Message-ID: <8d39216a-eb74-481a-27da-49480232da95@linux.microsoft.com> Date: Fri, 26 Aug 2022 12:36:21 -0400 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101 Thunderbird/91.13.0 Subject: Re: [edk2-devel] [edk2-platforms][PATCH v2 1/1] MinPlatformPkg: Add FspNvsBuffer compression option To: "Oram, Isaac W" , "Desimone, Nathaniel L" , "devel@edk2.groups.io" Cc: "Chiu, Chasel" , "Gao, Liming" , "Dong, Eric" References: <20220823032253.1686-1-mikuback@linux.microsoft.com> From: "Michael Kubacki" In-Reply-To: Content-Language: en-US Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit Honestly, I forgot about the open board packages, I can definitely update support for thosee. Automating Contribution Expectations ------------------------------------ As Tianocore modernizes its development process, PR template checklists and CI including board package builds would automate a broad range of contribution expectations including what packages need to build against what targets, toolchains, analysis tools, etc. In this case, I understand what needs to be done but others might not. Board vs FSP Compression ------------------------ I agree that the wrapper should be responsible for determining how to manage data. For the reasons Isaac mentioned - platform selection of compression algorithms, cohesive platform data managment in its data store, minimizing FSP size, etc. By FSP focusing on only what it requires, it provides opportunity for simpler and more flexible integration and optimization around its interface. LargeVariableLib Support ------------------------ I am in favor of not modifying LargeVariableLib. Going back to the FSP compression topic, I prefer simple (single responsibility) interfaces. FSP initializes silicon. As part of its input interface, it needs some context data (NVS buffer data) to determine its internal state transitions. By decoupling peripheral functionality from within FSP, it can easily be reasoned that data compression is not silicon init, therefore, FSP should not perform data compression. Interfaces often require data input. We (everyone) have a tendency to extend those interface with various options for data transformation over time. For example, this has made the UEFI variable interface error prone and cumbersome to adapt to different environments. When I look at LargeVariableWriteLib.h and LargeVariableReadLib.h, I see a wrapper around the UEFI variable services that is relatively easy to understand and to determine if it fits my use case - I want to work around the maximum UEFI variable size set on a platform. On the surface at least, the interface is simple - it mostly mirrors the UEFI RT Services functions, the use case is easy to understand, and the expected behavior is easy to comprehend. I'm trying to understand what advantage there is to complicating this. Compression already has an abstraction in its library classes - CompressLib and UefiDecompressLib. Those could be more symmetrical, etc. but they're relatively easy to understand and reason about as well. So this is a question of whether to invoke that abstraction outside the LargeVariableLib interfaces or underneath its implementation. When outside: - The caller is given a simpler LargeVariableLib interface to use. - The caller does not need to understand LargeVariableLib compression implementation details. - The caller is free to choose a different compression abstraction. - The caller is given flexibility to transform data given to LargeVariableLib in the order they choose. - Example: Encrypt then compress vs compress then encrypt When in VariableLargeLib: - The caller has to understand a more complex configuration interface. - The caller loses most of the above and still has to worry about linking suitable compression code. - They might save some lines of code but that is not valuable. As you can see, I would personlly prefer for LargeVariableLib to stay simple and let integration of data transformations occur as needed based on the caller's needs. But, I can try to add this in a reasonable way if it is strongly preferred. V2 Proposal ----------- For V2, would it be okay to simply add support for the boards as Isaac mentioned in the first mail. It could be taken further in the future if desired. Thanks, Michael On 8/23/2022 8:55 PM, Oram, Isaac W wrote: > A PCD to control library class instance seems redundant. The build already has capabilities to control library class instance per driver, per driver type, per architecture, etc. Let me know if there is some benefit to overriding PCD in board DSC vs overriding library class in board DSC that I am missing. > > We want to simplify what developers need to see and deal with. Minimize the number of basic decisions needed to get a functional port. I would prefer a single instance of LargeVariable*Lib. As a board port developer, if your silicon FSP supports compressing the data and your board port doesn't want the redundancy, that is a stage 7 optimization to write your own instance of the library class or your own version of SaveMemoryConfigDxe. No need to burden the rest of the boards with needing to know about and configure this to optimize a corner case. > > I also think decompression and compression decisions should reside at the boot loader level not internal to FSP. First to minimize FSP content, second because bootloaders already typically decide and implement decompression that matches their build specific compression. I will grant that it is probably mostly the same common algorithm implementation. > > > Ultimately though, those are mostly philosophical objections. If you really want two instances of the library, I can live with it. I will be able to have reasonable default behavior that is pretty simple for most. > > The more concrete objection is the asymmetric design, where common code implements compression and per platform code implements decompression. That seems distasteful. I can live with this too because it is kind of already this way, but more consistent combined with more encapsulated seems better than expanding the asymmetric design. > > Regards, > Isaac > > -----Original Message----- > From: Desimone, Nathaniel L > Sent: Tuesday, August 23, 2022 12:51 PM > To: Oram, Isaac W ; devel@edk2.groups.io; mikuback@linux.microsoft.com > Cc: Chiu, Chasel ; Gao, Liming ; Dong, Eric > Subject: RE: [edk2-devel] [edk2-platforms][PATCH v2 1/1] MinPlatformPkg: Add FspNvsBuffer compression option > > Agreed with Isaac that having the compression capability be more general use would be nice. However, I think it would better match the EDK II design style if we had a second implementation of LargeVariable*Lib that added the compression capability on top of the original LargeVariable*Lib. > > I also think having a PCD to explicitly control whether to apply compression to the FSP NVS buffer is important. Because there are some FSP implementations like ICX and SPR that already include the CompressLib inside the FSP binary itself. On those FSP implementations, the data in the FSP_NON_VOLATILE_STORAGE_HOB is already compressed. Said PCD would simply control which instance of the LibraryClass gets linked with SaveMemoryConfigDxe. > > Also agreed with Isaac to please be careful to make sure that any new LibraryClasses are in the Include/*.dsc files somewhere so existing boards don't need modifications to their BoardPkg.dsc file. > > Thanks, > Nate > > -----Original Message----- > From: Oram, Isaac W > Sent: Tuesday, August 23, 2022 11:34 AM > To: devel@edk2.groups.io; mikuback@linux.microsoft.com > Cc: Chiu, Chasel ; Desimone, Nathaniel L ; Gao, Liming ; Dong, Eric > Subject: RE: [edk2-devel] [edk2-platforms][PATCH v2 1/1] MinPlatformPkg: Add FspNvsBuffer compression option > > Implementing this way will break the build for most of the open MinPlatform ports. At minimum, please add the CompressLib to CoreCommonLib.dsc next to the UefiDecompressLib instantiation so that it is included automatically for most boards. > > Regarding the design, I agree this is a good capability. And it makes sense to me to be board controlled rather than FSP controlled. > > I wonder if it might be better to modify the LargeVariableRead/Write such that it compresses/decompresses large variables automatically. Could be controlled via library class instance or maybe via PatchablePerModule PCD. Boards would still have a lot of control if they wanted. > > Regards, > Isaac > > -----Original Message----- > From: devel@edk2.groups.io On Behalf Of Michael Kubacki > Sent: Monday, August 22, 2022 8:23 PM > To: devel@edk2.groups.io > Cc: Chiu, Chasel ; Desimone, Nathaniel L ; Oram, Isaac W ; Gao, Liming ; Dong, Eric > Subject: [edk2-devel] [edk2-platforms][PATCH v2 1/1] MinPlatformPkg: Add FspNvsBuffer compression option > > From: Michael Kubacki > > Adds a PCD called "PcdEnableCompressedFspNvsBuffer" that allows the "FspNvsBuffer" UEFI variable data to be saved as compressed data. > > Especially due to the nature of the data saved in this variable, it compresses well. For example, it has been found to reduce ~63KB of data to ~13KB. Boot time impact has been found to be negligible. > > The default value is FALSE to keep default behavior consistent. > > Decompression can be performed on the variable data using the standard UefiDecompressLib. > > Cc: Chasel Chiu > Cc: Nate DeSimone > Cc: Isaac Oram > Cc: Liming Gao > Cc: Eric Dong > Signed-off-by: Michael Kubacki > --- > > Notes: > v2: Rebase onto 9769bf28d1fc > > Platform/Intel/MinPlatformPkg/FspWrapper/SaveMemoryConfig/SaveMemoryConfig.c | 62 ++++++++++++++++---- > Platform/Intel/MinPlatformPkg/FspWrapper/SaveMemoryConfig/SaveMemoryConfig.inf | 4 ++ > Platform/Intel/MinPlatformPkg/MinPlatformPkg.dec | 6 ++ > Platform/Intel/MinPlatformPkg/MinPlatformPkg.dsc | 1 + > 4 files changed, 60 insertions(+), 13 deletions(-) > > diff --git a/Platform/Intel/MinPlatformPkg/FspWrapper/SaveMemoryConfig/SaveMemoryConfig.c b/Platform/Intel/MinPlatformPkg/FspWrapper/SaveMemoryConfig/SaveMemoryConfig.c > index 0215e8eeddfb..95b8cef8b32b 100644 > --- a/Platform/Intel/MinPlatformPkg/FspWrapper/SaveMemoryConfig/SaveMemoryConfig.c > +++ b/Platform/Intel/MinPlatformPkg/FspWrapper/SaveMemoryConfig/SaveMemo > +++ ryConfig.c > @@ -3,6 +3,7 @@ > exists, and saves the data to nvRAM. > > Copyright (c) 2017 - 2022, Intel Corporation. All rights reserved.
> +Copyright (c) Microsoft Corporation.
> SPDX-License-Identifier: BSD-2-Clause-Patent > > **/ > @@ -10,6 +11,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent #include #include #include > +#include > #include > #include > #include > @@ -19,6 +21,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent #include #include #include > +#include > #include > #include > > @@ -38,20 +41,26 @@ SaveMemoryConfigEntryPoint ( > IN EFI_SYSTEM_TABLE *SystemTable > ) > { > - EFI_STATUS Status; > - EFI_HOB_GUID_TYPE *GuidHob; > - VOID *HobData; > - VOID *VariableData; > - UINTN DataSize; > - UINTN BufferSize; > - BOOLEAN DataIsIdentical; > + EFI_STATUS Status; > + EFI_HOB_GUID_TYPE *GuidHob; > + VOID *HobData; > + VOID *VariableData; > + UINTN DataSize; > + UINTN BufferSize; > + BOOLEAN DataIsIdentical; > + VOID *CompressedData; > + UINT64 CompressedSize; > + UINTN CompressedAllocationPages; > > - DataSize = 0; > - BufferSize = 0; > - VariableData = NULL; > - GuidHob = NULL; > - HobData = NULL; > - DataIsIdentical = FALSE; > + DataSize = 0; > + BufferSize = 0; > + VariableData = NULL; > + GuidHob = NULL; > + HobData = NULL; > + DataIsIdentical = FALSE; > + CompressedData = NULL; > + CompressedSize = 0; > + CompressedAllocationPages = 0; > > // > // Search for the Memory Configuration GUID HOB. If it is not present, then @@ -73,6 +82,29 @@ SaveMemoryConfigEntryPoint ( > } > } > > + if (PcdGetBool (PcdEnableCompressedFspNvsBuffer)) { > + 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", __FUNCTION__)); > + 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", __FUNCTION__, Status)); > + ASSERT_EFI_ERROR (Status); > + return Status; > + } > + } > + > + 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)); > @@ -136,6 +168,10 @@ SaveMemoryConfigEntryPoint ( > DEBUG((DEBUG_ERROR, "Memory S3 Data HOB was not found\n")); > } > > + if (CompressedData != NULL) { > + FreePages (CompressedData, CompressedAllocationPages); } > + > // > // This driver does not produce any protocol services, so always unload it. > // > diff --git a/Platform/Intel/MinPlatformPkg/FspWrapper/SaveMemoryConfig/SaveMemoryConfig.inf b/Platform/Intel/MinPlatformPkg/FspWrapper/SaveMemoryConfig/SaveMemoryConfig.inf > index 61e85a658693..0f12deb131ca 100644 > --- a/Platform/Intel/MinPlatformPkg/FspWrapper/SaveMemoryConfig/SaveMemoryConfig.inf > +++ b/Platform/Intel/MinPlatformPkg/FspWrapper/SaveMemoryConfig/SaveMemo > +++ ryConfig.inf > @@ -26,6 +26,7 @@ [LibraryClasses] > LargeVariableReadLib > LargeVariableWriteLib > BaseLib > + CompressLib > > [Packages] > MdePkg/MdePkg.dec > @@ -45,6 +46,9 @@ [Guids] > gFspNonVolatileStorageHob2Guid ## CONSUMES > gFspNvsBufferVariableGuid ## PRODUCES > > +[Pcd] > + gMinPlatformPkgTokenSpaceGuid.PcdEnableCompressedFspNvsBuffer > + > [Depex] > gEfiVariableArchProtocolGuid AND > gEfiVariableWriteArchProtocolGuid > \ No newline at end of file > diff --git a/Platform/Intel/MinPlatformPkg/MinPlatformPkg.dec b/Platform/Intel/MinPlatformPkg/MinPlatformPkg.dec > index 8e603b7bf94b..94353cb76824 100644 > --- a/Platform/Intel/MinPlatformPkg/MinPlatformPkg.dec > +++ b/Platform/Intel/MinPlatformPkg/MinPlatformPkg.dec > @@ -306,6 +306,12 @@ [PcdsFixedAtBuild, PcdsPatchableInModule, PcdsDynamic, PcdsDynamicEx] > > gMinPlatformPkgTokenSpaceGuid.PcdPlatformMemoryCheckLevel|0|UINT32|0x30000009 > > + ## Controls whether the FSP NVS buffer 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.PcdEnableCompressedFspNvsBuffer|FALSE|BO > + OLEAN|0x30000010 > + > ## This PCD is to control which device is the potential trusted console input device.

> # For example:
> # USB Short Form: UsbHID(0xFFFF,0xFFFF,0x1,0x1)
diff --git a/Platform/Intel/MinPlatformPkg/MinPlatformPkg.dsc b/Platform/Intel/MinPlatformPkg/MinPlatformPkg.dsc > index 09aa6fe4d51c..ae170f87d548 100644 > --- a/Platform/Intel/MinPlatformPkg/MinPlatformPkg.dsc > +++ b/Platform/Intel/MinPlatformPkg/MinPlatformPkg.dsc > @@ -106,6 +106,7 @@ [LibraryClasses.common.DXE_DRIVER] > FspWrapperPlatformLib|MinPlatformPkg/FspWrapper/Library/DxeFspWrapperPlatformLib/DxeFspWrapperPlatformLib.inf > TestPointCheckLib|MinPlatformPkg/Test/Library/TestPointCheckLib/DxeTestPointCheckLib.inf > TestPointLib|MinPlatformPkg/Test/Library/TestPointLib/DxeTestPointLib.inf > + CompressLib|MinPlatformPkg/Library/CompressLib/CompressLib.inf > > [LibraryClasses.common.DXE_SMM_DRIVER] > TestPointCheckLib|MinPlatformPkg/Test/Library/TestPointCheckLib/SmmTestPointCheckLib.inf > -- > 2.28.0.windows.1 > > > > -=-=-=-=-=-= > Groups.io Links: You receive all messages sent to this group. > View/Reply Online (#92644): https://edk2.groups.io/g/devel/message/92644 > Mute This Topic: https://groups.io/mt/93197638/1492418 > Group Owner: devel+owner@edk2.groups.io > Unsubscribe: https://edk2.groups.io/g/devel/unsub [isaac.w.oram@intel.com] -=-=-=-=-=-= >