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

["Laszlo Ersek" <lersek@redhat.com>]

Hi Tom,

On 02/05/20 00:01, Lendacky, Thomas wrote:
> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=2198
> 
> 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")?

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;
>  }
>