Re: [PATCH RFC v3 03/22] OvmfPkg/MemEncryptSevLib: extend the workarea to include SNP enabled field

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

On 05/27/21 01:10, Brijesh Singh wrote:
> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3275
> 
> Extend the workarea to include the SEV-SNP enabled fields. This will be set
> when SEV-SNP is active in the guest VM.
> 
> 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/PlatformPei/PlatformPei.inf        |  1 +
>  OvmfPkg/Include/Library/MemEncryptSevLib.h |  3 ++-
>  OvmfPkg/PlatformPei/AmdSev.c               | 26 ++++++++++++++++++++++
>  OvmfPkg/ResetVector/Ia32/PageTables64.asm  | 12 ++++++++++
>  OvmfPkg/ResetVector/ResetVector.nasmb      |  1 +
>  5 files changed, 42 insertions(+), 1 deletion(-)

(1) Please split this in two patches -- the PlatformPei changes should
be a separate patch. And, I think those should come second, the
ResetVector + header file change should come first.

> 
> diff --git a/OvmfPkg/PlatformPei/PlatformPei.inf b/OvmfPkg/PlatformPei/PlatformPei.inf
> index 6ef77ba7bb21..bc1dcac48343 100644
> --- a/OvmfPkg/PlatformPei/PlatformPei.inf
> +++ b/OvmfPkg/PlatformPei/PlatformPei.inf
> @@ -110,6 +110,7 @@ [Pcd]
>    gUefiCpuPkgTokenSpaceGuid.PcdCpuBootLogicalProcessorNumber
>    gUefiCpuPkgTokenSpaceGuid.PcdCpuApStackSize
>    gUefiCpuPkgTokenSpaceGuid.PcdSevEsIsEnabled
> +  gUefiCpuPkgTokenSpaceGuid.PcdSevSnpIsEnabled
>  
>  [FixedPcd]
>    gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress
> diff --git a/OvmfPkg/Include/Library/MemEncryptSevLib.h b/OvmfPkg/Include/Library/MemEncryptSevLib.h
> index 2425d8ba0a36..24507de55c5d 100644
> --- a/OvmfPkg/Include/Library/MemEncryptSevLib.h
> +++ b/OvmfPkg/Include/Library/MemEncryptSevLib.h
> @@ -49,7 +49,8 @@ typedef struct {
>  //
>  typedef struct _SEC_SEV_ES_WORK_AREA {
>    UINT8    SevEsEnabled;
> -  UINT8    Reserved1[7];
> +  UINT8    SevSnpEnabled;
> +  UINT8    Reserved2[6];
>  
>    UINT64   RandomData;
>  
> diff --git a/OvmfPkg/PlatformPei/AmdSev.c b/OvmfPkg/PlatformPei/AmdSev.c
> index a8bf610022ba..67b78fd5fa36 100644
> --- a/OvmfPkg/PlatformPei/AmdSev.c
> +++ b/OvmfPkg/PlatformPei/AmdSev.c
> @@ -22,6 +22,27 @@
>  
>  #include "Platform.h"
>  
> +/**
> +
> +  Initialize SEV-SNP support if running as an SEV-SNP guest.
> +
> +  **/
> +STATIC
> +VOID
> +AmdSevSnpInitialize (
> +  VOID
> +  )
> +{
> +  RETURN_STATUS        PcdStatus;
> +
> +  if (!MemEncryptSevSnpIsEnabled ()) {
> +    return;
> +  }
> +
> +  PcdStatus = PcdSetBoolS (PcdSevSnpIsEnabled, TRUE);
> +  ASSERT_RETURN_ERROR (PcdStatus);
> +}
> +
>  /**
>  
>    Initialize SEV-ES support if running as an SEV-ES guest.
> @@ -209,4 +230,9 @@ AmdSevInitialize (
>    // Check and perform SEV-ES initialization if required.
>    //
>    AmdSevEsInitialize ();
> +
> +  //
> +  // Check and perform SEV-SNP initialization if required.
> +  //
> +  AmdSevSnpInitialize ();
>  }
> diff --git a/OvmfPkg/ResetVector/Ia32/PageTables64.asm b/OvmfPkg/ResetVector/Ia32/PageTables64.asm
> index 5fae8986d9da..6838cdeec9c3 100644
> --- a/OvmfPkg/ResetVector/Ia32/PageTables64.asm
> +++ b/OvmfPkg/ResetVector/Ia32/PageTables64.asm
> @@ -81,6 +81,11 @@ CheckSevFeatures:
>      ; the MSR check below will set the first byte of the workarea to one.
>      mov     byte[SEV_ES_WORK_AREA], 0
>  
> +    ; Set the SevSnpEnabled field in workarea to zero to communicate to the SEC
> +    ; phase that SEV-SNP is not enabled. If SEV-SNP is enabled, this function
> +    ; will set it to 1.
> +    mov       byte[SEV_ES_WORK_AREA_SNP], 0
> +
>      ;
>      ; Set up exception handlers to check for SEV-ES
>      ;   Load temporary RAM stack based on PCDs (see SevEsIdtVmmComm for
> @@ -136,6 +141,13 @@ CheckSevFeatures:
>      ; phase that SEV-ES is enabled.
>      mov       byte[SEV_ES_WORK_AREA], 1
>  
> +    bt        eax, 2
> +    jnc       GetSevEncBit
> +
> +    ; Set the second byte of the workarea to one to communicate to the SEC
> +    ; phase that the SEV-SNP is enabled
> +    mov       byte[SEV_ES_WORK_AREA_SNP], 1
> +
>  GetSevEncBit:
>      ; Get pte bit position to enable memory encryption
>      ; CPUID Fn8000_001F[EBX] - Bits 5:0

(2) Please mention in the commit message (of the ResetVector patch),
and/or a comment here in the code, that SEV-SNP is never enabled if
SEV-ES is disabled.

Section "15.34.10 SEV_STATUS MSR" in the APM (doc#24593 v3.37) does not
spell out this dependency.

Furthermore, the mSevStatus / mSevEsStatus / mSevSnpStatus variable
assignments in patch#2 do not form a "dependency cascade" like the one
seen here in the reset vector code.

While "SEV-ES depends on SEV" seems obvious to me (and so the related,
existent jumps in the assembly code are not surprising), the statement
"SEV-SNP depends on SEV-ES" is not *that* obvious to me. Thus a comment
would be welcome.

For *both* patches split out of this one:

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

Thanks
Laszlo

> diff --git a/OvmfPkg/ResetVector/ResetVector.nasmb b/OvmfPkg/ResetVector/ResetVector.nasmb
> index 5fbacaed5f9d..1971557b1c00 100644
> --- a/OvmfPkg/ResetVector/ResetVector.nasmb
> +++ b/OvmfPkg/ResetVector/ResetVector.nasmb
> @@ -73,6 +73,7 @@
>    %define GHCB_BASE (FixedPcdGet32 (PcdOvmfSecGhcbBase))
>    %define GHCB_SIZE (FixedPcdGet32 (PcdOvmfSecGhcbSize))
>    %define SEV_ES_WORK_AREA (FixedPcdGet32 (PcdSevEsWorkAreaBase))
> +  %define SEV_ES_WORK_AREA_SNP (FixedPcdGet32 (PcdSevEsWorkAreaBase) + 1)
>    %define SEV_ES_WORK_AREA_RDRAND (FixedPcdGet32 (PcdSevEsWorkAreaBase) + 8)
>    %define SEV_ES_WORK_AREA_ENC_MASK (FixedPcdGet32 (PcdSevEsWorkAreaBase) + 16)
>    %define SEV_ES_VC_TOP_OF_STACK (FixedPcdGet32 (PcdOvmfSecPeiTempRamBase) + FixedPcdGet32 (PcdOvmfSecPeiTempRamSize))
>