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

[Brijesh Singh <brijesh.singh@amd.com>]

Thanks Laszlo for copying me. From AMD, I will be soon start submitting
the SNP support in the OMVF. I look forward collaborating with Yao and
Min on software architecture.


On 3/9/21 6:25 PM, Yao, Jiewen wrote:
> 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://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D3249&amp;data=04%7C01%7Cbrijesh.singh%40amd.com%7Cd28ff222c8714f55263008d8e35af722%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637509327122407224%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=lvpMxaXmLtXn8cn%2BLx2MMU9blA0kJrEyQe5IbOW4YJg%3D&amp;reserved=0
>>>>
>>>> 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://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D3249&amp;data=04%7C01%7Cbrijesh.singh%40amd.com%7Cd28ff222c8714f55263008d8e35af722%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637509327122407224%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=lvpMxaXmLtXn8cn%2BLx2MMU9blA0kJrEyQe5IbOW4YJg%3D&amp;reserved=0
>>
>> (7b) whenever you post a new version of the patch set, please add a new
>> comment to <https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D3249&amp;data=04%7C01%7Cbrijesh.singh%40amd.com%7Cd28ff222c8714f55263008d8e35af722%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637509327122407224%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=lvpMxaXmLtXn8cn%2BLx2MMU9blA0kJrEyQe5IbOW4YJg%3D&amp;reserved=0>,
>> 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