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

["Min Xu" <min.m.xu@intel.com>]

Hi, Singh
I have a concern about the sevSnpBlock in ResetVectorVtf0.asm. Actually
SEV has inserted 3 blocks in ResetVectorVtf0.asm and the total bytes are
(26 + 22 + 20 = 68 bytes). If sevSnpBlock is added, then the total bytes
will be (68 +26 = 94 bytes).

I am not sure whether there will be more blocks added in
ResetVectorVtf0.asm in the future. But I don't think ResetVectorVtf0.asm
is a good place to add these data blobs. Can these data be packed into a
single file, for example, SevMetadata.asm, then a pointer is inserted in
ResetVectorVtf0.asm which then points to the SevMetadata. In this way we
can keep ResetVectorVtf0.asm clean, small and straight forward.

Another reason is that I am working on the Intel TDX which will update
the ResetVectorVtf0.asm as well. My change depends on the assumption that
the distance between ResetVector(0xfffffff0) and EarlyBspInitReal16 is
less than 128 bytes. The blocks in ResetVectorVtf0.asm make it impossible.

Thanks!

> -----Original Message-----
> From: Brijesh Singh <brijesh.singh@amd.com>
> Sent: Wednesday, March 24, 2021 11:32 PM
> To: devel@edk2.groups.io
> Cc: Brijesh Singh <brijesh.singh@amd.com>; James Bottomley
> <jejb@linux.ibm.com>; Xu, Min M <min.m.xu@intel.com>; Yao, Jiewen
> <jiewen.yao@intel.com>; Tom Lendacky <thomas.lendacky@amd.com>;
> Justen, Jordan L <jordan.l.justen@intel.com>; Ard Biesheuvel
> <ardb+tianocore@kernel.org>; Laszlo Ersek <lersek@redhat.com>
> Subject: [RFC PATCH 01/19] OvmfPkg: Reserve the Secrets and Cpuid page for
> the SEV-SNP guest
> 
> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3275
> 
> 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|gUefiOvmfPkgTokenS
> paceGu
> +id.PcdOvmfSnpCpuidSize
> +
> +0x001000|0x001000
> +gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSnpSecretsBase|gUefiOvmfPkgToken
> Space
> +Guid.PcdOvmfSnpSecretsSize
> +
> +0x002000|0x006000
> 
> gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPageTablesBase|gUefiOvmfPkgTok
> enSpaceGuid.PcdOvmfSecPageTablesSize
> 
> -0x006000|0x001000
> +0x008000|0x001000
> 
> gUefiOvmfPkgTokenSpaceGuid.PcdOvmfLockBoxStorageBase|gUefiOvmfPkgTo
> kenSpaceGuid.PcdOvmfLockBoxStorageSize
> 
> -0x007000|0x001000
> +0x009000|0x001000
> 
> gEfiMdePkgTokenSpaceGuid.PcdGuidedExtractHandlerTableAddress|gUefiOvm
> fPkgTokenSpaceGuid.PcdGuidedExtractHandlerTableSize
> 
> -0x008000|0x001000
> +0x00A000|0x001000
> 
> gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbPageTableBase|gUefiOvmfPkg
> TokenSpaceGuid.PcdOvmfSecGhcbPageTableSize
> 
> -0x009000|0x002000
> +0x00B000|0x002000
> 
> gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbBase|gUefiOvmfPkgTokenSpa
> ceGuid.PcdOvmfSecGhcbSize
> 
> -0x00B000|0x001000
> -
> gUefiCpuPkgTokenSpaceGuid.PcdSevEsWorkAreaBase|gUefiCpuPkgTokenSpac
> eGuid.PcdSevEsWorkAreaSize
> -
> -0x00C000|0x001000
> +0x00D000|0x001000
> 
> gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbBackupBase|gUefiOvmfPkgTo
> kenSpaceGuid.PcdOvmfSecGhcbBackupSize
> 
> +0x00F000|0x001000
> +gUefiCpuPkgTokenSpaceGuid.PcdSevEsWorkAreaBase|gUefiCpuPkgTokenSpa
> ceGui
> +d.PcdSevEsWorkAreaSize
> +
>  0x010000|0x010000
> 
> gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPeiTempRamBase|gUefiOvmfPkgTo
> kenSpaceGuid.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"
> --
> 2.17.1