Re: [RFC PATCH 01/19] OvmfPkg: Reserve the Secrets and Cpuid page for the SEV-SNP guest

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

On 04/13/21 13:29, Brijesh Singh wrote:
> 
> On 4/13/21 4:49 AM, Laszlo Ersek wrote:
>> On 04/12/21 16:52, Brijesh Singh wrote:
>>> Hi James and Laszlo,
>>>
>>> I was planning to work to add the support to reserve the Secrets and
>>> CPUID page in E820 map and then create the EFI configuration table entry
>>> for it so that guest OS can reach to it. We have two packages
>>> "SecretDxe" and "SecretPei" in OvmfPkg/AmdSev. Any issues if I use them
>>> in the OvmfPkg.dsc ? Here is what I was thinking:
>>>
>>> 1) Rename the PcdSevLaunchSecretBase -> PcdSevSecretsBase
>>>
>>> 2) When SNP is enabled then VMM use this page as secrets page for the SNP
>>>
>>> 3) When SEV or SEV-ES is enabled then VMM uses this page as a launch
>>> secret page
>>>
>>> This will allow me to drop PcdOvmfSnpSecretsBase. This will not just
>>> save 4-bytes but also minimize the code duplication.
>> I'm pretty unhappy about needing a separate page for each such purpose.
>> We're wasting room in MEMFD. The GUIDed structs that we expose to QEMU
>> seem to be flexible enough to describe non-page-aligned addresses,
>> right? Can we pack larger amounts of cruft into MEMFD pages?
> 
> With the GUID approach we should be able to pack multiple fields into a
> page but unfortunately in the case of SEV-SNP both the CPUID and Secrets
> need to be a page size. Without SNP support the we reserve the following
> page for the SEV/SEV-ES:
> 
> 1 page for Launch Secret.
> 
> 2 pages for the GHCB
> 
> 1 page for EsWorkArea
> 
> Both the EsWorkArea and LaunchSecret does not need to be the page
> aligned or page sized. Since the SNP needs a full page for the secrets
> so  I was inclined to use the same secrets page for both SEV and SNP. 
> At the end all we need to do is  reserve one extra page for CPUID to
> make the SNP work.
> 
> In future the EsWorkArea page can be used to pack additional information
> without needed to reserve full page (if feature does not require page).

Thank you for doing this analysis. I would much welcome a separate
series, just for "compressing" as many of the artifacts we now have down
to as few pages as possible. Could you propose a series like that?
Please CC Tom, James and Jiewen for review.

For this, feel free to rename PCDs as needed, and also to introduce new
(packed, if needed) structure types; probably under <OvmfPkg/Include/Guid>.

If some constants have to be doubly-defined, for C code and for assembly
code separately, for example, I'm fine with that -- normally, that's bad
practice, and if we can avoid it, that's great; but being conservative
with MEMFD is more important to me. (I hope I'm not going to eat my
words a few months down the road; this is how I feel right now anyway.)

Thanks!
Laszlo

> 
> 
>>
>> I'm not looking forward to the day when we run out of slack in MEMFD and
>> we get to shift PEIFV / DXEFV. (Every time we need to increase the DXEFV
>> size, the same risk exists -- which is why I've been thinking for a
>> while now that OVMF includes too many features already.) This can
>> introduce obscure changes to the UEFI memory map, which has caused
>> compat problems in the past, for example with the "crash" utility.
>>
>> The feature creep in OVMF has gone off the rails in the last few years,
>> really. (Not that I'm not guilty myself.)
>>
>> Thanks,
>> Laszlo
>>
>>> Thoughts ?
>>>
>>> -Brijesh
>>>
>>> On 3/24/21 10:31 AM, Brijesh Singh wrote:
>>>> BZ: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D3275&amp;data=04%7C01%7Cbrijesh.singh%40amd.com%7C04be68371db9458cbdc108d8fe61713a%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637539041773579324%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=J62%2BVSCZRawAkzsi9xtS43cpowZxSCx%2BwcDYNwdF3qA%3D&amp;reserved=0
>>>>
>>>> During the SEV-SNP guest launch sequence, two special pages need to
>>>> be inserted, the secrets page and cpuid page. The secrets page,
>>>> contain the VM platform communication keys. The guest BIOS and OS
>>>> can use this key to communicate with the SEV firmware to get the
>>>> attestation report. The Cpuid page, contain the CPUIDs entries
>>>> filtered through the AMD-SEV firmware.
>>>>
>>>> The VMM will locate the secrets and cpuid page addresses through a
>>>> fixed GUID and pass them to SEV firmware to populate further.
>>>> For more information about the page content, see the SEV-SNP spec.
>>>>
>>>> To simplify the pre-validation range calculation in the next patch,
>>>> the CPUID and Secrets pages are moved to the start of the
>>>> MEMFD_BASE_ADDRESS.
>>>>
>>>> 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>
>>>> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
>>>> ---
>>>>  OvmfPkg/OvmfPkg.dec                          |  8 +++++++
>>>>  OvmfPkg/OvmfPkgX64.fdf                       | 24 ++++++++++++--------
>>>>  OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm | 19 ++++++++++++++++
>>>>  OvmfPkg/ResetVector/ResetVector.inf          |  4 ++++
>>>>  OvmfPkg/ResetVector/ResetVector.nasmb        |  2 ++
>>>>  5 files changed, 48 insertions(+), 9 deletions(-)
>>>>
>>>> diff --git a/OvmfPkg/OvmfPkg.dec b/OvmfPkg/OvmfPkg.dec
>>>> index 4348bb45c6..062926772d 100644
>>>> --- a/OvmfPkg/OvmfPkg.dec
>>>> +++ b/OvmfPkg/OvmfPkg.dec
>>>> @@ -317,6 +317,14 @@
>>>>    gUefiOvmfPkgTokenSpaceGuid.PcdSevLaunchSecretBase|0x0|UINT32|0x42
>>>>    gUefiOvmfPkgTokenSpaceGuid.PcdSevLaunchSecretSize|0x0|UINT32|0x43
>>>>  
>>>> +  ## The base address of the CPUID page used by SEV-SNP
>>>> +  gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSnpCpuidBase|0|UINT32|0x48
>>>> +  gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSnpCpuidSize|0|UINT32|0x49
>>>> +
>>>> +  ## The base address of the Secrets page used by SEV-SNP
>>>> +  gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSnpSecretsBase|0|UINT32|0x50
>>>> +  gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSnpSecretsSize|0|UINT32|0x51
>>>> +
>>>>  [PcdsDynamic, PcdsDynamicEx]
>>>>    gUefiOvmfPkgTokenSpaceGuid.PcdEmuVariableEvent|0|UINT64|2
>>>>    gUefiOvmfPkgTokenSpaceGuid.PcdOvmfFlashVariablesEnable|FALSE|BOOLEAN|0x10
>>>> diff --git a/OvmfPkg/OvmfPkgX64.fdf b/OvmfPkg/OvmfPkgX64.fdf
>>>> index d519f85328..ea214600be 100644
>>>> --- a/OvmfPkg/OvmfPkgX64.fdf
>>>> +++ b/OvmfPkg/OvmfPkgX64.fdf
>>>> @@ -67,27 +67,33 @@ ErasePolarity = 1
>>>>  BlockSize     = 0x10000
>>>>  NumBlocks     = 0xD0
>>>>  
>>>> -0x000000|0x006000
>>>> +0x000000|0x001000
>>>> +gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSnpCpuidBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSnpCpuidSize
>>>> +
>>>> +0x001000|0x001000
>>>> +gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSnpSecretsBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSnpSecretsSize
>>>> +
>>>> +0x002000|0x006000
>>>>  gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPageTablesBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPageTablesSize
>>>>  
>>>> -0x006000|0x001000
>>>> +0x008000|0x001000
>>>>  gUefiOvmfPkgTokenSpaceGuid.PcdOvmfLockBoxStorageBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfLockBoxStorageSize
>>>>  
>>>> -0x007000|0x001000
>>>> +0x009000|0x001000
>>>>  gEfiMdePkgTokenSpaceGuid.PcdGuidedExtractHandlerTableAddress|gUefiOvmfPkgTokenSpaceGuid.PcdGuidedExtractHandlerTableSize
>>>>  
>>>> -0x008000|0x001000
>>>> +0x00A000|0x001000
>>>>  gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbPageTableBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbPageTableSize
>>>>  
>>>> -0x009000|0x002000
>>>> +0x00B000|0x002000
>>>>  gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbSize
>>>>  
>>>> -0x00B000|0x001000
>>>> -gUefiCpuPkgTokenSpaceGuid.PcdSevEsWorkAreaBase|gUefiCpuPkgTokenSpaceGuid.PcdSevEsWorkAreaSize
>>>> -
>>>> -0x00C000|0x001000
>>>> +0x00D000|0x001000
>>>>  gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbBackupBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbBackupSize
>>>>  
>>>> +0x00F000|0x001000
>>>> +gUefiCpuPkgTokenSpaceGuid.PcdSevEsWorkAreaBase|gUefiCpuPkgTokenSpaceGuid.PcdSevEsWorkAreaSize
>>>> +
>>>>  0x010000|0x010000
>>>>  gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPeiTempRamBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPeiTempRamSize
>>>>  
>>>> diff --git a/OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm b/OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm
>>>> index 9c0b5853a4..5456f02924 100644
>>>> --- a/OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm
>>>> +++ b/OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm
>>>> @@ -47,6 +47,25 @@ TIMES (15 - ((guidedStructureEnd - guidedStructureStart + 15) % 16)) DB 0
>>>>  ;
>>>>  guidedStructureStart:
>>>>  
>>>> +;
>>>> +; SEV-SNP boot support
>>>> +;
>>>> +; sevSnpBlock:
>>>> +;   For the initial boot of SEV-SNP guest, a Secrets and CPUID page must be
>>>> +;   reserved by the BIOS at a RAM area defined by SEV_SNP_SECRETS_PAGE
>>>> +;   and SEV_SNP_CPUID_PAGE. A VMM will locate this information using the
>>>> +;   SEV-SNP boot block.
>>>> +;
>>>> +; GUID (SEV-SNP boot block): bd39c0c2-2f8e-4243-83e8-1b74cebcb7d9
>>>> +;
>>>> +sevSnpBootBlockStart:
>>>> +    DD      SEV_SNP_SECRETS_PAGE
>>>> +    DD      SEV_SNP_CPUID_PAGE
>>>> +    DW      sevSnpBootBlockEnd - sevSnpBootBlockStart
>>>> +    DB      0xC2, 0xC0, 0x39, 0xBD, 0x8e, 0x2F, 0x43, 0x42
>>>> +    DB      0x83, 0xE8, 0x1B, 0x74, 0xCE, 0xBC, 0xB7, 0xD9
>>>> +sevSnpBootBlockEnd:
>>>> +
>>>>  ;
>>>>  ; SEV Secret block
>>>>  ;
>>>> diff --git a/OvmfPkg/ResetVector/ResetVector.inf b/OvmfPkg/ResetVector/ResetVector.inf
>>>> index dc38f68919..d890bb6b29 100644
>>>> --- a/OvmfPkg/ResetVector/ResetVector.inf
>>>> +++ b/OvmfPkg/ResetVector/ResetVector.inf
>>>> @@ -37,6 +37,10 @@
>>>>    gUefiCpuPkgTokenSpaceGuid.PcdSevEsWorkAreaBase
>>>>    gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbBase
>>>>    gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbSize
>>>> +  gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSnpCpuidBase
>>>> +  gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSnpCpuidSize
>>>> +  gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSnpSecretsBase
>>>> +  gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSnpSecretsSize
>>>>    gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbPageTableBase
>>>>    gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbPageTableSize
>>>>    gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPageTablesBase
>>>> diff --git a/OvmfPkg/ResetVector/ResetVector.nasmb b/OvmfPkg/ResetVector/ResetVector.nasmb
>>>> index 5fbacaed5f..2c194958f4 100644
>>>> --- a/OvmfPkg/ResetVector/ResetVector.nasmb
>>>> +++ b/OvmfPkg/ResetVector/ResetVector.nasmb
>>>> @@ -75,6 +75,8 @@
>>>>    %define SEV_ES_WORK_AREA (FixedPcdGet32 (PcdSevEsWorkAreaBase))
>>>>    %define SEV_ES_WORK_AREA_RDRAND (FixedPcdGet32 (PcdSevEsWorkAreaBase) + 8)
>>>>    %define SEV_ES_WORK_AREA_ENC_MASK (FixedPcdGet32 (PcdSevEsWorkAreaBase) + 16)
>>>> +  %define SEV_SNP_SECRETS_PAGE FixedPcdGet32 (PcdOvmfSnpSecretsBase)
>>>> +  %define SEV_SNP_CPUID_PAGE  FixedPcdGet32 (PcdOvmfSnpCpuidBase)
>>>>    %define SEV_ES_VC_TOP_OF_STACK (FixedPcdGet32 (PcdOvmfSecPeiTempRamBase) + FixedPcdGet32 (PcdOvmfSecPeiTempRamSize))
>>>>  %include "Ia32/Flat32ToFlat64.asm"
>>>>  %include "Ia32/PageTables64.asm"
>