Re: [PATCH] OvmfPkg/MemEncryptSevLib: check CPUID when read msr during PEI phase


qi zhou
 

Here I will add some debug infomation about the reboot issue. on the master branch, I added some logs, see below:

https://1drv.ms/u/s!As-Ec5SPH0fuimANnT5FPm8cmeM6
---
.../Library/BaseMemEncryptSevLib/PeiMemEncryptSevLibInternal.c | 3 +++
1 file changed, 3 insertions(+)

diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/PeiMemEncryptSevLibInternal.c b/OvmfPkg/Library/BaseMemEncryptSevLib/PeiMemEncryptSevLibInternal.c
index e2fd109d12..8ab5849386 100644
--- a/OvmfPkg/Library/BaseMemEncryptSevLib/PeiMemEncryptSevLibInternal.c
+++ b/OvmfPkg/Library/BaseMemEncryptSevLib/PeiMemEncryptSevLibInternal.c
@@ -44,6 +44,7 @@ InternalMemEncryptSevStatus (
ReadSevMsr = FALSE;

SevEsWorkArea = (SEC_SEV_ES_WORK_AREA *) FixedPcdGet32 (PcdSevEsWorkAreaBase);
+ DEBUG ((DEBUG_INFO, "SevEsWorkAera: %p, SevEsWorkArea->EncryptionMask: %lu\n", SevEsWorkArea, SevEsWorkArea ? SevEsWorkArea->EncryptionMask : 0));
if (SevEsWorkArea != NULL && SevEsWorkArea->EncryptionMask != 0) {
//
// The MSR has been read before, so it is safe to read it again and avoid
@@ -71,7 +72,9 @@ InternalMemEncryptSevStatus (
//
// Check MSR_0xC0010131 Bit 0 (Sev Enabled)
//
+ DEBUG ((DEBUG_INFO, "Begin read msr\n"));
Msr.Uint32 = AsmReadMsr32 (MSR_SEV_STATUS);
+ DEBUG ((DEBUG_INFO, "End read msr\n"));
if (Msr.Bits.SevBit) {
mSevStatus = TRUE;
}
--
2.25.1

Then rebuild the ovmf package, launch qemu with windows 10 guest os installed, repeat reboot win10 guest os 4-10 times, then there may got reboot fail
see this screenshot: https://1drv.ms/u/s!As-Ec5SPH0fuil9hsVV_ooZG0mcw?e=xgfJoL

From: qi zhou <atmgnd@outlook.com>
Sent: Thursday, November 25, 2021 21:12
To: devel@edk2.groups.io <devel@edk2.groups.io>
Cc: brijesh.singh@amd.com <brijesh.singh@amd.com>; erdemaktas@google.com <erdemaktas@google.com>; jejb@linux.ibm.com <jejb@linux.ibm.com>; jiewen.yao@intel.com <jiewen.yao@intel.com>; min.m.xu@intel.com <min.m.xu@intel.com>; thomas.lendacky@amd.com <thomas.lendacky@amd.com>
Subject: [PATCH] OvmfPkg/MemEncryptSevLib: check CPUID when read msr during PEI phase
 
From 5b10265fa5c7b5ca728b4f18488089de6535ed28 Mon Sep 17 00:00:00 2001
From: Qi Zhou <atmgnd@outlook.com>
Date: Thu, 25 Nov 2021 20:25:55 +0800
Subject: [PATCH] OvmfPkg/MemEncryptSevLib: check CPUID when read msr during
 PEI phase

Tested on Intel Platform, It is like 'SEV-ES work area' can be modified by
os(Windows etc), and will not restored on reboot, the
SevEsWorkArea->EncryptionMask may have a random value after reboot. then it
may casue fail on reboot. The msr bits already cached by mSevStatusChecked,
there is no need to try cache again in PEI phase.

Signed-off-by: Qi Zhou <atmgnd@outlook.com>
---
 .../PeiMemEncryptSevLibInternal.c             | 55 +++++++------------
 1 file changed, 19 insertions(+), 36 deletions(-)

diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/PeiMemEncryptSevLibInternal.c b/OvmfPkg/Library/BaseMemEncryptSevLib/PeiMemEncryptSevLibInternal.c
index e2fd109d12..0819f50669 100644
--- a/OvmfPkg/Library/BaseMemEncryptSevLib/PeiMemEncryptSevLibInternal.c
+++ b/OvmfPkg/Library/BaseMemEncryptSevLib/PeiMemEncryptSevLibInternal.c
@@ -38,49 +38,32 @@ InternalMemEncryptSevStatus (
   UINT32                            RegEax;
   MSR_SEV_STATUS_REGISTER           Msr;
   CPUID_MEMORY_ENCRYPTION_INFO_EAX  Eax;
-  BOOLEAN                           ReadSevMsr;
-  SEC_SEV_ES_WORK_AREA              *SevEsWorkArea;
 
-  ReadSevMsr = FALSE;
-
-  SevEsWorkArea = (SEC_SEV_ES_WORK_AREA *) FixedPcdGet32 (PcdSevEsWorkAreaBase);
-  if (SevEsWorkArea != NULL && SevEsWorkArea->EncryptionMask != 0) {
-    //
-    // The MSR has been read before, so it is safe to read it again and avoid
-    // having to validate the CPUID information.
+  //
+  // Check if memory encryption leaf exist
+  //
+  AsmCpuid (CPUID_EXTENDED_FUNCTION, &RegEax, NULL, NULL, NULL);
+  if (RegEax >= CPUID_MEMORY_ENCRYPTION_INFO) {
     //
-    ReadSevMsr = TRUE;
-  } else {
+    // CPUID Fn8000_001F[EAX] Bit 1 (Sev supported)
     //
-    // Check if memory encryption leaf exist
-    //
-    AsmCpuid (CPUID_EXTENDED_FUNCTION, &RegEax, NULL, NULL, NULL);
-    if (RegEax >= CPUID_MEMORY_ENCRYPTION_INFO) {
+    AsmCpuid (CPUID_MEMORY_ENCRYPTION_INFO, &Eax.Uint32, NULL, NULL, NULL);
+
+    if (Eax.Bits.SevBit) {
       //
-      // CPUID Fn8000_001F[EAX] Bit 1 (Sev supported)
+      // Check MSR_0xC0010131 Bit 0 (Sev Enabled)
       //
-      AsmCpuid (CPUID_MEMORY_ENCRYPTION_INFO, &Eax.Uint32, NULL, NULL, NULL);
-
-      if (Eax.Bits.SevBit) {
-        ReadSevMsr = TRUE;
+      Msr.Uint32 = AsmReadMsr32 (MSR_SEV_STATUS);
+      if (Msr.Bits.SevBit) {
+        mSevStatus = TRUE;
       }
-    }
-  }
-
-  if (ReadSevMsr) {
-    //
-    // Check MSR_0xC0010131 Bit 0 (Sev Enabled)
-    //
-    Msr.Uint32 = AsmReadMsr32 (MSR_SEV_STATUS);
-    if (Msr.Bits.SevBit) {
-      mSevStatus = TRUE;
-    }
 
-    //
-    // Check MSR_0xC0010131 Bit 1 (Sev-Es Enabled)
-    //
-    if (Msr.Bits.SevEsBit) {
-      mSevEsStatus = TRUE;
+      //
+      // Check MSR_0xC0010131 Bit 1 (Sev-Es Enabled)
+      //
+      if (Msr.Bits.SevEsBit) {
+        mSevEsStatus = TRUE;
+      }
     }
   }
 
--
2.25.1

Join devel@edk2.groups.io to automatically receive all group messages.