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

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

On 03/09/21 14:06, Laszlo Ersek wrote:
> 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.

(8) As-is, the patch set does not enable the new library instance under
OvmfPkg to be built, at all. That's wrong; we shouldn't add a new lib
instance that can't even be build-tested -- the CI on github.com won't
cover the new code.

Therefore -- at least until there is an actual driver module that
consumes the new lib instance --, please add the lib instance to the
appropriate [Components] section(s) in the main OvmfPkg DSC files (IA32,
IA32X64, X64). These lines can be backed out later (when a UEFI
executable will depend on the lib instance).

(9) Before you submit a patch set to the list for review, please subject
it to CI, by opening a pull request.

Please see the details in steps 7 and 8 at
<https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-Development-Process>.

The only difference that's relevant here is that you shouldn't (and
can't) set the "push" label -- the goal is not to merge the set, but to
unleash CI on it.

Thanks
Laszlo