Re: [PATCH 1/2] OvmfPkg/AmdSev: Reorder MEMFD pages to match the order in OvmfPkgX64.fdf


Dov Murik
 

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

Join devel@edk2.groups.io to automatically receive all group messages.