Re: [PATCH V3 0/3] Add TdxLib support for Intel TDX

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

Very good suggestion. Thanks Laszlo.

For 3), Min Xu and I will be the reviewer for Intel TDX change for OVMF.

For 6), agree. Although there is some architecture difference, e.g, AMD using PSP - a co-processor while Intel using TDX module - a new CPU execution mode, we should align as much as possible between Intel TDX and AMD SEV, especially for pure software architecture.
I will be the Intel reviewer for confidential computing topic.
Welcome AMD/IBM/... having a representative too.

Min and I will sync and submit the patch for maintainer.txt


Thank you
Yao Jiewen 


> -----Original Message-----
> From: Laszlo Ersek <lersek@redhat.com>
> Sent: Tuesday, March 9, 2021 9:06 PM
> To: Xu, Min M <min.m.xu@intel.com>; devel@edk2.groups.io
> Cc: Liming Gao <gaoliming@byosoft.com.cn>; Liu, Zhiguang
> <zhiguang.liu@intel.com>; Justen, Jordan L <jordan.l.justen@intel.com>; Yao,
> Jiewen <jiewen.yao@intel.com>; Tom Lendacky <thomas.lendacky@amd.com>;
> Brijesh Singh <brijesh.singh@amd.com>; James Bottomley
> <jejb@linux.ibm.com>; Tobin Feldman-Fitzthum <tobin@ibm.com>; Dov Murik
> <Dov.Murik1@il.ibm.com>; Dr. David Alan Gilbert <dgilbert@redhat.com>
> Subject: Re: [PATCH V3 0/3] Add TdxLib support for Intel TDX
> 
> On 03/09/21 13:57, Laszlo Ersek wrote:
> > On 03/09/21 07:12, Min Xu wrote:
> >> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3249
> >>
> >> The patch series provides lib support for Intel Trust Domain Extensions
> >> (Intel TDX).
> >>
> >> Intel's Trust Domain Extensions (Intel TDX) refers to an Intel technology
> >> that extends Virtual Machines Extensions (VMX) and Multi-Key Total Memory
> >> Encryption (MKTME) with a new kind of virutal machines guest called a
> >> Trust Domain (TD). A TD is desinged to run in a CPU mode that protects the
> >> confidentiality of TD memory contents and the TD's CPU state from other
> >> software, including the hosting Virtual-Machine Monitor (VMM), unless
> >> explicitly shared by the TD itself.
> >>
> >> The Intel TDX module uses the instruction-set architecture for Intel TDX
> >> and the MKTME engine in the SOC to help serve as an intermediary between
> >> the host VMM and the guest TD. TDCALL is the instruction which allows TD
> >> guest privileged software to make a call for service into an underlying
> >> TDX-module.
> >>
> >> TdxLib is created with functions to perform the related Tdx operation.
> >> This includes functions for:
> >>   - TdCall         : to cause a VM exit to the Intel TDX module
> >>   - TdVmCall       : it is a leaf function 0 for TDCALL
> >>   - TdVmCallCpuid  : enable the TD guest to request VMM to emulate CPUID
> >>   - TdReport       : to retrieve TDREPORT_STRUCT
> >>   - TdAcceptPages  : to accept pending private pages
> >>   - TdExtendRtmr   : to extend one of the RTMR registers
> >>
> >> The base function in MdePkg will not do anything and will return an error
> >> if a return value is required. It is expected that other packages
> >> (like OvmfPkg) will create a version of the library to fully support a TD
> >> guest.
> >>
> >> We create an OVMF version of this library to begin the process of providing
> >> full support of TDX in OVMF.
> >>
> >> To support the emulation and test purpose, 2 PCDs are added in OvmfPkg.dec
> >>   - PcdUseTdxAcceptPage
> >>     Indicate whether TdCall(AcceptPage) is used.
> >>   - PcdUseTdxEmulation
> >>     Indicate whether TdxEmulation is used.
> >
> > (1) per Jiewen's feedback, please drop these PCDs -- importantly, please
> > drop DB-encoded instructions in assembly source code
> >
> > (2) It's not really helpful to post three versions of a patch set over
> > the course of a few hours. I don't suggest posting more frequently than
> > once per day, unless agreed otherwise.
> >
> > (3) Please add a new section to Maintainers.txt for TDX content in
> > OvmfPkg. At least two Intel developers should be listed there as
> > Reviewers. I'd like to permanently delegate TDX reviews to Intel
> > contributors.
> >
> > See also the "OvmfPkg: SEV-related modules" section in "Maintainers.txt".
> >
> > (4) The patches contain numerous style issues:
> >
> > - overlong lines,
> >
> > - incomplete "@retval" comments,
> >
> > - Library #include directives mixed with non-library #include directives,
> >
> > - variables that should be STATIC but are not declared like that,
> >
> > - whitespace errors: missing space character between function designator
> > (or macro name) and opening paren
> >
> > - more whitespace errors: missing space characters around "if" and
> > "else" keywords
> >
> > (5) Some of the source files have outdated license blocks (e.g.,
> > open-coding the 2-clause BSDL and stating a copyright year of 2020,
> > rather than stating 2021 and using "SPDX-License-Identifier:
> > BSD-2-Clause-Patent")
> >
> > Please go over the patches with a fine-toothed comb and refresh them.
> >
> > (6) It would be nice if SEV-related patch sets and TDX-related patch
> > sets were cross-CC'd between AMD and Intel contributors. (With the
> > intent being code reuse, and perhaps "design reuse".)
> >
> > Maybe we should have an additional "confidential computing" reviewers
> > section in "Maintainers.txt", covering both SEV and TDX modules. This
> > would allow for a wider set of CC's, without obscuring who should review
> > TDX vs. who should review SEV. I think this unified section should list
> > a number of IBM developers too.
> 
> (7) Some more admin stuff:
> 
> (7a) every patch in this series should carry the following line in the
> commit message:
> 
> Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3249
> 
> (7b) whenever you post a new version of the patch set, please add a new
> comment to <https://bugzilla.tianocore.org/show_bug.cgi?id=3249>,
> linking the just-posted version (the cover letter email) from the
> mailing list archive.
> 
> This is important in case we want to review the evolution of the patch
> series later. It's more difficult to find relevant email threads later
> than to link each posting immediately in the bugzilla ticket.
> 
> Thanks
> Laszlo