Re: [PATCH v6 1/6] OvmfPkg/BaseMemEncryptLib: Detect SEV live migration feature.

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

On 8/2/21 7:31 AM, Ashish Kalra wrote:
> From: Ashish Kalra <ashish.kalra@amd.com>
> 
> Add support to check if we are running inside KVM HVM and
> KVM HVM supports SEV Live Migration feature.
> 
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
> Signed-off-by: Ashish Kalra <ashish.kalra@amd.com>
> ---
>  OvmfPkg/Include/Library/MemEncryptSevLib.h                            | 27 ++++++++++
>  OvmfPkg/Library/BaseMemEncryptSevLib/DxeMemEncryptSevLibInternal.c    | 39 +++++++++++++++
>  OvmfPkg/Library/BaseMemEncryptSevLib/PeiDxeMemEncryptSevLibInternal.c | 52 ++++++++++++++++++++
>  OvmfPkg/Library/BaseMemEncryptSevLib/PeiMemEncryptSevLibInternal.c    | 39 +++++++++++++++
>  OvmfPkg/Library/BaseMemEncryptSevLib/SecMemEncryptSevLibInternal.c    | 18 +++++++
>  5 files changed, 175 insertions(+)
> 
> diff --git a/OvmfPkg/Include/Library/MemEncryptSevLib.h b/OvmfPkg/Include/Library/MemEncryptSevLib.h
> index 76d06c206c..59f694fb8a 100644
> --- a/OvmfPkg/Include/Library/MemEncryptSevLib.h
> +++ b/OvmfPkg/Include/Library/MemEncryptSevLib.h
> @@ -90,6 +90,18 @@ MemEncryptSevIsEnabled (
>    VOID
>    );
>  
> +/**
> +  Returns a boolean to indicate whether SEV live migration is enabled.
> +
> +  @retval TRUE           SEV live migration is enabled
> +  @retval FALSE          SEV live migration is not enabled
> +**/
> +BOOLEAN
> +EFIAPI
> +MemEncryptSevLiveMigrationIsEnabled (
> +  VOID
> +  );
> +
>  /**
>    This function clears memory encryption bit for the memory region specified by
>    BaseAddress and NumPages from the current page table context.
> @@ -222,4 +234,19 @@ MemEncryptSevClearMmioPageEncMask (
>    IN UINTN                    NumPages
>    );
>  
> +#define KVM_FEATURE_MIGRATION_CONTROL   BIT17
> +
> +/**
> +  Figures out if we are running inside KVM HVM and
> +  KVM HVM supports SEV Live Migration feature.
> +
> +  @retval TRUE           SEV live migration is supported.
> +  @retval FALSE          SEV live migration is not supported.
> +**/
> +BOOLEAN
> +EFIAPI
> +KvmDetectSevLiveMigrationFeature(
> +  VOID
> +  );
> +

I don't think KvmDetectSevLiveMigrationFeature() should be in
OvmfPkg/Include/Library/MemEncryptSevLib.h since it isn't called except as
a helper by InternalDetectSevLiveMigrationFeature(). You should probably
create a new PeiDxeMemEncryptSevLibInternal.h header file for that
function that lives in OvmfPkg/Library/BaseMemEncryptSevLib.

>  #endif // _MEM_ENCRYPT_SEV_LIB_H_
> diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/DxeMemEncryptSevLibInternal.c b/OvmfPkg/Library/BaseMemEncryptSevLib/DxeMemEncryptSevLibInternal.c
> index 2816f859a0..ead754cd7b 100644
> --- a/OvmfPkg/Library/BaseMemEncryptSevLib/DxeMemEncryptSevLibInternal.c
> +++ b/OvmfPkg/Library/BaseMemEncryptSevLib/DxeMemEncryptSevLibInternal.c
> @@ -20,6 +20,8 @@
>  STATIC BOOLEAN mSevStatus = FALSE;
>  STATIC BOOLEAN mSevEsStatus = FALSE;
>  STATIC BOOLEAN mSevStatusChecked = FALSE;
> +STATIC BOOLEAN mSevLiveMigrationStatus = FALSE;
> +STATIC BOOLEAN mSevLiveMigrationStatusChecked = FALSE;
>  
>  STATIC UINT64  mSevEncryptionMask = 0;
>  STATIC BOOLEAN mSevEncryptionMaskSaved = FALSE;
> @@ -87,6 +89,24 @@ InternalMemEncryptSevStatus (
>    mSevStatusChecked = TRUE;
>  }
>  
> +/**
> +  Figures out if we are running inside KVM HVM and
> +  KVM HVM supports SEV Live Migration feature.
> +**/
> +STATIC
> +VOID
> +EFIAPI
> +InternalDetectSevLiveMigrationFeature(
> +  VOID
> +  )
> +{
> +  if (KvmDetectSevLiveMigrationFeature()) {

Add a space before the "()"

> +        mSevLiveMigrationStatus = TRUE;
> +  }
> +
> +  mSevLiveMigrationStatusChecked = TRUE;
> +}
> +
>  /**
>    Returns a boolean to indicate whether SEV-ES is enabled.
>  
> @@ -125,6 +145,25 @@ MemEncryptSevIsEnabled (
>    return mSevStatus;
>  }
>  
> +/**
> +  Returns a boolean to indicate whether SEV live migration is enabled.
> +
> +  @retval TRUE           SEV live migration is enabled
> +  @retval FALSE          SEV live migration is not enabled
> +**/
> +BOOLEAN
> +EFIAPI
> +MemEncryptSevLiveMigrationIsEnabled (
> +  VOID
> +  )
> +{
> +  if (!mSevLiveMigrationStatusChecked) {
> +    InternalDetectSevLiveMigrationFeature ();
> +  }
> +
> +  return mSevLiveMigrationStatus;
> +}
> +
>  /**
>    Returns the SEV encryption mask.
>  
> diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/PeiDxeMemEncryptSevLibInternal.c b/OvmfPkg/Library/BaseMemEncryptSevLib/PeiDxeMemEncryptSevLibInternal.c
> index b4a9f464e2..d7fc973134 100644
> --- a/OvmfPkg/Library/BaseMemEncryptSevLib/PeiDxeMemEncryptSevLibInternal.c
> +++ b/OvmfPkg/Library/BaseMemEncryptSevLib/PeiDxeMemEncryptSevLibInternal.c
> @@ -61,3 +61,55 @@ MemEncryptSevLocateInitialSmramSaveStateMapPages (
>  
>    return RETURN_SUCCESS;
>  }
> +
> +/**
> +  Figures out if we are running inside KVM HVM and
> +  KVM HVM supports SEV Live Migration feature.
> +
> +  @retval TRUE           SEV live migration is supported.
> +  @retval FALSE          SEV live migration is not supported.
> +**/
> +BOOLEAN
> +EFIAPI
> +KvmDetectSevLiveMigrationFeature(

Add a space before the "("

> +  VOID
> +  )
> +{
> +  CHAR8 Signature[13];
> +  UINT32 mKvmLeaf;
> +  UINT32 RegEax, RegEbx, RegEcx, RegEdx;

Coding style requires these to be four separate declarations.

> +
> +  Signature[12] = '\0';
> +  for (mKvmLeaf = 0x40000000; mKvmLeaf < 0x40010000; mKvmLeaf += 0x100) {

I still really don't understand the need for the CPUID loop. KVM only ever
programs CPUID function 0x40000000, right?

> +    AsmCpuid (
> +      mKvmLeaf,
> +      NULL,
> +      (UINT32 *) &Signature[0],
> +      (UINT32 *) &Signature[4],
> +      (UINT32 *) &Signature[8]);
> +
> +    if (AsciiStrCmp (Signature, "KVMKVMKVM") == 0) {
> +      DEBUG ((
> +        DEBUG_INFO,
> +        "%a: KVM Detected, signature = %a\n",
> +        __FUNCTION__,
> +        Signature
> +        ));
> +
> +      RegEax = mKvmLeaf + 1;
> +      RegEcx = 0;
> +      AsmCpuid (mKvmLeaf + 1, &RegEax, &RegEbx, &RegEcx, &RegEdx);
> +      if ((RegEax & KVM_FEATURE_MIGRATION_CONTROL) != 0) {
> +        DEBUG ((
> +          DEBUG_INFO,
> +          "%a: SEV Live Migration feature supported\n",
> +          __FUNCTION__
> +          ));
> +
> +        return TRUE;
> +      }
> +    }
> +  }
> +
> +  return FALSE;
> +}
> diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/PeiMemEncryptSevLibInternal.c b/OvmfPkg/Library/BaseMemEncryptSevLib/PeiMemEncryptSevLibInternal.c
> index e2fd109d12..9db6c2ef71 100644
> --- a/OvmfPkg/Library/BaseMemEncryptSevLib/PeiMemEncryptSevLibInternal.c
> +++ b/OvmfPkg/Library/BaseMemEncryptSevLib/PeiMemEncryptSevLibInternal.c
> @@ -20,6 +20,8 @@
>  STATIC BOOLEAN mSevStatus = FALSE;
>  STATIC BOOLEAN mSevEsStatus = FALSE;
>  STATIC BOOLEAN mSevStatusChecked = FALSE;
> +STATIC BOOLEAN mSevLiveMigrationStatus = FALSE;
> +STATIC BOOLEAN mSevLiveMigrationStatusChecked = FALSE;
>  
>  STATIC UINT64  mSevEncryptionMask = 0;
>  STATIC BOOLEAN mSevEncryptionMaskSaved = FALSE;
> @@ -87,6 +89,24 @@ InternalMemEncryptSevStatus (
>    mSevStatusChecked = TRUE;
>  }
>  
> +/**
> +  Figures out if we are running inside KVM HVM and
> +  KVM HVM supports SEV Live Migration feature.
> +**/
> +STATIC
> +VOID
> +EFIAPI
> +InternalDetectSevLiveMigrationFeature(

Add a space before "("

> +  VOID
> +  )
> +{
> +  if (KvmDetectSevLiveMigrationFeature()) {

Add a space before "()"

Thanks,
Tom

> +    mSevLiveMigrationStatus = TRUE;
> +  }
> +
> +  mSevLiveMigrationStatusChecked = TRUE;
> +}
> +
>  /**
>    Returns a boolean to indicate whether SEV-ES is enabled.
>  
> @@ -125,6 +145,25 @@ MemEncryptSevIsEnabled (
>    return mSevStatus;
>  }
>  
> +/**
> +  Returns a boolean to indicate whether SEV live migration is enabled.
> +
> +  @retval TRUE           SEV live migration is enabled
> +  @retval FALSE          SEV live migration is not enabled
> +**/
> +BOOLEAN
> +EFIAPI
> +MemEncryptSevLiveMigrationIsEnabled (
> +  VOID
> +  )
> +{
> +  if (!mSevLiveMigrationStatusChecked) {
> +    InternalDetectSevLiveMigrationFeature ();
> +  }
> +
> +  return mSevLiveMigrationStatus;
> +}
> +
>  /**
>    Returns the SEV encryption mask.
>  
> diff --git a/OvmfPkg/Library/BaseMemEncryptSevLib/SecMemEncryptSevLibInternal.c b/OvmfPkg/Library/BaseMemEncryptSevLib/SecMemEncryptSevLibInternal.c
> index 56d8f3f318..d9f7befcd2 100644
> --- a/OvmfPkg/Library/BaseMemEncryptSevLib/SecMemEncryptSevLibInternal.c
> +++ b/OvmfPkg/Library/BaseMemEncryptSevLib/SecMemEncryptSevLibInternal.c
> @@ -100,6 +100,24 @@ MemEncryptSevIsEnabled (
>    return Msr.Bits.SevBit ? TRUE : FALSE;
>  }
>  
> +/**
> +  Returns a boolean to indicate whether SEV live migration is enabled.
> +
> +  @retval TRUE           SEV live migration is enabled
> +  @retval FALSE          SEV live migration is not enabled
> +**/
> +BOOLEAN
> +EFIAPI
> +MemEncryptSevLiveMigrationIsEnabled (
> +  VOID
> +  )
> +{
> +  //
> +  // Not used in SEC phase.
> +  //
> +  return FALSE;
> +}
> +
>  /**
>    Returns the SEV encryption mask.
>  
>