Re: [PATCH V3 15/29] OvmfPkg: Update SecEntry.nasm to support Tdx

["Yao, Jiewen" <jiewen.yao@intel.com>]

Comment on config-B.

> -----Original Message-----
> From: Gerd Hoffmann <kraxel@redhat.com>
> Sent: Wednesday, November 17, 2021 11:20 PM
> To: Xu, Min M <min.m.xu@intel.com>
> Cc: devel@edk2.groups.io; Ard Biesheuvel <ardb+tianocore@kernel.org>; Justen,
> Jordan L <jordan.l.justen@intel.com>; Brijesh Singh <brijesh.singh@amd.com>;
> Erdem Aktas <erdemaktas@google.com>; James Bottomley
> <jejb@linux.ibm.com>; Yao, Jiewen <jiewen.yao@intel.com>; Tom Lendacky
> <thomas.lendacky@amd.com>
> Subject: Re: [PATCH V3 15/29] OvmfPkg: Update SecEntry.nasm to support Tdx
> 
> On Tue, Nov 16, 2021 at 12:11:33PM +0000, Xu, Min M wrote:
> > On November 3, 2021 2:31 PM, Gerd Hoffmann wrote:
> > >   Hi,
> > >
> > > >  - AcceptPages:
> > > >    To mitigate the performance impact of accepting pages in SEC phase on
> > > >    BSP, BSP will parse memory resources and assign each AP the task of
> > > >    accepting a subset of pages. This command may be called several times
> > > >    until all memory resources are processed. In accepting pages, PageLevel
> > > >    may fall back to smaller one if SIZE_MISMATCH error is returned.
> > >
> > > Why add an assembler version of this?  There already is a C version (in TdxLib,
> > > patch #2).  When adding lazy accept at some point in the future we will stop
> > > accepting all pages in the SEC phase anyway.  There is Mp support (patch #14)
> > > so you can distribute the load to all CPUs in PEI / DXE phase if you want
> > > (although the benefits of parallel accept will be much smaller once lazy
> > > accept is there).
> > There are below considerations about accept pages in SEC phase.
> >
> > 1. There is a minimal memory requirement in DxeCore [1]. In legacy
> > Ovmf the memory is initialized in PEI phase.
> 
> Yes.
> 
> > But TDVF has 2 configurations (Config-A and Config-B) [2]. PEI phase
> > is skipped in Config-B.  So we have to accept memories in SEC phase.
> 
> I'm sure I've asked this before:  Why skip the PEI phase?  So far
> I have not seen any convincing argument for it.

[Jiewen]
First, keeping or skipping PEI phase is a platform decision, not an architecture decision.

Skipping PEI phase is valid architecture design.

In EDKII, most platforms choose to include PEI.
And we also have multiple platforms skipping PEI phase. For example:
https://github.com/tianocore/edk2-platforms/blob/master/Platform/ARM/JunoPkg/ArmJuno.fdf
https://github.com/tianocore/edk2-platforms/blob/master/Platform/BeagleBoard/BeagleBoardPkg/BeagleBoardPkg.fdf
https://github.com/tianocore/edk2-platforms/blob/master/Platform/Hisilicon/HiKey/HiKey.fdf
https://github.com/tianocore/edk2-platforms/blob/master/Platform/Hisilicon/HiKey960/HiKey960.fdf
https://github.com/tianocore/edk2-platforms/blob/master/Platform/RaspberryPi/RPi3/RPi3.fdf
https://github.com/tianocore/edk2-platforms/blob/master/Platform/RaspberryPi/RPi4/RPi4.fdf

Second, the confidential computing changes the threat model completely.
One of our goal is to simplify the design for CC-firmware (TDVF) - remove unnecessary modules, remove unnecessary interface, make the image smaller and faster.
That will reduce the validation effort, too.

That is the main motivation.

Third, I have explained this clearly in the original email and review meeting.
Because it is a big change, the original maintainer (Laszlo) recommend we use two configuration.
Config-A is to keep current architecture, to maximum compatible with OVMF. And we don't remove VMM out of TCB.
Config-B is to have a new TDVF design, to maximum satisfy the security requirement. And we remove VMM out of TCB.

We agreed, and we are proceeding.


> Jiewen argued this is a simplification.  Which is not completely wrong,
> but it's also only half the truth.  Switching all OVMF builds over to
> PEI-less boot doesn't work because some features supported by OVMF
> depend on PEI Modules.  Therefore TDX Config-B skipping the PEI phase
> means we would have to maintain two boot work flows (with and without
> PEI phase) for OVMF.  Which in turn would imply more work for
> maintenance, testing and so on.

[Jiewen] I am not asking your to OVMF build to PEI-less.
But if you want to do, I will not object.

As EDKII maintainer, I don't see a burden to maintain multiple boot work flow.
We are already doing that on other project. As least for me, I don't see any problem.
If you think it is a burden, you may work partial of the feature.
EDKII is a big project. Nobody knows everything, including me. I will also rely on other people to handle some features I am not familiar with.
I don't see any problem.

On contrast, if we keep PEI for config B, it adds extra burden from security assurance perspective.
That means, every issue in PEI may be exposed to TDVF.

Comparing the effort to maintain the work flow and the effort to handle potential security issue, I would choose to maintain the work flow.
I have experience to wait for 1 year embargo to fix EDKII security issue, it is very painful and brings huge burden.




> Also note that accepting all memory in SEC phase would be temporary
> only.  Once we have support for lazy accept in OVMF we will accept most
> memory in DXE phase (or leave it to the linux kernel), whereas SEC/PEI
> will accept only a small fraction of the memory.  Just enough to allow
> DXE phase initialize memory management & lazy accept support.
> 
> > This is to make the code consistent in Config-A and Config-B.
> 
> I want TDVF be consistent with the rest of OVMF.  Makes long-term
> maintenance easier.  Building a single binary for both SEV and TDX with
> full confidential computing support (including config-b features) will
> be easier too.

[Jiewen] I am not convinced that TDVF be consist with rest of OVMF.
The goal of project is different. The choice can be different.
I don't see a reason that one platform must be in this way, just because other platform does in this way.

Easy and difficult are very subjective term.
I think a PEI-less TDVF is much easier to maintain, comparing with complicated OVMF flow, at least from security perspective.
The less code we have, the less issue we have.




> > 2. TDVF's MP support is via Mailbox [3] . BSP wakes up APs by putting
> > command/parameters in Mailbox. So we have this patch [4].  While
> > EDK2's MP support (CpuMpPei/CpuDxe) is via INIT-SIPI-SIPI.  They're
> > different.  We cannot distribute the load to all CPUs with EDK2's MP
> > service.
> 
> > Patch #14 [5] is the commit to enable TDX in MpInitLib. Actually this
> > is to make sure the CpuMpPei/CpuDxe will not break in Td guest in
> > run-time. Patch #14 is rather simple, for example, ApWorker is not
> > supported.
> 
> Well, MpInitLib seems to have full support (including ApWorker) for SEV.
> I'd expect you can add TDX support too, and schedule the jobs you want
> run on the APs via TDX mailbox instead of using IPIs.
> 
> And I think to support parallel lazy accept in the DXE phase (for lazy
> accept) you will need proper MpInitLib support anyway.
> 
> > 3. In current TDVF implementation, Stack is not set for APs. So C
> > functions cannot be called for APs. If stack is set for APs, then more
> > memories should be reserved in MEMFD.  For example, if 1 AP needs 4k
> > size stack, then 15 AP need 60k memory. (We assume only 1+15 CPUs
> > accept memory). This makes things complicated.
> 
> I think skipping PEI phase and moving stuff to SEC phase makes things
> complicated.  Reserving stacks in MEMFD would only be needed because you
> can't allocate memory in the SEC phase.  When you initialize the APs in
> PEI instead you can just allocate memory and the MEMFD dependency goes
> away.
> 
> > Based on above considerations, we use the current design that BSP-APs
> > accept memory in SEC phase (in assembly code).
> 
> I think the design doesn't make much sense for the reasons outlined
> above.
> 
> I'd suggest to put aside parallel accept for now and just let the BSP
> accept the memory for initial TDX support.  Focus on adding lazy accept
> support next.  Finally re-evaluate parallel accept support, *after*
> merging lazy accept.
> 
> With a major change in the memory acceptance workflow being planned it
> doesn't look like a good idea to me to tune memory accept performance
> *now*.
> 
> My naive expectation would be that parallel accept in SEC/PEI simply
> isn't worth the effort due to the small amount of memory being needed.
> Parallel accept in DXE probably is useful, but how much it speeds up
> boot depends on how much accepted memory we must hand over to the linux
> kernel so it has enough room to successfully initialize memory
> management & lazy accept.
> 
> take care,
>   Gerd