From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [63.128.21.124]) by mx.groups.io with SMTP id smtpd.web12.8156.1615294635487340868 for ; Tue, 09 Mar 2021 04:57:16 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=JYFbiKVq; spf=pass (domain: redhat.com, ip: 63.128.21.124, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1615294634; h=from:from: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=NYH8imys9XhUOU57HM7wnalqFOaKx/1oaPNjYKlhFtA=; b=JYFbiKVqqyADimgMECMDWvTSMvxOEqSiy88IzM5wOCkEKKlvJyhei1TyXQktJiGElpTJjy k0nYwTsTrm63fXLoCBMpj4/FaqHpBW4/UHQuFXhPUlJMUU6i/lhcuHRsTTd2GoCE158E84 D/iitX7/G48gfNgUbFKz3PmWbZhsIZc= 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-380-jvC1Cd4hM2-u2DA12slg_w-1; Tue, 09 Mar 2021 07:57:10 -0500 X-MC-Unique: jvC1Cd4hM2-u2DA12slg_w-1 Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.phx2.redhat.com [10.5.11.15]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 02AA926862; Tue, 9 Mar 2021 12:57:09 +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 0457161F38; Tue, 9 Mar 2021 12:57:05 +0000 (UTC) Subject: Re: [PATCH V3 0/3] Add TdxLib support for Intel TDX 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" References: From: "Laszlo Ersek" Message-ID: Date: Tue, 9 Mar 2021 13:57:05 +0100 MIME-Version: 1.0 In-Reply-To: X-Scanned-By: MIMEDefang 2.79 on 10.5.11.15 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 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. Thanks, Laszlo > > intel-trust-domain-extensions.html>, defitions in TdxLib comes from: > [1] Intel TDX(R) Module 1.0 EAS > [2] Intel(R) TDX Guest-Hypervisor Communication Interface > > Cc: Liming Gao > Cc: Zhiguang Liu > Cc: Jordan Justen > Cc: Laszlo Ersek > Cc: Jiewen Yao > > Signed-off-by: Min Xu > > Min Xu (3): > MdePkg: Add Tdx support lib > OvmfPkg: Add PCDs for TdxLib > OvmfPkg: Implement library support for TdxLib SEC and DXE on OVMF > > MdePkg/Include/IndustryStandard/Tdx.h | 201 +++++++++++++++++++++ > MdePkg/Include/Library/TdxLib.h | 165 ++++++++++++++++++ > MdePkg/Include/Protocol/Tdx.h | 29 ++++ > MdePkg/Library/TdxLib/TdxLibNull.c | 155 +++++++++++++++++ > MdePkg/Library/TdxLib/TdxLibNull.inf | 33 ++++ > OvmfPkg/Library/TdxLib/AcceptPages.c | 68 ++++++++ > OvmfPkg/Library/TdxLib/Rtmr.c | 80 +++++++++ > OvmfPkg/Library/TdxLib/TdReport.c | 102 +++++++++++ > OvmfPkg/Library/TdxLib/TdxLib.inf | 48 ++++++ > OvmfPkg/Library/TdxLib/TdxLibSec.inf | 45 +++++ > OvmfPkg/Library/TdxLib/X64/Tdcall.nasm | 125 ++++++++++++++ > OvmfPkg/Library/TdxLib/X64/Tdvmcall.nasm | 211 +++++++++++++++++++++++ > OvmfPkg/OvmfPkg.dec | 6 + > 13 files changed, 1268 insertions(+) > create mode 100644 MdePkg/Include/IndustryStandard/Tdx.h > create mode 100644 MdePkg/Include/Library/TdxLib.h > create mode 100644 MdePkg/Include/Protocol/Tdx.h > create mode 100644 MdePkg/Library/TdxLib/TdxLibNull.c > create mode 100644 MdePkg/Library/TdxLib/TdxLibNull.inf > create mode 100644 OvmfPkg/Library/TdxLib/AcceptPages.c > create mode 100644 OvmfPkg/Library/TdxLib/Rtmr.c > create mode 100644 OvmfPkg/Library/TdxLib/TdReport.c > create mode 100644 OvmfPkg/Library/TdxLib/TdxLib.inf > create mode 100644 OvmfPkg/Library/TdxLib/TdxLibSec.inf > create mode 100644 OvmfPkg/Library/TdxLib/X64/Tdcall.nasm > create mode 100644 OvmfPkg/Library/TdxLib/X64/Tdvmcall.nasm >