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

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

On 9/24/19 6:59 AM, Laszlo Ersek wrote:
> 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"?

Will do.

> 
>>  
>>  [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)

Yup. I'll change that to "running as an SEV-ES guest".

> 
>> +
>> +  **/
>> +STATIC
>> +VOID
>> +AmdSevEsInitialize (
>> +  VOID
>> +  )
>> +{
>> +  RETURN_STATUS     PcdStatus;
>> +
>> +  if (!MemEncryptSevEsIsEnabled ()) {
>> +    return;
>> +  }
>> +
>> +  PcdStatus = PcdSetBoolS (PcdSevEsActive, 1);
> 
> (3) Please write TRUE, not 1.

Will do.

Thanks,
Tom

> 
>> +  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
>