Re: [PATCH 2/2] OvmfPkg/AmdSev/SecretDxe: make secret location naming generic

["Yao, Jiewen" <jiewen.yao@intel.com>]

Thank you James.
I think you are on the right way. We will think about the feature in TDVF and do development in edk2-staging tree at first.

The API change looks good to me. I don’t think you need change OvmfPkg/AmdSev/SecretDxe at this moment.
We are also thinking how to support both SEV and TDX at same tree. But it is not fully finalized yet.
Not only Secret handling, but also MemEncryptionLib, IoMmu, UnacceptedMemory, etc. We will do that step by step in the near future.

I believe we can cross the bridge when we come to it.

Thank you
Yao Jiewen

> -----Original Message-----
> From: James Bottomley <jejb@linux.ibm.com>
> Sent: Thursday, December 17, 2020 12:54 AM
> To: Dov Murik <dovmurik@linux.vnet.ibm.com>
> Cc: devel@edk2.groups.io; Dov.Murik1@il.ibm.com; ashish.kalra@amd.com;
> brijesh.singh@amd.com; tobin@ibm.com; david.kaplan@amd.com;
> jon.grimm@amd.com; thomas.lendacky@amd.com; frankeh@us.ibm.com;
> Dr . David Alan Gilbert <dgilbert@redhat.com>; Laszlo Ersek
> <lersek@redhat.com>; Justen, Jordan L <jordan.l.justen@intel.com>; Ard
> Biesheuvel <ard.biesheuvel@arm.com>; Yao, Jiewen <jiewen.yao@intel.com>
> Subject: Re: [PATCH 2/2] OvmfPkg/AmdSev/SecretDxe: make secret location
> naming generic
> 
> On Wed, 2020-12-16 at 03:27 -0500, Dov Murik wrote:
> > On Tue, Dec 15, 2020 at 05:41:46PM -0800, James Bottomley wrote:
> > > It is anticipated that this part of the code will work for both
> > > Intel TDX and AMD SEV, so remove the SEV specific naming and change
> > > to ConfidentialComputing as a more architecture neutral prefix.
> > > Apart from the symbol rename, there are no code changes.
> > >
> > > Signed-off-by: James Bottomley <
> > > James.Bottomley@HansenPartnership.com>
> > > ---
> > >  OvmfPkg/OvmfPkg.dec                                    |  2 +-
> > >  OvmfPkg/AmdSev/SecretDxe/SecretDxe.inf                 |  2 +-
> > >  ...SevLaunchSecret.h => ConfidentialComputingSecret.h} | 10 +++++-
> > > ----
> > >  OvmfPkg/AmdSev/SecretDxe/SecretDxe.c                   |  6 +++---
> > >  4 files changed, 10 insertions(+), 10 deletions(-)
> > >  rename OvmfPkg/Include/Guid/{SevLaunchSecret.h =>
> > > ConfidentialComputingSecret.h} (69%)
> > >
> > > diff --git a/OvmfPkg/OvmfPkg.dec b/OvmfPkg/OvmfPkg.dec
> > > index 8a294116efaa..50d7b27d941c 100644
> > > --- a/OvmfPkg/OvmfPkg.dec
> > > +++ b/OvmfPkg/OvmfPkg.dec
> > > @@ -117,7 +117,7 @@ [Guids]
> > >    gLinuxEfiInitrdMediaGuid              = {0x5568e427, 0x68fc,
> > > 0x4f3d, {0xac, 0x74, 0xca, 0x55, 0x52, 0x31, 0xcc, 0x68}}
> > >    gQemuKernelLoaderFsMediaGuid          = {0x1428f772, 0xb64a,
> > > 0x441e, {0xb8, 0xc3, 0x9e, 0xbd, 0xd7, 0xf8, 0x93, 0xc7}}
> > >    gGrubFileGuid                         = {0xb5ae312c, 0xbc8a,
> > > 0x43b1, {0x9c, 0x62, 0xeb, 0xb8, 0x26, 0xdd, 0x5d, 0x07}}
> > > -  gSevLaunchSecretGuid                  = {0xadf956ad, 0xe98c,
> > > 0x484c, {0xae, 0x11, 0xb5, 0x1c, 0x7d, 0x33, 0x64, 0x47}}
> > > +  gConfidentialComputingSecretGuid      = {0xadf956ad, 0xe98c,
> > > 0x484c, {0xae, 0x11, 0xb5, 0x1c, 0x7d, 0x33, 0x64, 0x47}}
> > >
> > >  [Ppis]
> > >    # PPI whose presence in the PPI database signals that the TPM
> > > base address
> > > diff --git a/OvmfPkg/AmdSev/SecretDxe/SecretDxe.inf
> > > b/OvmfPkg/AmdSev/SecretDxe/SecretDxe.inf
> > > index 62ab00a3d382..40bda7ff846c 100644
> > > --- a/OvmfPkg/AmdSev/SecretDxe/SecretDxe.inf
> > > +++ b/OvmfPkg/AmdSev/SecretDxe/SecretDxe.inf
> > > @@ -27,7 +27,7 @@ [LibraryClasses]
> > >    UefiDriverEntryPoint
> > >
> > >  [Guids]
> > > -  gSevLaunchSecretGuid
> > > +  gConfidentialComputingSecretGuid
> > >
> > >  [FixedPcd]
> > >    gUefiOvmfPkgTokenSpaceGuid.PcdSevLaunchSecretBase
> > > diff --git a/OvmfPkg/Include/Guid/SevLaunchSecret.h
> > > b/OvmfPkg/Include/Guid/ConfidentialComputingSecret.h
> > > similarity index 69%
> > > rename from OvmfPkg/Include/Guid/SevLaunchSecret.h
> > > rename to OvmfPkg/Include/Guid/ConfidentialComputingSecret.h
> > > index dfd89646651b..7026fc5b089f 100644
> > > --- a/OvmfPkg/Include/Guid/SevLaunchSecret.h
> > > +++ b/OvmfPkg/Include/Guid/ConfidentialComputingSecret.h
> > > @@ -6,12 +6,12 @@
> > >     SPDX-License-Identifier: BSD-2-Clause-Patent
> > >   **/
> >
> > The comment at the top of this file (not visible in this diff) also
> > mentions "SEV Launch Secret" which should be renamed to "Confidential
> > Computing Secret".
> 
> Yes, I can update that.  The other thing I didn't change is the tree
> location ... it's still OvmfPkg/AmdSev/SecretDxe.  That's because I
> wasn't sure what the TDX implementation would look like.  It's possible
> they might have their own SecretDxe simply using the header for the
> structure and GUID (which means everything is correct) or whether both
> SEV and TDX should use the same .c file.
> 
> I think this raises the broader question of how much collaboration
> should there be between the two systems.  I did a small amount of .dsc
> file stripping in the previous patch, but it sounds like Intel has done
> a whole lot more for TDVF ... removing the entire PEI phase was what I
> heard in the webinar yesterday ... so I think we could get a lot of
> cross fertilization doing combinations at that level.  We might need to
> think about what features are general to a OVMF supporting a
> confidential VM, like stripping, and what are technology specific, like
> the exact mechanism of secret injection.
> 
> James
>