[PATCH V1 1/1] OvmfPkg/IntelTdxX64: Raise DXEFV size to 13MB

[PATCH V1 1/1] OvmfPkg/IntelTdxX64: Raise DXEFV size to 13MB

[Min Xu]

From: Min M Xu <min.m.xu@intel.com>

BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=4236

Similarly to the "cadence" mentioned in commit d272449 ("OvmfPkg:
raise DXEFV size to 11 MB", 2018-05-29), it's been ~1.75 years since
commit 5e75c4d ("OvmfPkg: raise DXEFV size to 12 MB", 2020-03-11),
and we've outgrown DXEFV again (with NOOPT builds).  Increase the DXEFV
size of IntelTdxX64 to 13MB now.

Tested on:
 - X64, Intel's TDX platform, RHEL-8.6 guest

Test steps:
 - Configure 4 vCPUs and 4G memory
 - boot and bring up the RHEL 8.6 guest

Cc: Anthony Perard <anthony.perard@citrix.com>
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Brijesh Singh <brijesh.singh@amd.com>
Cc: Erdem Aktas <erdemaktas@google.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: James Bottomley <jejb@linux.ibm.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Julien Grall <julien@xen.org>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Peter Grehan <grehan@freebsd.org>
Cc: Rebecca Cran <rebecca@bsdio.com>
Cc: Sebastien Boeuf <sebastien.boeuf@intel.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Signed-off-by: Min Xu <min.m.xu@intel.com>
---
 OvmfPkg/IntelTdx/IntelTdxX64.fdf | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/OvmfPkg/IntelTdx/IntelTdxX64.fdf b/OvmfPkg/IntelTdx/IntelTdxX64.fdf
index e79ad3e10217..0bb70ee2b7c3 100644
--- a/OvmfPkg/IntelTdx/IntelTdxX64.fdf
+++ b/OvmfPkg/IntelTdx/IntelTdxX64.fdf
@@ -62,10 +62,10 @@ FV = SECFV
 
 [FD.MEMFD]
 BaseAddress   = $(MEMFD_BASE_ADDRESS)
-Size          = 0xD00000
+Size          = 0xE00000
 ErasePolarity = 1
 BlockSize     = 0x10000
-NumBlocks     = 0xD0
+NumBlocks     = 0xE0
 
 0x000000|0x006000
 gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPageTablesBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPageTablesSize
@@ -97,7 +97,7 @@ gUefiOvmfPkgTokenSpaceGuid.PcdOvmfCpuidBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfCp
 0x010000|0x010000
 gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPeiTempRamBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPeiTempRamSize
 
-0x100000|0xC00000
+0x100000|0xD00000
 gUefiOvmfPkgTokenSpaceGuid.PcdOvmfDxeMemFvBase|gUefiOvmfPkgTokenSpaceGuid.PcdOvmfDxeMemFvSize
 FV = DXEFV
 
-- 
2.29.2.windows.2

Re: [PATCH V1 1/1] OvmfPkg/IntelTdxX64: Raise DXEFV size to 13MB

[Gerd Hoffmann]

On Thu, Jan 05, 2023 at 03:21:05PM +0800, Min Xu wrote:
> From: Min M Xu <min.m.xu@intel.com>
> 
> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=4236
> 
> Similarly to the "cadence" mentioned in commit d272449 ("OvmfPkg:
> raise DXEFV size to 11 MB", 2018-05-29), it's been ~1.75 years since
> commit 5e75c4d ("OvmfPkg: raise DXEFV size to 12 MB", 2020-03-11),
> and we've outgrown DXEFV again (with NOOPT builds).  Increase the DXEFV
> size of IntelTdxX64 to 13MB now.

Not sure this makes sense given that you want split DXEFV into two
smaller volumes for IntelTdx (patch series sent in december IIRC).

Also we maybe should move the MEMFD section to a Include/Fdf snippet
to reduce code duplication and make it easier to keep things in sync?

take care,
  Gerd

Re: [PATCH V1 1/1] OvmfPkg/IntelTdxX64: Raise DXEFV size to 13MB

[Laszlo Ersek]

On 1/5/23 12:31, Gerd Hoffmann wrote:
> On Thu, Jan 05, 2023 at 03:21:05PM +0800, Min Xu wrote:
>> From: Min M Xu <min.m.xu@intel.com>
>>
>> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=4236
>>
>> Similarly to the "cadence" mentioned in commit d272449 ("OvmfPkg:
>> raise DXEFV size to 11 MB", 2018-05-29), it's been ~1.75 years since
>> commit 5e75c4d ("OvmfPkg: raise DXEFV size to 12 MB", 2020-03-11),
>> and we've outgrown DXEFV again (with NOOPT builds).  Increase the DXEFV
>> size of IntelTdxX64 to 13MB now.
> 
> Not sure this makes sense given that you want split DXEFV into two
> smaller volumes for IntelTdx (patch series sent in december IIRC).
> 
> Also we maybe should move the MEMFD section to a Include/Fdf snippet
> to reduce code duplication and make it easier to keep things in sync?

sigh, I followed up on this, but mistakenly, I only sent it to Gerd.
Reposting to the list as well:

... the only comment I have here is for this lasst paragraph.

The problem with centralized code is the same as the advantage of
centralized code. You modify it once and it affects everything. But for
example I cannot *test* everything. Code duplication in edk2 has helped
in the past with separating responsibilities.

One example has been Xen. Originally, in OvmfPkg, Xen support was
integrated into a bunch of other modules, and there was no dedicated Xen
platform. When Xen got introduced to ArmVirtPkg, it got its own
dedicated platform, and it showed us how much easier maintenance would
be that way. Xen-based developers and QEMU/KVM-based developers
virtually never cross-tested (nobody has the "other" setup, or the time
to test it). So Xen-oriented patches in OvmfPkg kept breaking QEMU/KVM
functionality, and of course vice versa; whereas in ArmVirtPkg, no such
cross-interference was seen. After a while we separated Xen to a new
platform under OvmfPkg too (TianoCore#1689, TianoCore#2122), and there
was much consequent rejoicing. Jordan slightly disagreed with the split,
and I agreed with him from a purity sense, but from a practical
maintenance POV, the joint Xen<->QEMU/KVM codebase had been a nightmare.

This was one of my main driving principles during the initial
discussions about TDX. I think extracting further commonalities between
the TDX platform(s) and the traditional platforms works against that. It
creates an area where modifications must be tested at the same (for the
same patches) by multiple disparate teams. I know I couldn't do that.
Cloning BZs and posting ported patches helps.

At the same time: if regular maintainers *can* and *are willing* to test
such central changes in *all* affected platforms, then yes,
centralization is absolutely vital, because then it *saves* work. So I
guess it must reflect the community's structure.

Laszlo

Re: [PATCH V1 1/1] OvmfPkg/IntelTdxX64: Raise DXEFV size to 13MB

[Min Xu]

On January 5, 2023 7:32 PM, Gerd Hoffmann wrote:
> 
> On Thu, Jan 05, 2023 at 03:21:05PM +0800, Min Xu wrote:
> > From: Min M Xu <min.m.xu@intel.com>
> >
> > BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=4236
> >
> > Similarly to the "cadence" mentioned in commit d272449 ("OvmfPkg:
> > raise DXEFV size to 11 MB", 2018-05-29), it's been ~1.75 years since
> > commit 5e75c4d ("OvmfPkg: raise DXEFV size to 12 MB", 2020-03-11), and
> > we've outgrown DXEFV again (with NOOPT builds).  Increase the DXEFV
> > size of IntelTdxX64 to 13MB now.
> 
> Not sure this makes sense given that you want split DXEFV into two smaller
> volumes for IntelTdx (patch series sent in december IIRC).
> 
Thanks Gerd for the comments.
I think raising DXEFV size and split DXEFV into 2 FVs are for different purpose and they're not conflicted.

The purpose of raising DXEFV size is to enlarge the FV so that more drivers can be hosted.

The purpose of splitting DXEFV are:
1) Reduce the attack surface in CC guest.
2) Improve boot perf in CC guest.

I think IntelTdxX64 needs both 2 changes.

Thanks
Min

Re: [PATCH V1 1/1] OvmfPkg/IntelTdxX64: Raise DXEFV size to 13MB

[Gerd Hoffmann]

> > Also we maybe should move the MEMFD section to a Include/Fdf snippet
> > to reduce code duplication and make it easier to keep things in sync?
> 
> The problem with centralized code is the same as the advantage of
> centralized code. You modify it once and it affects everything. But for
> example I cannot *test* everything. Code duplication in edk2 has helped
> in the past with separating responsibilities.

Sure, we have to check where sharing makes sense and where not on a
case-by-case base.  Some truely generic bits (like the list of usb
drivers, or crypto/network support) might make sense to share across all
platforms.

Platform-specific bits are a different story.  They would probably be
restricted to the three kvm variants we have (i.e. OvmfPkgIa32,
OvmfPkgIa32X64, OvmfPkgX64).  Possibly AmdSev and IntelTdx too (again
depending on the kind if change).

> This was one of my main driving principles during the initial
> discussions about TDX. I think extracting further commonalities between
> the TDX platform(s) and the traditional platforms works against that.
> It creates an area where modifications must be tested at the same (for the
> same patches) by multiple disparate teams. I know I couldn't do that.
> Cloning BZs and posting ported patches helps.
> 
> At the same time: if regular maintainers *can* and *are willing* to test
> such central changes in *all* affected platforms, then yes,
> centralization is absolutely vital, because then it *saves* work. So I
> guess it must reflect the community's structure.

I think with PlatformInitLib is a nice solution for that.  It allows to
share code instead of cut+pasting stuff.  But the different platforms
still have their own setup code.  They can use all, some or none of
the helper functions from the library and we also don't have a single
code path which must detect and handle all possible cases at runtime.

take care,
  Gerd

Re: [PATCH V1 1/1] OvmfPkg/IntelTdxX64: Raise DXEFV size to 13MB

[Gerd Hoffmann]

On Fri, Jan 06, 2023 at 12:49:21AM +0000, Xu, Min M wrote:
> On January 5, 2023 7:32 PM, Gerd Hoffmann wrote:
> > 
> > On Thu, Jan 05, 2023 at 03:21:05PM +0800, Min Xu wrote:
> > > From: Min M Xu <min.m.xu@intel.com>
> > >
> > > BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=4236
> > >
> > > Similarly to the "cadence" mentioned in commit d272449 ("OvmfPkg:
> > > raise DXEFV size to 11 MB", 2018-05-29), it's been ~1.75 years since
> > > commit 5e75c4d ("OvmfPkg: raise DXEFV size to 12 MB", 2020-03-11), and
> > > we've outgrown DXEFV again (with NOOPT builds).  Increase the DXEFV
> > > size of IntelTdxX64 to 13MB now.
> > 
> > Not sure this makes sense given that you want split DXEFV into two smaller
> > volumes for IntelTdx (patch series sent in december IIRC).
> > 
> Thanks Gerd for the comments.
> I think raising DXEFV size and split DXEFV into 2 FVs are for different purpose and they're not conflicted.
> 
> The purpose of raising DXEFV size is to enlarge the FV so that more drivers can be hosted.
> 
> The purpose of splitting DXEFV are:
> 1) Reduce the attack surface in CC guest.
> 2) Improve boot perf in CC guest.

This also makes DXEFV smaller to make room for the new Firmware Volume.
So you change DXEFV size twice in a row.  I would avoid that and just
pick reasonable sizes for both volumes in the split patch series
instead.

take care,
  Gerd