On 9/30/22 18:06, Dionna Glaze wrote:
From: Sophia Wolf <phiaw...@google.com>
When a guest OS does not support unaccepted memory, the unaccepted
memory must be accepted before returning a memory map to the caller.
EfiMemoryAcceptProtocol is defined in MdePkg and is implemented /
Installed in AmdSevDxe for AMD SEV-SNP memory acceptance.
Cc: Gerd Hoffmann <kra...@redhat.com>
Cc: James Bottomley <j...@linux.ibm.com>
Cc: Jiewen Yao <jiewen....@intel.com>
Cc: Tom Lendacky <thomas.lenda...@amd.com>
Signed-off-by: Dionna Glaze <dionnagl...@google.com>
Just some formatting suggestions and one area of cleanup from previous
version of the patch below. Assuming you take care of those:
Reviewed-by: Tom Lendacky <thomas.lenda...@amd.com>
---
OvmfPkg/AmdSevDxe/AmdSevDxe.c | 57
++++++++++++++++++--
OvmfPkg/AmdSevDxe/AmdSevDxe.inf | 3 ++
OvmfPkg/Library/BaseMemEncryptSevLib/X64/DxeSnpSystemRamValidate.c | 24
+++++++--
3 files changed, 76 insertions(+), 8 deletions(-)
diff --git a/OvmfPkg/AmdSevDxe/AmdSevDxe.c b/OvmfPkg/AmdSevDxe/AmdSevDxe.c
index 662d3c4ccb..77d3caa833 100644
--- a/OvmfPkg/AmdSevDxe/AmdSevDxe.c
+++ b/OvmfPkg/AmdSevDxe/AmdSevDxe.c
@@ -20,6 +20,7 @@
#include <Library/UefiBootServicesTableLib.h>
#include <Guid/ConfidentialComputingSevSnpBlob.h>
#include <Library/PcdLib.h>
+#include <Protocol/MemoryAccept.h>
STATIC CONFIDENTIAL_COMPUTING_SNP_BLOB_LOCATION mSnpBootDxeTable = {
SIGNATURE_32 ('A', 'M', 'D', 'E'),
@@ -31,6 +32,38 @@ STATIC CONFIDENTIAL_COMPUTING_SNP_BLOB_LOCATION
mSnpBootDxeTable = {
FixedPcdGet32 (PcdOvmfCpuidSize),
};
+STATIC EFI_HANDLE mAmdSevDxeHandle = NULL;
+
+STATIC
+EFI_STATUS
+EFIAPI
+AmdSevMemoryAccept (
+ IN EFI_MEMORY_ACCEPT_PROTOCOL *This,
+ IN EFI_PHYSICAL_ADDRESS StartAddress,
+ IN UINTN Size
+)
+{
+ //
+ // The StartAddress must be page-aligned, and the Size must be a positive
+ // multiple of SIZE_4KB. Use an assert instead of returning an erros since
+ // this is an EDK2-internal protocol.
+ //
+ ASSERT (((StartAddress & ~(SIZE_4KB - 1)) == 0) &&
+ ((Size & ~(SIZE_4KB - 1)) == 0) &&
+ (Size != 0));
Create a generic alignment check macro?
#define IS_ALIGNED(x, y) (((x) & ((y) - 1)) == 0)
Maybe keep the ASSERTs separate so they better identify which condition
caused the assert, e.g.:
ASSERT (IS_ALIGNED (StartAddress, SIZE_4KB));
ASSERT (IS_ALIGNED (Size, SIZE_4KB));
ASSERT (Size != 0);
?
Not sure if those are worth it or not, though.
+
+ MemEncryptSevSnpPreValidateSystemRam (
+ StartAddress,
+ EFI_SIZE_TO_PAGES (Size)
+ );
+
+ return EFI_SUCCESS;
+}
+
+STATIC EFI_MEMORY_ACCEPT_PROTOCOL mMemoryAcceptProtocol = {
+ AmdSevMemoryAccept
+};
+
EFI_STATUS
EFIAPI
AmdSevDxeEntryPoint (
@@ -147,11 +180,27 @@ AmdSevDxeEntryPoint (
}
}
- //
- // If its SEV-SNP active guest then install the
CONFIDENTIAL_COMPUTING_SEV_SNP_BLOB.
- // It contains the location for both the Secrets and CPUID page.
- //
+ if (EFI_ERROR (Status)) {
+ DEBUG ((DEBUG_ERROR, "Install EfiMemoryAcceptProtocol failed.\n"));
+ }
Looks like this shouldn't be here.
+
if (MemEncryptSevSnpIsEnabled ()) {
+ //
+ // Memory acceptance began being required in SEV-SNP, so install the
+ // memory accept protocol implementation for a SEV-SNP active guest.
+ //
+ Status = gBS->InstallProtocolInterface (
+ &mAmdSevDxeHandle,
+ &gEfiMemoryAcceptProtocolGuid,
+ EFI_NATIVE_INTERFACE,
+ &mMemoryAcceptProtocol
+ );
Need to indent these two more spaces to align with the "s" in Install.
Thanks,
Tom
+ ASSERT_EFI_ERROR (Status);
+
+ //
+ // If its SEV-SNP active guest then install the
CONFIDENTIAL_COMPUTING_SEV_SNP_BLOB.
+ // It contains the location for both the Secrets and CPUID page.
+ //
return gBS->InstallConfigurationTable (
&gConfidentialComputingSevSnpBlobGuid,
&mSnpBootDxeTable
diff --git a/OvmfPkg/AmdSevDxe/AmdSevDxe.inf b/OvmfPkg/AmdSevDxe/AmdSevDxe.inf
index 9acf860cf2..5ddddabc32 100644
--- a/OvmfPkg/AmdSevDxe/AmdSevDxe.inf
+++ b/OvmfPkg/AmdSevDxe/AmdSevDxe.inf
@@ -47,6 +47,9 @@
gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSnpSecretsBase
gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSnpSecretsSize
+[Protocols]
+ gEfiMemoryAcceptProtocolGuid
+
[Guids]
gConfidentialComputingSevSnpBlobGuid
diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/DxeSnpSystemRamValidate.c b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/DxeSnpSystemRamValidate.c
index d3a95e4913..ee3710f7b3 100644
--- a/OvmfPkg/Library/BaseMemEncryptSevLib/X64/DxeSnpSystemRamValidate.c
+++ b/OvmfPkg/Library/BaseMemEncryptSevLib/X64/DxeSnpSystemRamValidate.c
@@ -14,6 +14,7 @@
#include <Library/MemEncryptSevLib.h>
#include "SnpPageStateChange.h"
+#include "VirtualMemory.h"
/**
Pre-validate the system RAM when SEV-SNP is enabled in the guest VM.
@@ -29,12 +30,27 @@ MemEncryptSevSnpPreValidateSystemRam (
IN UINTN NumPages
)
{
+ EFI_STATUS Status;
+
if (!MemEncryptSevSnpIsEnabled ()) {
return;
}
- //
- // All the pre-validation must be completed in the PEI phase.
- //
- ASSERT (FALSE);
+ // DXE pre-validation may happen with the memory accept protocol.
+ // The protocol should only be called outside the prevalidated ranges
+ // that the PEI stage code explicitly skips. Specifically, only memory
+ // ranges that are classified as unaccepted.
+ if (BaseAddress >= SIZE_4GB) {
+ Status = InternalMemEncryptSevCreateIdentityMap1G (
+ 0,
+ BaseAddress,
+ EFI_PAGES_TO_SIZE (NumPages)
+ );
+ if (EFI_ERROR (Status)) {
+ ASSERT (FALSE);
+ CpuDeadLoop ();
+ }
+ }
+
+ InternalSetPageState (BaseAddress, NumPages, SevSnpPagePrivate, TRUE);
}
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#94673): https://edk2.groups.io/g/devel/message/94673
Mute This Topic: https://groups.io/mt/94028458/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
