Topics

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


Brijesh Singh
 

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|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"
--
2.17.1


Min Xu
 

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


Laszlo Ersek
 

On 04/06/21 10:11, Xu, Min M wrote:
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.
That's a problem. These info blocks are placed in the reset vector
because then they can be found by QEMU easily -- they are not
compressed, and they appear at a known location in the guest physical
address space. (More precisely, a GUID-ed structure chain starts at a
known location, and then QEMU can traverse the chain of structures, for
learning various bits of information about the firmware.)

Do we absolutely need a short jump?

Thanks
Laszlo


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


Min Xu
 

Hi, Laszlo

For Intel TDX supported guest, all processors start in 32-bit protected
mode, while for Non-Td guest, it starts in 16-bit real mode. To make the
ResetVector work on both Td-guest and Non-Td guest, ResetVector are
updated as below:
------------------------------------------------------------------
ALIGN 16
resetVector:
;
; Reset Vector
;
; This is where the processor will begin execution
;
nop
nop
smsw ax
test al, 1
jnz EarlyBspPmEntry
jmp EarlyBspInitReal16

ALIGN 16
fourGigabytes:
------------------------------------------------------------------

For Non-Td guest, jmp to EarlyBspInitReal16 in 16-bit real mode is ok.

For Td-guest, first jmp to EarlyBspPmEntry in 32-bit protected mode, then
in EarlyBspPmEntry jmp to MainTd which is the the main entry for Td-guest.
This requires the distance between ResetVector and EarlyBspPmEntry less
than 128 bytes.

Intel TDX also has metadata which is consumed by QEMU. We put the metadata
in a single file (TdxMetadata.asm) and put it at the end of ResetVectorVtf0.
Then a pointer is placed in a known location in ResetVector.nasm. In this way
QEMU can easily read the Metadata by the pointer.
------------------------------------------------------------------
ALIGN 8
;
; TDX Virtual Firmware injects metadata in VTF0.
; The address of the metadata is injected in this location (0xffffffe8)
;
DD (OVMF_IMAGE_SIZE_IN_KB * 1024 - (fourGigabytes - TdxMetadataGuid - 16))
;
; The VTF signature
;
; VTF-0 means that the VTF (Volume Top File) code does not require
; any fixups.
;
vtfSignature:
DB 'V', 'T', 'F', 0
------------------------------------------------------------------

The space in ResetVector is very precious and we all want a known location so that QEMU
can find the metadata easily. Putting the metadata in a single file give the developers
more flexible (They can put anything they want). So I think a pointer (point to a metadata
file) in a known location maybe a better solution.

Thanks!

-----Original Message-----
From: Laszlo Ersek <lersek@redhat.com>
Sent: Tuesday, April 6, 2021 8:17 PM
To: Xu, Min M <min.m.xu@intel.com>; Brijesh Singh
<brijesh.singh@amd.com>; devel@edk2.groups.io
Cc: James Bottomley <jejb@linux.ibm.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>
Subject: Re: [RFC PATCH 01/19] OvmfPkg: Reserve the Secrets and Cpuid page
for the SEV-SNP guest

On 04/06/21 10:11, Xu, Min M wrote:
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.

That's a problem. These info blocks are placed in the reset vector because
then they can be found by QEMU easily -- they are not compressed, and they
appear at a known location in the guest physical address space. (More
precisely, a GUID-ed structure chain starts at a known location, and then
QEMU can traverse the chain of structures, for learning various bits of
information about the firmware.)

Do we absolutely need a short jump?

Thanks
Laszlo


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|BOOLE
AN
|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|gUefiOvmfPkgToken
S
paceGu
+id.PcdOvmfSnpCpuidSize
+
+0x001000|0x001000
+gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSnpSecretsBase|gUefiOvmfPkgToke
n
Space
+Guid.PcdOvmfSnpSecretsSize
+
+0x002000|0x006000

gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPageTablesBase|gUefiOvmfPkgTo
k
enSpaceGuid.PcdOvmfSecPageTablesSize

-0x006000|0x001000
+0x008000|0x001000

gUefiOvmfPkgTokenSpaceGuid.PcdOvmfLockBoxStorageBase|gUefiOvmfPkgT
o
kenSpaceGuid.PcdOvmfLockBoxStorageSize

-0x007000|0x001000
+0x009000|0x001000

gEfiMdePkgTokenSpaceGuid.PcdGuidedExtractHandlerTableAddress|gUefiOv
m
fPkgTokenSpaceGuid.PcdGuidedExtractHandlerTableSize

-0x008000|0x001000
+0x00A000|0x001000

gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbPageTableBase|gUefiOvmfP
kg
TokenSpaceGuid.PcdOvmfSecGhcbPageTableSize

-0x009000|0x002000
+0x00B000|0x002000

gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbBase|gUefiOvmfPkgTokenSp
a
ceGuid.PcdOvmfSecGhcbSize

-0x00B000|0x001000
-
gUefiCpuPkgTokenSpaceGuid.PcdSevEsWorkAreaBase|gUefiCpuPkgTokenSpa
c
eGuid.PcdSevEsWorkAreaSize
-
-0x00C000|0x001000
+0x00D000|0x001000

gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbBackupBase|gUefiOvmfPkgT
o
kenSpaceGuid.PcdOvmfSecGhcbBackupSize

+0x00F000|0x001000
+gUefiCpuPkgTokenSpaceGuid.PcdSevEsWorkAreaBase|gUefiCpuPkgTokenSp
a
ceGui
+d.PcdSevEsWorkAreaSize
+
0x010000|0x010000

gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPeiTempRamBase|gUefiOvmfPkg
To
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


James Bottomley
 

On Tue, 2021-04-06 at 14:16 +0200, Laszlo Ersek wrote:
On 04/06/21 10:11, Xu, Min M wrote:
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.
Why? I think the short jump can cover 32k as an offset.

That's a problem. These info blocks are placed in the reset vector
because then they can be found by QEMU easily -- they are not
compressed, and they appear at a known location in the guest physical
address space. (More precisely, a GUID-ed structure chain starts at a
known location, and then QEMU can traverse the chain of structures,
for learning various bits of information about the firmware.)

Do we absolutely need a short jump?
Yes, I explained this before:

https://listman.redhat.com/archives/edk2-devel-archive/2020-November/msg01063.html

Sorry, it's buried in the middle about why we can only have 32k worth
of entries. However, the restriction is 32k not 128 bytes unless Intel
is doing something strange.

James


James Bottomley
 

On Wed, 2021-04-07 at 00:21 +0000, Xu, Min M wrote:
Hi, Laszlo

For Intel TDX supported guest, all processors start in 32-bit
protected
mode, while for Non-Td guest, it starts in 16-bit real mode. To make
the
ResetVector work on both Td-guest and Non-Td guest, ResetVector are
updated as below:
------------------------------------------------------------------
ALIGN 16
resetVector:
;
; Reset Vector
;
; This is where the processor will begin execution
;
nop
nop
smsw ax
test al, 1
jnz EarlyBspPmEntry
jmp EarlyBspInitReal16
Well, then use the rel8 jump like the compiler would in this situation:

smsw ax
test al, 1
jz 1f
jmp EarlyBspPmEntry
1:
jmp EarlyBspInitReal16

So now both entries can be 32k away.

James


Laszlo Ersek
 

On 04/07/21 02:21, Xu, Min M wrote:

Intel TDX also has metadata which is consumed by QEMU. We put the metadata
in a single file (TdxMetadata.asm) and put it at the end of ResetVectorVtf0.
Then a pointer is placed in a known location in ResetVector.nasm. In this way
QEMU can easily read the Metadata by the pointer.
------------------------------------------------------------------
ALIGN 8
;
; TDX Virtual Firmware injects metadata in VTF0.
; The address of the metadata is injected in this location (0xffffffe8)
;
DD (OVMF_IMAGE_SIZE_IN_KB * 1024 - (fourGigabytes - TdxMetadataGuid - 16))
;
; The VTF signature
;
; VTF-0 means that the VTF (Volume Top File) code does not require
; any fixups.
;
vtfSignature:
DB 'V', 'T', 'F', 0
------------------------------------------------------------------

The space in ResetVector is very precious and we all want a known location so that QEMU
can find the metadata easily. Putting the metadata in a single file give the developers
more flexible (They can put anything they want). So I think a pointer (point to a metadata
file) in a known location maybe a better solution.
Assuming a QEMU version has been released that looks for the chain of
GUID-ed structs already, then I think such a change would break
compatibility with that QEMU version.

If we definitely need a separate spot to include more information in the
flash, for QEMU's parsing, then please introduce a new GUIDed structure,
which contains nothing but a pointer to that spot.

Thanks
Laszlo


Laszlo Ersek
 

On 04/07/21 15:22, Laszlo Ersek wrote:
On 04/07/21 02:21, Xu, Min M wrote:

Intel TDX also has metadata which is consumed by QEMU. We put the metadata
in a single file (TdxMetadata.asm) and put it at the end of ResetVectorVtf0.
Then a pointer is placed in a known location in ResetVector.nasm. In this way
QEMU can easily read the Metadata by the pointer.
------------------------------------------------------------------
ALIGN 8
;
; TDX Virtual Firmware injects metadata in VTF0.
; The address of the metadata is injected in this location (0xffffffe8)
;
DD (OVMF_IMAGE_SIZE_IN_KB * 1024 - (fourGigabytes - TdxMetadataGuid - 16))
;
; The VTF signature
;
; VTF-0 means that the VTF (Volume Top File) code does not require
; any fixups.
;
vtfSignature:
DB 'V', 'T', 'F', 0
------------------------------------------------------------------

The space in ResetVector is very precious and we all want a known location so that QEMU
can find the metadata easily. Putting the metadata in a single file give the developers
more flexible (They can put anything they want). So I think a pointer (point to a metadata
file) in a known location maybe a better solution.
Assuming a QEMU version has been released that looks for the chain of
GUID-ed structs already, then I think such a change would break
compatibility with that QEMU version.

If we definitely need a separate spot to include more information in the
flash, for QEMU's parsing, then please introduce a new GUIDed structure,
which contains nothing but a pointer to that spot.
Ah, upon re-reading James's earlier message which he just referenced,
namely at

https://listman.redhat.com/archives/edk2-devel-archive/2020-November/msg01063.html

it's clear that James had already proposed this:

That's not to say we can't have a larger table ... we definitely can,
but it can't be where it is now. we'd have to do something different
like make the table guid describe where the actual table is located
(as a 32 bit offset and length) so as not to break up the 16 bit jump
code.
Thanks
Laszlo


Laszlo Ersek
 

On 04/07/21 02:44, James Bottomley wrote:
On Wed, 2021-04-07 at 00:21 +0000, Xu, Min M wrote:
Hi, Laszlo

For Intel TDX supported guest, all processors start in 32-bit
protected
mode, while for Non-Td guest, it starts in 16-bit real mode. To make
the
ResetVector work on both Td-guest and Non-Td guest, ResetVector are
updated as below:
------------------------------------------------------------------
ALIGN 16
resetVector:
;
; Reset Vector
;
; This is where the processor will begin execution
;
nop
nop
smsw ax
test al, 1
jnz EarlyBspPmEntry
jmp EarlyBspInitReal16
Well, then use the rel8 jump like the compiler would in this situation:

smsw ax
test al, 1
jz 1f
jmp EarlyBspPmEntry
1:
jmp EarlyBspInitReal16

So now both entries can be 32k away.
The problem is that we need NASM to generate such *shared* entry code
that behaves correctly when executed in either 16-bit or 32-bit mode.

The rel8 near jumps ("short jumps") are like that -- for example, the
"74 cb" opcode decodes to the same "JZ rel8" in both modes.

But the rel16 ("non-short") near jumps turn into rel32 near jumps when
decoded in 32-bit mode. For example, "E9 cw" decodes to "JMP rel16" in
16-bit mode, but it gets parsed as "E9 cd" (= "JMP rel32") in 32-bit mode.

So the idea is to add more BITS directives, for covering the non-short
near jumps themselves:

; instructions up to and including the rel8 JZ decode identically
; between BITS 16 and BITS 32
BITS 16
smsw ax
test al, 1
jz Real

; the unconditional near jumps are mode-specific
BITS 32
jmp near EarlyBspPmEntry
BITS 16
Real:
jmp near EarlyBspInitReal16

; --------------------

BITS 16
EarlyBspInitReal16:
nop

BITS 32
EarlyBspPmEntry:
nop
$ nasm -f bin jz.nasmb

Decoded (executed) in 16-bit mode:

$ ndisasm -b 16 -k 7,5 -k 0x10,1 jz
00000000 0F01E0 smsw ax
00000003 A801 test al,0x1
00000005 7405 jz 0xc ; taken
00000007 skipping 0x5 bytes
0000000C E90000 jmp word 0xf
0000000F 90 nop
00000010 skipping 0x1 bytes

Decoded (executed) in 32-bit mode:

$ ndisasm -b 32 -k 0xc,4 jz
00000000 0F01E0 smsw eax
00000003 A801 test al,0x1
00000005 7405 jz 0xc ; not taken
00000007 E904000000 jmp dword 0x10
0000000C skipping 0x4 bytes
00000010 90 nop


With the garbage *not* hidden:

$ ndisasm -b 16 -s 0xc jz

00000000 0F01E0 smsw ax
00000003 A801 test al,0x1
00000005 7405 jz 0xc ; taken
00000007 E90400 jmp word 0xe ; garbage
0000000A 0000 add [bx+si],al ; garbage
0000000C E90000 jmp word 0xf
0000000F 90 nop
00000010 90 nop ; garbage

$ ndisasm -b 32 -s 0x10 jz

00000000 0F01E0 smsw eax
00000003 A801 test al,0x1
00000005 7405 jz 0xc ; not taken
00000007 E904000000 jmp dword 0x10
0000000C E9 db 0xe9 ; garbage
0000000D 0000 add [eax],al ; garbage
0000000F 90 nop ; garbage
00000010 90 nop

Thanks
Laszlo


James Bottomley
 

On Wed, 2021-04-07 at 17:02 +0200, Laszlo Ersek wrote:
On 04/07/21 02:44, James Bottomley wrote:
On Wed, 2021-04-07 at 00:21 +0000, Xu, Min M wrote:
Hi, Laszlo

For Intel TDX supported guest, all processors start in 32-bit
protected mode, while for Non-Td guest, it starts in 16-bit real
mode. To make the ResetVector work on both Td-guest and Non-Td
guest, ResetVector are updated as below:
---------------------------------------------------------------
---
ALIGN 16
resetVector:
;
; Reset Vector
;
; This is where the processor will begin execution
;
nop
nop
smsw ax
test al, 1
jnz EarlyBspPmEntry
jmp EarlyBspInitReal16
Well, then use the rel8 jump like the compiler would in this
situation:

smsw ax
test al, 1
jz 1f
jmp EarlyBspPmEntry
1:
jmp EarlyBspInitReal16

So now both entries can be 32k away.
The problem is that we need NASM to generate such *shared* entry code
that behaves correctly when executed in either 16-bit or 32-bit mode.

The rel8 near jumps ("short jumps") are like that -- for example, the
"74 cb" opcode decodes to the same "JZ rel8" in both modes.

But the rel16 ("non-short") near jumps turn into rel32 near jumps
when decoded in 32-bit mode. For example, "E9 cw" decodes to "JMP
rel16" in 16-bit mode, but it gets parsed as "E9 cd" (= "JMP rel32")
in 32-bit mode.

So the idea is to add more BITS directives, for covering the non-
short near jumps themselves:
Absolutely ... sorry, I should have said this was just the first thing
I thought of. The key point is don't do a rel8 jump over the guid
table, use the rel8 jump within the reset vector to sort out the final
destination and use the wider jumps to go over the guid table. As you
say, we have to be very careful about the wider jumps given the
differences between the entry modes.

James


Min Xu
 

On April 7, 2021 9:23 PM, Laszlo wrote:

On 04/07/21 02:21, Xu, Min M wrote:

Intel TDX also has metadata which is consumed by QEMU. We put the
metadata in a single file (TdxMetadata.asm) and put it at the end of
ResetVectorVtf0.
Then a pointer is placed in a known location in ResetVector.nasm. In
this way QEMU can easily read the Metadata by the pointer.
------------------------------------------------------------------
ALIGN 8
;
; TDX Virtual Firmware injects metadata in VTF0.
; The address of the metadata is injected in this location
(0xffffffe8) ;
DD (OVMF_IMAGE_SIZE_IN_KB * 1024 - (fourGigabytes -
TdxMetadataGuid - 16))
;
; The VTF signature
;
; VTF-0 means that the VTF (Volume Top File) code does not require ;
any fixups.
;
vtfSignature:
DB 'V', 'T', 'F', 0
------------------------------------------------------------------

The space in ResetVector is very precious and we all want a known
location so that QEMU can find the metadata easily. Putting the
metadata in a single file give the developers more flexible (They can
put anything they want). So I think a pointer (point to a metadata
file) in a known location maybe a better solution.
Assuming a QEMU version has been released that looks for the chain of GUID-
ed structs already, then I think such a change would break compatibility with
that QEMU version.
Agree. The existing GUIDed structs should be kept. Otherwise the QEMU version
Which has been released would be broken.


If we definitely need a separate spot to include more information in the flash,
for QEMU's parsing, then please introduce a new GUIDed structure, which
contains nothing but a pointer to that spot.
I suggest if new information is to be added in the flash in the future, then we'd
better pack the information in a separate spot and place a pointer to that spot
in a known location. Intel TDX has metadata too. I believe AMD SEV will add
more information in the future. Even ARM may has its metadata. So putting the
metadata in separate spot is more friendly and flexible.

Since it is a pointer in a known location, for example, 0xffffffe0, then do we still
need a GUIDed structure?

Thanks
Laszlo


Min Xu
 

On Wednesday, April 7, 2021 11:03 PM, Laszlo wrote:
On 04/07/21 02:44, James Bottomley wrote:
On Wed, 2021-04-07 at 00:21 +0000, Xu, Min M wrote:
Hi, Laszlo

For Intel TDX supported guest, all processors start in 32-bit
protected mode, while for Non-Td guest, it starts in 16-bit real
mode. To make the ResetVector work on both Td-guest and Non-Td guest,
ResetVector are updated as below:
------------------------------------------------------------------
ALIGN 16
resetVector:
;
; Reset Vector
;
; This is where the processor will begin execution
;
nop
nop
smsw ax
test al, 1
jnz EarlyBspPmEntry
jmp EarlyBspInitReal16
Well, then use the rel8 jump like the compiler would in this situation:

smsw ax
test al, 1
jz 1f
jmp EarlyBspPmEntry
1:
jmp EarlyBspInitReal16

So now both entries can be 32k away.
The problem is that we need NASM to generate such *shared* entry code that
behaves correctly when executed in either 16-bit or 32-bit mode.

The rel8 near jumps ("short jumps") are like that -- for example, the
"74 cb" opcode decodes to the same "JZ rel8" in both modes.

But the rel16 ("non-short") near jumps turn into rel32 near jumps when
decoded in 32-bit mode. For example, "E9 cw" decodes to "JMP rel16" in 16-bit
mode, but it gets parsed as "E9 cd" (= "JMP rel32") in 32-bit mode.

So the idea is to add more BITS directives, for covering the non-short near
jumps themselves:
Yes this is the root cause. TDX requires the startup mode to be 32-bit
protected mode while the legacy VM startup mode is 16-bit real mode.
Add more BITS directives can work round this. I have tried it and it works.

So my initial solution is to use *jmp rel8* because it works in both 16-bit
and 32-bit mode. But *jmp rel8* depends on the distance which should
be less than 128 bytes. If more metadata is added in the ResetVector.asm
then we have to use the BITS solution.


; instructions up to and including the rel8 JZ decode identically ;
between BITS 16 and BITS 32 BITS 16
smsw ax
test al, 1
jz Real

; the unconditional near jumps are mode-specific BITS 32
jmp near EarlyBspPmEntry
BITS 16
Real:
jmp near EarlyBspInitReal16

; --------------------

BITS 16
EarlyBspInitReal16:
nop

BITS 32
EarlyBspPmEntry:
nop
$ nasm -f bin jz.nasmb

Decoded (executed) in 16-bit mode:

$ ndisasm -b 16 -k 7,5 -k 0x10,1 jz
00000000 0F01E0 smsw ax
00000003 A801 test al,0x1
00000005 7405 jz 0xc ; taken
00000007 skipping 0x5 bytes
0000000C E90000 jmp word 0xf
0000000F 90 nop
00000010 skipping 0x1 bytes

Decoded (executed) in 32-bit mode:

$ ndisasm -b 32 -k 0xc,4 jz
00000000 0F01E0 smsw eax
00000003 A801 test al,0x1
00000005 7405 jz 0xc ; not taken
00000007 E904000000 jmp dword 0x10
0000000C skipping 0x4 bytes
00000010 90 nop


With the garbage *not* hidden:

$ ndisasm -b 16 -s 0xc jz

00000000 0F01E0 smsw ax
00000003 A801 test al,0x1
00000005 7405 jz 0xc ; taken
00000007 E90400 jmp word 0xe ; garbage
0000000A 0000 add [bx+si],al ; garbage
0000000C E90000 jmp word 0xf
0000000F 90 nop
00000010 90 nop ; garbage

$ ndisasm -b 32 -s 0x10 jz

00000000 0F01E0 smsw eax
00000003 A801 test al,0x1
00000005 7405 jz 0xc ; not taken
00000007 E904000000 jmp dword 0x10
0000000C E9 db 0xe9 ; garbage
0000000D 0000 add [eax],al ; garbage
0000000F 90 nop ; garbage
00000010 90 nop

Thanks
Laszlo





Lendacky, Thomas
 

On 4/8/21 1:24 AM, Xu, Min M wrote:
On Wednesday, April 7, 2021 11:03 PM, Laszlo wrote:
On 04/07/21 02:44, James Bottomley wrote:
On Wed, 2021-04-07 at 00:21 +0000, Xu, Min M wrote:
Hi, Laszlo

For Intel TDX supported guest, all processors start in 32-bit
protected mode, while for Non-Td guest, it starts in 16-bit real
mode. To make the ResetVector work on both Td-guest and Non-Td guest,
ResetVector are updated as below:
------------------------------------------------------------------
ALIGN 16
resetVector:
;
; Reset Vector
;
; This is where the processor will begin execution
;
nop
nop
smsw ax
test al, 1
jnz EarlyBspPmEntry
jmp EarlyBspInitReal16
Well, then use the rel8 jump like the compiler would in this situation:

smsw ax
test al, 1
jz 1f
jmp EarlyBspPmEntry
1:
jmp EarlyBspInitReal16

So now both entries can be 32k away.
The problem is that we need NASM to generate such *shared* entry code that
behaves correctly when executed in either 16-bit or 32-bit mode.

The rel8 near jumps ("short jumps") are like that -- for example, the
"74 cb" opcode decodes to the same "JZ rel8" in both modes.

But the rel16 ("non-short") near jumps turn into rel32 near jumps when
decoded in 32-bit mode. For example, "E9 cw" decodes to "JMP rel16" in 16-bit
mode, but it gets parsed as "E9 cd" (= "JMP rel32") in 32-bit mode.

So the idea is to add more BITS directives, for covering the non-short near
jumps themselves:
Yes this is the root cause. TDX requires the startup mode to be 32-bit
protected mode while the legacy VM startup mode is 16-bit real mode.
Add more BITS directives can work round this. I have tried it and it works.

So my initial solution is to use *jmp rel8* because it works in both 16-bit
and 32-bit mode. But *jmp rel8* depends on the distance which should
be less than 128 bytes. If more metadata is added in the ResetVector.asm
then we have to use the BITS solution.
To me, it sounds like the BITS solution should be the approach you use
from the start.

Thanks,
Tom



; instructions up to and including the rel8 JZ decode identically ;
between BITS 16 and BITS 32 BITS 16
smsw ax
test al, 1
jz Real

; the unconditional near jumps are mode-specific BITS 32
jmp near EarlyBspPmEntry
BITS 16
Real:
jmp near EarlyBspInitReal16

; --------------------

BITS 16
EarlyBspInitReal16:
nop

BITS 32
EarlyBspPmEntry:
nop
$ nasm -f bin jz.nasmb

Decoded (executed) in 16-bit mode:

$ ndisasm -b 16 -k 7,5 -k 0x10,1 jz
00000000 0F01E0 smsw ax
00000003 A801 test al,0x1
00000005 7405 jz 0xc ; taken
00000007 skipping 0x5 bytes
0000000C E90000 jmp word 0xf
0000000F 90 nop
00000010 skipping 0x1 bytes

Decoded (executed) in 32-bit mode:

$ ndisasm -b 32 -k 0xc,4 jz
00000000 0F01E0 smsw eax
00000003 A801 test al,0x1
00000005 7405 jz 0xc ; not taken
00000007 E904000000 jmp dword 0x10
0000000C skipping 0x4 bytes
00000010 90 nop


With the garbage *not* hidden:

$ ndisasm -b 16 -s 0xc jz

00000000 0F01E0 smsw ax
00000003 A801 test al,0x1
00000005 7405 jz 0xc ; taken
00000007 E90400 jmp word 0xe ; garbage
0000000A 0000 add [bx+si],al ; garbage
0000000C E90000 jmp word 0xf
0000000F 90 nop
00000010 90 nop ; garbage

$ ndisasm -b 32 -s 0x10 jz

00000000 0F01E0 smsw eax
00000003 A801 test al,0x1
00000005 7405 jz 0xc ; not taken
00000007 E904000000 jmp dword 0x10
0000000C E9 db 0xe9 ; garbage
0000000D 0000 add [eax],al ; garbage
0000000F 90 nop ; garbage
00000010 90 nop

Thanks
Laszlo





Laszlo Ersek
 

On 04/08/21 15:31, Tom Lendacky wrote:
On 4/8/21 1:24 AM, Xu, Min M wrote:
On Wednesday, April 7, 2021 11:03 PM, Laszlo wrote:
On 04/07/21 02:44, James Bottomley wrote:
On Wed, 2021-04-07 at 00:21 +0000, Xu, Min M wrote:
Hi, Laszlo

For Intel TDX supported guest, all processors start in 32-bit
protected mode, while for Non-Td guest, it starts in 16-bit real
mode. To make the ResetVector work on both Td-guest and Non-Td guest,
ResetVector are updated as below:
------------------------------------------------------------------
ALIGN 16
resetVector:
;
; Reset Vector
;
; This is where the processor will begin execution
;
nop
nop
smsw ax
test al, 1
jnz EarlyBspPmEntry
jmp EarlyBspInitReal16
Well, then use the rel8 jump like the compiler would in this situation:

smsw ax
test al, 1
jz 1f
jmp EarlyBspPmEntry
1:
jmp EarlyBspInitReal16

So now both entries can be 32k away.
The problem is that we need NASM to generate such *shared* entry code that
behaves correctly when executed in either 16-bit or 32-bit mode.

The rel8 near jumps ("short jumps") are like that -- for example, the
"74 cb" opcode decodes to the same "JZ rel8" in both modes.

But the rel16 ("non-short") near jumps turn into rel32 near jumps when
decoded in 32-bit mode. For example, "E9 cw" decodes to "JMP rel16" in 16-bit
mode, but it gets parsed as "E9 cd" (= "JMP rel32") in 32-bit mode.

So the idea is to add more BITS directives, for covering the non-short near
jumps themselves:
Yes this is the root cause. TDX requires the startup mode to be 32-bit
protected mode while the legacy VM startup mode is 16-bit real mode.
Add more BITS directives can work round this. I have tried it and it works.

So my initial solution is to use *jmp rel8* because it works in both 16-bit
and 32-bit mode. But *jmp rel8* depends on the distance which should
be less than 128 bytes. If more metadata is added in the ResetVector.asm
then we have to use the BITS solution.
To me, it sounds like the BITS solution should be the approach you use
from the start.
I agree; we have an extensible approach in place already (with the
GUIDed struct list), and QEMU already knows about one magic GPA (for
locating the head of the list). Using one conditional rel8 jump, and two
non-conditional mode-specific (rel16/rel32) jumps, doesn't cost much in
the assembly code, it's compatible with future GUID additions, and
shouldn't affect QEMU's GUIDed struct list traversal.

Thanks
Laszlo


Laszlo Ersek
 

Hi Min,

On 04/08/21 15:31, Lendacky, Thomas wrote:
On 4/8/21 1:24 AM, Xu, Min M wrote:
Yes this is the root cause. TDX requires the startup mode to be 32-bit
protected mode while the legacy VM startup mode is 16-bit real mode.
Add more BITS directives can work round this. I have tried it and it works.

So my initial solution is to use *jmp rel8* because it works in both 16-bit
and 32-bit mode. But *jmp rel8* depends on the distance which should
be less than 128 bytes. If more metadata is added in the ResetVector.asm
then we have to use the BITS solution.
To me, it sounds like the BITS solution should be the approach you use
from the start.
BTW, have you considered using a separate ResetVector module for TDX?
That would obviate this multi-mode trickery. (Most recently raised by
Paolo.)

I think TDX will need a separate platform DSC / FDF / fw binary anyway.
I realize that's not a done deal yet; it may depend on who provides the
firmware binary (guest owner or cloud owner) -- of course the guest
owner will have to perform the attestation in either case, but the
"provenance" question remains open, IIUC.

And even if TDX gets its own firmware platform, it's a separate question
whether the ResetVector module itself should be split. I'm inclined to
think a separation at that level would make development and maintenance
easier.

(FWIW, Xen PVH has its own reset vector module, in OvmfPkg/XenResetVector.)

Thanks
Laszlo


Yao, Jiewen
 

Hi Laszlo
Thanks.

We did provide a separate binary in the beginning - see https://github.com/tianocore/edk2-staging/tree/TDVF, with same goal - easy to maintain and develop. A clean solution, definitely.

However, we got requirement to deliver one binary solution together with 1) normal OVMF, 2) AMD-SEV, 3) Intel-TDX.
Now, we are struggling to merge them......

For DXE, we hope to isolate TDX driver whenever it is possible.
But we only have one reset vector here. Sigh...

-----Original Message-----
From: Laszlo Ersek <lersek@redhat.com>
Sent: Friday, April 9, 2021 9:33 PM
To: Xu, Min M <min.m.xu@intel.com>
Cc: devel@edk2.groups.io; thomas.lendacky@amd.com; jejb@linux.ibm.com;
Brijesh Singh <brijesh.singh@amd.com>; Yao, Jiewen <jiewen.yao@intel.com>;
Justen, Jordan L <jordan.l.justen@intel.com>; Ard Biesheuvel
<ardb+tianocore@kernel.org>; Paolo Bonzini <pbonzini@redhat.com>
Subject: Re: [edk2-devel] [RFC PATCH 01/19] OvmfPkg: Reserve the Secrets and
Cpuid page for the SEV-SNP guest

Hi Min,

On 04/08/21 15:31, Lendacky, Thomas wrote:
On 4/8/21 1:24 AM, Xu, Min M wrote:
Yes this is the root cause. TDX requires the startup mode to be 32-bit
protected mode while the legacy VM startup mode is 16-bit real mode.
Add more BITS directives can work round this. I have tried it and it works.

So my initial solution is to use *jmp rel8* because it works in both 16-bit
and 32-bit mode. But *jmp rel8* depends on the distance which should
be less than 128 bytes. If more metadata is added in the ResetVector.asm
then we have to use the BITS solution.
To me, it sounds like the BITS solution should be the approach you use
from the start.
BTW, have you considered using a separate ResetVector module for TDX?
That would obviate this multi-mode trickery. (Most recently raised by
Paolo.)

I think TDX will need a separate platform DSC / FDF / fw binary anyway.
I realize that's not a done deal yet; it may depend on who provides the
firmware binary (guest owner or cloud owner) -- of course the guest
owner will have to perform the attestation in either case, but the
"provenance" question remains open, IIUC.

And even if TDX gets its own firmware platform, it's a separate question
whether the ResetVector module itself should be split. I'm inclined to
think a separation at that level would make development and maintenance
easier.

(FWIW, Xen PVH has its own reset vector module, in OvmfPkg/XenResetVector.)

Thanks
Laszlo


Brijesh Singh
 

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.

Thoughts ?

-Brijesh

On 3/24/21 10:31 AM, Brijesh Singh wrote:
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|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"


Laszlo Ersek
 

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?

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://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|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"


Brijesh Singh
 

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&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"


Laszlo Ersek
 

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&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"