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

["Brijesh Singh" <brijesh.singh@amd.com>]

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


>
> 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&data=04%7C01%7Cbrijesh.singh%40amd.com%7C04be68371db9458cbdc108d8fe61713a%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637539041773579324%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=J62%2BVSCZRawAkzsi9xtS43cpowZxSCx%2BwcDYNwdF3qA%3D&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"