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

["Laszlo Ersek" <lersek@redhat.com>]

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