* [PATCH v1] MinPlatformPkg: Fix SetLargeVariable fail issue @ 2023-05-09 5:38 Xiaoqiang Zhang 2023-05-10 4:34 ` Nate DeSimone 0 siblings, 1 reply; 3+ messages in thread From: Xiaoqiang Zhang @ 2023-05-09 5:38 UTC (permalink / raw) To: devel; +Cc: Xiaoqiang Zhang, Chasel Chiu, Nate DeSimone, Liming Gao, Eric Dong 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 <chasel.chiu@intel.com> Cc: Nate DeSimone <nathaniel.l.desimone@intel.com> Cc: Liming Gao <gaoliming@byosoft.com.cn> Cc: Eric Dong <eric.dong@intel.com> Signed-off-by: Xiaoqiang Zhang <xiaoqiang.zhang@intel.com> --- 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 <Library/PrintLib.h> #include <Library/VariableReadLib.h> #include <Library/VariableWriteLib.h> - +#include <Library/LargeVariableReadLib.h> #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, &BufferSize, 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 ^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH v1] MinPlatformPkg: Fix SetLargeVariable fail issue 2023-05-09 5:38 [PATCH v1] MinPlatformPkg: Fix SetLargeVariable fail issue Xiaoqiang Zhang @ 2023-05-10 4:34 ` Nate DeSimone 2023-05-10 5:12 ` Zhang, Xiaoqiang 0 siblings, 1 reply; 3+ messages in thread From: Nate DeSimone @ 2023-05-10 4:34 UTC (permalink / raw) To: Zhang, Xiaoqiang, devel@edk2.groups.io Cc: Chiu, Chasel, Gao, Liming, Dong, Eric 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 <xiaoqiang.zhang@intel.com> Sent: Monday, May 8, 2023 10:39 PM To: devel@edk2.groups.io Cc: Zhang, Xiaoqiang <xiaoqiang.zhang@intel.com>; Chiu, Chasel <chasel.chiu@intel.com>; Desimone, Nathaniel L <nathaniel.l.desimone@intel.com>; Gao, Liming <gaoliming@byosoft.com.cn>; Dong, Eric <eric.dong@intel.com> Subject: [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 <chasel.chiu@intel.com> Cc: Nate DeSimone <nathaniel.l.desimone@intel.com> Cc: Liming Gao <gaoliming@byosoft.com.cn> Cc: Eric Dong <eric.dong@intel.com> Signed-off-by: Xiaoqiang Zhang <xiaoqiang.zhang@intel.com> --- 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 <Library/PrintLib.h> #include <Library/VariableReadLib.h> #include <Library/VariableWriteLib.h> - +#include <Library/LargeVariableReadLib.h> #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, &BufferSize, 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 ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH v1] MinPlatformPkg: Fix SetLargeVariable fail issue 2023-05-10 4:34 ` Nate DeSimone @ 2023-05-10 5:12 ` Zhang, Xiaoqiang 0 siblings, 0 replies; 3+ messages in thread From: Zhang, Xiaoqiang @ 2023-05-10 5:12 UTC (permalink / raw) To: Desimone, Nathaniel L, devel@edk2.groups.io Cc: Chiu, Chasel, Gao, Liming, Dong, Eric 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 <nathaniel.l.desimone@intel.com> Sent: Wednesday, May 10, 2023 12:35 PM To: Zhang, Xiaoqiang <xiaoqiang.zhang@intel.com>; devel@edk2.groups.io Cc: Chiu, Chasel <chasel.chiu@intel.com>; Gao, Liming <gaoliming@byosoft.com.cn>; Dong, Eric <eric.dong@intel.com> Subject: RE: [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 <xiaoqiang.zhang@intel.com> Sent: Monday, May 8, 2023 10:39 PM To: devel@edk2.groups.io Cc: Zhang, Xiaoqiang <xiaoqiang.zhang@intel.com>; Chiu, Chasel <chasel.chiu@intel.com>; Desimone, Nathaniel L <nathaniel.l.desimone@intel.com>; Gao, Liming <gaoliming@byosoft.com.cn>; Dong, Eric <eric.dong@intel.com> Subject: [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 <chasel.chiu@intel.com> Cc: Nate DeSimone <nathaniel.l.desimone@intel.com> Cc: Liming Gao <gaoliming@byosoft.com.cn> Cc: Eric Dong <eric.dong@intel.com> Signed-off-by: Xiaoqiang Zhang <xiaoqiang.zhang@intel.com> --- 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 <Library/PrintLib.h> #include <Library/VariableReadLib.h> #include <Library/VariableWriteLib.h> - +#include <Library/LargeVariableReadLib.h> #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, &BufferSize, 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 ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2023-05-10 5:12 UTC | newest] Thread overview: 3+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2023-05-09 5:38 [PATCH v1] MinPlatformPkg: Fix SetLargeVariable fail issue Xiaoqiang Zhang 2023-05-10 4:34 ` Nate DeSimone 2023-05-10 5:12 ` Zhang, Xiaoqiang
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox