Re: [edk2-devel] [edk2-platforms][PATCH v2 1/1] MinPlatformPkg: Add FspNvsBuffer compression option

["Michael Kubacki" <mikuback@linux.microsoft.com>]

This helps resolve most of my concern but opens up a few logistical 
complexities.

Compression Point in Boot Flow
------------------------------
The original patch placed compression in the SaveMemoryConfig DXE 
driver. This proposal would move compression to PEI.

Specifically, it would need to occur in post-memory PEI which shouldn't 
be a problem to intercept within FSP but typically decompression is done 
in PEI and compression in DXE. It may bloat platform PEI code to 
redundantly carry PEI compression code for this purpose.

Compression Ratio
-----------------
Since the memory data is readily available to the FSP wrapper in 
plaintext, the wrapper should have a good sense of compression ratio 
with their chosen algorithm. Ultimately, the wrapper may even experiment 
and choose an algorithm that compresses particularly well or timely 
given the data structure.

Stable Compression Interface
----------------------------
I assume if FSP dispatch mode were to take this PPI as an official 
interface, it would necessitate being defined in some spec so wrappers 
can expect how the interface works and be guaranteed the interface 
remain consistent over time.

Not sure what was in mind here - PI Spec, FSP Spec, etc. In any case, it 
will take time to pitch the proposal, spec body make a decision, and 
release the spec update.

FSP Backporting and Support
---------------------------
Placing the logic in FSP would also require an FSP update for support 
which means backporting and tracking which versions pick up support. The 
forms a logistical exercise that would also take a while to work out.

---

It still seems like quite a bit of effort for relatively small ROI. Is 
that path strongly preferred?

Thanks,
Michael


On 8/31/2022 5:00 PM, Nate DeSimone wrote:
> I agree with Michael that keeping the API interface for LargeVariableLib focused on a single capability would more closely follow the single responsibility principle and therefore be desirable. Adding the capability to select between compressed vs. un-compressed would complicate the API to the point of making the library rigid.
> 
> Where things become unclear to me is how compression should be applied. Ultimately, the knowledge of whether the NVS buffer data/memory training data is sparse enough to merit spending CPU cycles on compression is knowledge that the producer of the data set would have the most insight on. Most of the past MRC implementations on typical Intel platforms just happen to produce sparse data, but that is not necessarily always going to be the case. Moreover, due to the NVS data structures frequently involving repetitions of similar data, compression ratios would be best if the entire data set was compressed as a single block. Functionally, this limits the number of places that we could do compression effectively to two:
> 
> 1. In the implementation of LargeVariableLib
> 2. Before calling LargeVariableLib
> 
> We have already thoroughly discussed #1, so I don't think we need to beat that dead horse any further. That leads me to #2. Again, since the producer of the data will have the best knowledge on the compressibility of that data, I think it makes sense for the request to compress to come from the FSP. I also agree that it makes sense that platforms would want to have control over the compression algorithms used. That makes me lean towards adding new PPIs for compression and decompression. In FSP dispatch mode, those PPIs can be produced by platform and consumed by FSP. The FSP would invoke the PPI to compress the data, and then the data that comes out of FspNvsBuffer would already have the platform preferred compression applied to the fragments of the data for which compression makes sense. What do you think?
> 
> Thanks,
> Nate
> 
> -----Original Message-----
> From: Michael Kubacki <mikuback@linux.microsoft.com>
> Sent: Friday, August 26, 2022 9:36 AM
> To: Oram, Isaac W <isaac.w.oram@intel.com>; Desimone, Nathaniel L <nathaniel.l.desimone@intel.com>; devel@edk2.groups.io
> Cc: Chiu, Chasel <chasel.chiu@intel.com>; Gao, Liming <gaoliming@byosoft.com.cn>; Dong, Eric <eric.dong@intel.com>
> Subject: Re: [edk2-devel] [edk2-platforms][PATCH v2 1/1] MinPlatformPkg: Add FspNvsBuffer compression option
> 
> 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 <nathaniel.l.desimone@intel.com>
>> Sent: Tuesday, August 23, 2022 12:51 PM
>> To: Oram, Isaac W <isaac.w.oram@intel.com>; devel@edk2.groups.io;
>> mikuback@linux.microsoft.com
>> Cc: Chiu, Chasel <chasel.chiu@intel.com>; Gao, Liming
>> <gaoliming@byosoft.com.cn>; Dong, Eric <eric.dong@intel.com>
>> 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 <isaac.w.oram@intel.com>
>> Sent: Tuesday, August 23, 2022 11:34 AM
>> To: devel@edk2.groups.io; mikuback@linux.microsoft.com
>> Cc: Chiu, Chasel <chasel.chiu@intel.com>; Desimone, Nathaniel L
>> <nathaniel.l.desimone@intel.com>; Gao, Liming
>> <gaoliming@byosoft.com.cn>; Dong, Eric <eric.dong@intel.com>
>> 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 <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 <chasel.chiu@intel.com>; Desimone, Nathaniel L
>> <nathaniel.l.desimone@intel.com>; Oram, Isaac W
>> <isaac.w.oram@intel.com>; Gao, Liming <gaoliming@byosoft.com.cn>;
>> Dong, Eric <eric.dong@intel.com>
>> Subject: [edk2-devel] [edk2-platforms][PATCH v2 1/1] MinPlatformPkg:
>> Add FspNvsBuffer compression option
>>
>> From: Michael Kubacki <michael.kubacki@microsoft.com>
>>
>> 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 <chasel.chiu@intel.com>
>> Cc: Nate DeSimone <nathaniel.l.desimone@intel.com>
>> Cc: Isaac Oram <isaac.w.oram@intel.com>
>> Cc: Liming Gao <gaoliming@byosoft.com.cn>
>> Cc: Eric Dong <eric.dong@intel.com>
>> Signed-off-by: Michael Kubacki <michael.kubacki@microsoft.com>
>> ---
>>
>> 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/SaveMemory
>> Config.c
>> b/Platform/Intel/MinPlatformPkg/FspWrapper/SaveMemoryConfig/SaveMemory
>> Config.c index 0215e8eeddfb..95b8cef8b32b 100644
>> ---
>> a/Platform/Intel/MinPlatformPkg/FspWrapper/SaveMemoryConfig/SaveMemory
>> Config.c
>> +++ b/Platform/Intel/MinPlatformPkg/FspWrapper/SaveMemoryConfig/SaveMe
>> +++ mo
>> +++ ryConfig.c
>> @@ -3,6 +3,7 @@
>>      exists, and saves the data to nvRAM.
>>    
>>    Copyright (c) 2017 - 2022, Intel Corporation. All rights
>> reserved.<BR>
>> +Copyright (c) Microsoft Corporation.<BR>
>>    SPDX-License-Identifier: BSD-2-Clause-Patent
>>    
>>    **/
>> @@ -10,6 +11,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
>> #include <Base.h>  #include <Uefi.h>  #include <Library/BaseLib.h>
>> +#include <Library/CompressLib.h>
>>    #include <Library/UefiBootServicesTableLib.h>
>>    #include <Library/UefiRuntimeServicesTableLib.h>
>>    #include <Library/HobLib.h>
>> @@ -19,6 +21,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
>> #include <Library/BaseMemoryLib.h>  #include
>> <Library/LargeVariableReadLib.h>  #include
>> <Library/LargeVariableWriteLib.h>
>> +#include <Library/PcdLib.h>
>>    #include <Library/VariableWriteLib.h>
>>    #include <Guid/FspNonVolatileStorageHob2.h>
>>    
>> @@ -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/SaveMemory
>> Config.inf
>> b/Platform/Intel/MinPlatformPkg/FspWrapper/SaveMemoryConfig/SaveMemory
>> Config.inf index 61e85a658693..0f12deb131ca 100644
>> ---
>> a/Platform/Intel/MinPlatformPkg/FspWrapper/SaveMemoryConfig/SaveMemory
>> Config.inf
>> +++ b/Platform/Intel/MinPlatformPkg/FspWrapper/SaveMemoryConfig/SaveMe
>> +++ mo
>> +++ 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|0x3
>> 0000009
>>    
>> +  ## 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.<BR><BR>
>>      # For example:<BR>
>>      # USB Short Form: UsbHID(0xFFFF,0xFFFF,0x1,0x1)<BR> 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/SmmTes
>> tPointCheckLib.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] -=-=-=-=-=-=
>>
> 
> 
> 
> 
>