* [edk2-platforms:PATCH V1] MinPlatformPkg/SaveMemoryConfig: Support NVS Data compression.
@ 2023-05-06 0:03 Raghava Gudla
2023-05-08 19:15 ` Isaac Oram
0 siblings, 1 reply; 13+ messages in thread
From: Raghava Gudla @ 2023-05-06 0:03 UTC (permalink / raw)
To: devel; +Cc: Raghava Gudla, Chasel Chiu, Nate DeSimone, Isaac Oram
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.chiu@intel.com>
Cc: Nate DeSimone <nathaniel.l.desimone@intel.com>
Cc: Isaac Oram <isaac.w.oram@intel.com>
Signed-off-by: Raghava Gudla <raghava.gudla@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/SaveMemoryConfig.c b/Platform/Intel/MinPlatformPkg/FspWrapper/SaveMemoryConfig/SaveMemoryConfig.c
index 0215e8eed..8aa935b54 100644
--- a/Platform/Intel/MinPlatformPkg/FspWrapper/SaveMemoryConfig/SaveMemoryConfig.c
+++ b/Platform/Intel/MinPlatformPkg/FspWrapper/SaveMemoryConfig/SaveMemoryConfig.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);
+ 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/SaveMemoryConfig.inf b/Platform/Intel/MinPlatformPkg/FspWrapper/SaveMemoryConfig/SaveMemoryConfig.inf
index 61e85a658..77920d031 100644
--- a/Platform/Intel/MinPlatformPkg/FspWrapper/SaveMemoryConfig/SaveMemoryConfig.inf
+++ b/Platform/Intel/MinPlatformPkg/FspWrapper/SaveMemoryConfig/SaveMemoryConfig.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
+ gEfiVariableWriteArchProtocolGuid
diff --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/BaseBmpSupportLib.inf
LargeVariableReadLib|MinPlatformPkg/Library/BaseLargeVariableLib/BaseLargeVariableReadLib.inf
LargeVariableWriteLib|MinPlatformPkg/Library/BaseLargeVariableLib/BaseLargeVariableWriteLib.inf
+ CompressLib|MinPlatformPkg/Library/CompressLib/CompressLib.inf
#
# CryptLib
diff --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|0x90000027
gMinPlatformPkgTokenSpaceGuid.PcdFadtMajorVersion|0x06|UINT8|0x90000030
gMinPlatformPkgTokenSpaceGuid.PcdFadtMinorVersion|0x03|UINT8|0x90000031
+## 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|BOOLEAN|0x90000032
[PcdsFixedAtBuild]
--
2.19.1.windows.1
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [edk2-platforms:PATCH V1] MinPlatformPkg/SaveMemoryConfig: Support NVS Data compression.
2023-05-06 0:03 [edk2-platforms:PATCH V1] MinPlatformPkg/SaveMemoryConfig: Support NVS Data compression Raghava Gudla
@ 2023-05-08 19:15 ` Isaac Oram
2023-05-08 19:36 ` Chiu, Chasel
0 siblings, 1 reply; 13+ messages in thread
From: Isaac Oram @ 2023-05-08 19:15 UTC (permalink / raw)
To: Gudla, Raghava, devel@edk2.groups.io
Cc: Chiu, Chasel, Desimone, Nathaniel L, Kubacki, Michael,
Chaganty, Rangasai V
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.gudla@intel.com>
Sent: Friday, May 5, 2023 5:03 PM
To: devel@edk2.groups.io
Cc: Gudla, Raghava <raghava.gudla@intel.com>; Chiu, Chasel <chasel.chiu@intel.com>; Desimone, Nathaniel L <nathaniel.l.desimone@intel.com>; Oram, Isaac W <isaac.w.oram@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.chiu@intel.com>
Cc: Nate DeSimone <nathaniel.l.desimone@intel.com>
Cc: Isaac Oram <isaac.w.oram@intel.com>
Signed-off-by: Raghava Gudla <raghava.gudla@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/SaveMemoryConfig.c b/Platform/Intel/MinPlatformPkg/FspWrapper/SaveMemoryConfig/SaveMemoryConfig.c
index 0215e8eed..8aa935b54 100644
--- a/Platform/Intel/MinPlatformPkg/FspWrapper/SaveMemoryConfig/SaveMemoryConfig.c
+++ b/Platform/Intel/MinPlatformPkg/FspWrapper/SaveMemoryConfig/SaveMemo
+++ 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);+ 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/SaveMemoryConfig.inf b/Platform/Intel/MinPlatformPkg/FspWrapper/SaveMemoryConfig/SaveMemoryConfig.inf
index 61e85a658..77920d031 100644
--- a/Platform/Intel/MinPlatformPkg/FspWrapper/SaveMemoryConfig/SaveMemoryConfig.inf
+++ b/Platform/Intel/MinPlatformPkg/FspWrapper/SaveMemoryConfig/SaveMemo
+++ 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/BaseBmpSupportLib.inf LargeVariableReadLib|MinPlatformPkg/Library/BaseLargeVariableLib/BaseLargeVariableReadLib.inf LargeVariableWriteLib|MinPlatformPkg/Library/BaseLargeVariableLib/BaseLargeVariableWriteLib.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|0x90000027 gMinPlatformPkgTokenSpaceGuid.PcdFadtMajorVersion|0x06|UINT8|0x90000030 gMinPlatformPkgTokenSpaceGuid.PcdFadtMinorVersion|0x03|UINT8|0x90000031+## 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|BOOLEAN|0x90000032 [PcdsFixedAtBuild] --
2.19.1.windows.1
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [edk2-platforms:PATCH V1] MinPlatformPkg/SaveMemoryConfig: Support NVS Data compression.
2023-05-08 19:15 ` Isaac Oram
@ 2023-05-08 19:36 ` Chiu, Chasel
2023-05-08 21:48 ` [edk2-devel] " Michael D Kinney
0 siblings, 1 reply; 13+ messages in thread
From: Chiu, Chasel @ 2023-05-08 19:36 UTC (permalink / raw)
To: Oram, Isaac W, Gudla, Raghava, devel@edk2.groups.io
Cc: Desimone, Nathaniel L, Kubacki, Michael, Chaganty, Rangasai V,
Chiu, Chasel
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.oram@intel.com>
> Sent: Monday, May 8, 2023 12:15 PM
> To: Gudla, Raghava <raghava.gudla@intel.com>; devel@edk2.groups.io
> Cc: Chiu, Chasel <chasel.chiu@intel.com>; Desimone, Nathaniel L
> <nathaniel.l.desimone@intel.com>; Kubacki, Michael
> <michael.kubacki@microsoft.com>; Chaganty, Rangasai V
> <rangasai.v.chaganty@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.gudla@intel.com>
> Sent: Friday, May 5, 2023 5:03 PM
> To: devel@edk2.groups.io
> Cc: Gudla, Raghava <raghava.gudla@intel.com>; Chiu, Chasel
> <chasel.chiu@intel.com>; Desimone, Nathaniel L
> <nathaniel.l.desimone@intel.com>; Oram, Isaac W <isaac.w.oram@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.chiu@intel.com>
> Cc: Nate DeSimone <nathaniel.l.desimone@intel.com>
> Cc: Isaac Oram <isaac.w.oram@intel.com>
>
> Signed-off-by: Raghava Gudla <raghava.gudla@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/SaveMemor
> yConfig.c
> b/Platform/Intel/MinPlatformPkg/FspWrapper/SaveMemoryConfig/SaveMemor
> yConfig.c
> index 0215e8eed..8aa935b54 100644
> ---
> a/Platform/Intel/MinPlatformPkg/FspWrapper/SaveMemoryConfig/SaveMemor
> yConfig.c
> +++
> b/Platform/Intel/MinPlatformPkg/FspWrapper/SaveMemoryConfig/SaveMemo
> +++ 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);+
> 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/SaveMemor
> yConfig.inf
> b/Platform/Intel/MinPlatformPkg/FspWrapper/SaveMemoryConfig/SaveMemor
> yConfig.inf
> index 61e85a658..77920d031 100644
> ---
> a/Platform/Intel/MinPlatformPkg/FspWrapper/SaveMemoryConfig/SaveMemor
> yConfig.inf
> +++
> b/Platform/Intel/MinPlatformPkg/FspWrapper/SaveMemoryConfig/SaveMemo
> +++ 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/BaseBmpSupportLi
> 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|0x900000
> 30
> gMinPlatformPkgTokenSpaceGuid.PcdFadtMinorVersion|0x03|UINT8|0x900000
> 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|BOOL
> EAN|0x90000032 [PcdsFixedAtBuild] --
> 2.19.1.windows.1
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [edk2-devel] [edk2-platforms:PATCH V1] MinPlatformPkg/SaveMemoryConfig: Support NVS Data compression.
2023-05-08 19:36 ` Chiu, Chasel
@ 2023-05-08 21:48 ` Michael D Kinney
2023-05-09 15:25 ` Michael Kubacki
0 siblings, 1 reply; 13+ messages in thread
From: Michael D Kinney @ 2023-05-08 21:48 UTC (permalink / raw)
To: devel@edk2.groups.io, Chiu, Chasel, Oram, Isaac W, Gudla, Raghava
Cc: Desimone, Nathaniel L, Kubacki, Michael, Chaganty, Rangasai V,
Kinney, Michael D
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.oram@intel.com>; Gudla, Raghava
> <raghava.gudla@intel.com>; devel@edk2.groups.io
> Cc: Desimone, Nathaniel L <nathaniel.l.desimone@intel.com>; Kubacki,
> Michael <michael.kubacki@microsoft.com>; Chaganty, Rangasai V
> <rangasai.v.chaganty@intel.com>; Chiu, Chasel <chasel.chiu@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.oram@intel.com>
> > Sent: Monday, May 8, 2023 12:15 PM
> > To: Gudla, Raghava <raghava.gudla@intel.com>; devel@edk2.groups.io
> > Cc: Chiu, Chasel <chasel.chiu@intel.com>; Desimone, Nathaniel L
> > <nathaniel.l.desimone@intel.com>; Kubacki, Michael
> > <michael.kubacki@microsoft.com>; Chaganty, Rangasai V
> > <rangasai.v.chaganty@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.gudla@intel.com>
> > Sent: Friday, May 5, 2023 5:03 PM
> > To: devel@edk2.groups.io
> > Cc: Gudla, Raghava <raghava.gudla@intel.com>; Chiu, Chasel
> > <chasel.chiu@intel.com>; Desimone, Nathaniel L
> > <nathaniel.l.desimone@intel.com>; Oram, Isaac W
> <isaac.w.oram@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.chiu@intel.com>
> > Cc: Nate DeSimone <nathaniel.l.desimone@intel.com>
> > Cc: Isaac Oram <isaac.w.oram@intel.com>
> >
> > Signed-off-by: Raghava Gudla <raghava.gudla@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);+
> > 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
>
>
>
>
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [edk2-devel] [edk2-platforms:PATCH V1] MinPlatformPkg/SaveMemoryConfig: Support NVS Data compression.
2023-05-08 21:48 ` [edk2-devel] " Michael D Kinney
@ 2023-05-09 15:25 ` Michael Kubacki
2023-05-09 17:34 ` Isaac Oram
0 siblings, 1 reply; 13+ messages in thread
From: Michael Kubacki @ 2023-05-09 15:25 UTC (permalink / raw)
To: devel, michael.d.kinney, Chiu, Chasel, Oram, Isaac W,
Gudla, Raghava
Cc: Desimone, Nathaniel L, Kubacki, Michael, Chaganty, Rangasai V
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 <devel@edk2.groups.io> On Behalf Of Chiu,
>> Chasel
>> Sent: Monday, May 8, 2023 12:37 PM
>> To: Oram, Isaac W <isaac.w.oram@intel.com>; Gudla, Raghava
>> <raghava.gudla@intel.com>; devel@edk2.groups.io
>> Cc: Desimone, Nathaniel L <nathaniel.l.desimone@intel.com>; Kubacki,
>> Michael <michael.kubacki@microsoft.com>; Chaganty, Rangasai V
>> <rangasai.v.chaganty@intel.com>; Chiu, Chasel <chasel.chiu@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.oram@intel.com>
>>> Sent: Monday, May 8, 2023 12:15 PM
>>> To: Gudla, Raghava <raghava.gudla@intel.com>; devel@edk2.groups.io
>>> Cc: Chiu, Chasel <chasel.chiu@intel.com>; Desimone, Nathaniel L
>>> <nathaniel.l.desimone@intel.com>; Kubacki, Michael
>>> <michael.kubacki@microsoft.com>; Chaganty, Rangasai V
>>> <rangasai.v.chaganty@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.gudla@intel.com>
>>> Sent: Friday, May 5, 2023 5:03 PM
>>> To: devel@edk2.groups.io
>>> Cc: Gudla, Raghava <raghava.gudla@intel.com>; Chiu, Chasel
>>> <chasel.chiu@intel.com>; Desimone, Nathaniel L
>>> <nathaniel.l.desimone@intel.com>; Oram, Isaac W
>> <isaac.w.oram@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.chiu@intel.com>
>>> Cc: Nate DeSimone <nathaniel.l.desimone@intel.com>
>>> Cc: Isaac Oram <isaac.w.oram@intel.com>
>>>
>>> Signed-off-by: Raghava Gudla <raghava.gudla@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);+
>>> 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
>>
>>
>>
>>
>>
>
>
>
>
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [edk2-devel] [edk2-platforms:PATCH V1] MinPlatformPkg/SaveMemoryConfig: Support NVS Data compression.
2023-05-09 15:25 ` Michael Kubacki
@ 2023-05-09 17:34 ` Isaac Oram
2023-05-09 17:41 ` Gudla, Raghava
0 siblings, 1 reply; 13+ messages in thread
From: Isaac Oram @ 2023-05-09 17:34 UTC (permalink / raw)
To: devel@edk2.groups.io, mikuback@linux.microsoft.com,
Kinney, Michael D, Chiu, Chasel, Gudla, Raghava
Cc: Desimone, Nathaniel L, Kubacki, Michael, Chaganty, Rangasai V
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.kinney@intel.com>; Chiu, Chasel <chasel.chiu@intel.com>; Oram, Isaac W <isaac.w.oram@intel.com>; Gudla, Raghava <raghava.gudla@intel.com>
Cc: Desimone, Nathaniel L <nathaniel.l.desimone@intel.com>; Kubacki, Michael <michael.kubacki@microsoft.com>; Chaganty, Rangasai V <rangasai.v.chaganty@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.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 <devel@edk2.groups.io> On Behalf Of Chiu,
>> Chasel
>> Sent: Monday, May 8, 2023 12:37 PM
>> To: Oram, Isaac W <isaac.w.oram@intel.com>; Gudla, Raghava
>> <raghava.gudla@intel.com>; devel@edk2.groups.io
>> Cc: Desimone, Nathaniel L <nathaniel.l.desimone@intel.com>; Kubacki,
>> Michael <michael.kubacki@microsoft.com>; Chaganty, Rangasai V
>> <rangasai.v.chaganty@intel.com>; Chiu, Chasel <chasel.chiu@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.oram@intel.com>
>>> Sent: Monday, May 8, 2023 12:15 PM
>>> To: Gudla, Raghava <raghava.gudla@intel.com>; devel@edk2.groups.io
>>> Cc: Chiu, Chasel <chasel.chiu@intel.com>; Desimone, Nathaniel L
>>> <nathaniel.l.desimone@intel.com>; Kubacki, Michael
>>> <michael.kubacki@microsoft.com>; Chaganty, Rangasai V
>>> <rangasai.v.chaganty@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.gudla@intel.com>
>>> Sent: Friday, May 5, 2023 5:03 PM
>>> To: devel@edk2.groups.io
>>> Cc: Gudla, Raghava <raghava.gudla@intel.com>; Chiu, Chasel
>>> <chasel.chiu@intel.com>; Desimone, Nathaniel L
>>> <nathaniel.l.desimone@intel.com>; Oram, Isaac W
>> <isaac.w.oram@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.chiu@intel.com>
>>> Cc: Nate DeSimone <nathaniel.l.desimone@intel.com>
>>> Cc: Isaac Oram <isaac.w.oram@intel.com>
>>>
>>> Signed-off-by: Raghava Gudla <raghava.gudla@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);+
>>> 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/Base
>> LargeVariableReadLib|Larg
>>> eVariableReadLib.inf
>>>
>> LargeVariableWriteLib|MinPlatformPkg/Library/BaseLargeVariableLib/Bas
>> 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
>>
>>
>>
>>
>>
>
>
>
>
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [edk2-devel] [edk2-platforms:PATCH V1] MinPlatformPkg/SaveMemoryConfig: Support NVS Data compression.
2023-05-09 17:34 ` Isaac Oram
@ 2023-05-09 17:41 ` Gudla, Raghava
2023-05-09 18:05 ` Michael Kubacki
0 siblings, 1 reply; 13+ messages in thread
From: Gudla, Raghava @ 2023-05-09 17:41 UTC (permalink / raw)
To: Oram, Isaac W, devel@edk2.groups.io, mikuback@linux.microsoft.com,
Kinney, Michael D, Chiu, Chasel
Cc: Desimone, Nathaniel L, Kubacki, Michael, Chaganty, Rangasai V
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.oram@intel.com>
Sent: Tuesday, May 9, 2023 10:35 AM
To: devel@edk2.groups.io; mikuback@linux.microsoft.com; Kinney, Michael D <michael.d.kinney@intel.com>; Chiu, Chasel <chasel.chiu@intel.com>; Gudla, Raghava <raghava.gudla@intel.com>
Cc: Desimone, Nathaniel L <nathaniel.l.desimone@intel.com>; Kubacki, Michael <michael.kubacki@microsoft.com>; Chaganty, Rangasai V <rangasai.v.chaganty@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.kinney@intel.com>; Chiu, Chasel <chasel.chiu@intel.com>; Oram, Isaac W <isaac.w.oram@intel.com>; Gudla, Raghava <raghava.gudla@intel.com>
Cc: Desimone, Nathaniel L <nathaniel.l.desimone@intel.com>; Kubacki, Michael <michael.kubacki@microsoft.com>; Chaganty, Rangasai V <rangasai.v.chaganty@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.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 <devel@edk2.groups.io> On Behalf Of Chiu,
>> Chasel
>> Sent: Monday, May 8, 2023 12:37 PM
>> To: Oram, Isaac W <isaac.w.oram@intel.com>; Gudla, Raghava
>> <raghava.gudla@intel.com>; devel@edk2.groups.io
>> Cc: Desimone, Nathaniel L <nathaniel.l.desimone@intel.com>; Kubacki,
>> Michael <michael.kubacki@microsoft.com>; Chaganty, Rangasai V
>> <rangasai.v.chaganty@intel.com>; Chiu, Chasel <chasel.chiu@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.oram@intel.com>
>>> Sent: Monday, May 8, 2023 12:15 PM
>>> To: Gudla, Raghava <raghava.gudla@intel.com>; devel@edk2.groups.io
>>> Cc: Chiu, Chasel <chasel.chiu@intel.com>; Desimone, Nathaniel L
>>> <nathaniel.l.desimone@intel.com>; Kubacki, Michael
>>> <michael.kubacki@microsoft.com>; Chaganty, Rangasai V
>>> <rangasai.v.chaganty@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.gudla@intel.com>
>>> Sent: Friday, May 5, 2023 5:03 PM
>>> To: devel@edk2.groups.io
>>> Cc: Gudla, Raghava <raghava.gudla@intel.com>; Chiu, Chasel
>>> <chasel.chiu@intel.com>; Desimone, Nathaniel L
>>> <nathaniel.l.desimone@intel.com>; Oram, Isaac W
>> <isaac.w.oram@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.chiu@intel.com>
>>> Cc: Nate DeSimone <nathaniel.l.desimone@intel.com>
>>> Cc: Isaac Oram <isaac.w.oram@intel.com>
>>>
>>> Signed-off-by: Raghava Gudla <raghava.gudla@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);+
>>> 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/Base
>> LargeVariableReadLib|Larg
>>> eVariableReadLib.inf
>>>
>> LargeVariableWriteLib|MinPlatformPkg/Library/BaseLargeVariableLib/Bas
>> 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
>>
>>
>>
>>
>>
>
>
>
>
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [edk2-devel] [edk2-platforms:PATCH V1] MinPlatformPkg/SaveMemoryConfig: Support NVS Data compression.
2023-05-09 17:41 ` Gudla, Raghava
@ 2023-05-09 18:05 ` Michael Kubacki
2023-05-09 18:56 ` Chiu, Chasel
0 siblings, 1 reply; 13+ messages in thread
From: Michael Kubacki @ 2023-05-09 18:05 UTC (permalink / raw)
To: Gudla, Raghava, Oram, Isaac W, devel@edk2.groups.io,
Kinney, Michael D, Chiu, Chasel
Cc: Desimone, Nathaniel L, Kubacki, Michael, Chaganty, Rangasai V
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.oram@intel.com>
> Sent: Tuesday, May 9, 2023 10:35 AM
> To: devel@edk2.groups.io; mikuback@linux.microsoft.com; Kinney, Michael D <michael.d.kinney@intel.com>; Chiu, Chasel <chasel.chiu@intel.com>; Gudla, Raghava <raghava.gudla@intel.com>
> Cc: Desimone, Nathaniel L <nathaniel.l.desimone@intel.com>; Kubacki, Michael <michael.kubacki@microsoft.com>; Chaganty, Rangasai V <rangasai.v.chaganty@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.kinney@intel.com>; Chiu, Chasel <chasel.chiu@intel.com>; Oram, Isaac W <isaac.w.oram@intel.com>; Gudla, Raghava <raghava.gudla@intel.com>
> Cc: Desimone, Nathaniel L <nathaniel.l.desimone@intel.com>; Kubacki, Michael <michael.kubacki@microsoft.com>; Chaganty, Rangasai V <rangasai.v.chaganty@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.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 <devel@edk2.groups.io> On Behalf Of Chiu,
>>> Chasel
>>> Sent: Monday, May 8, 2023 12:37 PM
>>> To: Oram, Isaac W <isaac.w.oram@intel.com>; Gudla, Raghava
>>> <raghava.gudla@intel.com>; devel@edk2.groups.io
>>> Cc: Desimone, Nathaniel L <nathaniel.l.desimone@intel.com>; Kubacki,
>>> Michael <michael.kubacki@microsoft.com>; Chaganty, Rangasai V
>>> <rangasai.v.chaganty@intel.com>; Chiu, Chasel <chasel.chiu@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.oram@intel.com>
>>>> Sent: Monday, May 8, 2023 12:15 PM
>>>> To: Gudla, Raghava <raghava.gudla@intel.com>; devel@edk2.groups.io
>>>> Cc: Chiu, Chasel <chasel.chiu@intel.com>; Desimone, Nathaniel L
>>>> <nathaniel.l.desimone@intel.com>; Kubacki, Michael
>>>> <michael.kubacki@microsoft.com>; Chaganty, Rangasai V
>>>> <rangasai.v.chaganty@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.gudla@intel.com>
>>>> Sent: Friday, May 5, 2023 5:03 PM
>>>> To: devel@edk2.groups.io
>>>> Cc: Gudla, Raghava <raghava.gudla@intel.com>; Chiu, Chasel
>>>> <chasel.chiu@intel.com>; Desimone, Nathaniel L
>>>> <nathaniel.l.desimone@intel.com>; Oram, Isaac W
>>> <isaac.w.oram@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.chiu@intel.com>
>>>> Cc: Nate DeSimone <nathaniel.l.desimone@intel.com>
>>>> Cc: Isaac Oram <isaac.w.oram@intel.com>
>>>>
>>>> Signed-off-by: Raghava Gudla <raghava.gudla@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);+
>>>> 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/Base
>>> LargeVariableReadLib|Larg
>>>> eVariableReadLib.inf
>>>>
>>> LargeVariableWriteLib|MinPlatformPkg/Library/BaseLargeVariableLib/Bas
>>> 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
>>>
>>>
>>>
>>>
>>>
>>
>>
>>
>>
>>
>
>
>
>
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [edk2-devel] [edk2-platforms:PATCH V1] MinPlatformPkg/SaveMemoryConfig: Support NVS Data compression.
2023-05-09 18:05 ` Michael Kubacki
@ 2023-05-09 18:56 ` Chiu, Chasel
2023-05-09 19:17 ` Michael Kubacki
0 siblings, 1 reply; 13+ messages in thread
From: Chiu, Chasel @ 2023-05-09 18:56 UTC (permalink / raw)
To: Michael Kubacki, Gudla, Raghava, Oram, Isaac W,
devel@edk2.groups.io, Kinney, Michael D
Cc: Desimone, Nathaniel L, Kubacki, Michael, Chaganty, Rangasai V
Hi Michael Kubacki,
Since Rahava has rebased the patch it might be easier to add " Signed-off-by: Michael Kubacki <michael.kubacki@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 <mikuback@linux.microsoft.com>
> Sent: Tuesday, May 9, 2023 11:06 AM
> To: Gudla, Raghava <raghava.gudla@intel.com>; Oram, Isaac W
> <isaac.w.oram@intel.com>; devel@edk2.groups.io; Kinney, Michael D
> <michael.d.kinney@intel.com>; Chiu, Chasel <chasel.chiu@intel.com>
> Cc: Desimone, Nathaniel L <nathaniel.l.desimone@intel.com>; Kubacki, Michael
> <michael.kubacki@microsoft.com>; Chaganty, Rangasai V
> <rangasai.v.chaganty@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.oram@intel.com>
> > Sent: Tuesday, May 9, 2023 10:35 AM
> > To: devel@edk2.groups.io; mikuback@linux.microsoft.com; Kinney,
> > Michael D <michael.d.kinney@intel.com>; Chiu, Chasel
> > <chasel.chiu@intel.com>; Gudla, Raghava <raghava.gudla@intel.com>
> > Cc: Desimone, Nathaniel L <nathaniel.l.desimone@intel.com>; Kubacki,
> > Michael <michael.kubacki@microsoft.com>; Chaganty, Rangasai V
> > <rangasai.v.chaganty@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.kinney@intel.com>; Chiu, Chasel <chasel.chiu@intel.com>;
> > Oram, Isaac W <isaac.w.oram@intel.com>; Gudla, Raghava
> > <raghava.gudla@intel.com>
> > Cc: Desimone, Nathaniel L <nathaniel.l.desimone@intel.com>; Kubacki,
> > Michael <michael.kubacki@microsoft.com>; Chaganty, Rangasai V
> > <rangasai.v.chaganty@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.oram@intel.com>; Gudla, Raghava
> >>> <raghava.gudla@intel.com>; devel@edk2.groups.io
> >>> Cc: Desimone, Nathaniel L <nathaniel.l.desimone@intel.com>; Kubacki,
> >>> Michael <michael.kubacki@microsoft.com>; Chaganty, Rangasai V
> >>> <rangasai.v.chaganty@intel.com>; Chiu, Chasel
> >>> <chasel.chiu@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.oram@intel.com>
> >>>> Sent: Monday, May 8, 2023 12:15 PM
> >>>> To: Gudla, Raghava <raghava.gudla@intel.com>; devel@edk2.groups.io
> >>>> Cc: Chiu, Chasel <chasel.chiu@intel.com>; Desimone, Nathaniel L
> >>>> <nathaniel.l.desimone@intel.com>; Kubacki, Michael
> >>>> <michael.kubacki@microsoft.com>; Chaganty, Rangasai V
> >>>> <rangasai.v.chaganty@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.gudla@intel.com>
> >>>> Sent: Friday, May 5, 2023 5:03 PM
> >>>> To: devel@edk2.groups.io
> >>>> Cc: Gudla, Raghava <raghava.gudla@intel.com>; Chiu, Chasel
> >>>> <chasel.chiu@intel.com>; Desimone, Nathaniel L
> >>>> <nathaniel.l.desimone@intel.com>; Oram, Isaac W
> >>> <isaac.w.oram@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.chiu@intel.com>
> >>>> Cc: Nate DeSimone <nathaniel.l.desimone@intel.com>
> >>>> Cc: Isaac Oram <isaac.w.oram@intel.com>
> >>>>
> >>>> Signed-off-by: Raghava Gudla <raghava.gudla@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
> >>>
> >>>
> >>>
> >>>
> >>>
> >>
> >>
> >>
> >>
> >>
> >
> >
> >
> >
> >
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [edk2-devel] [edk2-platforms:PATCH V1] MinPlatformPkg/SaveMemoryConfig: Support NVS Data compression.
2023-05-09 18:56 ` Chiu, Chasel
@ 2023-05-09 19:17 ` Michael Kubacki
2023-05-09 19:29 ` Chiu, Chasel
0 siblings, 1 reply; 13+ messages in thread
From: Michael Kubacki @ 2023-05-09 19:17 UTC (permalink / raw)
To: Chiu, Chasel, Michael Kubacki, Gudla, Raghava, Oram, Isaac W,
devel@edk2.groups.io, Kinney, Michael D
Cc: Desimone, Nathaniel L, Chaganty, Rangasai V
I haven't done a deep comparison of the changes, but I'd lean toward using what is in the branch that I referenced earlier since it has been used for a while now.
Is there anything of value in the new set of changes?
Thanks,
Michael
-----Original Message-----
From: Chiu, Chasel <chasel.chiu@intel.com>
Sent: Tuesday, May 9, 2023 2:56 PM
To: Michael Kubacki <mikuback@linux.microsoft.com>; Gudla, Raghava <raghava.gudla@intel.com>; Oram, Isaac W <isaac.w.oram@intel.com>; devel@edk2.groups.io; Kinney, Michael D <michael.d.kinney@intel.com>
Cc: Desimone, Nathaniel L <nathaniel.l.desimone@intel.com>; Michael Kubacki <Michael.Kubacki@microsoft.com>; Chaganty, Rangasai V <rangasai.v.chaganty@intel.com>
Subject: [EXTERNAL] RE: [edk2-devel] [edk2-platforms:PATCH V1] MinPlatformPkg/SaveMemoryConfig: Support NVS Data compression.
Hi Michael Kubacki,
Since Rahava has rebased the patch it might be easier to add " Signed-off-by: Michael Kubacki <michael.kubacki@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 <mikuback@linux.microsoft.com>
> Sent: Tuesday, May 9, 2023 11:06 AM
> To: Gudla, Raghava <raghava.gudla@intel.com>; Oram, Isaac W
> <isaac.w.oram@intel.com>; devel@edk2.groups.io; Kinney, Michael D
> <michael.d.kinney@intel.com>; Chiu, Chasel <chasel.chiu@intel.com>
> Cc: Desimone, Nathaniel L <nathaniel.l.desimone@intel.com>; Kubacki,
> Michael <michael.kubacki@microsoft.com>; Chaganty, Rangasai V
> <rangasai.v.chaganty@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.oram@intel.com>
> > Sent: Tuesday, May 9, 2023 10:35 AM
> > To: devel@edk2.groups.io; mikuback@linux.microsoft.com; Kinney,
> > Michael D <michael.d.kinney@intel.com>; Chiu, Chasel
> > <chasel.chiu@intel.com>; Gudla, Raghava <raghava.gudla@intel.com>
> > Cc: Desimone, Nathaniel L <nathaniel.l.desimone@intel.com>; Kubacki,
> > Michael <michael.kubacki@microsoft.com>; Chaganty, Rangasai V
> > <rangasai.v.chaganty@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.kinney@intel.com>; Chiu, Chasel <chasel.chiu@intel.com>;
> > Oram, Isaac W <isaac.w.oram@intel.com>; Gudla, Raghava
> > <raghava.gudla@intel.com>
> > Cc: Desimone, Nathaniel L <nathaniel.l.desimone@intel.com>; Kubacki,
> > Michael <michael.kubacki@microsoft.com>; Chaganty, Rangasai V
> > <rangasai.v.chaganty@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://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fed
> > k2.groups.io%2Fg%2Fdevel%2Fmessage%2F92644&data=05%7C01%7Cmichael.ku
> > backi%40microsoft.com%7C05c93413c875400acef708db50bf197d%7C72f988bf8
> > 6f141af91ab2d7cd011db47%7C1%7C0%7C638192553936149621%7CUnknown%7CTWF
> > pbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI
> > 6Mn0%3D%7C3000%7C%7C%7C&sdata=E4hpQLMgLy0BKHngjpec88AI0v3TZ8xgxebjU0
> > SS0FM%3D&reserved=0
> >
> > 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://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgi
> > thub.com%2Fmicrosoft%2Fmu_common_intel_min_platform%2Fblob%2Frelease
> > &data=05%7C01%7Cmichael.kubacki%40microsoft.com%7C05c93413c875400ace
> > f708db50bf197d%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C63819255
> > 3936305834%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luM
> > zIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=lXFLZV78CPev
> > UL7De1%2B6RtHaKWQcjnXx6IFABcmWJ54%3D&reserved=0
> >
> /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.oram@intel.com>; Gudla, Raghava
> >>> <raghava.gudla@intel.com>; devel@edk2.groups.io
> >>> Cc: Desimone, Nathaniel L <nathaniel.l.desimone@intel.com>;
> >>> Kubacki, Michael <michael.kubacki@microsoft.com>; Chaganty,
> >>> Rangasai V <rangasai.v.chaganty@intel.com>; Chiu, Chasel
> >>> <chasel.chiu@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.oram@intel.com>
> >>>> Sent: Monday, May 8, 2023 12:15 PM
> >>>> To: Gudla, Raghava <raghava.gudla@intel.com>;
> >>>> devel@edk2.groups.io
> >>>> Cc: Chiu, Chasel <chasel.chiu@intel.com>; Desimone, Nathaniel L
> >>>> <nathaniel.l.desimone@intel.com>; Kubacki, Michael
> >>>> <michael.kubacki@microsoft.com>; Chaganty, Rangasai V
> >>>> <rangasai.v.chaganty@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.gudla@intel.com>
> >>>> Sent: Friday, May 5, 2023 5:03 PM
> >>>> To: devel@edk2.groups.io
> >>>> Cc: Gudla, Raghava <raghava.gudla@intel.com>; Chiu, Chasel
> >>>> <chasel.chiu@intel.com>; Desimone, Nathaniel L
> >>>> <nathaniel.l.desimone@intel.com>; Oram, Isaac W
> >>> <isaac.w.oram@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.chiu@intel.com>
> >>>> Cc: Nate DeSimone <nathaniel.l.desimone@intel.com>
> >>>> Cc: Isaac Oram <isaac.w.oram@intel.com>
> >>>>
> >>>> Signed-off-by: Raghava Gudla <raghava.gudla@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/B
> >>> LargeVariableReadLib|as
> >>> LargeVariableReadLib|e
> >>> LargeVariableReadLib|Larg
> >>>> eVariableReadLib.inf
> >>>>
> >>> LargeVariableWriteLib|MinPlatformPkg/Library/BaseLargeVariableLib/
> >>> LargeVariableWriteLib|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
> >>>> 31+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
> >>>
> >>>
> >>>
> >>>
> >>>
> >>
> >>
> >>
> >>
> >>
> >
> >
> >
> >
> >
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [edk2-devel] [edk2-platforms:PATCH V1] MinPlatformPkg/SaveMemoryConfig: Support NVS Data compression.
2023-05-09 19:17 ` Michael Kubacki
@ 2023-05-09 19:29 ` Chiu, Chasel
2023-05-09 21:06 ` Chiu, Chasel
0 siblings, 1 reply; 13+ messages in thread
From: Chiu, Chasel @ 2023-05-09 19:29 UTC (permalink / raw)
To: devel@edk2.groups.io, Kubacki, Michael, Michael Kubacki,
Gudla, Raghava, Oram, Isaac W, Kinney, Michael D
Cc: Desimone, Nathaniel L, Chaganty, Rangasai V
Just reviewed the branch code and it is functionality the same so agree we can switch to Mike's implementation.
Would you help to rebase and send updated patch for reviewing?
Thanks,
Chasel
> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Michael
> Kubacki via groups.io
> Sent: Tuesday, May 9, 2023 12:18 PM
> To: Chiu, Chasel <chasel.chiu@intel.com>; Michael Kubacki
> <mikuback@linux.microsoft.com>; Gudla, Raghava <raghava.gudla@intel.com>;
> Oram, Isaac W <isaac.w.oram@intel.com>; devel@edk2.groups.io; Kinney,
> Michael D <michael.d.kinney@intel.com>
> Cc: Desimone, Nathaniel L <nathaniel.l.desimone@intel.com>; Chaganty,
> Rangasai V <rangasai.v.chaganty@intel.com>
> Subject: Re: [edk2-devel] [edk2-platforms:PATCH V1]
> MinPlatformPkg/SaveMemoryConfig: Support NVS Data compression.
>
> I haven't done a deep comparison of the changes, but I'd lean toward using
> what is in the branch that I referenced earlier since it has been used for a while
> now.
>
> Is there anything of value in the new set of changes?
>
> Thanks,
> Michael
>
> -----Original Message-----
> From: Chiu, Chasel <chasel.chiu@intel.com>
> Sent: Tuesday, May 9, 2023 2:56 PM
> To: Michael Kubacki <mikuback@linux.microsoft.com>; Gudla, Raghava
> <raghava.gudla@intel.com>; Oram, Isaac W <isaac.w.oram@intel.com>;
> devel@edk2.groups.io; Kinney, Michael D <michael.d.kinney@intel.com>
> Cc: Desimone, Nathaniel L <nathaniel.l.desimone@intel.com>; Michael Kubacki
> <Michael.Kubacki@microsoft.com>; Chaganty, Rangasai V
> <rangasai.v.chaganty@intel.com>
> Subject: [EXTERNAL] RE: [edk2-devel] [edk2-platforms:PATCH V1]
> MinPlatformPkg/SaveMemoryConfig: Support NVS Data compression.
>
>
> Hi Michael Kubacki,
>
> Since Rahava has rebased the patch it might be easier to add " Signed-off-by:
> Michael Kubacki <michael.kubacki@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 <mikuback@linux.microsoft.com>
> > Sent: Tuesday, May 9, 2023 11:06 AM
> > To: Gudla, Raghava <raghava.gudla@intel.com>; Oram, Isaac W
> > <isaac.w.oram@intel.com>; devel@edk2.groups.io; Kinney, Michael D
> > <michael.d.kinney@intel.com>; Chiu, Chasel <chasel.chiu@intel.com>
> > Cc: Desimone, Nathaniel L <nathaniel.l.desimone@intel.com>; Kubacki,
> > Michael <michael.kubacki@microsoft.com>; Chaganty, Rangasai V
> > <rangasai.v.chaganty@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.oram@intel.com>
> > > Sent: Tuesday, May 9, 2023 10:35 AM
> > > To: devel@edk2.groups.io; mikuback@linux.microsoft.com; Kinney,
> > > Michael D <michael.d.kinney@intel.com>; Chiu, Chasel
> > > <chasel.chiu@intel.com>; Gudla, Raghava <raghava.gudla@intel.com>
> > > Cc: Desimone, Nathaniel L <nathaniel.l.desimone@intel.com>; Kubacki,
> > > Michael <michael.kubacki@microsoft.com>; Chaganty, Rangasai V
> > > <rangasai.v.chaganty@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.kinney@intel.com>; Chiu, Chasel <chasel.chiu@intel.com>;
> > > Oram, Isaac W <isaac.w.oram@intel.com>; Gudla, Raghava
> > > <raghava.gudla@intel.com>
> > > Cc: Desimone, Nathaniel L <nathaniel.l.desimone@intel.com>; Kubacki,
> > > Michael <michael.kubacki@microsoft.com>; Chaganty, Rangasai V
> > > <rangasai.v.chaganty@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://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fed
> > >
> k2.groups.io%2Fg%2Fdevel%2Fmessage%2F92644&data=05%7C01%7Cmichael.
> ku
> > >
> backi%40microsoft.com%7C05c93413c875400acef708db50bf197d%7C72f988bf
> 8
> > >
> 6f141af91ab2d7cd011db47%7C1%7C0%7C638192553936149621%7CUnknown
> %7CTWF
> > >
> pbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI
> > >
> 6Mn0%3D%7C3000%7C%7C%7C&sdata=E4hpQLMgLy0BKHngjpec88AI0v3TZ8xg
> xebjU0
> > > SS0FM%3D&reserved=0
> > >
> > > 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://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgi
> > >
> thub.com%2Fmicrosoft%2Fmu_common_intel_min_platform%2Fblob%2Freleas
> e
> > >
> &data=05%7C01%7Cmichael.kubacki%40microsoft.com%7C05c93413c875400a
> ce
> > >
> f708db50bf197d%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C63819
> 255
> > >
> 3936305834%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoi
> V2luM
> > >
> zIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=lXFLZV78C
> Pev
> > > UL7De1%2B6RtHaKWQcjnXx6IFABcmWJ54%3D&reserved=0
> > >
> >
> /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.oram@intel.com>; Gudla, Raghava
> > >>> <raghava.gudla@intel.com>; devel@edk2.groups.io
> > >>> Cc: Desimone, Nathaniel L <nathaniel.l.desimone@intel.com>;
> > >>> Kubacki, Michael <michael.kubacki@microsoft.com>; Chaganty,
> > >>> Rangasai V <rangasai.v.chaganty@intel.com>; Chiu, Chasel
> > >>> <chasel.chiu@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.oram@intel.com>
> > >>>> Sent: Monday, May 8, 2023 12:15 PM
> > >>>> To: Gudla, Raghava <raghava.gudla@intel.com>;
> > >>>> devel@edk2.groups.io
> > >>>> Cc: Chiu, Chasel <chasel.chiu@intel.com>; Desimone, Nathaniel L
> > >>>> <nathaniel.l.desimone@intel.com>; Kubacki, Michael
> > >>>> <michael.kubacki@microsoft.com>; Chaganty, Rangasai V
> > >>>> <rangasai.v.chaganty@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.gudla@intel.com>
> > >>>> Sent: Friday, May 5, 2023 5:03 PM
> > >>>> To: devel@edk2.groups.io
> > >>>> Cc: Gudla, Raghava <raghava.gudla@intel.com>; Chiu, Chasel
> > >>>> <chasel.chiu@intel.com>; Desimone, Nathaniel L
> > >>>> <nathaniel.l.desimone@intel.com>; Oram, Isaac W
> > >>> <isaac.w.oram@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.chiu@intel.com>
> > >>>> Cc: Nate DeSimone <nathaniel.l.desimone@intel.com>
> > >>>> Cc: Isaac Oram <isaac.w.oram@intel.com>
> > >>>>
> > >>>> Signed-off-by: Raghava Gudla <raghava.gudla@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/B
> > >>> LargeVariableReadLib|as
> > >>> LargeVariableReadLib|e
> > >>> LargeVariableReadLib|Larg
> > >>>> eVariableReadLib.inf
> > >>>>
> > >>> LargeVariableWriteLib|MinPlatformPkg/Library/BaseLargeVariableLib/
> > >>> LargeVariableWriteLib|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
> > >>>> 31+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
> > >>>
> > >>>
> > >>>
> > >>>
> > >>>
> > >>
> > >>
> > >>
> > >>
> > >>
> > >
> > >
> > >
> > >
> > >
>
>
>
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [edk2-devel] [edk2-platforms:PATCH V1] MinPlatformPkg/SaveMemoryConfig: Support NVS Data compression.
2023-05-09 19:29 ` Chiu, Chasel
@ 2023-05-09 21:06 ` Chiu, Chasel
2023-05-09 21:41 ` Michael Kubacki
0 siblings, 1 reply; 13+ messages in thread
From: Chiu, Chasel @ 2023-05-09 21:06 UTC (permalink / raw)
To: Kubacki, Michael, Michael Kubacki
Cc: Desimone, Nathaniel L, Chaganty, Rangasai V, Gudla, Raghava,
Oram, Isaac W, Kinney, Michael D, devel@edk2.groups.io
Hi Michael,
Please also add CompressLib to CoreCommonLib.dsc to prevent build failure from open source platforms.
Thanks,
Chasel
> -----Original Message-----
> From: Chiu, Chasel
> Sent: Tuesday, May 9, 2023 12:30 PM
> To: devel@edk2.groups.io; Kubacki, Michael <michael.kubacki@microsoft.com>;
> Michael Kubacki <mikuback@linux.microsoft.com>; Gudla, Raghava
> <raghava.gudla@intel.com>; Oram, Isaac W <isaac.w.oram@intel.com>; Kinney,
> Michael D <michael.d.kinney@intel.com>
> Cc: Desimone, Nathaniel L <nathaniel.l.desimone@intel.com>; Chaganty,
> Rangasai V <rangasai.v.chaganty@intel.com>
> Subject: RE: [edk2-devel] [edk2-platforms:PATCH V1]
> MinPlatformPkg/SaveMemoryConfig: Support NVS Data compression.
>
>
> Just reviewed the branch code and it is functionality the same so agree we can
> switch to Mike's implementation.
> Would you help to rebase and send updated patch for reviewing?
>
> Thanks,
> Chasel
>
>
>
> > -----Original Message-----
> > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Michael
> > Kubacki via groups.io
> > Sent: Tuesday, May 9, 2023 12:18 PM
> > To: Chiu, Chasel <chasel.chiu@intel.com>; Michael Kubacki
> > <mikuback@linux.microsoft.com>; Gudla, Raghava
> > <raghava.gudla@intel.com>; Oram, Isaac W <isaac.w.oram@intel.com>;
> > devel@edk2.groups.io; Kinney, Michael D <michael.d.kinney@intel.com>
> > Cc: Desimone, Nathaniel L <nathaniel.l.desimone@intel.com>; Chaganty,
> > Rangasai V <rangasai.v.chaganty@intel.com>
> > Subject: Re: [edk2-devel] [edk2-platforms:PATCH V1]
> > MinPlatformPkg/SaveMemoryConfig: Support NVS Data compression.
> >
> > I haven't done a deep comparison of the changes, but I'd lean toward
> > using what is in the branch that I referenced earlier since it has
> > been used for a while now.
> >
> > Is there anything of value in the new set of changes?
> >
> > Thanks,
> > Michael
> >
> > -----Original Message-----
> > From: Chiu, Chasel <chasel.chiu@intel.com>
> > Sent: Tuesday, May 9, 2023 2:56 PM
> > To: Michael Kubacki <mikuback@linux.microsoft.com>; Gudla, Raghava
> > <raghava.gudla@intel.com>; Oram, Isaac W <isaac.w.oram@intel.com>;
> > devel@edk2.groups.io; Kinney, Michael D <michael.d.kinney@intel.com>
> > Cc: Desimone, Nathaniel L <nathaniel.l.desimone@intel.com>; Michael
> > Kubacki <Michael.Kubacki@microsoft.com>; Chaganty, Rangasai V
> > <rangasai.v.chaganty@intel.com>
> > Subject: [EXTERNAL] RE: [edk2-devel] [edk2-platforms:PATCH V1]
> > MinPlatformPkg/SaveMemoryConfig: Support NVS Data compression.
> >
> >
> > Hi Michael Kubacki,
> >
> > Since Rahava has rebased the patch it might be easier to add " Signed-off-by:
> > Michael Kubacki <michael.kubacki@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 <mikuback@linux.microsoft.com>
> > > Sent: Tuesday, May 9, 2023 11:06 AM
> > > To: Gudla, Raghava <raghava.gudla@intel.com>; Oram, Isaac W
> > > <isaac.w.oram@intel.com>; devel@edk2.groups.io; Kinney, Michael D
> > > <michael.d.kinney@intel.com>; Chiu, Chasel <chasel.chiu@intel.com>
> > > Cc: Desimone, Nathaniel L <nathaniel.l.desimone@intel.com>; Kubacki,
> > > Michael <michael.kubacki@microsoft.com>; Chaganty, Rangasai V
> > > <rangasai.v.chaganty@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.oram@intel.com>
> > > > Sent: Tuesday, May 9, 2023 10:35 AM
> > > > To: devel@edk2.groups.io; mikuback@linux.microsoft.com; Kinney,
> > > > Michael D <michael.d.kinney@intel.com>; Chiu, Chasel
> > > > <chasel.chiu@intel.com>; Gudla, Raghava <raghava.gudla@intel.com>
> > > > Cc: Desimone, Nathaniel L <nathaniel.l.desimone@intel.com>;
> > > > Kubacki, Michael <michael.kubacki@microsoft.com>; Chaganty,
> > > > Rangasai V <rangasai.v.chaganty@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.kinney@intel.com>; Chiu, Chasel
> > > > <chasel.chiu@intel.com>; Oram, Isaac W <isaac.w.oram@intel.com>;
> > > > Gudla, Raghava <raghava.gudla@intel.com>
> > > > Cc: Desimone, Nathaniel L <nathaniel.l.desimone@intel.com>;
> > > > Kubacki, Michael <michael.kubacki@microsoft.com>; Chaganty,
> > > > Rangasai V <rangasai.v.chaganty@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://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2F
> > > > ed
> > > >
> >
> k2.groups.io%2Fg%2Fdevel%2Fmessage%2F92644&data=05%7C01%7Cmichael.
> > ku
> > > >
> >
> backi%40microsoft.com%7C05c93413c875400acef708db50bf197d%7C72f988bf
> > 8
> > > >
> >
> 6f141af91ab2d7cd011db47%7C1%7C0%7C638192553936149621%7CUnknown
> > %7CTWF
> > > >
> >
> pbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI
> > > >
> >
> 6Mn0%3D%7C3000%7C%7C%7C&sdata=E4hpQLMgLy0BKHngjpec88AI0v3TZ8xg
> > xebjU0
> > > > SS0FM%3D&reserved=0
> > > >
> > > > 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://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2F
> > > > gi
> > > >
> >
> thub.com%2Fmicrosoft%2Fmu_common_intel_min_platform%2Fblob%2Freleas
> > e
> > > >
> >
> &data=05%7C01%7Cmichael.kubacki%40microsoft.com%7C05c93413c875400a
> > ce
> > > >
> >
> f708db50bf197d%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C63819
> > 255
> > > >
> >
> 3936305834%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoi
> > V2luM
> > > >
> >
> zIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=lXFLZV78C
> > Pev
> > > > UL7De1%2B6RtHaKWQcjnXx6IFABcmWJ54%3D&reserved=0
> > > >
> > >
> >
> /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.oram@intel.com>; Gudla, Raghava
> > > >>> <raghava.gudla@intel.com>; devel@edk2.groups.io
> > > >>> Cc: Desimone, Nathaniel L <nathaniel.l.desimone@intel.com>;
> > > >>> Kubacki, Michael <michael.kubacki@microsoft.com>; Chaganty,
> > > >>> Rangasai V <rangasai.v.chaganty@intel.com>; Chiu, Chasel
> > > >>> <chasel.chiu@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.oram@intel.com>
> > > >>>> Sent: Monday, May 8, 2023 12:15 PM
> > > >>>> To: Gudla, Raghava <raghava.gudla@intel.com>;
> > > >>>> devel@edk2.groups.io
> > > >>>> Cc: Chiu, Chasel <chasel.chiu@intel.com>; Desimone, Nathaniel L
> > > >>>> <nathaniel.l.desimone@intel.com>; Kubacki, Michael
> > > >>>> <michael.kubacki@microsoft.com>; Chaganty, Rangasai V
> > > >>>> <rangasai.v.chaganty@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.gudla@intel.com>
> > > >>>> Sent: Friday, May 5, 2023 5:03 PM
> > > >>>> To: devel@edk2.groups.io
> > > >>>> Cc: Gudla, Raghava <raghava.gudla@intel.com>; Chiu, Chasel
> > > >>>> <chasel.chiu@intel.com>; Desimone, Nathaniel L
> > > >>>> <nathaniel.l.desimone@intel.com>; Oram, Isaac W
> > > >>> <isaac.w.oram@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.chiu@intel.com>
> > > >>>> Cc: Nate DeSimone <nathaniel.l.desimone@intel.com>
> > > >>>> Cc: Isaac Oram <isaac.w.oram@intel.com>
> > > >>>>
> > > >>>> Signed-off-by: Raghava Gudla <raghava.gudla@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.d
> > > >>>> +++ sc
> > > >>>> @@ -147,6 +147,7 @@
> > > >>>>
> > > >>>>
> > > >>>
> > >
> BmpSupportLib|MdeModulePkg/Library/BaseBmpSupportLib/BaseBmpSuppor
> > > >>> tLi
> > > >>>> b.inf
> > > >>>>
> > > >>> LargeVariableReadLib|MinPlatformPkg/Library/BaseLargeVariableLib
> > > >>> LargeVariableReadLib|/B
> > > >>> LargeVariableReadLib|as
> > > >>> LargeVariableReadLib|e
> > > >>> LargeVariableReadLib|Larg
> > > >>>> eVariableReadLib.inf
> > > >>>>
> > > >>> LargeVariableWriteLib|MinPlatformPkg/Library/BaseLargeVariableLi
> > > >>> LargeVariableWriteLib|b/
> > > >>> LargeVariableWriteLib|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
> > > >>>> 31+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
> > > >>>
> > > >>>
> > > >>>
> > > >>>
> > > >>>
> > > >>
> > > >>
> > > >>
> > > >>
> > > >>
> > > >
> > > >
> > > >
> > > >
> > > >
> >
> >
> >
> >
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [edk2-devel] [edk2-platforms:PATCH V1] MinPlatformPkg/SaveMemoryConfig: Support NVS Data compression.
2023-05-09 21:06 ` Chiu, Chasel
@ 2023-05-09 21:41 ` Michael Kubacki
0 siblings, 0 replies; 13+ messages in thread
From: Michael Kubacki @ 2023-05-09 21:41 UTC (permalink / raw)
To: devel, chasel.chiu, Kubacki, Michael
Cc: Desimone, Nathaniel L, Chaganty, Rangasai V, Gudla, Raghava,
Oram, Isaac W, Kinney, Michael D
Will do. I'll send the patch series soon.
Thanks,
Michael
On 5/9/2023 5:06 PM, Chiu, Chasel wrote:
>
> Hi Michael,
>
> Please also add CompressLib to CoreCommonLib.dsc to prevent build failure from open source platforms.
>
> Thanks,
> Chasel
>
>
>> -----Original Message-----
>> From: Chiu, Chasel
>> Sent: Tuesday, May 9, 2023 12:30 PM
>> To: devel@edk2.groups.io; Kubacki, Michael <michael.kubacki@microsoft.com>;
>> Michael Kubacki <mikuback@linux.microsoft.com>; Gudla, Raghava
>> <raghava.gudla@intel.com>; Oram, Isaac W <isaac.w.oram@intel.com>; Kinney,
>> Michael D <michael.d.kinney@intel.com>
>> Cc: Desimone, Nathaniel L <nathaniel.l.desimone@intel.com>; Chaganty,
>> Rangasai V <rangasai.v.chaganty@intel.com>
>> Subject: RE: [edk2-devel] [edk2-platforms:PATCH V1]
>> MinPlatformPkg/SaveMemoryConfig: Support NVS Data compression.
>>
>>
>> Just reviewed the branch code and it is functionality the same so agree we can
>> switch to Mike's implementation.
>> Would you help to rebase and send updated patch for reviewing?
>>
>> Thanks,
>> Chasel
>>
>>
>>
>>> -----Original Message-----
>>> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Michael
>>> Kubacki via groups.io
>>> Sent: Tuesday, May 9, 2023 12:18 PM
>>> To: Chiu, Chasel <chasel.chiu@intel.com>; Michael Kubacki
>>> <mikuback@linux.microsoft.com>; Gudla, Raghava
>>> <raghava.gudla@intel.com>; Oram, Isaac W <isaac.w.oram@intel.com>;
>>> devel@edk2.groups.io; Kinney, Michael D <michael.d.kinney@intel.com>
>>> Cc: Desimone, Nathaniel L <nathaniel.l.desimone@intel.com>; Chaganty,
>>> Rangasai V <rangasai.v.chaganty@intel.com>
>>> Subject: Re: [edk2-devel] [edk2-platforms:PATCH V1]
>>> MinPlatformPkg/SaveMemoryConfig: Support NVS Data compression.
>>>
>>> I haven't done a deep comparison of the changes, but I'd lean toward
>>> using what is in the branch that I referenced earlier since it has
>>> been used for a while now.
>>>
>>> Is there anything of value in the new set of changes?
>>>
>>> Thanks,
>>> Michael
>>>
>>> -----Original Message-----
>>> From: Chiu, Chasel <chasel.chiu@intel.com>
>>> Sent: Tuesday, May 9, 2023 2:56 PM
>>> To: Michael Kubacki <mikuback@linux.microsoft.com>; Gudla, Raghava
>>> <raghava.gudla@intel.com>; Oram, Isaac W <isaac.w.oram@intel.com>;
>>> devel@edk2.groups.io; Kinney, Michael D <michael.d.kinney@intel.com>
>>> Cc: Desimone, Nathaniel L <nathaniel.l.desimone@intel.com>; Michael
>>> Kubacki <Michael.Kubacki@microsoft.com>; Chaganty, Rangasai V
>>> <rangasai.v.chaganty@intel.com>
>>> Subject: [EXTERNAL] RE: [edk2-devel] [edk2-platforms:PATCH V1]
>>> MinPlatformPkg/SaveMemoryConfig: Support NVS Data compression.
>>>
>>>
>>> Hi Michael Kubacki,
>>>
>>> Since Rahava has rebased the patch it might be easier to add " Signed-off-by:
>>> Michael Kubacki <michael.kubacki@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 <mikuback@linux.microsoft.com>
>>>> Sent: Tuesday, May 9, 2023 11:06 AM
>>>> To: Gudla, Raghava <raghava.gudla@intel.com>; Oram, Isaac W
>>>> <isaac.w.oram@intel.com>; devel@edk2.groups.io; Kinney, Michael D
>>>> <michael.d.kinney@intel.com>; Chiu, Chasel <chasel.chiu@intel.com>
>>>> Cc: Desimone, Nathaniel L <nathaniel.l.desimone@intel.com>; Kubacki,
>>>> Michael <michael.kubacki@microsoft.com>; Chaganty, Rangasai V
>>>> <rangasai.v.chaganty@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.oram@intel.com>
>>>>> Sent: Tuesday, May 9, 2023 10:35 AM
>>>>> To: devel@edk2.groups.io; mikuback@linux.microsoft.com; Kinney,
>>>>> Michael D <michael.d.kinney@intel.com>; Chiu, Chasel
>>>>> <chasel.chiu@intel.com>; Gudla, Raghava <raghava.gudla@intel.com>
>>>>> Cc: Desimone, Nathaniel L <nathaniel.l.desimone@intel.com>;
>>>>> Kubacki, Michael <michael.kubacki@microsoft.com>; Chaganty,
>>>>> Rangasai V <rangasai.v.chaganty@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.kinney@intel.com>; Chiu, Chasel
>>>>> <chasel.chiu@intel.com>; Oram, Isaac W <isaac.w.oram@intel.com>;
>>>>> Gudla, Raghava <raghava.gudla@intel.com>
>>>>> Cc: Desimone, Nathaniel L <nathaniel.l.desimone@intel.com>;
>>>>> Kubacki, Michael <michael.kubacki@microsoft.com>; Chaganty,
>>>>> Rangasai V <rangasai.v.chaganty@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://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2F
>>>>> ed
>>>>>
>>>
>> k2.groups.io%2Fg%2Fdevel%2Fmessage%2F92644&data=05%7C01%7Cmichael.
>>> ku
>>>>>
>>>
>> backi%40microsoft.com%7C05c93413c875400acef708db50bf197d%7C72f988bf
>>> 8
>>>>>
>>>
>> 6f141af91ab2d7cd011db47%7C1%7C0%7C638192553936149621%7CUnknown
>>> %7CTWF
>>>>>
>>>
>> pbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI
>>>>>
>>>
>> 6Mn0%3D%7C3000%7C%7C%7C&sdata=E4hpQLMgLy0BKHngjpec88AI0v3TZ8xg
>>> xebjU0
>>>>> SS0FM%3D&reserved=0
>>>>>
>>>>> 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://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2F
>>>>> gi
>>>>>
>>>
>> thub.com%2Fmicrosoft%2Fmu_common_intel_min_platform%2Fblob%2Freleas
>>> e
>>>>>
>>>
>> &data=05%7C01%7Cmichael.kubacki%40microsoft.com%7C05c93413c875400a
>>> ce
>>>>>
>>>
>> f708db50bf197d%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C63819
>>> 255
>>>>>
>>>
>> 3936305834%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoi
>>> V2luM
>>>>>
>>>
>> zIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=lXFLZV78C
>>> Pev
>>>>> UL7De1%2B6RtHaKWQcjnXx6IFABcmWJ54%3D&reserved=0
>>>>>
>>>>
>>>
>> /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.oram@intel.com>; Gudla, Raghava
>>>>>>> <raghava.gudla@intel.com>; devel@edk2.groups.io
>>>>>>> Cc: Desimone, Nathaniel L <nathaniel.l.desimone@intel.com>;
>>>>>>> Kubacki, Michael <michael.kubacki@microsoft.com>; Chaganty,
>>>>>>> Rangasai V <rangasai.v.chaganty@intel.com>; Chiu, Chasel
>>>>>>> <chasel.chiu@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.oram@intel.com>
>>>>>>>> Sent: Monday, May 8, 2023 12:15 PM
>>>>>>>> To: Gudla, Raghava <raghava.gudla@intel.com>;
>>>>>>>> devel@edk2.groups.io
>>>>>>>> Cc: Chiu, Chasel <chasel.chiu@intel.com>; Desimone, Nathaniel L
>>>>>>>> <nathaniel.l.desimone@intel.com>; Kubacki, Michael
>>>>>>>> <michael.kubacki@microsoft.com>; Chaganty, Rangasai V
>>>>>>>> <rangasai.v.chaganty@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.gudla@intel.com>
>>>>>>>> Sent: Friday, May 5, 2023 5:03 PM
>>>>>>>> To: devel@edk2.groups.io
>>>>>>>> Cc: Gudla, Raghava <raghava.gudla@intel.com>; Chiu, Chasel
>>>>>>>> <chasel.chiu@intel.com>; Desimone, Nathaniel L
>>>>>>>> <nathaniel.l.desimone@intel.com>; Oram, Isaac W
>>>>>>> <isaac.w.oram@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.chiu@intel.com>
>>>>>>>> Cc: Nate DeSimone <nathaniel.l.desimone@intel.com>
>>>>>>>> Cc: Isaac Oram <isaac.w.oram@intel.com>
>>>>>>>>
>>>>>>>> Signed-off-by: Raghava Gudla <raghava.gudla@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.d
>>>>>>>> +++ sc
>>>>>>>> @@ -147,6 +147,7 @@
>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>
>> BmpSupportLib|MdeModulePkg/Library/BaseBmpSupportLib/BaseBmpSuppor
>>>>>>> tLi
>>>>>>>> b.inf
>>>>>>>>
>>>>>>> LargeVariableReadLib|MinPlatformPkg/Library/BaseLargeVariableLib
>>>>>>> LargeVariableReadLib|/B
>>>>>>> LargeVariableReadLib|as
>>>>>>> LargeVariableReadLib|e
>>>>>>> LargeVariableReadLib|Larg
>>>>>>>> eVariableReadLib.inf
>>>>>>>>
>>>>>>> LargeVariableWriteLib|MinPlatformPkg/Library/BaseLargeVariableLi
>>>>>>> LargeVariableWriteLib|b/
>>>>>>> LargeVariableWriteLib|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
>>>>>>>> 31+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
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>
>>>
>>>
>>>
>>>
>
>
>
>
>
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2023-05-09 21:41 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-05-06 0:03 [edk2-platforms:PATCH V1] MinPlatformPkg/SaveMemoryConfig: Support NVS Data compression Raghava Gudla
2023-05-08 19:15 ` Isaac Oram
2023-05-08 19:36 ` Chiu, Chasel
2023-05-08 21:48 ` [edk2-devel] " Michael D Kinney
2023-05-09 15:25 ` Michael Kubacki
2023-05-09 17:34 ` Isaac Oram
2023-05-09 17:41 ` Gudla, Raghava
2023-05-09 18:05 ` Michael Kubacki
2023-05-09 18:56 ` Chiu, Chasel
2023-05-09 19:17 ` Michael Kubacki
2023-05-09 19:29 ` Chiu, Chasel
2023-05-09 21:06 ` Chiu, Chasel
2023-05-09 21:41 ` Michael Kubacki
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox