-----Original Message-----
From: Chiu, Chasel
Sent: Tuesday, May 9, 2023 12:30 PM
To: devel@edk2.groups.io; Kubacki, Michael <michael.kuba...@microsoft.com>;
Michael Kubacki <mikub...@linux.microsoft.com>; Gudla, Raghava
<raghava.gu...@intel.com>; Oram, Isaac W <isaac.w.o...@intel.com>; Kinney,
Michael D <michael.d.kin...@intel.com>
Cc: Desimone, Nathaniel L <nathaniel.l.desim...@intel.com>; Chaganty,
Rangasai V <rangasai.v.chaga...@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.c...@intel.com>; Michael Kubacki
<mikub...@linux.microsoft.com>; Gudla, Raghava
<raghava.gu...@intel.com>; Oram, Isaac W <isaac.w.o...@intel.com>;
devel@edk2.groups.io; Kinney, Michael D <michael.d.kin...@intel.com>
Cc: Desimone, Nathaniel L <nathaniel.l.desim...@intel.com>; Chaganty,
Rangasai V <rangasai.v.chaga...@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.c...@intel.com>
Sent: Tuesday, May 9, 2023 2:56 PM
To: Michael Kubacki <mikub...@linux.microsoft.com>; Gudla, Raghava
<raghava.gu...@intel.com>; Oram, Isaac W <isaac.w.o...@intel.com>;
devel@edk2.groups.io; Kinney, Michael D <michael.d.kin...@intel.com>
Cc: Desimone, Nathaniel L <nathaniel.l.desim...@intel.com>; Michael
Kubacki <michael.kuba...@microsoft.com>; Chaganty, Rangasai V
<rangasai.v.chaga...@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.kuba...@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 <mikub...@linux.microsoft.com>
Sent: Tuesday, May 9, 2023 11:06 AM
To: Gudla, Raghava <raghava.gu...@intel.com>; Oram, Isaac W
<isaac.w.o...@intel.com>; devel@edk2.groups.io; Kinney, Michael D
<michael.d.kin...@intel.com>; Chiu, Chasel <chasel.c...@intel.com>
Cc: Desimone, Nathaniel L <nathaniel.l.desim...@intel.com>; Kubacki,
Michael <michael.kuba...@microsoft.com>; Chaganty, Rangasai V
<rangasai.v.chaga...@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.o...@intel.com>
Sent: Tuesday, May 9, 2023 10:35 AM
To: devel@edk2.groups.io; mikub...@linux.microsoft.com; Kinney,
Michael D <michael.d.kin...@intel.com>; Chiu, Chasel
<chasel.c...@intel.com>; Gudla, Raghava <raghava.gu...@intel.com>
Cc: Desimone, Nathaniel L <nathaniel.l.desim...@intel.com>;
Kubacki, Michael <michael.kuba...@microsoft.com>; Chaganty,
Rangasai V <rangasai.v.chaga...@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.kin...@intel.com>; Chiu, Chasel
<chasel.c...@intel.com>; Oram, Isaac W <isaac.w.o...@intel.com>;
Gudla, Raghava <raghava.gu...@intel.com>
Cc: Desimone, Nathaniel L <nathaniel.l.desim...@intel.com>;
Kubacki, Michael <michael.kuba...@microsoft.com>; Chaganty,
Rangasai V <rangasai.v.chaga...@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.o...@intel.com>; Gudla, Raghava
<raghava.gu...@intel.com>; devel@edk2.groups.io
Cc: Desimone, Nathaniel L <nathaniel.l.desim...@intel.com>;
Kubacki, Michael <michael.kuba...@microsoft.com>; Chaganty,
Rangasai V <rangasai.v.chaga...@intel.com>; Chiu, Chasel
<chasel.c...@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.o...@intel.com>
Sent: Monday, May 8, 2023 12:15 PM
To: Gudla, Raghava <raghava.gu...@intel.com>;
devel@edk2.groups.io
Cc: Chiu, Chasel <chasel.c...@intel.com>; Desimone, Nathaniel L
<nathaniel.l.desim...@intel.com>; Kubacki, Michael
<michael.kuba...@microsoft.com>; Chaganty, Rangasai V
<rangasai.v.chaga...@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.gu...@intel.com>
Sent: Friday, May 5, 2023 5:03 PM
To: devel@edk2.groups.io
Cc: Gudla, Raghava <raghava.gu...@intel.com>; Chiu, Chasel
<chasel.c...@intel.com>; Desimone, Nathaniel L
<nathaniel.l.desim...@intel.com>; Oram, Isaac W
<isaac.w.o...@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.c...@intel.com>
Cc: Nate DeSimone <nathaniel.l.desim...@intel.com>
Cc: Isaac Oram <isaac.w.o...@intel.com>
Signed-off-by: Raghava Gudla <raghava.gu...@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
