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

["Gerd Hoffmann" <kraxel@redhat.com>]

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 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.

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.

> 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