[edk2-devel] [RFC PATCH 01/19] OvmfPkg: Reserve the Secrets and Cpuid page for the SEV-SNP guest
Laszlo Ersek
lersek at redhat.com
Tue Apr 13 13:13:44 UTC 2021
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&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 at linux.ibm.com>
>>>> Cc: Min Xu <min.m.xu at intel.com>
>>>> Cc: Jiewen Yao <jiewen.yao at intel.com>
>>>> Cc: Tom Lendacky <thomas.lendacky at amd.com>
>>>> Cc: Jordan Justen <jordan.l.justen at intel.com>
>>>> Cc: Ard Biesheuvel <ardb+tianocore at kernel.org>
>>>> Cc: Laszlo Ersek <lersek at redhat.com>
>>>> Signed-off-by: Brijesh Singh <brijesh.singh at 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"
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#74043): https://edk2.groups.io/g/devel/message/74043
Mute This Topic: https://groups.io/mt/81584577/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list