Reorder the pages in the MEMFD section of AmdSevX64.fdf so that it
matches the same order used in OvmfPkgX64.fdf.

After this change, this is the difference in the MEMFD of the two
targets:

$ diff -u \
<(sed -ne '/FD.MEMFD/,/FV.SECFV/p' OvmfPkg/OvmfPkgX64.fdf) \
<(sed -ne '/FD.MEMFD/,/FV.SECFV/p' OvmfPkg/AmdSev/AmdSevX64.fdf)
--- /dev/fd/63 2022-03-28 18:07:59.657531210 +0000
+++ /dev/fd/62 2022-03-28 18:07:59.657531210 +0000
@@ -32,6 +32,12 @@
0x00E000|0x001000
gUefiOvmfPkgTokenSpaceGuid.PcdOvmfCpuidBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfCpuidSize

+0x00F000|0x000C00
+gUefiOvmfPkgTokenSpaceGuid.PcdSevLaunchSecretBase|gUefiOvmfPkgTokenSpaceGuid.PcdSevLaunchSecretSize
+
+0x00FC00|0x000400
+gUefiOvmfPkgTokenSpaceGuid.PcdQemuHashTableBase|gUefiOvmfPkgTokenSpaceGuid.PcdQemuHashTableSize
+
0x010000|0x010000
gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPeiTempRamBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPeiTempRamSize

Cc: Ard Biesheuvel <ardb+tianocore@...>
Cc: Jiewen Yao <jiewen.yao@...>
Cc: Jordan Justen <jordan.l.justen@...>
Cc: Gerd Hoffmann <kraxel@...>
Cc: Brijesh Singh <brijesh.singh@...>
Cc: Erdem Aktas <erdemaktas@...>
Cc: James Bottomley <jejb@...>
Cc: Jiewen Yao <jiewen.yao@...>
Cc: Min Xu <min.m.xu@...>
Cc: Tom Lendacky <thomas.lendacky@...>
Cc: Tobin Feldman-Fitzthum <tobin@...>
Signed-off-by: Dov Murik <dovmurik@...>
---
OvmfPkg/AmdSev/AmdSevX64.fdf | 18 +++++++++---------
1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/OvmfPkg/AmdSev/AmdSevX64.fdf b/OvmfPkg/AmdSev/AmdSevX64.fdf
index 31f2be66361f..208f969cefc9 100644
--- a/OvmfPkg/AmdSev/AmdSevX64.fdf
+++ b/OvmfPkg/AmdSev/AmdSevX64.fdf
@@ -59,21 +59,21 @@ gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmf
0x00B000|0x001000
gUefiOvmfPkgTokenSpaceGuid.PcdOvmfWorkAreaBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfWorkAreaSize

-0x00C000|0x000C00
-gUefiOvmfPkgTokenSpaceGuid.PcdSevLaunchSecretBase|gUefiOvmfPkgTokenSpaceGuid.PcdSevLaunchSecretSize
-
-0x00CC00|0x000400
-gUefiOvmfPkgTokenSpaceGuid.PcdQemuHashTableBase|gUefiOvmfPkgTokenSpaceGuid.PcdQemuHashTableSize
-
-0x00D000|0x001000
+0x00C000|0x001000
gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbBackupBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecGhcbBackupSize

-0x00E000|0x001000
+0x00D000|0x001000
gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSnpSecretsBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSnpSecretsSize

-0x00F000|0x001000
+0x00E000|0x001000
gUefiOvmfPkgTokenSpaceGuid.PcdOvmfCpuidBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfCpuidSize

+0x00F000|0x000C00
+gUefiOvmfPkgTokenSpaceGuid.PcdSevLaunchSecretBase|gUefiOvmfPkgTokenSpaceGuid.PcdSevLaunchSecretSize
+
+0x00FC00|0x000400
+gUefiOvmfPkgTokenSpaceGuid.PcdQemuHashTableBase|gUefiOvmfPkgTokenSpaceGuid.PcdQemuHashTableSize
+
0x010000|0x010000
gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPeiTempRamBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPeiTempRamSize

--
2.20.1

On Mon, Mar 28, 2022 at 06:45:29PM +0000, Dov Murik wrote:

Reorder the pages in the MEMFD section of AmdSevX64.fdf so that it
matches the same order used in OvmfPkgX64.fdf.

Makes sense.

Acked-by: Gerd Hoffmann <kraxel@...>

Maybe also move this to a common include file, so it is less likely that
they run out of sync again?

take care,
Gerd

Thanks Gerd for reviewing.

On 29/03/2022 14:36, Gerd Hoffmann wrote:

On Mon, Mar 28, 2022 at 06:45:29PM +0000, Dov Murik wrote:

Reorder the pages in the MEMFD section of AmdSevX64.fdf so that it
matches the same order used in OvmfPkgX64.fdf.

Makes sense.

Acked-by: Gerd Hoffmann <kraxel@...>

Thanks.

Maybe also move this to a common include file, so it is less likely that
they run out of sync again?

I was contemplating that, but wasn't sure (I only use OvmfPkgX64 and
AmdSevX64 in my testing).

Is it common in edk2?
Would it apply to other OvmfPkg targets? I see similar MEMFD in
CloudHvX64.fdf .

-Dov

On Tue, Mar 29, 2022 at 03:32:36PM +0300, Dov Murik wrote:

Thanks Gerd for reviewing.

On 29/03/2022 14:36, Gerd Hoffmann wrote:

On Mon, Mar 28, 2022 at 06:45:29PM +0000, Dov Murik wrote:

Reorder the pages in the MEMFD section of AmdSevX64.fdf so that it
matches the same order used in OvmfPkgX64.fdf.

Makes sense.

Acked-by: Gerd Hoffmann <kraxel@...>

Thanks.

Maybe also move this to a common include file, so it is less likely that
they run out of sync again?

I was contemplating that, but wasn't sure (I only use OvmfPkgX64 and
AmdSevX64 in my testing).

Is it common in edk2?

We already have a few:

kraxel@sirius ~/projects/edk2 (gcc12)# ls OvmfPkg/*.fdf.inc
OvmfPkg/FvmainCompactScratchEnd.fdf.inc OvmfPkg/OvmfTpmDxe.fdf.inc OvmfPkg/VarStore.fdf.inc
OvmfPkg/OvmfPkgDefines.fdf.inc OvmfPkg/OvmfTpmPei.fdf.inc OvmfPkg/XenElfHeader.fdf.inc

Would it apply to other OvmfPkg targets? I see similar MEMFD in
CloudHvX64.fdf .

I'd create one for the confidential computing memory areas,
that would only affect the builds which support SEV (and soon TDX).

Not sure about CloudHvX64.fdf, as far I know it does not support
SEV/TDX, maybe those antries are only there because they have been
copied over from OvmfPkgX64.fdf

take care,
Gerd

On 30/03/2022 8:14, Gerd Hoffmann wrote:

On Tue, Mar 29, 2022 at 03:32:36PM +0300, Dov Murik wrote:

Thanks Gerd for reviewing.

On 29/03/2022 14:36, Gerd Hoffmann wrote:

On Mon, Mar 28, 2022 at 06:45:29PM +0000, Dov Murik wrote:

Reorder the pages in the MEMFD section of AmdSevX64.fdf so that it
matches the same order used in OvmfPkgX64.fdf.

Makes sense.

Acked-by: Gerd Hoffmann <kraxel@...>

Thanks.

Maybe also move this to a common include file, so it is less likely that
they run out of sync again?

I was contemplating that, but wasn't sure (I only use OvmfPkgX64 and
AmdSevX64 in my testing).

Is it common in edk2?

We already have a few:

kraxel@sirius ~/projects/edk2 (gcc12)# ls OvmfPkg/*.fdf.inc
OvmfPkg/FvmainCompactScratchEnd.fdf.inc OvmfPkg/OvmfTpmDxe.fdf.inc OvmfPkg/VarStore.fdf.inc
OvmfPkg/OvmfPkgDefines.fdf.inc OvmfPkg/OvmfTpmPei.fdf.inc OvmfPkg/XenElfHeader.fdf.inc

I saw these, but saw no !include directives in MEMFD areas, which are
more sensitive because the addresses and sizes must match the
surrounding definitions (unlike a list of INF directive like in
NetworkPkg/Network.fdf.inc or general settings like in
OvmfPkg/OvmfPkgDefines.fdf.inc).

Would it apply to other OvmfPkg targets? I see similar MEMFD in
CloudHvX64.fdf .

I'd create one for the confidential computing memory areas,
that would only affect the builds which support SEV (and soon TDX).

Almost all the MEMFD entries are somehow related to confidential
computing, isn't that the case? For example PcdOvmfWorkAreaBase -- I
see it appears in the *.fdf of almost all targets.

I want to reduce duplication (= extract common parts to an .inc file),
but wonder what would be a clear and safe way to do it.

Suggestions:


Option 1:

Extract all the MEMFD entries starting from:

0x000000|0x006000
gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPageTablesBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPageTablesSize

up to (including):

0x00E000|0x001000
gUefiOvmfPkgTokenSpaceGuid.PcdOvmfCpuidBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfCpuidSize


into OvmfMemFdPart1.fdf.inc, and !include it in OvmfPkgX64 and AmdSevX64.



Option 2:

Extract entire MEMFD part from OvmfPkgX64.fdf into OvmfMemFd.fdf.inc.

In the middle of it add something like:

!if $(SEV_LAUNCH_SECRET_ENABLE) == TRUE
0x00F000|0x000C00
gUefiOvmfPkgTokenSpaceGuid.PcdSevLaunchSecretBase|gUefiOvmfPkgTokenSpaceGuid.PcdSevLaunchSecretSize

0x00FC00|0x000400
gUefiOvmfPkgTokenSpaceGuid.PcdQemuHashTableBase|gUefiOvmfPkgTokenSpaceGuid.PcdQemuHashTableSize
!endif


and set that DEFINE in AmdSevX64.fdf only.

Not sure about CloudHvX64.fdf, as far I know it does not support
SEV/TDX, maybe those antries are only there because they have been
copied over from OvmfPkgX64.fdf

The TDX series ("[PATCH V12 00/47] Enable Intel TDX in OvmfPkg
(Config-A)") modifies CloudHvX64.*, and also the CloudHv/README mentions
TDX. So I assume they intend to support it.


-Dov