Re: [edk2-devel] [PATCH V7 1/1] OvmfPkg: Enable TDX in ResetVector

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

I think we are discussing two topics. Please allow me to separate them.

1) Topic one: A unified build for config-A and config-B

I think we have discussed that before in EDKII, when Laszlo suggested:
A) don't put all TDX features into OvmfPkg.dsc, just put a basic feature them - we call it config-A.
B) Put full TDX feature into another TdvfPkg.dsc - we call it config-B.
Once we finish A) and B), we can evaluate how to merge B into A.
I do recommend you track back or have a discussion in RedHat to see what is high level suggestion from RedHat.


2) Topic two: A unified metadata table for SEV and TDX.

To me, I don't see it is necessary. I would say: I agree with you that we can align the design as much as possible, such as MemEncryption, ExceptionLib, IOMMU, etc.
However, if there is something totally different, I see no benefit to merge.
One example I could give is ACPI MADT table, X86 system defines its own interrupt table (APIC), while ARM system defines its own interrupt table (GIC). There is NO need to define a common interrupt table to cover both X86 and ARM.

I think current two table approach is good enough. TDX owner maintains its own table. SEV owner maintains its own table. Just like APIC table and GIC in ACPI MADT.
Although they are called confidential computing technology, the hardware implementation is different and features are different.
>From software layer, we can have HAL. But it does not mean we only have one common HAL for SEV and TDX. Two different HAL implementation are acceptable.

For the one table proposal, I would like to understand
A) What is the problem statement with current implementation?
B) What is the goal we want to achieve?
C) What is the benefit we can get?

Please be as specific as possible.

BTW: For C), I don't think we will have smaller code size, because we align we have to define some unnecessary field.
For your statement to remove duplication, please give me some real example. The page table example is invalid, because TDX does not need define an page table entry.
SEV requires GHCB, CPUID page but TDX does not. While TDX need indicate which range extend to MRTD, which is NOT. Also TDX metadata table will indicate which region may use AUG page, and which use ADD page in the future. I am not sure if SEV need those info.


Thank you
Yao Jiewen

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Gerd
> Hoffmann
> Sent: Friday, September 24, 2021 5:24 PM
> To: Yao, Jiewen <jiewen.yao@intel.com>
> Cc: Xu, Min M <min.m.xu@intel.com>; Brijesh Singh <brijesh.singh@amd.com>;
> devel@edk2.groups.io; Ard Biesheuvel <ardb+tianocore@kernel.org>; Justen,
> Jordan L <jordan.l.justen@intel.com>; Erdem Aktas <erdemaktas@google.com>;
> James Bottomley <jejb@linux.ibm.com>; Tom Lendacky
> <thomas.lendacky@amd.com>
> Subject: Re: [edk2-devel] [PATCH V7 1/1] OvmfPkg: Enable TDX in ResetVector
> 
> On Fri, Sep 24, 2021 at 07:36:10AM +0000, Yao, Jiewen wrote:
> > That is my question.
> > AMD has its own extension. TDX has its own extension.
> > Why we have to unify the firmware binary, and to make both us unconfirmable?
> 
> Isn't that the plan anyway?  At least for "config-a" with a basic
> feature set?  See other mail just sent for comments on "config-b".
> 
> > Or do we want to unify ARM/AARch64/RISC-V ?
> 
> Not sure what you are trying to tell me.
> 
> take care,
>   Gerd
> 
> 
> 
> 
>