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@...> Sent: Thursday, November 25, 2021 21:12 To: devel@edk2.groups.io <devel@edk2.groups.io> Cc: brijesh.singh@... <brijesh.singh@...>; erdemaktas@... <erdemaktas@...>; jejb@... <jejb@...>; jiewen.yao@... <jiewen.yao@...>; min.m.xu@... <min.m.xu@...>; thomas.lendacky@... <thomas.lendacky@...> 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@...> 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@...> --- .../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 |
|