Re: [PATCH RFC v3 02/22] OvmfPkg/MemEncryptSevLib: add MemEncryptSevSnpEnabled()

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

On 05/27/21 01:10, Brijesh Singh wrote:
> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3275
> 
> Create a function that can be used to determine if VM is running as an
> SEV-SNP guest.
> 
> Cc: James Bottomley <jejb@linux.ibm.com>
> Cc: Min Xu <min.m.xu@intel.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Tom Lendacky <thomas.lendacky@amd.com>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Erdem Aktas <erdemaktas@google.com>
> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
> ---
>  OvmfPkg/Include/Library/MemEncryptSevLib.h    | 12 +++++++++
>  .../DxeMemEncryptSevLibInternal.c             | 27 +++++++++++++++++++
>  .../PeiMemEncryptSevLibInternal.c             | 27 +++++++++++++++++++
>  .../SecMemEncryptSevLibInternal.c             | 19 +++++++++++++
>  4 files changed, 85 insertions(+)
> 
> diff --git a/OvmfPkg/Include/Library/MemEncryptSevLib.h b/OvmfPkg/Include/Library/MemEncryptSevLib.h
> index 76d06c206c8b..2425d8ba0a36 100644
> --- a/OvmfPkg/Include/Library/MemEncryptSevLib.h
> +++ b/OvmfPkg/Include/Library/MemEncryptSevLib.h
> @@ -66,6 +66,18 @@ typedef enum {
>    MemEncryptSevAddressRangeError,
>  } MEM_ENCRYPT_SEV_ADDRESS_RANGE_STATE;
>  
> +/**
> +  Returns a boolean to indicate whether SEV-SNP is enabled
> +
> +  @retval TRUE           SEV-SNP is enabled
> +  @retval FALSE          SEV-SNP is not enabled
> +**/
> +BOOLEAN
> +EFIAPI
> +MemEncryptSevSnpIsEnabled (
> +  VOID
> +  );
> +
>  /**
>    Returns a boolean to indicate whether SEV-ES is enabled.
>  
> diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/DxeMemEncryptSevLibInternal.c b/OvmfPkg/Library/BaseMemEncryptSevLib/DxeMemEncryptSevLibInternal.c
> index 2816f859a0c4..057129723824 100644
> --- a/OvmfPkg/Library/BaseMemEncryptSevLib/DxeMemEncryptSevLibInternal.c
> +++ b/OvmfPkg/Library/BaseMemEncryptSevLib/DxeMemEncryptSevLibInternal.c
> @@ -19,6 +19,7 @@
>  
>  STATIC BOOLEAN mSevStatus = FALSE;
>  STATIC BOOLEAN mSevEsStatus = FALSE;
> +STATIC BOOLEAN mSevSnpStatus = FALSE;
>  STATIC BOOLEAN mSevStatusChecked = FALSE;
>  
>  STATIC UINT64  mSevEncryptionMask = 0;
> @@ -82,11 +83,37 @@ InternalMemEncryptSevStatus (
>      if (Msr.Bits.SevEsBit) {
>        mSevEsStatus = TRUE;
>      }
> +
> +    //
> +    // Check MSR_0xC0010131 Bit 2 (Sev-Snp Enabled)
> +    //
> +    if (Msr.Bits.SevSnpBit) {
> +      mSevSnpStatus = TRUE;
> +    }
>    }
>  
>    mSevStatusChecked = TRUE;
>  }
>  
> +/**
> +  Returns a boolean to indicate whether SEV-SNP is enabled.
> +
> +  @retval TRUE           SEV-SNP is enabled
> +  @retval FALSE          SEV-SNP is not enabled
> +**/
> +BOOLEAN
> +EFIAPI
> +MemEncryptSevSnpIsEnabled (
> +  VOID
> +  )
> +{
> +  if (!mSevStatusChecked) {
> +    InternalMemEncryptSevStatus ();
> +  }
> +
> +  return mSevSnpStatus;
> +}
> +
>  /**
>    Returns a boolean to indicate whether SEV-ES is enabled.
>  
> diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/PeiMemEncryptSevLibInternal.c b/OvmfPkg/Library/BaseMemEncryptSevLib/PeiMemEncryptSevLibInternal.c
> index e2fd109d120f..b561f211f577 100644
> --- a/OvmfPkg/Library/BaseMemEncryptSevLib/PeiMemEncryptSevLibInternal.c
> +++ b/OvmfPkg/Library/BaseMemEncryptSevLib/PeiMemEncryptSevLibInternal.c
> @@ -19,6 +19,7 @@
>  
>  STATIC BOOLEAN mSevStatus = FALSE;
>  STATIC BOOLEAN mSevEsStatus = FALSE;
> +STATIC BOOLEAN mSevSnpStatus = FALSE;
>  STATIC BOOLEAN mSevStatusChecked = FALSE;
>  
>  STATIC UINT64  mSevEncryptionMask = 0;
> @@ -82,11 +83,37 @@ InternalMemEncryptSevStatus (
>      if (Msr.Bits.SevEsBit) {
>        mSevEsStatus = TRUE;
>      }
> +
> +    //
> +    // Check MSR_0xC0010131 Bit 2 (Sev-Snp Enabled)
> +    //
> +    if (Msr.Bits.SevSnpBit) {
> +      mSevSnpStatus = TRUE;
> +    }
>    }
>  
>    mSevStatusChecked = TRUE;
>  }
>  
> +/**
> +  Returns a boolean to indicate whether SEV-SNP is enabled.
> +
> +  @retval TRUE           SEV-SNP is enabled
> +  @retval FALSE          SEV-SNP is not enabled
> +**/
> +BOOLEAN
> +EFIAPI
> +MemEncryptSevSnpIsEnabled (
> +  VOID
> +  )
> +{
> +  if (!mSevStatusChecked) {
> +    InternalMemEncryptSevStatus ();
> +  }
> +
> +  return mSevSnpStatus;
> +}
> +
>  /**
>    Returns a boolean to indicate whether SEV-ES is enabled.
>  
> diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/SecMemEncryptSevLibInternal.c b/OvmfPkg/Library/BaseMemEncryptSevLib/SecMemEncryptSevLibInternal.c
> index 56d8f3f3183f..69852779e2ff 100644
> --- a/OvmfPkg/Library/BaseMemEncryptSevLib/SecMemEncryptSevLibInternal.c
> +++ b/OvmfPkg/Library/BaseMemEncryptSevLib/SecMemEncryptSevLibInternal.c
> @@ -62,6 +62,25 @@ InternalMemEncryptSevStatus (
>    return ReadSevMsr ? AsmReadMsr32 (MSR_SEV_STATUS) : 0;
>  }
>  
> +/**
> +  Returns a boolean to indicate whether SEV-SNP is enabled.
> +
> +  @retval TRUE           SEV-SNP is enabled
> +  @retval FALSE          SEV-SNP is not enabled
> +**/
> +BOOLEAN
> +EFIAPI
> +MemEncryptSevSnpIsEnabled (
> +  VOID
> +  )
> +{
> +  MSR_SEV_STATUS_REGISTER           Msr;
> +
> +  Msr.Uint32 = InternalMemEncryptSevStatus ();
> +
> +  return Msr.Bits.SevSnpBit ? TRUE : FALSE;

According to the edk2 coding style, this should be written as:

  return (BOOLEAN)(Msr.Bits.SevSnpBit != 0);

primarily because "Msr.Bits.SevSnpBit" does not have type BOOLEAN, so it
should not be used stand-alone in a logical context -- instead, it
should be compared with zero explicitly.

However, I see that the other two functions MemEncryptSevEsIsEnabled()
and MemEncryptSevIsEnabled() in this library instance use the same
pattern. I agree consistency is more important here. And I don't think
we should add more patches just for updating the style there. These
functions are closely located together in the same source file; if we
ever update the style, it's not a big deal to update them together.

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Thanks
Laszlo


> +}
> +
>  /**
>    Returns a boolean to indicate whether SEV-ES is enabled.
>  
>