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

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

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Gerd
> Hoffmann
> Sent: Thursday, November 25, 2021 4:32 PM
> To: Yao, Jiewen <jiewen.yao@intel.com>
> Cc: devel@edk2.groups.io; jejb@linux.ibm.com; Xu, Min M
> <min.m.xu@intel.com>; 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>; Tom Lendacky
> <thomas.lendacky@amd.com>
> Subject: Re: [edk2-devel] [PATCH V3 15/29] OvmfPkg: Update SecEntry.nasm to
> support Tdx
> 
>   Hi,
> 
> > So, my judgement is by removing PEI, we can reduce the risk introduce
> > by PEI Core + PEI Arch PEIM*. Reducing code == Reducing Security Risk.
> 
> Yes, PEI Core goes away.
> 
> No, PEI Arch PEIM (aka OvmfPkg/PlatformPei) wouldn't go away, you would
> only move the code to SEC or DXE phase, the platform initialization has
> to happen somewhere.

[Jiewen] "Arch PEIM" refers to the PEIM required by the PEI Core.
The "platform initialization" is FeatureX. It is something you have to do, no mater you have PEI or non-PEI.


> Moving code to DXE has its problems as outlines by James at length.

[Jiewen] I don't think this statement is right, with assumption that your interpretation for James' "exposure" is correct: "Exposure == External Interface", and assumption that we are discussing general design principle.

I give my reason below:

1) From architecture perspective.

Please refer to PI specification 1.7A (https://uefi.org/sites/default/files/resources/PI_Spec_1_7_A_final_May1.pdf)
Section 2.1 - Introduction.
"Philosophically, the PEI phase is intended to be the thinnest amount of code to achieve the ends
listed above. As such, any more sophisticated algorithms or processing should be deferred to the
DXE phase of execution."

In history, the first EFI 1.02 implementation does not have PEI.
PEI was added, just because the EFI need permanent RAM, while it is not TRUE for the platform that requires DRAM init.
If the DRAM init simple, then you don't PEI, because it can be done in SEC.
PEI was invented, because the memory init becomes more and more complicated and has more dependency. We want to have a better architecture to support that.

The *intention* of PEI is *only* to initialize the environment  for the EFI. That is why it is called as "Pre-EFI-initialization".
This is very similar to the coreboot - ROM stage and RAM stage. (You can treat PEI as ROM stage and DXE as RAM stage)

PEI should only include memory init (biggest part) and related code. If you don't have a strong reason to put a feature to PEI, then you had better to put it to DXE, according to the architecture direction.

2) From security perspective.

I am not convinced that "exposure (external interface)" is a good reason to decide where the module should be.
Thinking of this, if you prefer to put a module to the PEI because PEI has *less* exposure, then PEI will have *more* exposure after that.
If you move half of DXE features to PEI, then PEI has same exposure as DXE. What benefit we can get?

In EDKII, the general security guideline is that module location should be based upon "privilege", following the security principle - "least privilege".
If a module does not requires high privilege, then it should be put to lower privilege location.
In one of my email, I listed Trust Region (TR) concept. I copy them here again.
(I added SEC/BDS/RT to them as well, because I miss that in the first email)

================
Second, in EDKII, we have similar concept - we call trust region (TR):
1) Recovery Trust Region (SEC, PEI)
2) Main Trust Region (DXE-before EndOfDxe)
3) MM Trust Region (SMM)
4) Boot Trust Region (DXE w/o CSM-after EndOfDxe, BDS)
5) Legacy Trust Region (DXE with CSM-after EndOfDxe, BDS)
6) OS Trust Region (OS Boot, RT)

We use term "transition" to describe the domain jump. We don't use term
"isolation".
We use "isolation" where two co-existed RT cannot tamper each other. For
example, MM trust region and Boot Trust Region are isolated.
Actually, the only isolation example we have in BIOS is x86 SMM or ARM
TrustZone.
================

For example, SMM has much smaller exposure (external interface) than DXE.
But SMM has much higher privilege than DXE by design. However, it does not make sense to move feature from DXE to SMM.
All direction we have is to remove module from SMM to DXE, because the privilege concern, regardless of the exposure.

The SEC and PEI are almost same from TR perspective, they are in recovery TR.
Recovery TR has a little high privilege than Main TR, because there are some security LOCK activities happened in Recovery TR.

For a specific case, PEI might be a better place. I agree. For example, flash lock. I will definitely vote to put it to PEI.
But in general, I don't see why moving feature from PEI to DXE becomes a problem, unless you can explain in detail what the "problem" is.



> Moving code to SEC has its problems too.  SEC is a much more restricted
> environment.  A direct consequence is that you have re-invented
> multiprocessor job scheduling (using tdx mailbox) instead of using
> standard mp service for parallel accept.  I do not account that as
> "reducing complexity".

[Jiewen] OK. Let me explain multiprocessor related topic.
I don't think we intent to "reduce" complexity in this area. I would say, it is same with or without PEI.

TDX (also SEV) has special requirement to *accept* memory, before use it. The accepting is time consuming process.
So the motivation is to use multiprocessor to accelerate the process.

We have at least three architecture places to accept the memory - SEC, PEI and DXE.

A) Accept in SEC
We need write multiprocessor code in SEC. This is already mandatory, even without accepting memory.
The TDX architecture already changes the reset vector flow - ALL processors jumps to the reset vector at same time.
Having multiprocessor code in SEC is unavoidable. We have to do it, to rendez-vous APs and initialize mailbox.
The code is written because of TDX architecture change, not because memory accepting. So I won't treat it as a burden to add additional feature to memory accept in SEC.
At same time, the benefic to accept memory in SEC is significant, you can have memory ready for use. I will explain later why that matters.

B) Accept in PEI
PEI has MP_PPI, that is TRUE. But the problem is: MP_PPI starts *later*.
MP_PPI starts *after* the memory discovery (https://github.com/tianocore/edk2/blob/master/UefiCpuPkg/CpuMpPei/CpuMpPei.c#L706), which mean the process will be:
Step 1: TdxPeim accepts PEI required memory without MP_PPI
Step 2: PlatformPei installs PEI required memory.
Step 3: MP_PPI starts.
Step 4: TdxPeim accepts additional memory with MP_PPI

Now, you can see the benefit to accept PEI memory is not there.
NOTE: PEI memory is ~64M if GPAW is 48, it is ~98M if GPAW is 52, which is a big number.

That mean, with the solution you proposed (use PEI MP_PPI), we will have trouble on boot performance.

C) Accept in DXE
Almost same as PEI.
You have to accept some memory before launch MP_SERVICE. Same boot performance issue.


As such, we conclude that doing memory accept in SEC is the best choice for TDX architecture.
You may argue that we re-invented is MP work in SEC.
Right, but this is done because of TDX architecture.
It is NOT related to config-A (w/ PEI) or config-B (w/o PEI).



> And I've not yet seen the other changes you
> have done for pei-less tdvf ...

[Jiewen] That is because there is no many change. :-)

Here is just some early code.
1) Platform Init
In config-B, it is at https://github.com/tianocore/edk2-staging/blob/TDVF/OvmfPkg/Library/TdvfPlatformLibQemu/Platform.c
In config-A, it is at https://github.com/mxu9/edk2/blob/tdvf_wave2.v3/OvmfPkg/PlatformPei/Platform.c

2) DXE hand off
In config-B, it is at https://github.com/tianocore/edk2-staging/blob/TDVF/OvmfPkg/Library/TdxStartupLib/DxeLoad.c
In config-A, it is at https://github.com/tianocore/edk2/blob/master/MdeModulePkg/Core/DxeIplPeim/DxeLoad.c

The reset should be almost same in from SEC/PEI perspective.
I don't see a reason to carry whole PEI just to run platform init (~300 line of code).

For config-B, Min is working on it and doing clean up.
You are welcome to provide feedback after we send out patch for config-B.




> take care,
>   Gerd
> 
> 
> 
> 
>