Re: [edk2-devel] [RFC PATCH v2 03/44] OvmfPkg: Add support to perform SEV-ES initialization

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

On 09/19/19 21:52, Lendacky, Thomas wrote:
> From: Tom Lendacky <thomas.lendacky@amd.com>
> 
> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=2198
> 
> When SEV-ES is active, then SEV is also active. Add support to the SEV
> initialization function to also check for SEV-ES being active. If SEV-ES
> is active, set the SEV-ES active PCD (PcdSevEsActive).
> 
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
> ---
>  OvmfPkg/OvmfPkgIa32.dsc             |  3 +++
>  OvmfPkg/OvmfPkgIa32X64.dsc          |  3 +++
>  OvmfPkg/OvmfPkgX64.dsc              |  3 +++
>  OvmfPkg/PlatformPei/PlatformPei.inf |  1 +
>  OvmfPkg/PlatformPei/AmdSev.c        | 26 ++++++++++++++++++++++++++
>  5 files changed, 36 insertions(+)
> 
> diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc
> index 6ab730018694..0ce5c01722ef 100644
> --- a/OvmfPkg/OvmfPkgIa32.dsc
> +++ b/OvmfPkg/OvmfPkgIa32.dsc
> @@ -558,6 +558,9 @@ [PcdsDynamicDefault]
>    # Set memory encryption mask
>    gEfiMdeModulePkgTokenSpaceGuid.PcdPteMemoryEncryptionAddressOrMask|0x0
>  
> +  # Set SEV-ES defaults
> +  gEfiMdeModulePkgTokenSpaceGuid.PcdSevEsActive|0
> +
>  !if $(SMM_REQUIRE) == TRUE
>    gUefiOvmfPkgTokenSpaceGuid.PcdQ35TsegMbytes|8
>    gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmSyncMode|0x01
> diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc
> index f163aa267132..e7455e35a55d 100644
> --- a/OvmfPkg/OvmfPkgIa32X64.dsc
> +++ b/OvmfPkg/OvmfPkgIa32X64.dsc
> @@ -570,6 +570,9 @@ [PcdsDynamicDefault]
>    # Set memory encryption mask
>    gEfiMdeModulePkgTokenSpaceGuid.PcdPteMemoryEncryptionAddressOrMask|0x0
>  
> +  # Set SEV-ES defaults
> +  gEfiMdeModulePkgTokenSpaceGuid.PcdSevEsActive|0
> +
>  !if $(SMM_REQUIRE) == TRUE
>    gUefiOvmfPkgTokenSpaceGuid.PcdQ35TsegMbytes|8
>    gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmSyncMode|0x01
> diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
> index fa98f16a3fb3..0b8305cd10a2 100644
> --- a/OvmfPkg/OvmfPkgX64.dsc
> +++ b/OvmfPkg/OvmfPkgX64.dsc
> @@ -569,6 +569,9 @@ [PcdsDynamicDefault]
>    # Set memory encryption mask
>    gEfiMdeModulePkgTokenSpaceGuid.PcdPteMemoryEncryptionAddressOrMask|0x0
>  
> +  # Set SEV-ES defaults
> +  gEfiMdeModulePkgTokenSpaceGuid.PcdSevEsActive|0
> +
>  !if $(SMM_REQUIRE) == TRUE
>    gUefiOvmfPkgTokenSpaceGuid.PcdQ35TsegMbytes|8
>    gUefiCpuPkgTokenSpaceGuid.PcdCpuSmmSyncMode|0x01
> diff --git a/OvmfPkg/PlatformPei/PlatformPei.inf b/OvmfPkg/PlatformPei/PlatformPei.inf
> index d9fd9c8f05b3..2736347a2e03 100644
> --- a/OvmfPkg/PlatformPei/PlatformPei.inf
> +++ b/OvmfPkg/PlatformPei/PlatformPei.inf
> @@ -100,6 +100,7 @@ [Pcd]
>    gUefiCpuPkgTokenSpaceGuid.PcdCpuMaxLogicalProcessorNumber
>    gUefiCpuPkgTokenSpaceGuid.PcdCpuApInitTimeOutInMicroSeconds
>    gUefiCpuPkgTokenSpaceGuid.PcdCpuApStackSize
> +  gEfiMdeModulePkgTokenSpaceGuid.PcdSevEsActive

(1) Can you add this next to
"gEfiMdeModulePkgTokenSpaceGuid.PcdPteMemoryEncryptionAddressOrMask"?

>  
>  [FixedPcd]
>    gEfiMdePkgTokenSpaceGuid.PcdPciExpressBaseAddress
> diff --git a/OvmfPkg/PlatformPei/AmdSev.c b/OvmfPkg/PlatformPei/AmdSev.c
> index 2ae8126ccf8a..7ae2f26a2ba7 100644
> --- a/OvmfPkg/PlatformPei/AmdSev.c
> +++ b/OvmfPkg/PlatformPei/AmdSev.c
> @@ -19,6 +19,27 @@
>  
>  #include "Platform.h"
>  
> +/**
> +
> +  Initialize SEV-ES support if running an SEV-ES guest.

(2) s/an/in an/? (Just asking)

> +
> +  **/
> +STATIC
> +VOID
> +AmdSevEsInitialize (
> +  VOID
> +  )
> +{
> +  RETURN_STATUS     PcdStatus;
> +
> +  if (!MemEncryptSevEsIsEnabled ()) {
> +    return;
> +  }
> +
> +  PcdStatus = PcdSetBoolS (PcdSevEsActive, 1);

(3) Please write TRUE, not 1.

> +  ASSERT_RETURN_ERROR (PcdStatus);
> +}
> +
>  /**
>  
>    Function checks if SEV support is available, if present then it sets
> @@ -89,4 +110,9 @@ AmdSevInitialize (
>        EfiBootServicesData                // MemoryType
>        );
>    }
> +
> +  //
> +  // Check and perform SEV-ES initialization if required.
> +  //
> +  AmdSevEsInitialize ();
>  }
> 

With (1) and (3) fixed:

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

Thanks
Laszlo