[edk2-devel] [edk2-platforms][PATCH v2 1/1] MinPlatformPkg: Add FspNvsBuffer compression option
Oram, Isaac W
isaac.w.oram at intel.com
Wed Aug 24 00:55:16 UTC 2022
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 at intel.com>
Sent: Tuesday, August 23, 2022 12:51 PM
To: Oram, Isaac W <isaac.w.oram at intel.com>; devel at edk2.groups.io; mikuback at linux.microsoft.com
Cc: Chiu, Chasel <chasel.chiu at intel.com>; Gao, Liming <gaoliming at byosoft.com.cn>; Dong, Eric <eric.dong at 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 at intel.com>
Sent: Tuesday, August 23, 2022 11:34 AM
To: devel at edk2.groups.io; mikuback at linux.microsoft.com
Cc: Chiu, Chasel <chasel.chiu at intel.com>; Desimone, Nathaniel L <nathaniel.l.desimone at intel.com>; Gao, Liming <gaoliming at byosoft.com.cn>; Dong, Eric <eric.dong at 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 at edk2.groups.io <devel at edk2.groups.io> On Behalf Of Michael Kubacki
Sent: Monday, August 22, 2022 8:23 PM
To: devel at edk2.groups.io
Cc: Chiu, Chasel <chasel.chiu at intel.com>; Desimone, Nathaniel L <nathaniel.l.desimone at intel.com>; Oram, Isaac W <isaac.w.oram at intel.com>; Gao, Liming <gaoliming at byosoft.com.cn>; Dong, Eric <eric.dong at intel.com>
Subject: [edk2-devel] [edk2-platforms][PATCH v2 1/1] MinPlatformPkg: Add FspNvsBuffer compression option
From: Michael Kubacki <michael.kubacki at 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 at intel.com>
Cc: Nate DeSimone <nathaniel.l.desimone at intel.com>
Cc: Isaac Oram <isaac.w.oram at intel.com>
Cc: Liming Gao <gaoliming at byosoft.com.cn>
Cc: Eric Dong <eric.dong at intel.com>
Signed-off-by: Michael Kubacki <michael.kubacki at 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/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.<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/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.<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/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 at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [isaac.w.oram at intel.com] -=-=-=-=-=-=
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#92734): https://edk2.groups.io/g/devel/message/92734
Mute This Topic: https://groups.io/mt/93197638/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list