[edk2-devel] [PATCH wave 1 08/10] OvmfPkg/SEV: don't manage the lifecycle of the SMRAM at the default SMBASE

[Laszlo Ersek]

When OVMF runs in a SEV guest, the initial SMM Save State Map is

(1) allocated as EfiBootServicesData type memory in OvmfPkg/PlatformPei,
    function AmdSevInitialize(), for preventing unintended information
    sharing with the hypervisor;

(2) decrypted in AmdSevDxe;

(3) re-encrypted in OvmfPkg/Library/SmmCpuFeaturesLib, function
    SmmCpuFeaturesSmmRelocationComplete(), which is called by
    PiSmmCpuDxeSmm right after initial SMBASE relocation;

(4) released to DXE at the same location.

The SMRAM at the default SMBASE is a superset of the initial Save State
Map. The reserved memory allocation in InitializeRamRegions(), from the
previous patch, must override the allocating and freeing in (1) and (4),
respectively. (Note: the decrypting and re-encrypting in (2) and (3) are
unaffected.)

In AmdSevInitialize(), only assert the containment of the initial Save
State Map, in the larger area already allocated by InitializeRamRegions().

In SmmCpuFeaturesSmmRelocationComplete(), preserve the allocation of the
initial Save State Map into OS runtime, as part of the allocation done by
InitializeRamRegions(). Only assert containment.

These changes only affect the normal boot path (the UEFI memory map is
untouched during S3 resume).

Cc: Ard Biesheuvel <ard.biesheuvel at linaro.org>
Cc: Boris Ostrovsky <boris.ostrovsky at oracle.com>
Cc: Brijesh Singh <brijesh.singh at amd.com>
Cc: Igor Mammedov <imammedo at redhat.com>
Cc: Jiewen Yao <jiewen.yao at intel.com>
Cc: Joao M Martins <joao.m.martins at oracle.com>
Cc: Jordan Justen <jordan.l.justen at intel.com>
Cc: Jun Nakajima <jun.nakajima at intel.com>
Cc: Michael Kinney <michael.d.kinney at intel.com>
Cc: Paolo Bonzini <pbonzini at redhat.com>
Cc: Phillip Goerl <phillip.goerl at oracle.com>
Cc: Yingwen Chen <yingwen.chen at intel.com>
Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=1512
Signed-off-by: Laszlo Ersek <lersek at redhat.com>
---
 OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf |  4 ++++
 OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c   | 21 +++++++++++++++--
 OvmfPkg/PlatformPei/AmdSev.c                            | 24 ++++++++++++++++----
 3 files changed, 42 insertions(+), 7 deletions(-)

diff --git a/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf b/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf
index dd316f2b1bd8..97a10afb6e27 100644
--- a/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf
+++ b/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.inf
@@ -30,5 +30,9 @@ [LibraryClasses]
   BaseMemoryLib
   DebugLib
   MemEncryptSevLib
+  PcdLib
   SmmServicesTableLib
   UefiBootServicesTableLib
+
+[Pcd]
+  gUefiOvmfPkgTokenSpaceGuid.PcdQ35SmramAtDefaultSmbase
diff --git a/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c b/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c
index 0bfdeda78d33..7ef7ed98342e 100644
--- a/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c
+++ b/OvmfPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c
@@ -6,14 +6,17 @@
   SPDX-License-Identifier: BSD-2-Clause-Patent
 **/
 
+#include <IndustryStandard/Q35MchIch9.h>
 #include <Library/BaseLib.h>
 #include <Library/BaseMemoryLib.h>
 #include <Library/DebugLib.h>
 #include <Library/MemEncryptSevLib.h>
+#include <Library/PcdLib.h>
 #include <Library/SmmCpuFeaturesLib.h>
 #include <Library/SmmServicesTableLib.h>
 #include <Library/UefiBootServicesTableLib.h>
 #include <PiSmm.h>
+#include <Register/Intel/SmramSaveStateMap.h>
 #include <Register/QemuSmramSaveStateMap.h>
 
 //
@@ -215,8 +218,22 @@ SmmCpuFeaturesSmmRelocationComplete (
 
   ZeroMem ((VOID *)MapPagesBase, EFI_PAGES_TO_SIZE (MapPagesCount));
 
-  Status = gBS->FreePages (MapPagesBase, MapPagesCount);
-  ASSERT_EFI_ERROR (Status);
+  if (PcdGetBool (PcdQ35SmramAtDefaultSmbase)) {
+    //
+    // The initial SMRAM Save State Map has been covered as part of a larger
+    // reserved memory allocation in PlatformPei's InitializeRamRegions(). That
+    // allocation is supposed to survive into OS runtime; we must not release
+    // any part of it. Only re-assert the containment here.
+    //
+    ASSERT (SMM_DEFAULT_SMBASE <= MapPagesBase);
+    ASSERT (
+      (MapPagesBase + EFI_PAGES_TO_SIZE (MapPagesCount) <=
+       SMM_DEFAULT_SMBASE + MCH_DEFAULT_SMBASE_SIZE)
+      );
+  } else {
+    Status = gBS->FreePages (MapPagesBase, MapPagesCount);
+    ASSERT_EFI_ERROR (Status);
+  }
 }
 
 /**
diff --git a/OvmfPkg/PlatformPei/AmdSev.c b/OvmfPkg/PlatformPei/AmdSev.c
index 2ae8126ccf8a..e484f4b311fe 100644
--- a/OvmfPkg/PlatformPei/AmdSev.c
+++ b/OvmfPkg/PlatformPei/AmdSev.c
@@ -9,6 +9,7 @@
 //
 // The package level header files this module uses
 //
+#include <IndustryStandard/Q35MchIch9.h>
 #include <Library/DebugLib.h>
 #include <Library/HobLib.h>
 #include <Library/MemEncryptSevLib.h>
@@ -16,6 +17,7 @@
 #include <PiPei.h>
 #include <Register/Amd/Cpuid.h>
 #include <Register/Cpuid.h>
+#include <Register/Intel/SmramSaveStateMap.h>
 
 #include "Platform.h"
 
@@ -83,10 +85,22 @@ AmdSevInitialize (
                         );
     ASSERT_RETURN_ERROR (LocateMapStatus);
 
-    BuildMemoryAllocationHob (
-      MapPagesBase,                      // BaseAddress
-      EFI_PAGES_TO_SIZE (MapPagesCount), // Length
-      EfiBootServicesData                // MemoryType
-      );
+    if (mQ35SmramAtDefaultSmbase) {
+      //
+      // The initial SMRAM Save State Map has been covered as part of a larger
+      // reserved memory allocation in InitializeRamRegions().
+      //
+      ASSERT (SMM_DEFAULT_SMBASE <= MapPagesBase);
+      ASSERT (
+        (MapPagesBase + EFI_PAGES_TO_SIZE (MapPagesCount) <=
+         SMM_DEFAULT_SMBASE + MCH_DEFAULT_SMBASE_SIZE)
+        );
+    } else {
+      BuildMemoryAllocationHob (
+        MapPagesBase,                      // BaseAddress
+        EFI_PAGES_TO_SIZE (MapPagesCount), // Length
+        EfiBootServicesData                // MemoryType
+        );
+    }
   }
 }
-- 
2.19.1.3.g30247aa5d201



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#47932): https://edk2.groups.io/g/devel/message/47932
Mute This Topic: https://groups.io/mt/34274943/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-