[PATCH 0/2] Update Maintainers.txt for TDX and Confidential Computing

[PATCH 0/2] Update Maintainers.txt for TDX and Confidential Computing

[Min Xu]

Register reviewers for the TDX-related and Confidential Computing related
files in OvmfPkg.

Cc: Andrew Fish <afish@apple.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Leif Lindholm <leif@nuviainc.com>
Cc: Michael D Kinney <michael.d.kinney@intel.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Signed-off-by: Min Xu <min.m.xu@intel.com>

Min Xu (2):
  Maintainers.txt: Add reviewers for the OvmfPkg TDX-related files
  Maintainers.txt: Add reviewers for Confidential Computing related
    modules

 Maintainers.txt | 7 +++++++
 1 file changed, 7 insertions(+)

-- 
2.29.2.windows.2

[PATCH 1/2] Maintainers.txt: Add reviewers for the OvmfPkg TDX-related files

[Min Xu]

Register reviewers for the TDX-related files in OvmfPkg.

Cc: Andrew Fish <afish@apple.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Leif Lindholm <leif@nuviainc.com>
Cc: Michael D Kinney <michael.d.kinney@intel.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Signed-off-by: Min Xu <min.m.xu@intel.com>
---
 Maintainers.txt | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/Maintainers.txt b/Maintainers.txt
index e38497123857..7d9fe89d6d28 100644
--- a/Maintainers.txt
+++ b/Maintainers.txt
@@ -465,6 +465,10 @@ F: OvmfPkg/PlatformPei/AmdSev.c
 R: Tom Lendacky <thomas.lendacky@amd.com>
 R: Brijesh Singh <brijesh.singh@amd.com>
 
+OvmfPkg: TDX-related modules
+R: Jiewen Yao <jiewen.yao@intel.com>
+R: Min Xu <min.m.xu@intel.com>
+
 OvmfPkg: TCG- and TPM2-related modules
 F: OvmfPkg/Include/IndustryStandard/QemuTpm.h
 F: OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c
-- 
2.29.2.windows.2

[PATCH 2/2] Maintainers.txt: Add reviewers for Confidential Computing related modules

[Min Xu]

Register reviewers for the Confidential Computing related modules in
OvmfPkg.

Cc: Andrew Fish <afish@apple.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Leif Lindholm <leif@nuviainc.com>
Cc: Michael D Kinney <michael.d.kinney@intel.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Signed-off-by: Min Xu <min.m.xu@intel.com>
---
 Maintainers.txt | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/Maintainers.txt b/Maintainers.txt
index 7d9fe89d6d28..220af0ee9b80 100644
--- a/Maintainers.txt
+++ b/Maintainers.txt
@@ -469,6 +469,9 @@ OvmfPkg: TDX-related modules
 R: Jiewen Yao <jiewen.yao@intel.com>
 R: Min Xu <min.m.xu@intel.com>
 
+OvmfPkg: Confidential Computing related modules
+R: Jiewen Yao <jiewen.yao@intel.com>
+
 OvmfPkg: TCG- and TPM2-related modules
 F: OvmfPkg/Include/IndustryStandard/QemuTpm.h
 F: OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c
-- 
2.29.2.windows.2

Re: [PATCH 0/2] Update Maintainers.txt for TDX and Confidential Computing

[Yao, Jiewen]

Both 1 and 2 - Reviewed-by: Jiewen Yao <Jiewen.yao@intel.com>

> -----Original Message-----
> From: Xu, Min M <min.m.xu@intel.com>
> Sent: Wednesday, March 10, 2021 10:56 AM
> To: devel@edk2.groups.io
> Cc: Xu, Min M <min.m.xu@intel.com>; Andrew Fish <afish@apple.com>; Laszlo
> Ersek <lersek@redhat.com>; Leif Lindholm <leif@nuviainc.com>; Kinney,
> Michael D <michael.d.kinney@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>
> Subject: [PATCH 0/2] Update Maintainers.txt for TDX and Confidential
> Computing
> 
> Register reviewers for the TDX-related and Confidential Computing related
> files in OvmfPkg.
> 
> Cc: Andrew Fish <afish@apple.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Leif Lindholm <leif@nuviainc.com>
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Signed-off-by: Min Xu <min.m.xu@intel.com>
> 
> Min Xu (2):
>   Maintainers.txt: Add reviewers for the OvmfPkg TDX-related files
>   Maintainers.txt: Add reviewers for Confidential Computing related
>     modules
> 
>  Maintainers.txt | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> --
> 2.29.2.windows.2

Re: [PATCH 1/2] Maintainers.txt: Add reviewers for the OvmfPkg TDX-related files

[Laszlo Ersek]

On 03/10/21 03:55, Min Xu wrote:
> Register reviewers for the TDX-related files in OvmfPkg.
> 
> Cc: Andrew Fish <afish@apple.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Leif Lindholm <leif@nuviainc.com>
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Signed-off-by: Min Xu <min.m.xu@intel.com>
> ---
>  Maintainers.txt | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/Maintainers.txt b/Maintainers.txt
> index e38497123857..7d9fe89d6d28 100644
> --- a/Maintainers.txt
> +++ b/Maintainers.txt
> @@ -465,6 +465,10 @@ F: OvmfPkg/PlatformPei/AmdSev.c
>  R: Tom Lendacky <thomas.lendacky@amd.com>
>  R: Brijesh Singh <brijesh.singh@amd.com>
>  
> +OvmfPkg: TDX-related modules
> +R: Jiewen Yao <jiewen.yao@intel.com>
> +R: Min Xu <min.m.xu@intel.com>
> +
>  OvmfPkg: TCG- and TPM2-related modules
>  F: OvmfPkg/Include/IndustryStandard/QemuTpm.h
>  F: OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c
> 

I'm OK with the patch content, however the placement of the new section
is not right. We strive to keep the section headers alphabetically
ordered. Here's an excerpt (pre-patch):

OvmfPkg: SEV-related modules
OvmfPkg: TCG- and TPM2-related modules
OvmfPkg: Xen-related modules

I think that the new section belongs between TCG and Xen.

Thanks
Laszlo

Re: [PATCH 1/2] Maintainers.txt: Add reviewers for the OvmfPkg TDX-related files

[Laszlo Ersek]

On 03/10/21 14:12, Laszlo Ersek wrote:
> On 03/10/21 03:55, Min Xu wrote:
>> Register reviewers for the TDX-related files in OvmfPkg.
>>
>> Cc: Andrew Fish <afish@apple.com>
>> Cc: Laszlo Ersek <lersek@redhat.com>
>> Cc: Leif Lindholm <leif@nuviainc.com>
>> Cc: Michael D Kinney <michael.d.kinney@intel.com>
>> Cc: Jiewen Yao <jiewen.yao@intel.com>
>> Signed-off-by: Min Xu <min.m.xu@intel.com>
>> ---
>>  Maintainers.txt | 4 ++++
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/Maintainers.txt b/Maintainers.txt
>> index e38497123857..7d9fe89d6d28 100644
>> --- a/Maintainers.txt
>> +++ b/Maintainers.txt
>> @@ -465,6 +465,10 @@ F: OvmfPkg/PlatformPei/AmdSev.c
>>  R: Tom Lendacky <thomas.lendacky@amd.com>
>>  R: Brijesh Singh <brijesh.singh@amd.com>
>>  
>> +OvmfPkg: TDX-related modules
>> +R: Jiewen Yao <jiewen.yao@intel.com>
>> +R: Min Xu <min.m.xu@intel.com>
>> +
>>  OvmfPkg: TCG- and TPM2-related modules
>>  F: OvmfPkg/Include/IndustryStandard/QemuTpm.h
>>  F: OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c
>>
> 
> I'm OK with the patch content, however the placement of the new section
> is not right. We strive to keep the section headers alphabetically
> ordered. Here's an excerpt (pre-patch):
> 
> OvmfPkg: SEV-related modules
> OvmfPkg: TCG- and TPM2-related modules
> OvmfPkg: Xen-related modules
> 
> I think that the new section belongs between TCG and Xen.

Also -- I actually forgot the more important issue -- this patch does
not add any pathname patterns that the section is supposed to apply to!

Please refer to the "F:" patterns in the "Maintainers.txt" file, both
the explanation near the top of the file, and the various OvmfPkg examples.

And then, given that there are no TDX-related files in the tree at the
moment (so no F: pattern makes sense at the moment), I think adding this
section is premature. This patch should become part of your initial
TdxLib contribution -- the patch could be the last one in your series,
covering the pathnames introduced by the previous patches in your
series. So (just an example) we might have something like:

OvmfPkg: TDX-related modules
F: OvmfPkg/Library/TdxLib/
R: Jiewen Yao <jiewen.yao@intel.com>
R: Min Xu <min.m.xu@intel.com>

Subsequently to that, every time you introduced a new TDX-related module
(library instance, lib class header, driver, ...), you'd add matching
"F:" patterns to the above subsystem.

Thanks
Laszlo

Re: [PATCH 2/2] Maintainers.txt: Add reviewers for Confidential Computing related modules

[Laszlo Ersek]

Adding James, Brijesh, Tom; comments below:

On 03/10/21 03:55, Min Xu wrote:
> Register reviewers for the Confidential Computing related modules in
> OvmfPkg.
>
> Cc: Andrew Fish <afish@apple.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Leif Lindholm <leif@nuviainc.com>
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Signed-off-by: Min Xu <min.m.xu@intel.com>
> ---
>  Maintainers.txt | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/Maintainers.txt b/Maintainers.txt
> index 7d9fe89d6d28..220af0ee9b80 100644
> --- a/Maintainers.txt
> +++ b/Maintainers.txt
> @@ -469,6 +469,9 @@ OvmfPkg: TDX-related modules
>  R: Jiewen Yao <jiewen.yao@intel.com>
>  R: Min Xu <min.m.xu@intel.com>
>
> +OvmfPkg: Confidential Computing related modules
> +R: Jiewen Yao <jiewen.yao@intel.com>
> +
>  OvmfPkg: TCG- and TPM2-related modules
>  F: OvmfPkg/Include/IndustryStandard/QemuTpm.h
>  F: OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c
>

(1) Same comment as under patch#1 -- please honor the alphabetical
ordering of the subsystem titles.

(2) Reviewing this patch makes me realize we've missed some
"Maintainers.txt" updates in the past, in relation to SEV and/or
confidential computing.

Namely, we did not designated any reviewers for the following pathnames:

  OvmfPkg/AmdSev/
  OvmfPkg/Include/Guid/ConfidentialComputingSecret.h
  OvmfPkg/Library/PlatformBootManagerLibGrub/

(from <https://bugzilla.tianocore.org/show_bug.cgi?id=3077>), also

  OvmfPkg/ResetVector/

(from <https://bugzilla.tianocore.org/show_bug.cgi?id=2198>).

That should be fixed up before adding anything TDX related (I can submit
a patch series, but first, the next point needs to be cleared.)

(3) After racking my brain for half an hour, I can find no good way to
have TDX/SEV separation *plus* a Confidential Computing section in
"Maintainers.txt". Whatever I managed to think of requires us to either
duplicate email addresses, or duplicate pathnames ("F:" patterns) -- or
even both.

So... can we simply rename the current SEV subsystem to "Confidential
Computing", and keep both TDX and SEV modules under it? We could place a
unified email address list there, with Brijesh, James, Jiewen, Min, Tom.

I don't think this should cause any confusion, because:

- @intel.com emails are clearly closely associated with TDX, and
@amd.com emails are clearly closely associated with SEV,

- most filenames will (or do already) include "AmdSev" or "Tdx",

- future patches should clearly label themselves as "SEV only", "TDX
only", or "confidential computing in general" -- this should be clear
from the patch subjects.

IOW, there should be no confusion as to who's required to review what,
but at the same time we'd have a simple solution for cross-posting all
interested parties.

Thoughts?

Thanks,
Laszlo

Re: [PATCH 2/2] Maintainers.txt: Add reviewers for Confidential Computing related modules

[Lendacky, Thomas]

On 3/10/21 8:20 AM, Laszlo Ersek wrote:
> Adding James, Brijesh, Tom; comments below:
> 
> On 03/10/21 03:55, Min Xu wrote:
>> Register reviewers for the Confidential Computing related modules in
>> OvmfPkg.
>>
>> Cc: Andrew Fish <afish@apple.com>
>> Cc: Laszlo Ersek <lersek@redhat.com>
>> Cc: Leif Lindholm <leif@nuviainc.com>
>> Cc: Michael D Kinney <michael.d.kinney@intel.com>
>> Cc: Jiewen Yao <jiewen.yao@intel.com>
>> Signed-off-by: Min Xu <min.m.xu@intel.com>
>> ---

...

> 
> So... can we simply rename the current SEV subsystem to "Confidential
> Computing", and keep both TDX and SEV modules under it? We could place a
> unified email address list there, with Brijesh, James, Jiewen, Min, Tom.
> 
> I don't think this should cause any confusion, because:
> 
> - @intel.com emails are clearly closely associated with TDX, and
> @amd.com emails are clearly closely associated with SEV,
> 
> - most filenames will (or do already) include "AmdSev" or "Tdx",
> 
> - future patches should clearly label themselves as "SEV only", "TDX
> only", or "confidential computing in general" -- this should be clear
> from the patch subjects.
> 
> IOW, there should be no confusion as to who's required to review what,
> but at the same time we'd have a simple solution for cross-posting all
> interested parties.
> 
> Thoughts?

Works for me.

Thanks,
Tom

> 
> Thanks,
> Laszlo
> 

Re: [PATCH 2/2] Maintainers.txt: Add reviewers for Confidential Computing related modules

[Brijesh Singh]

On 3/10/21 9:09 AM, Tom Lendacky wrote:
> On 3/10/21 8:20 AM, Laszlo Ersek wrote:
>> Adding James, Brijesh, Tom; comments below:
>>
>> On 03/10/21 03:55, Min Xu wrote:
>>> Register reviewers for the Confidential Computing related modules in
>>> OvmfPkg.
>>>
>>> Cc: Andrew Fish <afish@apple.com>
>>> Cc: Laszlo Ersek <lersek@redhat.com>
>>> Cc: Leif Lindholm <leif@nuviainc.com>
>>> Cc: Michael D Kinney <michael.d.kinney@intel.com>
>>> Cc: Jiewen Yao <jiewen.yao@intel.com>
>>> Signed-off-by: Min Xu <min.m.xu@intel.com>
>>> ---
>
> ...
>
>>
>> So... can we simply rename the current SEV subsystem to "Confidential
>> Computing", and keep both TDX and SEV modules under it? We could place a
>> unified email address list there, with Brijesh, James, Jiewen, Min, Tom.
>>
>> I don't think this should cause any confusion, because:
>>
>> - @intel.com emails are clearly closely associated with TDX, and
>> @amd.com emails are clearly closely associated with SEV,
>>
>> - most filenames will (or do already) include "AmdSev" or "Tdx",
>>
>> - future patches should clearly label themselves as "SEV only", "TDX
>> only", or "confidential computing in general" -- this should be clear
>> from the patch subjects.
>>
>> IOW, there should be no confusion as to who's required to review what,
>> but at the same time we'd have a simple solution for cross-posting all
>> interested parties.
>>
>> Thoughts?
>
> Works for me.


Works for me as well.

Re: [PATCH 2/2] Maintainers.txt: Add reviewers for Confidential Computing related modules

[James Bottomley]

On Wed, 2021-03-10 at 15:20 +0100, Laszlo Ersek wrote:
[...]
> (2) Reviewing this patch makes me realize we've missed some
> "Maintainers.txt" updates in the past, in relation to SEV and/or
> confidential computing.
> 
> Namely, we did not designated any reviewers for the following
> pathnames:
> 
>   OvmfPkg/AmdSev/
>   OvmfPkg/Include/Guid/ConfidentialComputingSecret.h
>   OvmfPkg/Library/PlatformBootManagerLibGrub/
> 
> (from <https://bugzilla.tianocore.org/show_bug.cgi?id=3077>;), also
> 
>   OvmfPkg/ResetVector/
> 
> (from <https://bugzilla.tianocore.org/show_bug.cgi?id=2198>;).
> 
> That should be fixed up before adding anything TDX related (I can
> submit a patch series, but first, the next point needs to be
> cleared.)

I'm happy to be added for all of it ... the first three are all me and
the last one I added something to.

> (3) After racking my brain for half an hour, I can find no good way
> to have TDX/SEV separation *plus* a Confidential Computing section in
> "Maintainers.txt". Whatever I managed to think of requires us to
> either duplicate email addresses, or duplicate pathnames ("F:"
> patterns) -- or even both.
> 
> So... can we simply rename the current SEV subsystem to "Confidential
> Computing", and keep both TDX and SEV modules under it? We could
> place a unified email address list there, with Brijesh, James,
> Jiewen, Min, Tom.
> 
> I don't think this should cause any confusion, because:
> 
> - @intel.com emails are clearly closely associated with TDX, and
> @amd.com emails are clearly closely associated with SEV,
> 
> - most filenames will (or do already) include "AmdSev" or "Tdx",
> 
> - future patches should clearly label themselves as "SEV only", "TDX
> only", or "confidential computing in general" -- this should be clear
> from the patch subjects.

That should work ... it's entirely possible that SecretDxe and
SecretPei can work for Intel as well ... we don't know yet, so they may
not need a prefix.

> IOW, there should be no confusion as to who's required to review
> what, but at the same time we'd have a simple solution for cross-
> posting all interested parties.
> 
> Thoughts?

Works for me ... IBM is interested in both SEV and TDX and having them
be as similar as posisble.

James

Re: [edk2-devel] [PATCH 2/2] Maintainers.txt: Add reviewers for Confidential Computing related modules

[Yao, Jiewen]

Agree. Just one "confidential computing" can make things simpler.


> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of James
> Bottomley
> Sent: Wednesday, March 10, 2021 11:34 PM
> To: Laszlo Ersek <lersek@redhat.com>; Xu, Min M <min.m.xu@intel.com>; Yao,
> Jiewen <jiewen.yao@intel.com>; Brijesh Singh <brijesh.singh@amd.com>; Tom
> Lendacky <thomas.lendacky@amd.com>
> Cc: devel@edk2.groups.io; Andrew Fish <afish@apple.com>; Leif Lindholm
> <leif@nuviainc.com>; Kinney, Michael D <michael.d.kinney@intel.com>
> Subject: Re: [edk2-devel] [PATCH 2/2] Maintainers.txt: Add reviewers for
> Confidential Computing related modules
> 
> On Wed, 2021-03-10 at 15:20 +0100, Laszlo Ersek wrote:
> [...]
> > (2) Reviewing this patch makes me realize we've missed some
> > "Maintainers.txt" updates in the past, in relation to SEV and/or
> > confidential computing.
> >
> > Namely, we did not designated any reviewers for the following
> > pathnames:
> >
> >   OvmfPkg/AmdSev/
> >   OvmfPkg/Include/Guid/ConfidentialComputingSecret.h
> >   OvmfPkg/Library/PlatformBootManagerLibGrub/
> >
> > (from <https://bugzilla.tianocore.org/show_bug.cgi?id=3077>;), also
> >
> >   OvmfPkg/ResetVector/
> >
> > (from <https://bugzilla.tianocore.org/show_bug.cgi?id=2198>;).
> >
> > That should be fixed up before adding anything TDX related (I can
> > submit a patch series, but first, the next point needs to be
> > cleared.)
> 
> I'm happy to be added for all of it ... the first three are all me and
> the last one I added something to.
> 
> > (3) After racking my brain for half an hour, I can find no good way
> > to have TDX/SEV separation *plus* a Confidential Computing section in
> > "Maintainers.txt". Whatever I managed to think of requires us to
> > either duplicate email addresses, or duplicate pathnames ("F:"
> > patterns) -- or even both.
> >
> > So... can we simply rename the current SEV subsystem to "Confidential
> > Computing", and keep both TDX and SEV modules under it? We could
> > place a unified email address list there, with Brijesh, James,
> > Jiewen, Min, Tom.
> >
> > I don't think this should cause any confusion, because:
> >
> > - @intel.com emails are clearly closely associated with TDX, and
> > @amd.com emails are clearly closely associated with SEV,
> >
> > - most filenames will (or do already) include "AmdSev" or "Tdx",
> >
> > - future patches should clearly label themselves as "SEV only", "TDX
> > only", or "confidential computing in general" -- this should be clear
> > from the patch subjects.
> 
> That should work ... it's entirely possible that SecretDxe and
> SecretPei can work for Intel as well ... we don't know yet, so they may
> not need a prefix.
> 
> > IOW, there should be no confusion as to who's required to review
> > what, but at the same time we'd have a simple solution for cross-
> > posting all interested parties.
> >
> > Thoughts?
> 
> Works for me ... IBM is interested in both SEV and TDX and having them
> be as similar as posisble.
> 
> James
> 
> 
> 
> 
> 
>