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.

<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

-- 
2.29.2.windows.2



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#72565): https://edk2.groups.io/g/devel/message/72565
Mute This Topic: https://groups.io/mt/81195550/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

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
> 



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#72585): https://edk2.groups.io/g/devel/message/72585
Mute This Topic: https://groups.io/mt/81195550/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

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.

Thanks
Laszlo



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#72586): https://edk2.groups.io/g/devel/message/72586
Mute This Topic: https://groups.io/mt/81195550/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

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/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

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://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.
> 
> Thanks
> Laszlo



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#72598): https://edk2.groups.io/g/devel/message/72598
Mute This Topic: https://groups.io/mt/81195550/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

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


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#72633): https://edk2.groups.io/g/devel/message/72633
Mute This Topic: https://groups.io/mt/81195550/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-