Re: [edk2-devel] [PATCH v4 23/40] OvmfPkg/MemEncryptSevLib: Add an SEV-ES guest indicator function

["Lendacky, Thomas" <thomas.lendacky@amd.com>]

On 2/6/20 2:21 AM, Laszlo Ersek via Groups.Io wrote:
> Hi Tom,
> 
> On 02/05/20 00:01, Lendacky, Thomas wrote:
>> BZ: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D2198&data=02%7C01%7Cthomas.lendacky%40amd.com%7Ce2df7232720d4a15b38208d7aadd8f51%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637165740882485307&sdata=CIU4UwRsJzE6qjKOkQOaOrwuPUe1EerzpYZJXc4Phvc%3D&reserved=0
>>
>> Create a function that can be used to determine if the VM is running
>> as an SEV-ES guest.
>>
>> Cc: Jordan Justen <jordan.l.justen@intel.com>
>> Cc: Laszlo Ersek <lersek@redhat.com>
>> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
>> Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
>> ---
>>  OvmfPkg/Include/Library/MemEncryptSevLib.h    | 12 +++
>>  .../MemEncryptSevLibInternal.c                | 75 ++++++++++++-------
>>  2 files changed, 60 insertions(+), 27 deletions(-)
>>
>> diff --git a/OvmfPkg/Include/Library/MemEncryptSevLib.h b/OvmfPkg/Include/Library/MemEncryptSevLib.h
>> index 64dd6977b0f8..a50a0de9c870 100644
>> --- a/OvmfPkg/Include/Library/MemEncryptSevLib.h
>> +++ b/OvmfPkg/Include/Library/MemEncryptSevLib.h
>> @@ -13,6 +13,18 @@
>>  
>>  #include <Base.h>
>>  
>> +/**
>> +  Returns a boolean to indicate whether SEV-ES is enabled
>> +
>> +  @retval TRUE           SEV-ES is enabled
>> +  @retval FALSE          SEV-ES is not enabled
>> +**/
>> +BOOLEAN
>> +EFIAPI
>> +MemEncryptSevEsIsEnabled (
>> +  VOID
>> +  );
>> +
>>  /**
>>    Returns a boolean to indicate whether SEV is enabled
>>  
>> diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/MemEncryptSevLibInternal.c b/OvmfPkg/Library/BaseMemEncryptSevLib/MemEncryptSevLibInternal.c
>> index 96a66e373f11..c859bb141963 100644
>> --- a/OvmfPkg/Library/BaseMemEncryptSevLib/MemEncryptSevLibInternal.c
>> +++ b/OvmfPkg/Library/BaseMemEncryptSevLib/MemEncryptSevLibInternal.c
>> @@ -20,19 +20,17 @@
>>  #include <Uefi/UefiBaseType.h>
>>  
>>  STATIC BOOLEAN mSevStatus = FALSE;
>> +STATIC BOOLEAN mSevEsStatus = FALSE;
>>  STATIC BOOLEAN mSevStatusChecked = FALSE;
>>  
>>  /**
>>  
>> -  Returns a boolean to indicate whether SEV is enabled
>> -
>> -  @retval TRUE           SEV is enabled
>> -  @retval FALSE          SEV is not enabled
>> +  Reads and sets the status of SEV features
>>    **/
>>  STATIC
>> -BOOLEAN
>> +VOID
>>  EFIAPI
>> -InternalMemEncryptSevIsEnabled (
>> +InternalMemEncryptSevStatus (
>>    VOID
>>    )
>>  {
>> @@ -56,32 +54,55 @@ InternalMemEncryptSevIsEnabled (
>>        //
>>        Msr.Uint32 = AsmReadMsr32 (MSR_SEV_STATUS);
>>        if (Msr.Bits.SevBit) {
>> -        return TRUE;
>> +        mSevStatus = TRUE;
>> +      }
>> +
>> +      //
>> +      // Check MSR_0xC0010131 Bit 1 (Sev-Es Enabled)
>> +      //
>> +      if (Msr.Bits.SevEsBit) {
> 
> In the previous version this was also gated by a check on
> "Eax.Bits.SevEsBit". What's the reason for removing that check?
> 
> Is it simply superfluous to rely on that output of the CPUID because the
> MSR tells us anyway? IOW,
> 
> - if "Eax.Bits.SevEsBit" is clear, then "Msr.Bits.SevEsBit" will always
> be clear (i.e. "no support" implies "not enabled"),
> 
> - if "Msr.Bits.SevEsBit" is set, then "Eax.Bits.SevEsBit" is always set
> (i.e. "enabled" implies "supported")?

Correct, it's not needed. The only reason for the check is to verify that
the proper level of support is present so that the (non-interceptable)
RDMSR of MSR 0xC0010131 can be safely issued. The MSR value indicates
the SEV-ES support regardless of what is set in the CPUID function, so
they are not tied together in that way. The MSR value is based on, among
other things, the SEV/SEV-ES setting in the VMCB used for the VMRUN.

The GHCB spec calls for the SEV bit to be set as an indication that there
is SEV support before issuing the RDMSR (in case that CPUID function is
used in the future by others for some reason).

Thanks,
Tom

> 
> Thanks
> Laszlo
> 
>> +        mSevEsStatus = TRUE;
>>        }
>>      }
>>    }
>>  
>> -  return FALSE;
>> -}
>> -
>> -/**
>> -  Returns a boolean to indicate whether SEV is enabled
>> -
>> -  @retval TRUE           SEV is enabled
>> -  @retval FALSE          SEV is not enabled
>> -**/
>> -BOOLEAN
>> -EFIAPI
>> -MemEncryptSevIsEnabled (
>> -  VOID
>> -  )
>> -{
>> -  if (mSevStatusChecked) {
>> -    return mSevStatus;
>> -  }
>> -
>> -  mSevStatus = InternalMemEncryptSevIsEnabled();
>>    mSevStatusChecked = TRUE;
>> +}
>> +
>> +/**
>> +  Returns a boolean to indicate whether SEV-ES is enabled
>> +
>> +  @retval TRUE           SEV-ES is enabled
>> +  @retval FALSE          SEV-ES is not enabled
>> +**/
>> +BOOLEAN
>> +EFIAPI
>> +MemEncryptSevEsIsEnabled (
>> +  VOID
>> +  )
>> +{
>> +  if (!mSevStatusChecked) {
>> +    InternalMemEncryptSevStatus();
>> +  }
>> +
>> +  return mSevEsStatus;
>> +}
>> +
>> +/**
>> +  Returns a boolean to indicate whether SEV is enabled
>> +
>> +  @retval TRUE           SEV is enabled
>> +  @retval FALSE          SEV is not enabled
>> +**/
>> +BOOLEAN
>> +EFIAPI
>> +MemEncryptSevIsEnabled (
>> +  VOID
>> +  )
>> +{
>> +  if (!mSevStatusChecked) {
>> +    InternalMemEncryptSevStatus();
>> +  }
>>  
>>    return mSevStatus;
>>  }
>>
> 
> 
> 
>