Re: [edk2-devel] [PATCH v1] MinPlatformPkg: Fix SetLargeVariable fail issue

2023-05-10 Thread Zhang, Xiaoqiang 
Thanks Nate for the comprehensive proposal!

Since I found SetLargeVariable only used in SaveMemoryConfig DXE driver now, so 
how about we fix this issue first to unblock the verification on server 
platform. And after that, we can handle the case you mentioned.

-Original Message-
From: Desimone, Nathaniel L  
Sent: Wednesday, May 10, 2023 12:35 PM
To: Zhang, Xiaoqiang ; devel@edk2.groups.io
Cc: Chiu, Chasel ; Gao, Liming 
; Dong, Eric 
Subject: RE: [PATCH v1] MinPlatformPkg: Fix SetLargeVariable fail issue

Great catch Xiaoqiang!

Only issue I see is that for 100% correctness we will need to add a check to 
see if we are past ExitBootServices(). 

This issue is if we are in OS runtime and we try to overwrite an existing 
variable when there isn't enough space to do so without a reclaim... the 
current value of the large variable will be deleted and it will be impossible 
to store the new value until after a platform reset.

To do this check you will need to add an AtOsRuntime() API to the 
VariableWriteLib implemented in 
MinPlatformPkg/Library/DxeRuntimeVariableWriteLib and 
MinPlatformPkg/Library/SmmVariableWriteLib to check for OS runtime. The DXE 
implementation can just be a passthrough to EfiAtRuntime() in 
MdePkg/Library/UefiRuntimeLib. But the SMM one will need to add a notification 
callback for gEdkiiSmmExitBootServicesProtocolGuid and set a global variable to 
indicate whether that event has been triggered yet. Because UefiRuntimeLib can 
only be used by DXE_RUNTIME drivers, you will need to make a separate version 
of this library for boot time only DXE_DRIVERs that has an implementation of 
AtOsRuntime() that always returns FALSE.

-Original Message-
From: Zhang, Xiaoqiang  
Sent: Monday, May 8, 2023 10:39 PM
To: devel@edk2.groups.io
Cc: Zhang, Xiaoqiang ; Chiu, Chasel 
; Desimone, Nathaniel L 
; Gao, Liming ; Dong, 
Eric 
Subject: [PATCH v1] MinPlatformPkg: Fix SetLargeVariable fail issue

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4454

On Server platform, when the large variable "FspNvsBuffer" is already in the 
UEFI variable store and the remaining variable storage space is less than the 
large variable size. And also not in OS runtime then we need to add the size of 
the current data that will end up being replaced by the new data to the 
remaining space before we decide that there is not enough space to store the 
large variable.

Cc: Chasel Chiu 
Cc: Nate DeSimone 
Cc: Liming Gao 
Cc: Eric Dong 

Signed-off-by: Xiaoqiang Zhang 
---
 
Platform/Intel/MinPlatformPkg/Library/BaseLargeVariableLib/LargeVariableWriteLib.c
   | 10 +-
 
Platform/Intel/MinPlatformPkg/Library/BaseLargeVariableLib/BaseLargeVariableWriteLib.inf
 |  1 +
 2 files changed, 10 insertions(+), 1 deletion(-)

diff --git 
a/Platform/Intel/MinPlatformPkg/Library/BaseLargeVariableLib/LargeVariableWriteLib.c
 
b/Platform/Intel/MinPlatformPkg/Library/BaseLargeVariableLib/LargeVariableWriteLib.c
index de23ae6160..da820f65b9 100644
--- 
a/Platform/Intel/MinPlatformPkg/Library/BaseLargeVariableLib/LargeVariableWriteLib.c
+++ b/Platform/Intel/MinPlatformPkg/Library/BaseLargeVariableLib/LargeVa
+++ riableWriteLib.c
@@ -22,7 +22,7 @@
 #include 
 #include 
 #include 
-
+#include 
 #include "LargeVariableCommon.h"
 
 /**
@@ -270,6 +270,7 @@ SetLargeVariable (
   UINT8 *OffsetPtr;
   UINTN BytesRemaining;
   UINTN SizeToSave;
+  UINTN BufferSize = 0;
 
   //
   // Check input parameters.
@@ -365,6 +366,13 @@ SetLargeVariable (
 // Non-Volatile storage to store the data.
 //
 RemainingVariableStorage = GetRemainingVariableStorageSpace ();
+//
+// Check if current variable already existed in NV storage variable space
+//
+Status = GetLargeVariable (VariableName, VendorGuid, , NULL);
+if ((Status == EFI_BUFFER_TOO_SMALL) && (BufferSize != 0)) {
+  RemainingVariableStorage = RemainingVariableStorage + BufferSize;
+}
 if (DataSize > RemainingVariableStorage) {
   DEBUG ((DEBUG_ERROR, "SetLargeVariable: Not enough NV storage space to 
store the data\n"));
   Status = EFI_OUT_OF_RESOURCES;
diff --git 
a/Platform/Intel/MinPlatformPkg/Library/BaseLargeVariableLib/BaseLargeVariableWriteLib.inf
 
b/Platform/Intel/MinPlatformPkg/Library/BaseLargeVariableLib/BaseLargeVariableWriteLib.inf
index 2493a94596..cbc2a5d93a 100644
--- 
a/Platform/Intel/MinPlatformPkg/Library/BaseLargeVariableLib/BaseLargeVariableWriteLib.inf
+++ b/Platform/Intel/MinPlatformPkg/Library/BaseLargeVariableLib/BaseLar
+++ geVariableWriteLib.inf
@@ -49,3 +49,4 @@
   PrintLib
   VariableReadLib
   VariableWriteLib
+  LargeVariableReadLib
--
2.39.1.windows.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#104591): https://edk2.groups.io/g/devel/message/104591
Mute This Topic: https://groups.io/mt/98786447/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub

Re: [edk2-devel] [PATCH v1] MinPlatformPkg: Fix SetLargeVariable fail issue

2023-05-09 Thread Nate DeSimone 
Great catch Xiaoqiang!

Only issue I see is that for 100% correctness we will need to add a check to 
see if we are past ExitBootServices(). 

This issue is if we are in OS runtime and we try to overwrite an existing 
variable when there isn't enough space to do so without a reclaim... the 
current value of the large variable will be deleted and it will be impossible 
to store the new value until after a platform reset.

To do this check you will need to add an AtOsRuntime() API to the 
VariableWriteLib implemented in 
MinPlatformPkg/Library/DxeRuntimeVariableWriteLib and 
MinPlatformPkg/Library/SmmVariableWriteLib to check for OS runtime. The DXE 
implementation can just be a passthrough to EfiAtRuntime() in 
MdePkg/Library/UefiRuntimeLib. But the SMM one will need to add a notification 
callback for gEdkiiSmmExitBootServicesProtocolGuid and set a global variable to 
indicate whether that event has been triggered yet. Because UefiRuntimeLib can 
only be used by DXE_RUNTIME drivers, you will need to make a separate version 
of this library for boot time only DXE_DRIVERs that has an implementation of 
AtOsRuntime() that always returns FALSE.

-Original Message-
From: Zhang, Xiaoqiang  
Sent: Monday, May 8, 2023 10:39 PM
To: devel@edk2.groups.io
Cc: Zhang, Xiaoqiang ; Chiu, Chasel 
; Desimone, Nathaniel L 
; Gao, Liming ; Dong, 
Eric 
Subject: [PATCH v1] MinPlatformPkg: Fix SetLargeVariable fail issue

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4454

On Server platform, when the large variable "FspNvsBuffer" is already in the 
UEFI variable store and the remaining variable storage space is less than the 
large variable size. And also not in OS runtime then we need to add the size of 
the current data that will end up being replaced by the new data to the 
remaining space before we decide that there is not enough space to store the 
large variable.

Cc: Chasel Chiu 
Cc: Nate DeSimone 
Cc: Liming Gao 
Cc: Eric Dong 

Signed-off-by: Xiaoqiang Zhang 
---
 
Platform/Intel/MinPlatformPkg/Library/BaseLargeVariableLib/LargeVariableWriteLib.c
   | 10 +-
 
Platform/Intel/MinPlatformPkg/Library/BaseLargeVariableLib/BaseLargeVariableWriteLib.inf
 |  1 +
 2 files changed, 10 insertions(+), 1 deletion(-)

diff --git 
a/Platform/Intel/MinPlatformPkg/Library/BaseLargeVariableLib/LargeVariableWriteLib.c
 
b/Platform/Intel/MinPlatformPkg/Library/BaseLargeVariableLib/LargeVariableWriteLib.c
index de23ae6160..da820f65b9 100644
--- 
a/Platform/Intel/MinPlatformPkg/Library/BaseLargeVariableLib/LargeVariableWriteLib.c
+++ b/Platform/Intel/MinPlatformPkg/Library/BaseLargeVariableLib/LargeVa
+++ riableWriteLib.c
@@ -22,7 +22,7 @@
 #include 
 #include 
 #include 
-
+#include 
 #include "LargeVariableCommon.h"
 
 /**
@@ -270,6 +270,7 @@ SetLargeVariable (
   UINT8 *OffsetPtr;
   UINTN BytesRemaining;
   UINTN SizeToSave;
+  UINTN BufferSize = 0;
 
   //
   // Check input parameters.
@@ -365,6 +366,13 @@ SetLargeVariable (
 // Non-Volatile storage to store the data.
 //
 RemainingVariableStorage = GetRemainingVariableStorageSpace ();
+//
+// Check if current variable already existed in NV storage variable space
+//
+Status = GetLargeVariable (VariableName, VendorGuid, , NULL);
+if ((Status == EFI_BUFFER_TOO_SMALL) && (BufferSize != 0)) {
+  RemainingVariableStorage = RemainingVariableStorage + BufferSize;
+}
 if (DataSize > RemainingVariableStorage) {
   DEBUG ((DEBUG_ERROR, "SetLargeVariable: Not enough NV storage space to 
store the data\n"));
   Status = EFI_OUT_OF_RESOURCES;
diff --git 
a/Platform/Intel/MinPlatformPkg/Library/BaseLargeVariableLib/BaseLargeVariableWriteLib.inf
 
b/Platform/Intel/MinPlatformPkg/Library/BaseLargeVariableLib/BaseLargeVariableWriteLib.inf
index 2493a94596..cbc2a5d93a 100644
--- 
a/Platform/Intel/MinPlatformPkg/Library/BaseLargeVariableLib/BaseLargeVariableWriteLib.inf
+++ b/Platform/Intel/MinPlatformPkg/Library/BaseLargeVariableLib/BaseLar
+++ geVariableWriteLib.inf
@@ -49,3 +49,4 @@
   PrintLib
   VariableReadLib
   VariableWriteLib
+  LargeVariableReadLib
--
2.39.1.windows.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#104481): https://edk2.groups.io/g/devel/message/104481
Mute This Topic: https://groups.io/mt/98786447/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH v1] MinPlatformPkg: Fix SetLargeVariable fail issue

2023-05-09 Thread Xiaoqiang Zhang 
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4454

On Server platform, when the large variable "FspNvsBuffer" is
already in the UEFI variable store and the remaining variable
storage space is less than the large variable size. And also
not in OS runtime then we need to add the size of the current
data that will end up being replaced by the new data to the
remaining space before we decide that there is not enough
space to store the large variable.

Cc: Chasel Chiu 
Cc: Nate DeSimone 
Cc: Liming Gao 
Cc: Eric Dong 

Signed-off-by: Xiaoqiang Zhang 
---
 
Platform/Intel/MinPlatformPkg/Library/BaseLargeVariableLib/LargeVariableWriteLib.c
   | 10 +-
 
Platform/Intel/MinPlatformPkg/Library/BaseLargeVariableLib/BaseLargeVariableWriteLib.inf
 |  1 +
 2 files changed, 10 insertions(+), 1 deletion(-)

diff --git 
a/Platform/Intel/MinPlatformPkg/Library/BaseLargeVariableLib/LargeVariableWriteLib.c
 
b/Platform/Intel/MinPlatformPkg/Library/BaseLargeVariableLib/LargeVariableWriteLib.c
index de23ae6160..da820f65b9 100644
--- 
a/Platform/Intel/MinPlatformPkg/Library/BaseLargeVariableLib/LargeVariableWriteLib.c
+++ 
b/Platform/Intel/MinPlatformPkg/Library/BaseLargeVariableLib/LargeVariableWriteLib.c
@@ -22,7 +22,7 @@
 #include 
 #include 
 #include 
-
+#include 
 #include "LargeVariableCommon.h"
 
 /**
@@ -270,6 +270,7 @@ SetLargeVariable (
   UINT8 *OffsetPtr;
   UINTN BytesRemaining;
   UINTN SizeToSave;
+  UINTN BufferSize = 0;
 
   //
   // Check input parameters.
@@ -365,6 +366,13 @@ SetLargeVariable (
 // Non-Volatile storage to store the data.
 //
 RemainingVariableStorage = GetRemainingVariableStorageSpace ();
+//
+// Check if current variable already existed in NV storage variable space
+//
+Status = GetLargeVariable (VariableName, VendorGuid, , NULL);
+if ((Status == EFI_BUFFER_TOO_SMALL) && (BufferSize != 0)) {
+  RemainingVariableStorage = RemainingVariableStorage + BufferSize;
+}
 if (DataSize > RemainingVariableStorage) {
   DEBUG ((DEBUG_ERROR, "SetLargeVariable: Not enough NV storage space to 
store the data\n"));
   Status = EFI_OUT_OF_RESOURCES;
diff --git 
a/Platform/Intel/MinPlatformPkg/Library/BaseLargeVariableLib/BaseLargeVariableWriteLib.inf
 
b/Platform/Intel/MinPlatformPkg/Library/BaseLargeVariableLib/BaseLargeVariableWriteLib.inf
index 2493a94596..cbc2a5d93a 100644
--- 
a/Platform/Intel/MinPlatformPkg/Library/BaseLargeVariableLib/BaseLargeVariableWriteLib.inf
+++ 
b/Platform/Intel/MinPlatformPkg/Library/BaseLargeVariableLib/BaseLargeVariableWriteLib.inf
@@ -49,3 +49,4 @@
   PrintLib
   VariableReadLib
   VariableWriteLib
+  LargeVariableReadLib
-- 
2.39.1.windows.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#104388): https://edk2.groups.io/g/devel/message/104388
Mute This Topic: https://groups.io/mt/98786447/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-