RFC lazy accept changes from TDX adapted to SEV-SNP #4
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
1. Add definition EFI_RESOURCE_MEMORY_UNACCEPTED 0x7 to Hand-of-Block (HOB) resource type in the PiHob.h 2. Add EfiGcdMemoryTypeUnaccepted to EFI_GCD_MEMORY_TYPE. 3. Add EfiUnacceptedMemory to EFI_MEMORY_TYPE. CoreInitializeGcdServices() and CoreGetMemoryMap() are updated to handle the unaccepted memory type. Ref: microsoft/mu_basecore@97e9c31 Signed-off-by: Dionna Glaze <dionnaglaze@google.com>
AmdSevSnpInitialize is modified to partially prevalidate memory of a certain size specified by a fixed PCD name PcdOvmfSevSnpAcceptPartialMemorySize, which can be set by a build-time definition SEV_SNP_MEM_PARTIAL_ACCEPT. A value of 0 is interpreted as the entire memory space. MemEncryptSevSnpPrevalidateSystemMemory is changed to additionally record the range of memory it is called with as another prevalidated range to avoid. The internal function DetectPreValidatedOverlap is exported as a library function named MemEncryptDetectPreValidatedOverlap since it is now used in the construction of unaccepted memory HOBs. The last step of PlatformPei's InitializePlatform is to call a new function, AmdSevSnpTransferHobs, which modifies existing EFI_RESOURCE_SYSTEM_MEMORY HOBs to be either EFI_RESOURCE_MEMORY_UNACCEPTED in the case that it doesn't overlap with pre-validated ranges, or to change their size and start address to mirror the accepted range it overlaps with. Ref: tianocore/edk2-staging@8dc6718 Signed-off-by: Dionna Glaze <dionnaglaze@google.com>
ShellCommandRunMemMap() is updated to handle the case of unaccepted memory type. Ref: tianocore/edk2-staging@a2d32c8 Signed-off-by: Dionna Glaze <dionnaglaze@google.com>
EfiMemoryAcceptProtocol is defined in MdePkg, the method AcceptMemory() can be called when memory needs to be accepted. EfiMemoryAcceptProtocol can be installed by architecture-specific drivers such as TdxDxe.This allows different isolation architectures to realize their own low-level methods to accept memory. Ref: tianocore/edk2-staging@194ad0c Signed-off-by: Dionna Glaze <dionnaglaze@google.com>
Memory usage may exceed the amount accepted at the begining (SEC), DXE needs to accept memory dynamically when OUT_OF_RESOURCE occurs. EfiMemoryAcceptProtocol is defined in MdePkg and is implementated by AmdSevDxe for AMD SEV-SNP memory acceptance. Ref: tianocore/edk2-staging@cd5d400 Signed-off-by: Dionna Glaze <dionnaglaze@google.com>
When CoreAllocatePages()/CoreAllocatePool() meets error of EFI_OUT_OF_RESOURCES, locate the EfiMemoryAcceptProtocol and accept extra memory dynamically. Firstly, find the unaccpeted memory region with enough size in GCD entries. Then locate the EfiMemoryAcceptProtocol and accept the memory. Finally, update the GCD memory and gMemoryMap entries. After updating the memory infomation, CoreInternalAllocatePages()/ CoreInternalAllocatePool() will be recalled to allocate pages/pool. To do (existing issue): 1) CoreAddMemorySpace() should not be called directly in AcceptMemoryResource() until gMemoryMap is updated because it will try to allocate another piece of memory for GCD map entry. 2) AcceptMemoryResource() is called in both CoreAllocatePool() and CoreAllocatePages(). Refine this by changing FindFreePages() and the CoreConvertPagesEx() to handle *AllocateAddress* case. Ref: tianocore/edk2-staging@a67dd91 Ref: tianocore/edk2-staging@f81e65d Ref: tianocore/edk2-staging@b6fef70 Signed-off-by: Dionna Glaze <dionnaglaze@google.com>
|
Can this be simplified for the SNP case? The Page State Change call can take multiple pages/ranges in a single call, so there is a lot less vm-exiting doing a block accept vs. one at a time later. What would be the difference in time (from OVMF entry to grub menu) if you were to accept up to the first 4GB (2GB really because of the MMIO hole) and leave the remaining memory to be accepted by the kernel or DXE for memory allocations above 4GB? |
|
Given Intel's changes will be accepted into OVMF, only 2 commits here would be new for SNP, and 1 is a trivial implementation of the MemoryAccept protocol. I think accepting all hobs <= 4GiB instead of this range mess could be acceptable for most VMs, but for enclave-like UEFI applications, they'll want to have better startup behavior that doesn't need to accept so much memory up front. |
mxu9
reviewed
Jun 1, 2022
| @@ -232,7 +232,8 @@ typedef UINT32 EFI_RESOURCE_TYPE; | |||
| #define EFI_RESOURCE_MEMORY_MAPPED_IO_PORT 0x00000004 | |||
| #define EFI_RESOURCE_MEMORY_RESERVED 0x00000005 | |||
| #define EFI_RESOURCE_IO_RESERVED 0x00000006 | |||
| #define EFI_RESOURCE_MAX_MEMORY_TYPE 0x00000007 | |||
There was a problem hiding this comment.
Since EFI_RESOURCE_MEMORY_UNACCEPTED has not been officially published in PI spec, it cannot be declared in MdePkg. (This is told by the MdePkg maintainer -- Liming Gao gaoliming@byosoft.com.cn.) Instead it can be defined in MdeModulePkg.
There was a problem hiding this comment.
Do we know when the next spec will be published?
There was a problem hiding this comment.
I am afraid it will not be published in short-term. (maybe next year.)
But we still can define it in MdeModulePkg.
mdroth
pushed a commit
that referenced
this pull request
Mar 29, 2024
REF:https://bugzilla.tianocore.org/show_bug.cgi?id=4537 REF:https://bugzilla.tianocore.org/show_bug.cgi?id=4538 Bug Details: PixieFail Bug #4 CVE-2023-45232 CVSS 7.5 : CVSS:3.1/AV:N/AC:L/PR:N/UI:N/S:U/C:N/I:N/A:H CWE-835 Loop with Unreachable Exit Condition ('Infinite Loop') Infinite loop when parsing unknown options in the Destination Options header PixieFail Bug #5 CVE-2023-45233 CVSS 7.5 : CVSS:3.1/AV:N/AC:L/PR:N/UI:N/S:U/C:N/I:N/A:H CWE-835 Loop with Unreachable Exit Condition ('Infinite Loop') Infinite loop when parsing a PadN option in the Destination Options header Change Overview: Most importantly this change corrects the following incorrect math and cleans up the code. > // It is a PadN option > // > - Offset = (UINT8)(Offset + *(Option + Offset + 1) + 2); > + OptDataLen = ((EFI_IP6_OPTION *)(Option + Offset))->Length; > + Offset = IP6_NEXT_OPTION_OFFSET (Offset, OptDataLen); > case Ip6OptionSkip: > - Offset = (UINT8)(Offset + *(Option + Offset + 1)); > OptDataLen = ((EFI_IP6_OPTION *)(Option + Offset))->Length; > Offset = IP6_NEXT_OPTION_OFFSET (Offset, OptDataLen); Additionally, this change also corrects incorrect math where the calling function was calculating the HDR EXT optionLen as a uint8 instead of a uint16 > - OptionLen = (UINT8)((*Option + 1) * 8 - 2); > + OptionLen = IP6_HDR_EXT_LEN (*Option) - IP6_COMBINED_SIZE_OF_NEXT_HDR_AND_LEN; Additionally this check adds additional logic to santize the incoming data Cc: Saloni Kasbekar <saloni.kasbekar@intel.com> Cc: Zachary Clark-williams <zachary.clark-williams@intel.com> Signed-off-by: Doug Flick [MSFT] <doug.edk2@gmail.com> Reviewed-by: Saloni Kasbekar <saloni.kasbekar@intel.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The Intel folks have been working on "lazy accept" in Linux and their own fork of OVMF. Brijesh Singh wrote AMDESE/linux/v12+unaccepted for the Linux side of adaptation for SEV-SNP, and a colleague Sophia Wolf and I have had a go at adapting OVMF changes to work for SEV-SNP. Each commit here references the Intel edk2-staging TDVF branch they've created for TDX changes. I have not been able to boot v12+unaccepted with these changes, but want to get y'all's sense of if this is the right approach to adaptation for SEV-SNP.
The novel commits here are 3efcfe4 and c198512. The rest are verbatim from the TDVF branch.