Comparing this with v4 (which is basically identical to v5) and my notes for v4:
On 02/24/14 19:15, Jordan Justen wrote: > From: Laszlo Ersek <ler...@redhat.com> > > The S3 suspend/resume infrastructure depends on the LockBox library class. > The edk2 tree currently contains Null and SMM instances. The Null instance > is useless, and the SMM instance would require SMM emulation by including > the SMM core and adding several new drivers, which is deemed too complex. > > Hence add a simple LockBoxLib instance for OVMF. > > jordan.l.jus...@intel.com: > * use PCDs instead of EmuNvramLib > - clear memory in PlatformPei on non S3 boots > * allocate NVS memory and store a pointer to that memory > - reduces memory use at fixed locations > > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Laszlo Ersek <ler...@redhat.com> > Reviewed-by: Jordan Justen <jordan.l.jus...@intel.com> > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Jordan Justen <jordan.l.jus...@intel.com> > --- > > Laszlo, > > I think I took nearly all of your suggestions for v4 of this > patch. (Which I missed for v5 of the series.) > > How does it look? Would you prefer me to bump the series > to v6 to send this out in that context? > > -Jordan > > OvmfPkg/Library/LockBoxLib/LockBoxBase.c | 42 +++ > OvmfPkg/Library/LockBoxLib/LockBoxBaseLib.inf | 44 +++ > OvmfPkg/Library/LockBoxLib/LockBoxDxe.c | 119 ++++++++ > OvmfPkg/Library/LockBoxLib/LockBoxDxeLib.inf | 45 +++ > OvmfPkg/Library/LockBoxLib/LockBoxLib.c | 378 > ++++++++++++++++++++++++++ > OvmfPkg/Library/LockBoxLib/LockBoxLib.h | 60 ++++ > OvmfPkg/OvmfPkg.dec | 2 + > OvmfPkg/OvmfPkgIa32.dsc | 3 +- > OvmfPkg/OvmfPkgIa32.fdf | 3 + > OvmfPkg/OvmfPkgIa32X64.dsc | 3 +- > OvmfPkg/OvmfPkgIa32X64.fdf | 3 + > OvmfPkg/OvmfPkgX64.dsc | 3 +- > OvmfPkg/OvmfPkgX64.fdf | 3 + > OvmfPkg/PlatformPei/MemDetect.c | 17 ++ > OvmfPkg/PlatformPei/PlatformPei.inf | 2 + > 15 files changed, 724 insertions(+), 3 deletions(-) > create mode 100644 OvmfPkg/Library/LockBoxLib/LockBoxBase.c > create mode 100644 OvmfPkg/Library/LockBoxLib/LockBoxBaseLib.inf > create mode 100644 OvmfPkg/Library/LockBoxLib/LockBoxDxe.c > create mode 100644 OvmfPkg/Library/LockBoxLib/LockBoxDxeLib.inf > create mode 100644 OvmfPkg/Library/LockBoxLib/LockBoxLib.c > create mode 100644 OvmfPkg/Library/LockBoxLib/LockBoxLib.h > > diff --git a/OvmfPkg/Library/LockBoxLib/LockBoxBase.c > b/OvmfPkg/Library/LockBoxLib/LockBoxBase.c > new file mode 100644 > index 0000000..8f36770 > --- /dev/null > +++ b/OvmfPkg/Library/LockBoxLib/LockBoxBase.c > @@ -0,0 +1,42 @@ > +/** @file > + > + Copyright (c) 2006 - 2014, Intel Corporation. All rights reserved.<BR> > + > + This program and the accompanying materials > + are licensed and made available under the terms and conditions of the BSD > License > + which accompanies this distribution. The full text of the license may be > found at > + http://opensource.org/licenses/bsd-license.php. > + > + THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, > + WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR > IMPLIED. > + > +**/ > + > +#include <Uefi.h> > + > +#include <Library/DebugLib.h> > +#include <LockBoxLib.h> > + > +/** > + Allocates a buffer of type EfiACPIMemoryNVS. > + > + Allocates the number bytes specified by AllocationSize of type > + EfiACPIMemoryNVS and returns a pointer to the allocated buffer. > + If AllocationSize is 0, then a valid buffer of 0 size is > + returned. If there is not enough memory remaining to satisfy > + the request, then NULL is returned. > + > + @param AllocationSize The number of bytes to allocate. > + > + @return A pointer to the allocated buffer or NULL if allocation fails. > + > +**/ > +VOID * > +EFIAPI > +AllocateAcpiNvsPool ( > + IN UINTN AllocationSize > + ) > +{ > + ASSERT_EFI_ERROR (RETURN_UNSUPPORTED); > + return NULL; > +} > diff --git a/OvmfPkg/Library/LockBoxLib/LockBoxBaseLib.inf > b/OvmfPkg/Library/LockBoxLib/LockBoxBaseLib.inf > new file mode 100644 > index 0000000..caeb4e8 > --- /dev/null > +++ b/OvmfPkg/Library/LockBoxLib/LockBoxBaseLib.inf > @@ -0,0 +1,44 @@ > +## @file > +# > +# Library implementing the LockBox interface for OVMF > +# > +# Copyright (C) 2013, Red Hat, Inc. > +# Copyright (c) 2014, Intel Corporation. All rights reserved.<BR> > +# > +# This program and the accompanying materials are licensed and made > available > +# under the terms and conditions of the BSD License which accompanies this > +# distribution. The full text of the license may be found at > +# http://opensource.org/licenses/bsd-license.php > +# > +# THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, > +# WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR > +# IMPLIED. > +# > +## > + > +[Defines] > + INF_VERSION = 0x00010005 > + BASE_NAME = LockBoxLib > + FILE_GUID = 17CA9B37-5BAB-492C-A09C-7121FBE34CE6 > + MODULE_TYPE = BASE > + VERSION_STRING = 1.0 > + LIBRARY_CLASS = LockBoxBaseLib (1) I believe you updated the wrong field, relative to v5. I *think* LIBRARY_CLASS should always be "LockBoxLib" -- it specifies the interface. ("One Library Class that is satisfied by this Library Instance [...]") BASE_NAME should determine the base name for the build output. ("This is a single word identifier that will be used for the component name.") No need to repost just because of this, I think you can fix it up when you commit it. (2) The same applies to the DXE flavor. > + > + CONSTRUCTOR = LockBoxLibInitialize > + > +[Sources] > + LockBoxBase.c > + LockBoxLib.c > + > +[Packages] > + MdePkg/MdePkg.dec > + MdeModulePkg/MdeModulePkg.dec > + OvmfPkg/OvmfPkg.dec > + > +[LibraryClasses] > + BaseMemoryLib > + DebugLib > + > +[Pcd] > + gUefiOvmfPkgTokenSpaceGuid.PcdOvmfLockBoxStorageBase > + gUefiOvmfPkgTokenSpaceGuid.PcdOvmfLockBoxStorageSize > diff --git a/OvmfPkg/Library/LockBoxLib/LockBoxDxe.c > b/OvmfPkg/Library/LockBoxLib/LockBoxDxe.c > new file mode 100644 > index 0000000..d49485d > --- /dev/null > +++ b/OvmfPkg/Library/LockBoxLib/LockBoxDxe.c > @@ -0,0 +1,119 @@ > +/** @file > + > + Copyright (c) 2006 - 2014, Intel Corporation. All rights reserved.<BR> > + > + This program and the accompanying materials > + are licensed and made available under the terms and conditions of the BSD > License > + which accompanies this distribution. The full text of the license may be > found at > + http://opensource.org/licenses/bsd-license.php. > + > + THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, > + WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR > IMPLIED. > + > +**/ > + > +#include <Uefi.h> > + > +#include <Library/MemoryAllocationLib.h> > +#include <Library/UefiBootServicesTableLib.h> > +#include <Library/BaseMemoryLib.h> > +#include <Library/DebugLib.h> > +#include <LockBoxLib.h> > + > +/** > + Allocate memory below 4G memory address. > + > + This function allocates memory below 4G memory address. > + > + @param MemoryType Memory type of memory to allocate. > + @param Size Size of memory to allocate. > + > + @return Allocated address for output. > + > +**/ > +STATIC > +VOID * > +AllocateMemoryBelow4G ( > + IN EFI_MEMORY_TYPE MemoryType, > + IN UINTN Size > + ) > +{ > + UINTN Pages; > + EFI_PHYSICAL_ADDRESS Address; > + EFI_STATUS Status; > + VOID* Buffer; > + UINTN AllocRemaining; > + > + Pages = EFI_SIZE_TO_PAGES (Size); > + Address = 0xffffffff; This seems to come from "IntelFrameworkModulePkg/Universal/Acpi/AcpiS3SaveDxe/AcpiS3Save.c", OK... > + DEBUG((EFI_D_INFO, "AllocateMemoryBelow4G size=0x%x\n", Size)); (3) This is new. Unfortunately, the %x format specification is *theoretically* (== from a standards perspective) incorrect when Size (UINTN) is UINT64. In practice it doesn't matter, because - you only pass this one argument through the ellipsis, - you're allowed to push more than BasePrintLibSPrintMarker() will pop, - the output will be correct since we're little endian. In any case, you might want to fix it up when you commit it, by specifying %Lx, and converting Size to UINT64: DEBUG ((EFI_D_INFO, "%a: size=0x%Lx\n", __FUNCTION__, (UINT64) Size)); (4) Also, I'm noticing a serious violation of the coding style -- you omitted the space between "DEBUG" and the opening paren! :) > + > + // > + // Since we need to use gBS->AllocatePages to get a buffer below > + // 4GB, there is a good chance that space will be wasted for very > + // small allocation. We keep track of unused portions of the page > + // allocations, and use these to allocate memory for small buffers. > + // (5) I think this would be a fine place to ASSERT() the LOCK_BOX_GLOBAL_SIGNATURE, before accessing the other LOCK_BOX_GLOBAL fields. I'm kinda emphasizing this because right now the only place that checks the signature is LockBoxLibInitialize(), and that function only checks it to see if setting it is necessary. > + if ((UINTN) mLockBoxGlobal->SubPageRemaining >= Size) { SubPageRemaining is already UINTN, no need to cast. Harmless though. Otherwise, it's good. When entered for the very first time, SubPageRemaining will be zero, and Size will be positive (because we only reach this from SaveLockBox, and that one enforces Size>0), so this check will fail. Good. > + Buffer = (VOID*)(UINTN) mLockBoxGlobal->SubPageBuffer; > + mLockBoxGlobal->SubPageBuffer = mLockBoxGlobal->SubPageBuffer + Size; > + mLockBoxGlobal->SubPageRemaining = mLockBoxGlobal->SubPageRemaining - > Size; I'd like to advertise the += and -= operators! :) Harmless, of course. > + return Buffer; > + } > + > + Status = gBS->AllocatePages ( > + AllocateMaxAddress, > + MemoryType, > + Pages, > + &Address > + ); > + if (EFI_ERROR (Status)) { > + return NULL; > + } > + > + Buffer = (VOID *) (UINTN) Address; > + ZeroMem (Buffer, Size); (6) You should probably zero out the entire (rounded-up) area -- EFI_PAGES_TO_SIZE (Pages). Now the code leaves the trailing portion filled with garbage, but you do return small buffers from there. > + > + AllocRemaining = EFI_PAGES_TO_SIZE (Pages) - Size; > + if (AllocRemaining > (UINTN) mLockBoxGlobal->SubPageRemaining) { The cast is again superfluous, but harmless. > + mLockBoxGlobal->SubPageBuffer = (UINT32) (Address + Size); > + mLockBoxGlobal->SubPageRemaining = (UINT32) AllocRemaining; > + } Seems sensible. The last "tail portion" proved to small for this one allocation, but the new "tail portion" could be even smaller. So for the next allocation, it could be better to stick with the last tail portion, and waste the new tail portion. Good > + > + return Buffer; > +} > + > + > +/** > + Allocates a buffer of type EfiACPIMemoryNVS. > + > + Allocates the number bytes specified by AllocationSize of type > + EfiACPIMemoryNVS and returns a pointer to the allocated buffer. > + If AllocationSize is 0, then a valid buffer of 0 size is > + returned. If there is not enough memory remaining to satisfy > + the request, then NULL is returned. > + > + @param AllocationSize The number of bytes to allocate. > + > + @return A pointer to the allocated buffer or NULL if allocation fails. > + > +**/ > +VOID * > +EFIAPI > +AllocateAcpiNvsPool ( > + IN UINTN AllocationSize > + ) > +{ > + return AllocateMemoryBelow4G (EfiACPIMemoryNVS, AllocationSize); > +} > + > + > +EFI_STATUS > +EFIAPI > +LockBoxDxeLibInitialize ( > + IN EFI_HANDLE ImageHandle, > + IN EFI_SYSTEM_TABLE *SystemTable > + ) > +{ > + return LockBoxLibInitialize (); > +} > diff --git a/OvmfPkg/Library/LockBoxLib/LockBoxDxeLib.inf > b/OvmfPkg/Library/LockBoxLib/LockBoxDxeLib.inf > new file mode 100644 > index 0000000..fe8fa0c > --- /dev/null > +++ b/OvmfPkg/Library/LockBoxLib/LockBoxDxeLib.inf > @@ -0,0 +1,45 @@ > +## @file > +# > +# Library implementing the LockBox interface for OVMF > +# > +# Copyright (C) 2013, Red Hat, Inc. > +# Copyright (c) 2014, Intel Corporation. All rights reserved.<BR> > +# > +# This program and the accompanying materials are licensed and made > available > +# under the terms and conditions of the BSD License which accompanies this > +# distribution. The full text of the license may be found at > +# http://opensource.org/licenses/bsd-license.php > +# > +# THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, > +# WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR > +# IMPLIED. > +# > +## > + > +[Defines] > + INF_VERSION = 0x00010005 > + BASE_NAME = LockBoxLib > + FILE_GUID = 17CA9B37-5BAB-492C-A09C-7121FBE34CE6 (OK, you already updated this GUID in your repo to differ from the BASE module type.) > + MODULE_TYPE = DXE_DRIVER > + VERSION_STRING = 1.0 > + LIBRARY_CLASS = LockBoxDxeLib > + > + CONSTRUCTOR = LockBoxDxeLibInitialize > + > +[Sources] > + LockBoxDxe.c > + LockBoxLib.c > + > +[Packages] > + MdePkg/MdePkg.dec > + MdeModulePkg/MdeModulePkg.dec > + OvmfPkg/OvmfPkg.dec > + > +[LibraryClasses] > + BaseMemoryLib > + DebugLib > + UefiBootServicesTableLib > + > +[Pcd] > + gUefiOvmfPkgTokenSpaceGuid.PcdOvmfLockBoxStorageBase > + gUefiOvmfPkgTokenSpaceGuid.PcdOvmfLockBoxStorageSize > diff --git a/OvmfPkg/Library/LockBoxLib/LockBoxLib.c > b/OvmfPkg/Library/LockBoxLib/LockBoxLib.c > new file mode 100644 > index 0000000..9bbbb03 > --- /dev/null > +++ b/OvmfPkg/Library/LockBoxLib/LockBoxLib.c > @@ -0,0 +1,378 @@ > +/** @file > + > + Library implementing the LockBox interface for OVMF > + > + Copyright (C) 2013, Red Hat, Inc. > + Copyright (c) 2010 - 2014, Intel Corporation. All rights reserved.<BR> > + > + This program and the accompanying materials are licensed and made available > + under the terms and conditions of the BSD License which accompanies this > + distribution. The full text of the license may be found at > + http://opensource.org/licenses/bsd-license.php > + > + THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, > WITHOUT > + WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED. > + > +**/ > + > +#include <Uefi.h> > +#include <Library/BaseMemoryLib.h> > +#include <Library/DebugLib.h> > +#include <Library/LockBoxLib.h> > +#include <Library/PcdLib.h> > +#include <LockBoxLib.h> > + > +#pragma pack(1) > +typedef struct { > + EFI_GUID Guid; > + EFI_PHYSICAL_ADDRESS OrigAddress; > + EFI_PHYSICAL_ADDRESS CopyAddress; > + UINT32 Size; > + UINT64 Attributes; > +} LOCK_BOX_ENTRY; > +#pragma pack() > + > +LOCK_BOX_GLOBAL *mLockBoxGlobal = NULL; > +STATIC LOCK_BOX_ENTRY *StartOfEntries = NULL; > +STATIC LOCK_BOX_ENTRY *EndOfEntries = NULL; > + > +RETURN_STATUS > +EFIAPI > +LockBoxLibInitialize ( > + VOID > + ) > +{ > + UINTN NumEntries; > + > + if (PcdGet32 (PcdOvmfLockBoxStorageSize) < sizeof (LOCK_BOX_GLOBAL)) { > + return RETURN_UNSUPPORTED; > + } > + > + mLockBoxGlobal = (LOCK_BOX_GLOBAL *)(UINTN) PcdGet32 > (PcdOvmfLockBoxStorageBase); > + StartOfEntries = ((LOCK_BOX_ENTRY *) (mLockBoxGlobal + 1)); > + NumEntries = ((PcdGet32 (PcdOvmfLockBoxStorageSize) - sizeof > (LOCK_BOX_GLOBAL)) / > + sizeof (LOCK_BOX_ENTRY)); > + EndOfEntries = StartOfEntries + NumEntries; > + if (mLockBoxGlobal->Signature != LOCK_BOX_GLOBAL_SIGNATURE) { > + // > + // Note: This code depends on the lock box being cleared in early > + // PEI before usage, so the SubPageBuffer and SubPageRemaining > + // fields don't need to be set to 0. > + // > + mLockBoxGlobal->Signature = LOCK_BOX_GLOBAL_SIGNATURE; > + } > + return RETURN_SUCCESS; > +} > + > + > +/** > + Find LockBox entry based on GUID. > + > + @param[in] Guid The GUID to search for. > + > + @return Address of the LOCK_BOX_ENTRY found. > + > + If NULL, then the item was not found, and there is no space > + left to store a new item. > + > + If non-NULL and LOCK_BOX_ENTRY.Guid matches the requested Guid, > + then the item was found. > + > + If non-NULL and LOCK_BOX_ENTRY.Guid does bit match the requested (7) s/bit/not/ (fixable at commit time) > + Guid, then the item was not found, but a new item can be inserted > + at the returned location. > +**/ > +STATIC > +LOCK_BOX_ENTRY * > +EFIAPI > +FindHeaderByGuid ( > + IN CONST EFI_GUID *Guid > + ) > +{ > + LOCK_BOX_ENTRY *Header; > + > + for (Header = StartOfEntries; Header < EndOfEntries; Header++) { > + if (Header->Size == 0 || CompareGuid (Guid, &Header->Guid)) { > + return Header; > + } > + } > + > + return NULL; > +} (8) The implementation is good, but I think there's a small gap in the interface. If you search for a GUID that consists of zeroes only, then you can't get "not found". You either get "no more room", or "item found". I'd prefer if callers of this function were instructed to discriminate based on LOCK_BOX_ENTRY.Size. This field can never be zero in stored entries, because both SaveLockBox() and UpdateLockBox() reject Length==0 with RETURN_INVALID_PARAMETER. In any case, we can fix this later. I really think we should, but it doesn't warrant a respin. > + > + > +/** > + This function will save confidential information to lockbox. > + > + @param Guid the guid to identify the confidential information > + @param Buffer the address of the confidential information > + @param Length the length of the confidential information > + > + @retval RETURN_SUCCESS the information is saved successfully. > + @retval RETURN_INVALID_PARAMETER the Guid is NULL, or Buffer is NULL, or > + Length is 0 > + @retval RETURN_ALREADY_STARTED the requested GUID already exist. > + @retval RETURN_OUT_OF_RESOURCES no enough resource to save the > information. > + @retval RETURN_ACCESS_DENIED it is too late to invoke this interface > + @retval RETURN_NOT_STARTED it is too early to invoke this interface > + @retval RETURN_UNSUPPORTED the service is not supported by > + implementaion. > +**/ > +RETURN_STATUS > +EFIAPI > +SaveLockBox ( > + IN GUID *Guid, > + IN VOID *Buffer, > + IN UINTN Length > + ) > +{ > + LOCK_BOX_ENTRY *Header; > + VOID *CopyBuffer; > + > + DEBUG ((DEBUG_VERBOSE, "%a: Guid=%g Buffer=%p Length=0x%x\n", __FUNCTION__, > + Guid, Buffer, (UINT32) Length)); > + > + if (Guid == NULL || Buffer == NULL || Length == 0) { > + return RETURN_INVALID_PARAMETER; > + } > + > + if (Length > 0xFFFFFFFF) { > + return RETURN_OUT_OF_RESOURCES; > + } > + > + Header = FindHeaderByGuid (Guid); > + if (Header == NULL) { > + return RETURN_OUT_OF_RESOURCES; > + } > + > + if (Header->Size > 0) { > + return RETURN_ALREADY_STARTED; > + } Ah, this is great! So what we need to update in (8) is actually just the leading comment. Cool. Please consider fixing that up when you commit. > + > + CopyBuffer = AllocateAcpiNvsPool (Length); > + if (CopyBuffer == NULL) { > + return RETURN_OUT_OF_RESOURCES; > + } > + > + // > + // overwrite the current terminator header with new metadata > + // > + CopyGuid (&Header->Guid, Guid); > + Header->OrigAddress = (UINTN) Buffer; > + Header->CopyAddress = (UINTN) CopyBuffer; > + Header->Size = (UINT32) Length; > + Header->Attributes = 0; > + > + // > + // copy contents > + // > + CopyMem (CopyBuffer, Buffer, Length); > + > + return RETURN_SUCCESS; > +} > + > + > +/** > + This function will set lockbox attributes. > + > + @param Guid the guid to identify the confidential information > + @param Attributes the attributes of the lockbox > + > + @retval RETURN_SUCCESS the information is saved successfully. > + @retval RETURN_INVALID_PARAMETER attributes is invalid. > + @retval RETURN_NOT_FOUND the requested GUID not found. > + @retval RETURN_ACCESS_DENIED it is too late to invoke this interface > + @retval RETURN_NOT_STARTED it is too early to invoke this interface > + @retval RETURN_UNSUPPORTED the service is not supported by > + implementaion. > +**/ > +RETURN_STATUS > +EFIAPI > +SetLockBoxAttributes ( > + IN GUID *Guid, > + IN UINT64 Attributes > + ) > +{ > + LOCK_BOX_ENTRY *Header; > + > + DEBUG ((DEBUG_VERBOSE, "%a: Guid=%g Attributes=0x%Lx\n", __FUNCTION__, > Guid, > + Attributes)); > + > + if (Guid == NULL) { > + return RETURN_INVALID_PARAMETER; > + } > + > + Header = FindHeaderByGuid (Guid); > + if (!Header || Header->Size == 0) { > + return RETURN_NOT_FOUND; > + } Another correct use of FindHeaderByGuid(). Good. > + Header->Attributes = Attributes; > + > + return RETURN_SUCCESS; > +} > + > + > +/** > + This function will update confidential information to lockbox. > + > + @param Guid the guid to identify the original confidential information > + @param Offset the offset of the original confidential information > + @param Buffer the address of the updated confidential information > + @param Length the length of the updated confidential information > + > + @retval RETURN_SUCCESS the information is saved successfully. > + @retval RETURN_INVALID_PARAMETER the Guid is NULL, or Buffer is NULL, or > + Length is 0. > + @retval RETURN_NOT_FOUND the requested GUID not found. > + @retval RETURN_BUFFER_TOO_SMALL the original buffer to too small to hold > + new information. > + @retval RETURN_ACCESS_DENIED it is too late to invoke this interface > + @retval RETURN_NOT_STARTED it is too early to invoke this interface > + @retval RETURN_UNSUPPORTED the service is not supported by > + implementaion. > +**/ > +RETURN_STATUS > +EFIAPI > +UpdateLockBox ( > + IN GUID *Guid, > + IN UINTN Offset, > + IN VOID *Buffer, > + IN UINTN Length > + ) > +{ > + LOCK_BOX_ENTRY *Header; > + > + DEBUG ((DEBUG_VERBOSE, "%a: Guid=%g Offset=0x%x Length=0x%x\n", > __FUNCTION__, > + Guid, (UINT32) Offset, (UINT32) Length)); > + > + if (Guid == NULL || Buffer == NULL || Length == 0) { > + return RETURN_INVALID_PARAMETER; > + } > + > + Header = FindHeaderByGuid (Guid); > + if (!Header || Header->Size == 0) { > + return RETURN_NOT_FOUND; > + } Correct use, OK. > + > + if (Header->Size < Offset || > + Length > Header->Size - Offset) { > + return RETURN_BUFFER_TOO_SMALL; > + } Thanks for updating this. > + > + CopyMem ((UINT8 *)(UINTN) (Header->CopyAddress) + Offset, Buffer, Length); > + > + return RETURN_SUCCESS; > +} > + > + > +/** > + This function will restore confidential information from lockbox. > + > + @param Guid the guid to identify the confidential information > + @param Buffer the address of the restored confidential information > + NULL means restored to original address, Length MUST be NULL > at > + same time. > + @param Length the length of the restored confidential information > + > + @retval RETURN_SUCCESS the information is restored successfully. > + @retval RETURN_INVALID_PARAMETER the Guid is NULL, or one of Buffer and > + Length is NULL. > + @retval RETURN_WRITE_PROTECTED Buffer and Length are NULL, but the > LockBox > + has no > LOCK_BOX_ATTRIBUTE_RESTORE_IN_PLACE > + attribute. > + @retval RETURN_BUFFER_TOO_SMALL the Length is too small to hold the > + confidential information. > + @retval RETURN_NOT_FOUND the requested GUID not found. > + @retval RETURN_NOT_STARTED it is too early to invoke this interface > + @retval RETURN_ACCESS_DENIED not allow to restore to the address > + @retval RETURN_UNSUPPORTED the service is not supported by > + implementaion. > +**/ > +RETURN_STATUS > +EFIAPI > +RestoreLockBox ( > + IN GUID *Guid, > + IN VOID *Buffer, OPTIONAL > + IN OUT UINTN *Length OPTIONAL > + ) > +{ > + LOCK_BOX_ENTRY *Header; > + > + DEBUG ((DEBUG_VERBOSE, "%a: Guid=%g Buffer=%p\n", __FUNCTION__, Guid, > + Buffer)); > + > + if ((Guid == NULL) || > + ((Buffer == NULL) && (Length != NULL)) || > + ((Buffer != NULL) && (Length == NULL))) { > + return EFI_INVALID_PARAMETER; > + } > + > + Header = FindHeaderByGuid (Guid); > + if (!Header || Header->Size == 0) { > + return RETURN_NOT_FOUND; > + } Correct use, OK. > + > + if (Buffer == NULL) { > + if (!(Header->Attributes & LOCK_BOX_ATTRIBUTE_RESTORE_IN_PLACE)) { > + return RETURN_WRITE_PROTECTED; > + } > + if (Header->OrigAddress + Header->Size > MAX_ADDRESS) { (9) This check is still off-by-one (it is too strict). Please search my v4 review for "Almost OK (both bitnesses), except we should write" Suppose: - Header->OrigAddress = 0xFFFFFFFF - Header->Size == 1 Known: - MAX_ADDRESS = 0xFFFFFFFF (for Ia32) The addition is done in 64-bit on Ia32 too (because Header->OrigAddress has type EFI_PHYSICAL_ADDRESS, ie. UINT64). Your version: (Header->OrigAddress + Header->Size > MAX_ADDRESS) == (0xFFFFFFFFull + 1 > 0xFFFFFFFF) == (0x100000000ull > 0xFFFFFFFF) == TRUE will reject the operation. My proposed version: (Header->OrigAddress + (Header->Size - 1) > MAX_ADDRESS) == (0xFFFFFFFFull + (1 - 1) > 0xFFFFFFFF) == (0xFFFFFFFFull + 0 > 0xFFFFFFFF) == (0xFFFFFFFFull > 0xFFFFFFFF) == FALSE would allow the operation. (I'll repeat that Size is positive here, so we can subtract like that.) > + return RETURN_UNSUPPORTED; > + } > + Buffer = (VOID *)(UINTN) Header->OrigAddress; > + } > + > + // > + // Set RestoreLength > + // > + if (Length != NULL) { > + if (Header->Size > *Length) { > + // > + // Input buffer is too small to hold all data. > + // > + *Length = Header->Size; > + return EFI_BUFFER_TOO_SMALL; > + } > + *Length = Header->Size; > + } Good. > + > + CopyMem (Buffer, (VOID*)(UINTN) Header->CopyAddress, Header->Size); > + > + return RETURN_SUCCESS; > +} Seems fine. > + > + > +/** > + This function will restore confidential information from all lockbox which > + have RestoreInPlace attribute. > + > + @retval RETURN_SUCCESS the information is restored successfully. > + @retval RETURN_NOT_STARTED it is too early to invoke this interface > + @retval RETURN_UNSUPPORTED the service is not supported by > + implementaion. > +**/ > +RETURN_STATUS > +EFIAPI > +RestoreAllLockBoxInPlace ( > + VOID > + ) > +{ > + LOCK_BOX_ENTRY *Header; > + > + for (Header = StartOfEntries; > + Header < EndOfEntries && Header->Size > 0; > + Header++) { > + if (Header->Attributes & LOCK_BOX_ATTRIBUTE_RESTORE_IN_PLACE) { > + VOID *Buffer; > + > + if (Header->OrigAddress + Header->Size > MAX_ADDRESS) { > + return RETURN_UNSUPPORTED; > + } (10) The logic and the update in v6 is good, but I think it is off-by-one, same as (9). > + Buffer = (VOID *)(UINTN) Header->OrigAddress; > + CopyMem (Buffer, (VOID*)(UINTN)Header->CopyAddress, Header->Size); > + DEBUG ((DEBUG_VERBOSE, "%a: Guid=%g Buffer=%p\n", __FUNCTION__, > + Header->Guid, Buffer)); > + } > + } > + return RETURN_SUCCESS; > +} > diff --git a/OvmfPkg/Library/LockBoxLib/LockBoxLib.h > b/OvmfPkg/Library/LockBoxLib/LockBoxLib.h > new file mode 100644 > index 0000000..fe2166b > --- /dev/null > +++ b/OvmfPkg/Library/LockBoxLib/LockBoxLib.h > @@ -0,0 +1,60 @@ > +/** @file > + > + Copyright (c) 2006 - 2014, Intel Corporation. All rights reserved.<BR> > + > + This program and the accompanying materials > + are licensed and made available under the terms and conditions of the BSD > License > + which accompanies this distribution. The full text of the license may be > found at > + http://opensource.org/licenses/bsd-license.php > + > + THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, > + WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR > IMPLIED. > + > +**/ > + > +#ifndef __LOCK_BOX_LIB_IMPL_H__ > +#define __LOCK_BOX_LIB_IMPL_H__ > + > +#pragma pack(1) > + > +typedef struct { > + UINT32 Signature; > + UINT32 SubPageBuffer; > + UINTN SubPageRemaining; > +} LOCK_BOX_GLOBAL; > + > +#define LOCK_BOX_GLOBAL_SIGNATURE SIGNATURE_32('L', 'B', 'G', 'S') > + > +extern LOCK_BOX_GLOBAL *mLockBoxGlobal; > + > +#pragma pack() > + > +/** > + Allocates a buffer of type EfiACPIMemoryNVS. > + > + Allocates the number bytes specified by AllocationSize of type > + EfiACPIMemoryNVS and returns a pointer to the allocated buffer. > + If AllocationSize is 0, then a valid buffer of 0 size is > + returned. If there is not enough memory remaining to satisfy > + the request, then NULL is returned. > + > + @param AllocationSize The number of bytes to allocate. > + > + @return A pointer to the allocated buffer or NULL if allocation fails. > + > +**/ > +VOID * > +EFIAPI > +AllocateAcpiNvsPool ( > + IN UINTN AllocationSize > + ); > + > + > +RETURN_STATUS > +EFIAPI > +LockBoxLibInitialize ( > + VOID > + ); > + > + > +#endif > diff --git a/OvmfPkg/OvmfPkg.dec b/OvmfPkg/OvmfPkg.dec > index 8a52bb1..b8a4cd5 100644 > --- a/OvmfPkg/OvmfPkg.dec > +++ b/OvmfPkg/OvmfPkg.dec > @@ -85,6 +85,8 @@ > gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPeiTempRamBase|0x0|UINT32|0x13 > gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPeiTempRamSize|0x0|UINT32|0x14 > gUefiOvmfPkgTokenSpaceGuid.PcdS3AcpiReservedMemoryBase|0x0|UINT32|0x17 > + gUefiOvmfPkgTokenSpaceGuid.PcdOvmfLockBoxStorageBase|0x0|UINT32|0x18 > + gUefiOvmfPkgTokenSpaceGuid.PcdOvmfLockBoxStorageSize|0x0|UINT32|0x19 > > [PcdsDynamic, PcdsDynamicEx] > gUefiOvmfPkgTokenSpaceGuid.PcdEmuVariableEvent|0|UINT64|2 > diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc > index cbb97d8..d4bfbfa 100644 > --- a/OvmfPkg/OvmfPkgIa32.dsc > +++ b/OvmfPkg/OvmfPkgIa32.dsc > @@ -99,7 +99,7 @@ > QemuFwCfgLib|OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLib.inf > VirtioLib|OvmfPkg/Library/VirtioLib/VirtioLib.inf > LoadLinuxLib|OvmfPkg/Library/LoadLinuxLib/LoadLinuxLib.inf > - LockBoxLib|MdeModulePkg/Library/LockBoxNullLib/LockBoxNullLib.inf > + LockBoxLib|OvmfPkg/Library/LockBoxLib/LockBoxBaseLib.inf > > CustomizedDisplayLib|MdeModulePkg/Library/CustomizedDisplayLib/CustomizedDisplayLib.inf > > !ifdef $(SOURCE_DEBUG_ENABLE) > @@ -244,6 +244,7 @@ > DpcLib|MdeModulePkg/Library/DxeDpcLib/DxeDpcLib.inf > PlatformBdsLib|OvmfPkg/Library/PlatformBdsLib/PlatformBdsLib.inf > > CpuExceptionHandlerLib|UefiCpuPkg/Library/CpuExceptionHandlerLib/DxeCpuExceptionHandlerLib.inf > + LockBoxLib|OvmfPkg/Library/LockBoxLib/LockBoxDxeLib.inf > > [LibraryClasses.common.UEFI_APPLICATION] > HobLib|MdePkg/Library/DxeHobLib/DxeHobLib.inf > diff --git a/OvmfPkg/OvmfPkgIa32.fdf b/OvmfPkg/OvmfPkgIa32.fdf > index d935d97..a03c52a 100644 > --- a/OvmfPkg/OvmfPkgIa32.fdf > +++ b/OvmfPkg/OvmfPkgIa32.fdf > @@ -138,6 +138,9 @@ NumBlocks = 0x80 > 0x000000|0x006000 > > gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPageTablesBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPageTablesSize > > +0x006000|0x001000 > +gUefiOvmfPkgTokenSpaceGuid.PcdOvmfLockBoxStorageBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfLockBoxStorageSize > + > 0x010000|0x008000 > > gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPeiTempRamBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPeiTempRamSize > > diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc > index 8e5baa2..90a73e0 100644 > --- a/OvmfPkg/OvmfPkgIa32X64.dsc > +++ b/OvmfPkg/OvmfPkgIa32X64.dsc > @@ -104,7 +104,7 @@ > QemuFwCfgLib|OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLib.inf > VirtioLib|OvmfPkg/Library/VirtioLib/VirtioLib.inf > LoadLinuxLib|OvmfPkg/Library/LoadLinuxLib/LoadLinuxLib.inf > - LockBoxLib|MdeModulePkg/Library/LockBoxNullLib/LockBoxNullLib.inf > + LockBoxLib|OvmfPkg/Library/LockBoxLib/LockBoxBaseLib.inf > > CustomizedDisplayLib|MdeModulePkg/Library/CustomizedDisplayLib/CustomizedDisplayLib.inf > > !ifdef $(SOURCE_DEBUG_ENABLE) > @@ -249,6 +249,7 @@ > DpcLib|MdeModulePkg/Library/DxeDpcLib/DxeDpcLib.inf > PlatformBdsLib|OvmfPkg/Library/PlatformBdsLib/PlatformBdsLib.inf > > CpuExceptionHandlerLib|UefiCpuPkg/Library/CpuExceptionHandlerLib/DxeCpuExceptionHandlerLib.inf > + LockBoxLib|OvmfPkg/Library/LockBoxLib/LockBoxDxeLib.inf > > [LibraryClasses.common.UEFI_APPLICATION] > HobLib|MdePkg/Library/DxeHobLib/DxeHobLib.inf > diff --git a/OvmfPkg/OvmfPkgIa32X64.fdf b/OvmfPkg/OvmfPkgIa32X64.fdf > index 29f365f..93fece0 100644 > --- a/OvmfPkg/OvmfPkgIa32X64.fdf > +++ b/OvmfPkg/OvmfPkgIa32X64.fdf > @@ -138,6 +138,9 @@ NumBlocks = 0x80 > 0x000000|0x006000 > > gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPageTablesBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPageTablesSize > > +0x006000|0x001000 > +gUefiOvmfPkgTokenSpaceGuid.PcdOvmfLockBoxStorageBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfLockBoxStorageSize > + > 0x010000|0x008000 > > gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPeiTempRamBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPeiTempRamSize > > diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc > index c934ddc..d9384f9 100644 > --- a/OvmfPkg/OvmfPkgX64.dsc > +++ b/OvmfPkg/OvmfPkgX64.dsc > @@ -104,7 +104,7 @@ > QemuFwCfgLib|OvmfPkg/Library/QemuFwCfgLib/QemuFwCfgLib.inf > VirtioLib|OvmfPkg/Library/VirtioLib/VirtioLib.inf > LoadLinuxLib|OvmfPkg/Library/LoadLinuxLib/LoadLinuxLib.inf > - LockBoxLib|MdeModulePkg/Library/LockBoxNullLib/LockBoxNullLib.inf > + LockBoxLib|OvmfPkg/Library/LockBoxLib/LockBoxBaseLib.inf > > CustomizedDisplayLib|MdeModulePkg/Library/CustomizedDisplayLib/CustomizedDisplayLib.inf > > !ifdef $(SOURCE_DEBUG_ENABLE) > @@ -249,6 +249,7 @@ > DpcLib|MdeModulePkg/Library/DxeDpcLib/DxeDpcLib.inf > PlatformBdsLib|OvmfPkg/Library/PlatformBdsLib/PlatformBdsLib.inf > > CpuExceptionHandlerLib|UefiCpuPkg/Library/CpuExceptionHandlerLib/DxeCpuExceptionHandlerLib.inf > + LockBoxLib|OvmfPkg/Library/LockBoxLib/LockBoxDxeLib.inf > > [LibraryClasses.common.UEFI_APPLICATION] > HobLib|MdePkg/Library/DxeHobLib/DxeHobLib.inf > diff --git a/OvmfPkg/OvmfPkgX64.fdf b/OvmfPkg/OvmfPkgX64.fdf > index c08024e..b1444d9 100644 > --- a/OvmfPkg/OvmfPkgX64.fdf > +++ b/OvmfPkg/OvmfPkgX64.fdf > @@ -138,6 +138,9 @@ NumBlocks = 0x80 > 0x000000|0x006000 > > gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPageTablesBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPageTablesSize > > +0x006000|0x001000 > +gUefiOvmfPkgTokenSpaceGuid.PcdOvmfLockBoxStorageBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfLockBoxStorageSize > + > 0x010000|0x008000 > > gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPeiTempRamBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPeiTempRamSize > > diff --git a/OvmfPkg/PlatformPei/MemDetect.c b/OvmfPkg/PlatformPei/MemDetect.c > index 37030e6..c1350b9 100644 > --- a/OvmfPkg/PlatformPei/MemDetect.c > +++ b/OvmfPkg/PlatformPei/MemDetect.c > @@ -24,6 +24,7 @@ Module Name: > // > // The Library classes this module consumes > // > +#include <Library/BaseMemoryLib.h> > #include <Library/DebugLib.h> > #include <Library/HobLib.h> > #include <Library/IoLib.h> > @@ -217,5 +218,21 @@ InitializeRamRegions ( > EfiACPIMemoryNVS > ); > #endif > + > + // > + // Reserve the lock box storage area > + // > + // Since this memory range will be used on S3 resume, it must be > + // reserved as ACPI NVS. > + // > + ZeroMem ( > + (VOID*)(UINTN) PcdGet32 (PcdOvmfLockBoxStorageBase), > + (UINTN) PcdGet32 (PcdOvmfLockBoxStorageSize) > + ); > + BuildMemoryAllocationHob ( > + (EFI_PHYSICAL_ADDRESS)(UINTN) PcdGet32 (PcdOvmfLockBoxStorageBase), > + (UINT64)(UINTN) PcdGet32 (PcdOvmfLockBoxStorageSize), > + EfiACPIMemoryNVS > + ); > } > } The "reservation not happening on Xen" comment has been addressed by your earlier refactoring of "MemDetect.c". OK. > diff --git a/OvmfPkg/PlatformPei/PlatformPei.inf > b/OvmfPkg/PlatformPei/PlatformPei.inf > index c915138..3b47bb7 100644 > --- a/OvmfPkg/PlatformPei/PlatformPei.inf > +++ b/OvmfPkg/PlatformPei/PlatformPei.inf > @@ -70,6 +70,8 @@ > gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPeiTempRamSize > gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPageTablesBase > gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPageTablesSize > + gUefiOvmfPkgTokenSpaceGuid.PcdOvmfLockBoxStorageBase > + gUefiOvmfPkgTokenSpaceGuid.PcdOvmfLockBoxStorageSize > gEfiIntelFrameworkModulePkgTokenSpaceGuid.PcdS3AcpiReservedMemorySize > gEfiMdeModulePkgTokenSpaceGuid.PcdVariableStoreSize > gEfiMdeModulePkgTokenSpaceGuid.PcdFlashNvStorageFtwSpareSize > Let me categorize: (1) fixable at commit time, (2) fixable at commit time, (3) fixable at commit time, (4) fixable at commit time, (5) fixable at commit time, (6) fixable at commit time, (7) fixable at commit time, (8) fixable at commit time (just the leading comment on FindHeaderByGuid()) (9) and (10): I think you could be disagreeing with me on these. In practice these off-by-one's shouldn't matter (they do err on the safe side!), and we can address them later in a separate patch, if we want. With the above remarks, and also referring to the FILE_GUID discrimination already in your repo: Reviewed-by: Laszlo Ersek <ler...@redhat.com> Thank you very much! Laszlo ------------------------------------------------------------------------------ Flow-based real-time traffic analytics software. Cisco certified tool. Monitor traffic, SLAs, QoS, Medianet, WAAS etc. with NetFlow Analyzer Customize your own dashboards, set traffic alerts and generate reports. Network behavioral analysis & security monitoring. All-in-one tool. http://pubads.g.doubleclick.net/gampad/clk?id=126839071&iu=/4140/ostg.clktrk _______________________________________________ edk2-devel mailing list edk2-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/edk2-devel
