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

["Dov Murik" <dovmurik@linux.ibm.com>]

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@redhat.com>
>>
>> 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