From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [216.205.24.124]) by mx.groups.io with SMTP id smtpd.web08.9774.1615300741392403981 for ; Tue, 09 Mar 2021 06:39:01 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=Fga9GULu; spf=pass (domain: redhat.com, ip: 216.205.24.124, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1615300740; h=from:from:reply-to:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=Y2BefXED0laEiH2r0HGxVJzMxbNsAC3eMTZMIQschfQ=; b=Fga9GULuitHYgJUmo5PUsgkgG7rOWpvBr0xz66XnzTbi9mA62q/eK/o9ig+Mjnd8xdJ3// dpiNV6O1kpqRA2hNWTEqeWNMSP2MA8NHNj6oyqwaye9+Ze/R4fObmANFhvBEZNbcbFHdfP EvAM0WASfaUiImR2yoWaYHNNkemJ1Qo= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-177-Oap2ETJAMaCJaS9PEr-sUA-1; Tue, 09 Mar 2021 09:38:56 -0500 X-MC-Unique: Oap2ETJAMaCJaS9PEr-sUA-1 Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.phx2.redhat.com [10.5.11.13]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 6AC2457; Tue, 9 Mar 2021 14:38:52 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-115-173.ams2.redhat.com [10.36.115.173]) by smtp.corp.redhat.com (Postfix) with ESMTP id 9EF332E05D; Tue, 9 Mar 2021 14:38:49 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH V3 0/3] Add TdxLib support for Intel TDX From: "Laszlo Ersek" To: Min Xu , devel@edk2.groups.io Cc: Liming Gao , Zhiguang Liu , Jordan Justen , Jiewen Yao , Tom Lendacky , Brijesh Singh , James Bottomley , Tobin Feldman-Fitzthum , Dov Murik , "Dr. David Alan Gilbert" Reply-To: devel@edk2.groups.io, lersek@redhat.com References: <2e6e8275-e5f2-c8f3-e30a-f1ee51d279fd@redhat.com> Message-ID: <818c393f-5c67-3093-5c09-51525434bf05@redhat.com> Date: Tue, 9 Mar 2021 15:38:48 +0100 MIME-Version: 1.0 In-Reply-To: <2e6e8275-e5f2-c8f3-e30a-f1ee51d279fd@redhat.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.13 Authentication-Results: relay.mimecast.com; auth=pass smtp.auth=CUSA124A263 smtp.mailfrom=lersek@redhat.com X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit 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 , > 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 . 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