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

["Brijesh Singh" <brijesh.singh@amd.com>]

On 11/29/21 1:04 PM, Tom Lendacky wrote:
> On 11/25/21 7:12 AM, qi zhou wrote:
>>  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) {
> 

What is missing in the original patch set is that now with the common 
work area we need to check the Guest Type before accessing the SevEs 
workarea type. I have a patch in my wip to cleanup the SEV feature 
detection check and patiently waiting for the SEV-SNP series to land so 
that I can submit other patches.


You need something like IsSevGuest() before accessing the SevEs 
workarea, see how its done for the SEC.

https://github.com/AMDESE/ovmf/blob/snp-v13/OvmfPkg/Sec/AmdSev.c#L234

In my WIP I am moving that to common BaseMemEncryptLib.

thanks

> This now defeats the purpose of the workarea the already validated CPUID 
> information. This CPUID information will now require validating.
> 
> Wouldn't the best thing be to clear the workarea in the early boot code?
> 
> Thanks,
> Tom
> 
>>       //
>> -    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;
>> +      }
>>       }
>>     }
>>