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

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

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

> 
> <https://software.intel.com/content/www/us/en/develop/articles/
> 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 <gaoliming@byosoft.com.cn>
> Cc: Zhiguang Liu <zhiguang.liu@intel.com>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> 
> Signed-off-by: Min Xu <min.m.xu@intel.com>
> 
> 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
>