Re: [edk2-devel] [PATCH 11/12] OvmfPkg/PlatformPei: Reserve GHCB backup pages if S3 is supported

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

On 1/5/21 4:13 AM, Laszlo Ersek wrote:
> On 12/15/20 21:51, Lendacky, Thomas wrote:
>> From: Tom Lendacky <thomas.lendacky@amd.com>
>>
>> BZ: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D3108&amp;data=04%7C01%7Cthomas.lendacky%40amd.com%7C330e4cc1c9954f57e60e08d8b1629676%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637454384297042607%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=tutrqsYVzXDNPUeIBu0XwhE7Jhj6UPCDbITGhLeVgWY%3D&amp;reserved=0
>>
>> Protect the GHCB backup pages used by an SEV-ES guest when S3 is
>> supported.
>>
>> Regarding the lifecycle of the GHCB backup pages:
>>   PcdOvmfSecGhcbBackupBase
>>
>> (a) when and how it is initialized after first boot of the VM
>>
>>   If SEV-ES is enabled, the GHCB backup pages when a nested #VC is
>>   received during the SEC phase
>>   [OvmfPkg/Library/VmgExitLib/SecVmgExitVcHandler.c].
> 
> (1) This sentence appears to miss a verb.

Yup. I'll change it to:

If SEV-ES is enabled, the GHCB backup pages are initialized when a nested
#VC is received during the SEC phase
[OvmfPkg/Library/VmgExitLib/SecVmgExitVcHandler.c].

Thanks,
Tom

> 
> With that fixed:
> 
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> 
> Thanks
> Laszlo
> 
>>
>> (b) how it is protected from memory allocations during DXE
>>
>>   If S3 and SEV-ES are enabled, then InitializeRamRegions()
>>   [OvmfPkg/PlatformPei/MemDetect.c] protects the ranges with an AcpiNVS
>>   memory allocation HOB, in PEI.
>>
>>   If S3 is disabled, then these ranges are not protected. PEI switches to
>>   the GHCB backup pages in permanent PEI memory and DXE will use these
>>   PEI GHCB backup pages, so we don't have to preserve
>>   PcdOvmfSecGhcbBackupBase.
>>
>> (c) how it is protected from the OS
>>
>>   If S3 is enabled, then (b) reserves it from the OS too.
>>
>>   If S3 is disabled, then the range needs no protection.
>>
>> (d) how it is accessed on the S3 resume path
>>
>>   It is rewritten same as in (a), which is fine because (b) reserved it.
>>
>> (e) how it is accessed on the warm reset path
>>
>>   It is rewritten same as in (a).
>>
>> Cc: Jordan Justen <jordan.l.justen@intel.com>
>> Cc: Laszlo Ersek <lersek@redhat.com>
>> Cc: Ard Biesheuvel <ard.biesheuvel@arm.com>
>> Cc: Anthony Perard <anthony.perard@citrix.com>
>> Cc: Julien Grall <julien@xen.org>
>> Cc: Brijesh Singh <brijesh.singh@amd.com>
>> Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
>> ---
>>  OvmfPkg/PlatformPei/PlatformPei.inf | 2 ++
>>  OvmfPkg/PlatformPei/MemDetect.c     | 5 +++++
>>  2 files changed, 7 insertions(+)
>>
>> diff --git a/OvmfPkg/PlatformPei/PlatformPei.inf b/OvmfPkg/PlatformPei/PlatformPei.inf
>> index c53be2f4925c..6ef77ba7bb21 100644
>> --- a/OvmfPkg/PlatformPei/PlatformPei.inf
>> +++ b/OvmfPkg/PlatformPei/PlatformPei.inf
>> @@ -118,6 +118,8 @@ [FixedPcd]
>>    gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiReservedMemoryType
>>    gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiRuntimeServicesCode
>>    gEmbeddedTokenSpaceGuid.PcdMemoryTypeEfiRuntimeServicesData
>> +  gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbBackupBase
>> +  gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbBackupSize
>>    gUefiCpuPkgTokenSpaceGuid.PcdSevEsWorkAreaBase
>>    gUefiCpuPkgTokenSpaceGuid.PcdSevEsWorkAreaSize
>>  
>> diff --git a/OvmfPkg/PlatformPei/MemDetect.c b/OvmfPkg/PlatformPei/MemDetect.c
>> index ffbbef891a11..c08aa2e45a53 100644
>> --- a/OvmfPkg/PlatformPei/MemDetect.c
>> +++ b/OvmfPkg/PlatformPei/MemDetect.c
>> @@ -888,6 +888,11 @@ InitializeRamRegions (
>>          (UINT64)(UINTN) PcdGet32 (PcdOvmfSecGhcbSize),
>>          EfiACPIMemoryNVS
>>          );
>> +      BuildMemoryAllocationHob (
>> +        (EFI_PHYSICAL_ADDRESS)(UINTN) PcdGet32 (PcdOvmfSecGhcbBackupBase),
>> +        (UINT64)(UINTN) PcdGet32 (PcdOvmfSecGhcbBackupSize),
>> +        EfiACPIMemoryNVS
>> +        );
>>      }
>>  #endif
>>    }
>>
>