[PATCH v11 0/4] Add safe unaccepted memory behavior

[PATCH v11 0/4] Add safe unaccepted memory behavior

[Dionna Glaze]

We make eager memory acceptance the default behavior at
ExitBootServices for SEV-SNP machines by using the standard-enforced
behavior that if the call returns an error code, then the map key is
incorrect and the caller must re-call GetMemoryMap to ensure the
contents are correct.

Eager memory acceptance is implemented by using the UEFI v2.9-added
EFI_EVENT_GROUP_BEFORE_EXIT_BOOT_SERVICES to check a support condition
before changing all unaccepted memory type regions to conventional
memory after first using the MemoryAccept protocol to accept all memory
in each region. This update to the memory map only happens once, since
there are no extra unaccepted memory regions to change on the forced
second call to ExitBootServices.

The new acceptance logic is required only for SEV-SNP since it is the
only memory-accepting virtualization technology with kernel support live
without unaccepted memory support.

To allow the OS loader to prevent the eager acceptance, and thus pass
the before-mentioned "support condition", we add a new protocol,
OvmfSevMemoryAcceptance.  This protocol has one interface,
AllowUnacceptedMemory(). The OS loader can inform the UEFI that it
supports the unaccepted memory type and accepts the responsibility to
accept it.

The OvmfSevMemoryAcceptance protocol is necessary for safe rollout of
the unaccepted memory type in SEV-SNP-enabled kernels, given the
gradual update of guest OS kernels.

All images that support unaccepted memory must now locate and call this
new OVMF_SEV_MEMORY_ACCEPTANCE_PROTOCOL and call the
AllowUnacceptedMemory function.

Changes since v10:
 - AmdSevDxe called AcceptMemory directly without locating the
   MemoryAccept protocol.
 - The protocol is no longer a candidate for standardization and has
   moved to OvmfPkg/Include/Protocol.
Changes since v9:
 - Renamed protocol to SevMemoryAcceptance.
 - Removed CocoDxe and moved all contained code to AmdSevDxe.
 - Renamed protocol header file to reference the bugzilla number.
Changes since v8:
 - First 3 patches removed since they were submitted separately.
 - Later patches rebased on edk2/master and modified to work with the
   current locations and namings of the unaccepted memory constants.
Changes since v7:
 - Rebased onto lazy accept v4 patch series, so memory accept protocol
   has the EDKII prefix, and the unaccepted memory type has the BZ3937
   prefix.
 - Removed a bad #include to a header removed in v7.
 - Renamed the protocol to BZ3987_MEMORY_ACCEPTANCE_PROTOCOL as per the
   discussion on the buganizer issue.
 - Uncrustify formatting

Changes since v6:
 - Added implementation of EFI_EVENT_GROUP_BEFORE_EXIT_BOOT_SERVICES.
 - Changed callback protocol of v5 to instead use the standardized event
   group for before_exit_boot_services.

Changes since v5:
 - Generic callback protocol moved to MdeModulePkg
 - Removed use of EFI_WARN_STALE_DATA and added comment that the callback
   should only return EFI_SUCCESS or EFI_INVALID_PARAMETER.
 - Removed errant log statement and fixed formatting.

Changes since v4:
 - Commit message wording
 - Replaced direct change to DxeMain with a more generic callback
   protocol.
 - Implemented the direct change as an instance of the callback protocol
   from a new CocoDxe driver.
 - Replaced "enable" protocol with a "disable" protocol, since the name
   was confusing. The AcceptAllUnacceptedMemory protocol directly names
   the behavior that is disabling.

Changes since v3:
 - "DxeMain accepts all memory" patch split into 3 to make each patch
   affect only one package at a time.

Changes since v2:
 - Removed the redundant memory accept interface and added the accept
   behavior to the DXE implementation of
   MemEncryptSevSnpPreValidateSystemRam.
 - Fixed missing #include in >=4GB patch.

Changes since v1:
 - Added a patch to classify SEV-SNP memory above 4GB unaccepted.
 - Fixed style problems in EfiMemoryAcceptProtocol implementation.

Cc: Ard Biescheuvel <ardb@kernel.org>
Cc: "Min M. Xu" <min.m.xu@intel.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: James Bottomley <jejb@linux.ibm.com>
Cc: Tom Lendacky <Thomas.Lendacky@amd.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Erdem Aktas <erdemaktas@google.com>
Cc: Andrew Fish <afish@apple.com>
Cc: "Michael D. Kinney" <michael.d.kinney@intel.com>

Signed-off-by: Dionna Glaze <dionnaglaze@google.com>

Dionna Glaze (4):
  OvmfPkg: Add memory acceptance event in AmdSevDxe
  MdePkg: Introduce the SevMemoryAcceptance protocol
  OvmfPkg: Implement AcceptAllUnacceptedMemory in AmdSevDxe
  OvmfPkg/PlatformPei: SEV-SNP make >=4GB unaccepted

 OvmfPkg/AmdSevDxe/AmdSevDxe.c                  | 123 ++++++++++++++++++++
 OvmfPkg/AmdSevDxe/AmdSevDxe.inf                |   2 +
 OvmfPkg/Include/Protocol/SevMemoryAcceptance.h |  42 +++++++
 OvmfPkg/OvmfPkg.dec                            |   1 +
 OvmfPkg/PlatformPei/AmdSev.c                   |   5 +
 5 files changed, 173 insertions(+)
 create mode 100644 OvmfPkg/Include/Protocol/SevMemoryAcceptance.h

-- 
2.39.1.456.gfc5497dd1b-goog

[PATCH v11 1/4] OvmfPkg: Add memory acceptance event in AmdSevDxe

[Dionna Glaze]

The added behavior is to accept all unaccepted memory at
ExitBootServices if the behavior is not disabled. This allows safe
upgrades for OS loaders to affirm their support for the unaccepted
memory type.

Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: James Bottomley <jejb@linux.ibm.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Cc: Ard Biesheuvel <ardb@kernel.org>
Cc: "Min M. Xu" <min.m.xu@intel.com>
Cc: Andrew Fish <afish@apple.com>
Cc: "Michael D. Kinney" <michael.d.kinney@intel.com>

Signed-off-by: Dionna Glaze <dionnaglaze@google.com>
---
 OvmfPkg/AmdSevDxe/AmdSevDxe.c   | 97 ++++++++++++++++++++
 OvmfPkg/AmdSevDxe/AmdSevDxe.inf |  1 +
 2 files changed, 98 insertions(+)

diff --git a/OvmfPkg/AmdSevDxe/AmdSevDxe.c b/OvmfPkg/AmdSevDxe/AmdSevDxe.c
index f7600c3c81..37d1a3ff55 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 <Pi/PrePiDxeCis.h>
 #include <Protocol/MemoryAccept.h>
 
 STATIC CONFIDENTIAL_COMPUTING_SNP_BLOB_LOCATION  mSnpBootDxeTable = {
@@ -34,6 +35,10 @@ STATIC CONFIDENTIAL_COMPUTING_SNP_BLOB_LOCATION  mSnpBootDxeTable = {
 
 STATIC EFI_HANDLE  mAmdSevDxeHandle = NULL;
 
+STATIC BOOLEAN  mAcceptAllMemoryAtEBS = TRUE;
+
+STATIC EFI_EVENT  mAcceptAllMemoryEvent = NULL;
+
 #define IS_ALIGNED(x, y)  ((((x) & ((y) - 1)) == 0))
 
 STATIC
@@ -62,6 +67,82 @@ AmdSevMemoryAccept (
   return EFI_SUCCESS;
 }
 
+STATIC
+EFI_STATUS
+AcceptAllMemory (
+  VOID
+  )
+{
+  EFI_GCD_MEMORY_SPACE_DESCRIPTOR  *AllDescMap;
+  UINTN                            NumEntries;
+  UINTN                            Index;
+  EFI_STATUS                       Status;
+
+  DEBUG ((DEBUG_INFO, "Accepting all memory\n"));
+
+  /*
+   * Get a copy of the memory space map to iterate over while
+   * changing the map.
+   */
+  Status = gDS->GetMemorySpaceMap (&NumEntries, &AllDescMap);
+  if (EFI_ERROR (Status)) {
+    return Status;
+  }
+
+  for (Index = 0; Index < NumEntries; Index++) {
+    CONST EFI_GCD_MEMORY_SPACE_DESCRIPTOR  *Desc;
+
+    Desc = &AllDescMap[Index];
+    if (Desc->GcdMemoryType != EFI_GCD_MEMORY_TYPE_UNACCEPTED) {
+      continue;
+    }
+
+    Status = AmdSevMemoryAccept (
+               NULL,
+               Desc->BaseAddress,
+               Desc->Length
+               );
+    if (EFI_ERROR (Status)) {
+      break;
+    }
+
+    Status = gDS->RemoveMemorySpace (Desc->BaseAddress, Desc->Length);
+    if (EFI_ERROR (Status)) {
+      break;
+    }
+
+    Status = gDS->AddMemorySpace (
+                    EfiGcdMemoryTypeSystemMemory,
+                    Desc->BaseAddress,
+                    Desc->Length,
+                    EFI_MEMORY_CPU_CRYPTO | EFI_MEMORY_XP | EFI_MEMORY_RO | EFI_MEMORY_RP
+                    );
+    if (EFI_ERROR (Status)) {
+      break;
+    }
+  }
+
+  gBS->FreePool (AllDescMap);
+  return Status;
+}
+
+VOID
+EFIAPI
+ResolveUnacceptedMemory (
+  IN EFI_EVENT  Event,
+  IN VOID       *Context
+  )
+{
+  EFI_STATUS  Status;
+
+  if (!mAcceptAllMemoryAtEBS) {
+    return;
+  }
+
+  Status = AcceptAllMemory ();
+  ASSERT_EFI_ERROR (Status);
+}
+
 STATIC EDKII_MEMORY_ACCEPT_PROTOCOL  mMemoryAcceptProtocol = {
   AmdSevMemoryAccept
 };
@@ -195,6 +276,22 @@ AmdSevDxeEntryPoint (
                     );
     ASSERT_EFI_ERROR (Status);
 
+    // SEV-SNP support does not automatically imply unaccepted memory support,
+    // so make ExitBootServices accept all unaccepted memory if support is
+    // not communicated.
+    Status = gBS->CreateEventEx (
+                    EVT_NOTIFY_SIGNAL,
+                    TPL_CALLBACK,
+                    ResolveUnacceptedMemory,
+                    NULL,
+                    &gEfiEventBeforeExitBootServicesGuid,
+                    &mAcceptAllMemoryEvent
+                    );
+
+    if (EFI_ERROR (Status)) {
+      DEBUG ((DEBUG_ERROR, "AllowUnacceptedMemory event creation for EventBeforeExitBootServices failed.\n"));
+    }
+
     //
     // 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.
diff --git a/OvmfPkg/AmdSevDxe/AmdSevDxe.inf b/OvmfPkg/AmdSevDxe/AmdSevDxe.inf
index cd1b686c53..5b443d45bc 100644
--- a/OvmfPkg/AmdSevDxe/AmdSevDxe.inf
+++ b/OvmfPkg/AmdSevDxe/AmdSevDxe.inf
@@ -52,6 +52,7 @@
 
 [Guids]
   gConfidentialComputingSevSnpBlobGuid
+  gEfiEventBeforeExitBootServicesGuid
 
 [Pcd]
   gUefiOvmfPkgTokenSpaceGuid.PcdOvmfHostBridgePciDevId
-- 
2.39.1.456.gfc5497dd1b-goog

[PATCH v11 2/4] MdePkg: Introduce the SevMemoryAcceptance protocol

[Dionna Glaze]

The default behavior for unaccepted memory in SEV-SNP is to accept all
memory when ExitBootServices is called. An OS loader can use this
protocol to disable this behavior to assume responsibility for memory
acceptance and to affirm that the OS can handle the unaccepted memory
type.

Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: James Bottomley <jejb@linux.ibm.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Cc: Ard Biesheuvel <ardb@kernel.org>
Cc: "Min M. Xu" <min.m.xu@intel.com>
Cc: Andrew Fish <afish@apple.com>
Cc: "Michael D. Kinney" <michael.d.kinney@intel.com>

Signed-off-by: Dionna Glaze <dionnaglaze@google.com>
---
 OvmfPkg/Include/Protocol/SevMemoryAcceptance.h | 42 ++++++++++++++++++++
 OvmfPkg/OvmfPkg.dec                            |  1 +
 2 files changed, 43 insertions(+)

diff --git a/OvmfPkg/Include/Protocol/SevMemoryAcceptance.h b/OvmfPkg/Include/Protocol/SevMemoryAcceptance.h
new file mode 100644
index 0000000000..c45b499006
--- /dev/null
+++ b/OvmfPkg/Include/Protocol/SevMemoryAcceptance.h
@@ -0,0 +1,42 @@
+/** @file
+  The file provides the protocol that disables the behavior that all memory
+  gets accepted at ExitBootServices(). This protocol is only meant to be called
+  by the OS loader, and not EDK2 itself. The SEV naming is due to the coincidence
+  that only SEV-SNP needs this protocol, since SEV-SNP kernel support released
+  before kernel support for unaccepted memory. The technology enablement thus
+  does not strictly imply support for the unaccepted memory type.
+
+  Copyright (c) 2023, Google LLC. All rights reserved.<BR>
+  SPDX-License-Identifier: BSD-2-Clause-Patent
+**/
+
+#ifndef SEV_MEMORY_ACCEPTANCE_H_
+#define SEV_MEMORY_ACCEPTANCE_H_
+
+#define OVMF_SEV_MEMORY_ACCEPTANCE_PROTOCOL_GUID \
+  {0xc5a010fe, \
+   0x38a7, \
+   0x4531, \
+   {0x8a, 0x4a, 0x05, 0x00, 0xd2, 0xfd, 0x16, 0x49}}
+
+typedef struct _OVMF_SEV_MEMORY_ACCEPTANCE_PROTOCOL
+    OVMF_SEV_MEMORY_ACCEPTANCE_PROTOCOL;
+
+/**
+  @param This A pointer to a OVMF_SEV_MEMORY_ACCEPTANCE_PROTOCOL.
+**/
+typedef
+  EFI_STATUS
+(EFIAPI *OVMF_SEV_ALLOW_UNACCEPTED_MEMORY)(
+  IN  OVMF_SEV_MEMORY_ACCEPTANCE_PROTOCOL  *This
+  );
+
+///
+/// The OVMF_SEV_MEMORY_ACCEPTANCE_PROTOCOL allows the OS loader to
+/// indicate to EDK2 that ExitBootServices should not accept all memory.
+///
+struct _OVMF_SEV_MEMORY_ACCEPTANCE_PROTOCOL {
+  OVMF_SEV_ALLOW_UNACCEPTED_MEMORY    AllowUnacceptedMemory;
+};
+
+#endif
diff --git a/OvmfPkg/OvmfPkg.dec b/OvmfPkg/OvmfPkg.dec
index 1b521f2604..a22eb246c6 100644
--- a/OvmfPkg/OvmfPkg.dec
+++ b/OvmfPkg/OvmfPkg.dec
@@ -184,6 +184,7 @@
   gEfiLegacyInterruptProtocolGuid       = {0x31ce593d, 0x108a, 0x485d, {0xad, 0xb2, 0x78, 0xf2, 0x1f, 0x29, 0x66, 0xbe}}
   gEfiVgaMiniPortProtocolGuid           = {0xc7735a2f, 0x88f5, 0x4882, {0xae, 0x63, 0xfa, 0xac, 0x8c, 0x8b, 0x86, 0xb3}}
   gOvmfLoadedX86LinuxKernelProtocolGuid = {0xa3edc05d, 0xb618, 0x4ff6, {0x95, 0x52, 0x76, 0xd7, 0x88, 0x63, 0x43, 0xc8}}
+  gOvmfSevMemoryAcceptanceProtocolGuid  = {0xc5a010fe, 0x38a7, 0x4531, {0x8a, 0x4a, 0x05, 0x00, 0xd2, 0xfd, 0x16, 0x49}}
   gQemuAcpiTableNotifyProtocolGuid      = {0x928939b2, 0x4235, 0x462f, {0x95, 0x80, 0xf6, 0xa2, 0xb2, 0xc2, 0x1a, 0x4f}}
   gEfiMpInitLibMpDepProtocolGuid        = {0xbb00a5ca, 0x8ce,  0x462f, {0xa5, 0x37, 0x43, 0xc7, 0x4a, 0x82, 0x5c, 0xa4}}
   gEfiMpInitLibUpDepProtocolGuid        = {0xa9e7cef1, 0x5682, 0x42cc, {0xb1, 0x23, 0x99, 0x30, 0x97, 0x3f, 0x4a, 0x9f}}
-- 
2.39.1.456.gfc5497dd1b-goog

[PATCH v11 3/4] OvmfPkg: Implement AcceptAllUnacceptedMemory in AmdSevDxe

[Dionna Glaze]

This protocol implementation disables the accept-all-memory behavior
of the BeforeExitBootServices event this driver adds.

Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: James Bottomley <jejb@linux.ibm.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Cc: Ard Biesheuvel <ardb@kernel.org>
Cc: "Min M. Xu" <min.m.xu@intel.com>
Cc: Andrew Fish <afish@apple.com>
Cc: "Michael D. Kinney" <michael.d.kinney@intel.com>

Signed-off-by: Dionna Glaze <dionnaglaze@google.com>
---
 OvmfPkg/AmdSevDxe/AmdSevDxe.c   | 26 ++++++++++++++++++++
 OvmfPkg/AmdSevDxe/AmdSevDxe.inf |  1 +
 2 files changed, 27 insertions(+)

diff --git a/OvmfPkg/AmdSevDxe/AmdSevDxe.c b/OvmfPkg/AmdSevDxe/AmdSevDxe.c
index 37d1a3ff55..9d05a16c6e 100644
--- a/OvmfPkg/AmdSevDxe/AmdSevDxe.c
+++ b/OvmfPkg/AmdSevDxe/AmdSevDxe.c
@@ -21,6 +21,7 @@
 #include <Guid/ConfidentialComputingSevSnpBlob.h>
 #include <Library/PcdLib.h>
 #include <Pi/PrePiDxeCis.h>
+#include <Protocol/SevMemoryAcceptance.h>
 #include <Protocol/MemoryAccept.h>
 
 STATIC CONFIDENTIAL_COMPUTING_SNP_BLOB_LOCATION  mSnpBootDxeTable = {
@@ -143,6 +144,21 @@ ResolveUnacceptedMemory (
   ASSERT_EFI_ERROR (Status);
 }
 
+STATIC
+EFI_STATUS
+EFIAPI
+AllowUnacceptedMemory (
+  IN  OVMF_SEV_MEMORY_ACCEPTANCE_PROTOCOL  *This
+  )
+{
+  mAcceptAllMemoryAtEBS = FALSE;
+  return EFI_SUCCESS;
+}
+
+STATIC
+OVMF_SEV_MEMORY_ACCEPTANCE_PROTOCOL
+  mMemoryAcceptanceProtocol = { AllowUnacceptedMemory };
+
 STATIC EDKII_MEMORY_ACCEPT_PROTOCOL  mMemoryAcceptProtocol = {
   AmdSevMemoryAccept
 };
@@ -292,6 +308,16 @@ AmdSevDxeEntryPoint (
       DEBUG ((DEBUG_ERROR, "AllowUnacceptedMemory event creation for EventBeforeExitBootServices failed.\n"));
     }
 
+    Status = gBS->InstallProtocolInterface (
+                    &mAmdSevDxeHandle,
+                    &gOvmfSevMemoryAcceptanceProtocolGuid,
+                    EFI_NATIVE_INTERFACE,
+                    &mMemoryAcceptanceProtocol
+                    );
+    if (EFI_ERROR (Status)) {
+      DEBUG ((DEBUG_ERROR, "Install OvmfSevMemoryAcceptanceProtocol failed.\n"));
+    }
+
     //
     // 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.
diff --git a/OvmfPkg/AmdSevDxe/AmdSevDxe.inf b/OvmfPkg/AmdSevDxe/AmdSevDxe.inf
index 5b443d45bc..e7c7d526c9 100644
--- a/OvmfPkg/AmdSevDxe/AmdSevDxe.inf
+++ b/OvmfPkg/AmdSevDxe/AmdSevDxe.inf
@@ -49,6 +49,7 @@
 
 [Protocols]
   gEdkiiMemoryAcceptProtocolGuid
+  gOvmfSevMemoryAcceptanceProtocolGuid
 
 [Guids]
   gConfidentialComputingSevSnpBlobGuid
-- 
2.39.1.456.gfc5497dd1b-goog

[PATCH v11 4/4] OvmfPkg/PlatformPei: SEV-SNP make >=4GB unaccepted

[Dionna Glaze]

Instead of eagerly accepting all memory in PEI, only accept memory under
the 4GB address. This allows a loaded image to use the
MEMORY_ACCEPTANCE_PROTOCOL to disable the accept behavior and indicate
that it can interpret the memory type accordingly.

This classification is safe since ExitBootServices will accept and
reclassify the memory as conventional if the disable protocol is not
used.

Cc: Ard Biescheuvel <ardb@kernel.org>
Cc: "Min M. Xu" <min.m.xu@intel.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: James Bottomley <jejb@linux.ibm.com>
Cc: Tom Lendacky <Thomas.Lendacky@amd.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Erdem Aktas <erdemaktas@google.com>

Signed-off-by: Dionna Glaze <dionnaglaze@google.com>
---
 OvmfPkg/PlatformPei/AmdSev.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/OvmfPkg/PlatformPei/AmdSev.c b/OvmfPkg/PlatformPei/AmdSev.c
index e4e7b72e67..7d824cc282 100644
--- a/OvmfPkg/PlatformPei/AmdSev.c
+++ b/OvmfPkg/PlatformPei/AmdSev.c
@@ -16,6 +16,7 @@
 #include <Library/MemEncryptSevLib.h>
 #include <Library/MemoryAllocationLib.h>
 #include <Library/PcdLib.h>
+#include <Pi/PrePiHob.h>
 #include <PiPei.h>
 #include <Register/Amd/Msr.h>
 #include <Register/Intel/SmramSaveStateMap.h>
@@ -63,6 +64,10 @@ AmdSevSnpInitialize (
   for (Hob.Raw = GetHobList (); !END_OF_HOB_LIST (Hob); Hob.Raw = GET_NEXT_HOB (Hob)) {
     if ((Hob.Raw != NULL) && (GET_HOB_TYPE (Hob) == EFI_HOB_TYPE_RESOURCE_DESCRIPTOR)) {
       ResourceHob = Hob.ResourceDescriptor;
+      if (ResourceHob->PhysicalStart >= SIZE_4GB) {
+        ResourceHob->ResourceType = BZ3937_EFI_RESOURCE_MEMORY_UNACCEPTED;
+        continue;
+      }
 
       if (ResourceHob->ResourceType == EFI_RESOURCE_SYSTEM_MEMORY) {
         MemEncryptSevSnpPreValidateSystemRam (
-- 
2.39.1.456.gfc5497dd1b-goog

Re: [PATCH v11 4/4] OvmfPkg/PlatformPei: SEV-SNP make >=4GB unaccepted

[Ard Biesheuvel]

On Thu, 26 Jan 2023 at 22:17, Dionna Glaze <dionnaglaze@google.com> wrote:
>
> Instead of eagerly accepting all memory in PEI, only accept memory under
> the 4GB address. This allows a loaded image to use the
> MEMORY_ACCEPTANCE_PROTOCOL to disable the accept behavior and indicate
> that it can interpret the memory type accordingly.
>
> This classification is safe since ExitBootServices will accept and
> reclassify the memory as conventional if the disable protocol is not
> used.
>
> Cc: Ard Biescheuvel <ardb@kernel.org>
> Cc: "Min M. Xu" <min.m.xu@intel.com>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Cc: James Bottomley <jejb@linux.ibm.com>
> Cc: Tom Lendacky <Thomas.Lendacky@amd.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Erdem Aktas <erdemaktas@google.com>
>
> Signed-off-by: Dionna Glaze <dionnaglaze@google.com>
> ---
>  OvmfPkg/PlatformPei/AmdSev.c | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/OvmfPkg/PlatformPei/AmdSev.c b/OvmfPkg/PlatformPei/AmdSev.c
> index e4e7b72e67..7d824cc282 100644
> --- a/OvmfPkg/PlatformPei/AmdSev.c
> +++ b/OvmfPkg/PlatformPei/AmdSev.c
> @@ -16,6 +16,7 @@
>  #include <Library/MemEncryptSevLib.h>
>  #include <Library/MemoryAllocationLib.h>
>  #include <Library/PcdLib.h>
> +#include <Pi/PrePiHob.h>
>  #include <PiPei.h>
>  #include <Register/Amd/Msr.h>
>  #include <Register/Intel/SmramSaveStateMap.h>
> @@ -63,6 +64,10 @@ AmdSevSnpInitialize (
>    for (Hob.Raw = GetHobList (); !END_OF_HOB_LIST (Hob); Hob.Raw = GET_NEXT_HOB (Hob)) {
>      if ((Hob.Raw != NULL) && (GET_HOB_TYPE (Hob) == EFI_HOB_TYPE_RESOURCE_DESCRIPTOR)) {
>        ResourceHob = Hob.ResourceDescriptor;
> +      if (ResourceHob->PhysicalStart >= SIZE_4GB) {

Shouldn't this check be inside the if () below? Or are all resources
that start at or above 4 GiB guaranteed to be system memory?

No need to resend - if needed, I can fix that up when applying.


> +        ResourceHob->ResourceType = BZ3937_EFI_RESOURCE_MEMORY_UNACCEPTED;
> +        continue;
> +      }
>
>        if (ResourceHob->ResourceType == EFI_RESOURCE_SYSTEM_MEMORY) {
>          MemEncryptSevSnpPreValidateSystemRam (
> --
> 2.39.1.456.gfc5497dd1b-goog
>

Re: [PATCH v11 4/4] OvmfPkg/PlatformPei: SEV-SNP make >=4GB unaccepted

[Dionna Glaze]

> Shouldn't this check be inside the if () below? Or are all resources
> that start at or above 4 GiB guaranteed to be system memory?
>
> No need to resend - if needed, I can fix that up when applying.
>

Ah, yes that sounds right.



-- 
-Dionna Glaze, PhD (she/her)

Re: [PATCH v11 0/4] Add safe unaccepted memory behavior

[Ard Biesheuvel]

On Thu, 26 Jan 2023 at 22:17, Dionna Glaze <dionnaglaze@google.com> wrote:
>
> We make eager memory acceptance the default behavior at
> ExitBootServices for SEV-SNP machines by using the standard-enforced
> behavior that if the call returns an error code, then the map key is
> incorrect and the caller must re-call GetMemoryMap to ensure the
> contents are correct.
>
> Eager memory acceptance is implemented by using the UEFI v2.9-added
> EFI_EVENT_GROUP_BEFORE_EXIT_BOOT_SERVICES to check a support condition
> before changing all unaccepted memory type regions to conventional
> memory after first using the MemoryAccept protocol to accept all memory
> in each region. This update to the memory map only happens once, since
> there are no extra unaccepted memory regions to change on the forced
> second call to ExitBootServices.
>
> The new acceptance logic is required only for SEV-SNP since it is the
> only memory-accepting virtualization technology with kernel support live
> without unaccepted memory support.
>
> To allow the OS loader to prevent the eager acceptance, and thus pass
> the before-mentioned "support condition", we add a new protocol,
> OvmfSevMemoryAcceptance.  This protocol has one interface,
> AllowUnacceptedMemory(). The OS loader can inform the UEFI that it
> supports the unaccepted memory type and accepts the responsibility to
> accept it.
>
> The OvmfSevMemoryAcceptance protocol is necessary for safe rollout of
> the unaccepted memory type in SEV-SNP-enabled kernels, given the
> gradual update of guest OS kernels.
>
> All images that support unaccepted memory must now locate and call this
> new OVMF_SEV_MEMORY_ACCEPTANCE_PROTOCOL and call the
> AllowUnacceptedMemory function.
>
> Changes since v10:
>  - AmdSevDxe called AcceptMemory directly without locating the
>    MemoryAccept protocol.
>  - The protocol is no longer a candidate for standardization and has
>    moved to OvmfPkg/Include/Protocol.
> Changes since v9:
>  - Renamed protocol to SevMemoryAcceptance.
>  - Removed CocoDxe and moved all contained code to AmdSevDxe.
>  - Renamed protocol header file to reference the bugzilla number.
> Changes since v8:
>  - First 3 patches removed since they were submitted separately.
>  - Later patches rebased on edk2/master and modified to work with the
>    current locations and namings of the unaccepted memory constants.
> Changes since v7:
>  - Rebased onto lazy accept v4 patch series, so memory accept protocol
>    has the EDKII prefix, and the unaccepted memory type has the BZ3937
>    prefix.
>  - Removed a bad #include to a header removed in v7.
>  - Renamed the protocol to BZ3987_MEMORY_ACCEPTANCE_PROTOCOL as per the
>    discussion on the buganizer issue.
>  - Uncrustify formatting
>
> Changes since v6:
>  - Added implementation of EFI_EVENT_GROUP_BEFORE_EXIT_BOOT_SERVICES.
>  - Changed callback protocol of v5 to instead use the standardized event
>    group for before_exit_boot_services.
>
> Changes since v5:
>  - Generic callback protocol moved to MdeModulePkg
>  - Removed use of EFI_WARN_STALE_DATA and added comment that the callback
>    should only return EFI_SUCCESS or EFI_INVALID_PARAMETER.
>  - Removed errant log statement and fixed formatting.
>
> Changes since v4:
>  - Commit message wording
>  - Replaced direct change to DxeMain with a more generic callback
>    protocol.
>  - Implemented the direct change as an instance of the callback protocol
>    from a new CocoDxe driver.
>  - Replaced "enable" protocol with a "disable" protocol, since the name
>    was confusing. The AcceptAllUnacceptedMemory protocol directly names
>    the behavior that is disabling.
>
> Changes since v3:
>  - "DxeMain accepts all memory" patch split into 3 to make each patch
>    affect only one package at a time.
>
> Changes since v2:
>  - Removed the redundant memory accept interface and added the accept
>    behavior to the DXE implementation of
>    MemEncryptSevSnpPreValidateSystemRam.
>  - Fixed missing #include in >=4GB patch.
>
> Changes since v1:
>  - Added a patch to classify SEV-SNP memory above 4GB unaccepted.
>  - Fixed style problems in EfiMemoryAcceptProtocol implementation.
>
> Cc: Ard Biescheuvel <ardb@kernel.org>
> Cc: "Min M. Xu" <min.m.xu@intel.com>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Cc: James Bottomley <jejb@linux.ibm.com>
> Cc: Tom Lendacky <Thomas.Lendacky@amd.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Erdem Aktas <erdemaktas@google.com>
> Cc: Andrew Fish <afish@apple.com>
> Cc: "Michael D. Kinney" <michael.d.kinney@intel.com>
>
> Signed-off-by: Dionna Glaze <dionnaglaze@google.com>
>
> Dionna Glaze (4):
>   OvmfPkg: Add memory acceptance event in AmdSevDxe
>   MdePkg: Introduce the SevMemoryAcceptance protocol
>   OvmfPkg: Implement AcceptAllUnacceptedMemory in AmdSevDxe
>   OvmfPkg/PlatformPei: SEV-SNP make >=4GB unaccepted
>

For the series,

Reviewed-by: Ard Biesheuvel <ardb@kernel.org>

Thanks a lot for your persistence.

Queued as #3954

>  OvmfPkg/AmdSevDxe/AmdSevDxe.c                  | 123 ++++++++++++++++++++
>  OvmfPkg/AmdSevDxe/AmdSevDxe.inf                |   2 +
>  OvmfPkg/Include/Protocol/SevMemoryAcceptance.h |  42 +++++++
>  OvmfPkg/OvmfPkg.dec                            |   1 +
>  OvmfPkg/PlatformPei/AmdSev.c                   |   5 +
>  5 files changed, 173 insertions(+)
>  create mode 100644 OvmfPkg/Include/Protocol/SevMemoryAcceptance.h
>
> --
> 2.39.1.456.gfc5497dd1b-goog
>

Re: [PATCH v11 2/4] MdePkg: Introduce the SevMemoryAcceptance protocol

[Lendacky, Thomas]

Might want to fix up the commit title from MdePkg to OvmfPkg before 
committing.

Thanks,
Tom

On 1/26/23 15:17, Dionna Glaze wrote:
> The default behavior for unaccepted memory in SEV-SNP is to accept all
> memory when ExitBootServices is called. An OS loader can use this
> protocol to disable this behavior to assume responsibility for memory
> acceptance and to affirm that the OS can handle the unaccepted memory
> type.
> 
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Cc: James Bottomley <jejb@linux.ibm.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Tom Lendacky <thomas.lendacky@amd.com>
> Cc: Ard Biesheuvel <ardb@kernel.org>
> Cc: "Min M. Xu" <min.m.xu@intel.com>
> Cc: Andrew Fish <afish@apple.com>
> Cc: "Michael D. Kinney" <michael.d.kinney@intel.com>
> 
> Signed-off-by: Dionna Glaze <dionnaglaze@google.com>
> ---
>   OvmfPkg/Include/Protocol/SevMemoryAcceptance.h | 42 ++++++++++++++++++++
>   OvmfPkg/OvmfPkg.dec                            |  1 +
>   2 files changed, 43 insertions(+)
> 
> diff --git a/OvmfPkg/Include/Protocol/SevMemoryAcceptance.h b/OvmfPkg/Include/Protocol/SevMemoryAcceptance.h
> new file mode 100644
> index 0000000000..c45b499006
> --- /dev/null
> +++ b/OvmfPkg/Include/Protocol/SevMemoryAcceptance.h
> @@ -0,0 +1,42 @@
> +/** @file
> +  The file provides the protocol that disables the behavior that all memory
> +  gets accepted at ExitBootServices(). This protocol is only meant to be called
> +  by the OS loader, and not EDK2 itself. The SEV naming is due to the coincidence
> +  that only SEV-SNP needs this protocol, since SEV-SNP kernel support released
> +  before kernel support for unaccepted memory. The technology enablement thus
> +  does not strictly imply support for the unaccepted memory type.
> +
> +  Copyright (c) 2023, Google LLC. All rights reserved.<BR>
> +  SPDX-License-Identifier: BSD-2-Clause-Patent
> +**/
> +
> +#ifndef SEV_MEMORY_ACCEPTANCE_H_
> +#define SEV_MEMORY_ACCEPTANCE_H_
> +
> +#define OVMF_SEV_MEMORY_ACCEPTANCE_PROTOCOL_GUID \
> +  {0xc5a010fe, \
> +   0x38a7, \
> +   0x4531, \
> +   {0x8a, 0x4a, 0x05, 0x00, 0xd2, 0xfd, 0x16, 0x49}}
> +
> +typedef struct _OVMF_SEV_MEMORY_ACCEPTANCE_PROTOCOL
> +    OVMF_SEV_MEMORY_ACCEPTANCE_PROTOCOL;
> +
> +/**
> +  @param This A pointer to a OVMF_SEV_MEMORY_ACCEPTANCE_PROTOCOL.
> +**/
> +typedef
> +  EFI_STATUS
> +(EFIAPI *OVMF_SEV_ALLOW_UNACCEPTED_MEMORY)(
> +  IN  OVMF_SEV_MEMORY_ACCEPTANCE_PROTOCOL  *This
> +  );
> +
> +///
> +/// The OVMF_SEV_MEMORY_ACCEPTANCE_PROTOCOL allows the OS loader to
> +/// indicate to EDK2 that ExitBootServices should not accept all memory.
> +///
> +struct _OVMF_SEV_MEMORY_ACCEPTANCE_PROTOCOL {
> +  OVMF_SEV_ALLOW_UNACCEPTED_MEMORY    AllowUnacceptedMemory;
> +};
> +
> +#endif
> diff --git a/OvmfPkg/OvmfPkg.dec b/OvmfPkg/OvmfPkg.dec
> index 1b521f2604..a22eb246c6 100644
> --- a/OvmfPkg/OvmfPkg.dec
> +++ b/OvmfPkg/OvmfPkg.dec
> @@ -184,6 +184,7 @@
>     gEfiLegacyInterruptProtocolGuid       = {0x31ce593d, 0x108a, 0x485d, {0xad, 0xb2, 0x78, 0xf2, 0x1f, 0x29, 0x66, 0xbe}}
>     gEfiVgaMiniPortProtocolGuid           = {0xc7735a2f, 0x88f5, 0x4882, {0xae, 0x63, 0xfa, 0xac, 0x8c, 0x8b, 0x86, 0xb3}}
>     gOvmfLoadedX86LinuxKernelProtocolGuid = {0xa3edc05d, 0xb618, 0x4ff6, {0x95, 0x52, 0x76, 0xd7, 0x88, 0x63, 0x43, 0xc8}}
> +  gOvmfSevMemoryAcceptanceProtocolGuid  = {0xc5a010fe, 0x38a7, 0x4531, {0x8a, 0x4a, 0x05, 0x00, 0xd2, 0xfd, 0x16, 0x49}}
>     gQemuAcpiTableNotifyProtocolGuid      = {0x928939b2, 0x4235, 0x462f, {0x95, 0x80, 0xf6, 0xa2, 0xb2, 0xc2, 0x1a, 0x4f}}
>     gEfiMpInitLibMpDepProtocolGuid        = {0xbb00a5ca, 0x8ce,  0x462f, {0xa5, 0x37, 0x43, 0xc7, 0x4a, 0x82, 0x5c, 0xa4}}
>     gEfiMpInitLibUpDepProtocolGuid        = {0xa9e7cef1, 0x5682, 0x42cc, {0xb1, 0x23, 0x99, 0x30, 0x97, 0x3f, 0x4a, 0x9f}}

Re: [PATCH v11 2/4] MdePkg: Introduce the SevMemoryAcceptance protocol

[Ard Biesheuvel]

On Fri, 27 Jan 2023 at 00:11, Tom Lendacky <thomas.lendacky@amd.com> wrote:
>
> Might want to fix up the commit title from MdePkg to OvmfPkg before
> committing.
>

I've fixed that up - thanks.


> On 1/26/23 15:17, Dionna Glaze wrote:
> > The default behavior for unaccepted memory in SEV-SNP is to accept all
> > memory when ExitBootServices is called. An OS loader can use this
> > protocol to disable this behavior to assume responsibility for memory
> > acceptance and to affirm that the OS can handle the unaccepted memory
> > type.
> >
> > Cc: Gerd Hoffmann <kraxel@redhat.com>
> > Cc: James Bottomley <jejb@linux.ibm.com>
> > Cc: Jiewen Yao <jiewen.yao@intel.com>
> > Cc: Tom Lendacky <thomas.lendacky@amd.com>
> > Cc: Ard Biesheuvel <ardb@kernel.org>
> > Cc: "Min M. Xu" <min.m.xu@intel.com>
> > Cc: Andrew Fish <afish@apple.com>
> > Cc: "Michael D. Kinney" <michael.d.kinney@intel.com>
> >
> > Signed-off-by: Dionna Glaze <dionnaglaze@google.com>
> > ---
> >   OvmfPkg/Include/Protocol/SevMemoryAcceptance.h | 42 ++++++++++++++++++++
> >   OvmfPkg/OvmfPkg.dec                            |  1 +
> >   2 files changed, 43 insertions(+)
> >
> > diff --git a/OvmfPkg/Include/Protocol/SevMemoryAcceptance.h b/OvmfPkg/Include/Protocol/SevMemoryAcceptance.h
> > new file mode 100644
> > index 0000000000..c45b499006
> > --- /dev/null
> > +++ b/OvmfPkg/Include/Protocol/SevMemoryAcceptance.h
> > @@ -0,0 +1,42 @@
> > +/** @file
> > +  The file provides the protocol that disables the behavior that all memory
> > +  gets accepted at ExitBootServices(). This protocol is only meant to be called
> > +  by the OS loader, and not EDK2 itself. The SEV naming is due to the coincidence
> > +  that only SEV-SNP needs this protocol, since SEV-SNP kernel support released
> > +  before kernel support for unaccepted memory. The technology enablement thus
> > +  does not strictly imply support for the unaccepted memory type.
> > +
> > +  Copyright (c) 2023, Google LLC. All rights reserved.<BR>
> > +  SPDX-License-Identifier: BSD-2-Clause-Patent
> > +**/
> > +
> > +#ifndef SEV_MEMORY_ACCEPTANCE_H_
> > +#define SEV_MEMORY_ACCEPTANCE_H_
> > +
> > +#define OVMF_SEV_MEMORY_ACCEPTANCE_PROTOCOL_GUID \
> > +  {0xc5a010fe, \
> > +   0x38a7, \
> > +   0x4531, \
> > +   {0x8a, 0x4a, 0x05, 0x00, 0xd2, 0xfd, 0x16, 0x49}}
> > +
> > +typedef struct _OVMF_SEV_MEMORY_ACCEPTANCE_PROTOCOL
> > +    OVMF_SEV_MEMORY_ACCEPTANCE_PROTOCOL;
> > +
> > +/**
> > +  @param This A pointer to a OVMF_SEV_MEMORY_ACCEPTANCE_PROTOCOL.
> > +**/
> > +typedef
> > +  EFI_STATUS
> > +(EFIAPI *OVMF_SEV_ALLOW_UNACCEPTED_MEMORY)(
> > +  IN  OVMF_SEV_MEMORY_ACCEPTANCE_PROTOCOL  *This
> > +  );
> > +
> > +///
> > +/// The OVMF_SEV_MEMORY_ACCEPTANCE_PROTOCOL allows the OS loader to
> > +/// indicate to EDK2 that ExitBootServices should not accept all memory.
> > +///
> > +struct _OVMF_SEV_MEMORY_ACCEPTANCE_PROTOCOL {
> > +  OVMF_SEV_ALLOW_UNACCEPTED_MEMORY    AllowUnacceptedMemory;
> > +};
> > +
> > +#endif
> > diff --git a/OvmfPkg/OvmfPkg.dec b/OvmfPkg/OvmfPkg.dec
> > index 1b521f2604..a22eb246c6 100644
> > --- a/OvmfPkg/OvmfPkg.dec
> > +++ b/OvmfPkg/OvmfPkg.dec
> > @@ -184,6 +184,7 @@
> >     gEfiLegacyInterruptProtocolGuid       = {0x31ce593d, 0x108a, 0x485d, {0xad, 0xb2, 0x78, 0xf2, 0x1f, 0x29, 0x66, 0xbe}}
> >     gEfiVgaMiniPortProtocolGuid           = {0xc7735a2f, 0x88f5, 0x4882, {0xae, 0x63, 0xfa, 0xac, 0x8c, 0x8b, 0x86, 0xb3}}
> >     gOvmfLoadedX86LinuxKernelProtocolGuid = {0xa3edc05d, 0xb618, 0x4ff6, {0x95, 0x52, 0x76, 0xd7, 0x88, 0x63, 0x43, 0xc8}}
> > +  gOvmfSevMemoryAcceptanceProtocolGuid  = {0xc5a010fe, 0x38a7, 0x4531, {0x8a, 0x4a, 0x05, 0x00, 0xd2, 0xfd, 0x16, 0x49}}
> >     gQemuAcpiTableNotifyProtocolGuid      = {0x928939b2, 0x4235, 0x462f, {0x95, 0x80, 0xf6, 0xa2, 0xb2, 0xc2, 0x1a, 0x4f}}
> >     gEfiMpInitLibMpDepProtocolGuid        = {0xbb00a5ca, 0x8ce,  0x462f, {0xa5, 0x37, 0x43, 0xc7, 0x4a, 0x82, 0x5c, 0xa4}}
> >     gEfiMpInitLibUpDepProtocolGuid        = {0xa9e7cef1, 0x5682, 0x42cc, {0xb1, 0x23, 0x99, 0x30, 0x97, 0x3f, 0x4a, 0x9f}}