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 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#72592): https://edk2.groups.io/g/devel/message/72592 Mute This Topic: https://groups.io/mt/81195550/21656 Group Owner: [email protected] Unsubscribe: https://edk2.groups.io/g/devel/unsub [[email protected]] -=-=-=-=-=-=-=-=-=-=-=-
