[PATCH 0/2] Update SevSecret API to work for TDX

[PATCH 0/2] Update SevSecret API to work for TDX

[James Bottomley]

This patch series changes the EFI configuration table information
which is queried by the bootloader to make it more compatible with
Intel TDX.  The first patch changes the ABI to make the table contain
two 64 bit integers instead of two 32 bit ones.  The second patch is a
cosmetic one to change the names of the GUIDs and tables to have a
confidential computing prefix instead of a SEV Launch one.

The first patch *must* be applied before the next stable tag to avoid
ABI breakage.  The second is purely cosmetic and doesn't change the
code output.

Ultimately there will still need to be a TDX collector for the secret,
which would feed the value into the SecretDxe, but these changes
should ensure that no further changes would be required by the secret
consumers.

James

---

James Bottomley (2):
  OvmfPkg: Change SEV Launch Secret API to be UINT64 for base and size
  OvmfPkg/AmdSev/SecretDxe: make secret location naming generic

 OvmfPkg/OvmfPkg.dec                                |  2 +-
 OvmfPkg/AmdSev/SecretDxe/SecretDxe.inf             |  2 +-
 ...aunchSecret.h => ConfidentialComputingSecret.h} | 14 +++++++-------
 OvmfPkg/AmdSev/SecretDxe/SecretDxe.c               |  6 +++---
 4 files changed, 12 insertions(+), 12 deletions(-)
 rename OvmfPkg/Include/Guid/{SevLaunchSecret.h => ConfidentialComputingSecret.h} (65%)

-- 
2.26.2

[PATCH 1/2] OvmfPkg: Change SEV Launch Secret API to be UINT64 for base and size

[James Bottomley]

Although the SEV secret location must always be below 4GB, the same is
not necessarily true for Intel TDX, so change the configuration table
to contain a pair of UINT64 parameters instead of UINT32 so that any X64
location can be represented.

Signed-off-by: James Bottomley <jejb@linux.ibm.com>

---

This represents an ABI change, so it is essential to get this in
before the next stable tag
---
 OvmfPkg/Include/Guid/SevLaunchSecret.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/OvmfPkg/Include/Guid/SevLaunchSecret.h b/OvmfPkg/Include/Guid/SevLaunchSecret.h
index fa5f3830bc2b..dfd89646651b 100644
--- a/OvmfPkg/Include/Guid/SevLaunchSecret.h
+++ b/OvmfPkg/Include/Guid/SevLaunchSecret.h
@@ -19,8 +19,8 @@
   }
 
 typedef struct {
-  UINT32 Base;
-  UINT32 Size;
+  UINT64 Base;
+  UINT64 Size;
 } SEV_LAUNCH_SECRET_LOCATION;
 
 extern EFI_GUID gSevLaunchSecretGuid;
-- 
2.26.2

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

[James Bottomley]

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
  **/
 
-#ifndef SEV_LAUNCH_SECRET_H_
-#define SEV_LAUNCH_SECRET_H_
+#ifndef CONFIDENTIAL_COMPUTING_SECRET_H_
+#define CONFIDENTIAL_COMPUTING_SECRET_H_
 
 #include <Uefi/UefiBaseType.h>
 
-#define SEV_LAUNCH_SECRET_GUID                          \
+#define CONFIDENTIAL_COMPUTING_SECRET_GUID              \
   { 0xadf956ad,                                         \
     0xe98c,                                             \
     0x484c,                                             \
@@ -21,8 +21,8 @@
 typedef struct {
   UINT64 Base;
   UINT64 Size;
-} SEV_LAUNCH_SECRET_LOCATION;
+} CONFIDENTIAL_COMPUTING_SECRET_LOCATION;
 
-extern EFI_GUID gSevLaunchSecretGuid;
+extern EFI_GUID gConfidentialComputingSecretGuid;
 
 #endif // SEV_LAUNCH_SECRET_H_
diff --git a/OvmfPkg/AmdSev/SecretDxe/SecretDxe.c b/OvmfPkg/AmdSev/SecretDxe/SecretDxe.c
index 5385a6aea275..308022b5b25e 100644
--- a/OvmfPkg/AmdSev/SecretDxe/SecretDxe.c
+++ b/OvmfPkg/AmdSev/SecretDxe/SecretDxe.c
@@ -6,9 +6,9 @@
 **/
 #include <PiDxe.h>
 #include <Library/UefiBootServicesTableLib.h>
-#include <Guid/SevLaunchSecret.h>
+#include <Guid/ConfidentialComputingSecret.h>
 
-STATIC SEV_LAUNCH_SECRET_LOCATION mSecretDxeTable = {
+STATIC CONFIDENTIAL_COMPUTING_SECRET_LOCATION mSecretDxeTable = {
   FixedPcdGet32 (PcdSevLaunchSecretBase),
   FixedPcdGet32 (PcdSevLaunchSecretSize),
 };
@@ -21,7 +21,7 @@ InitializeSecretDxe(
   )
 {
   return gBS->InstallConfigurationTable (
-                &gSevLaunchSecretGuid,
+                &gConfidentialComputingSecretGuid,
                 &mSecretDxeTable
                 );
 }
-- 
2.26.2

Re: [PATCH 0/2] Update SevSecret API to work for TDX

[Yao, Jiewen]

Series: Reviewed-by: Jiewen Yao <Jiewen.yao@intel.com>

> -----Original Message-----
> From: James Bottomley <jejb@linux.ibm.com>
> Sent: Wednesday, December 16, 2020 9:42 AM
> To: devel@edk2.groups.io
> Cc: dovmurik@linux.vnet.ibm.com; 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;
> jejb@linux.ibm.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: [PATCH 0/2] Update SevSecret API to work for TDX
> 
> This patch series changes the EFI configuration table information
> which is queried by the bootloader to make it more compatible with
> Intel TDX.  The first patch changes the ABI to make the table contain
> two 64 bit integers instead of two 32 bit ones.  The second patch is a
> cosmetic one to change the names of the GUIDs and tables to have a
> confidential computing prefix instead of a SEV Launch one.
> 
> The first patch *must* be applied before the next stable tag to avoid
> ABI breakage.  The second is purely cosmetic and doesn't change the
> code output.
> 
> Ultimately there will still need to be a TDX collector for the secret,
> which would feed the value into the SecretDxe, but these changes
> should ensure that no further changes would be required by the secret
> consumers.
> 
> James
> 
> ---
> 
> James Bottomley (2):
>   OvmfPkg: Change SEV Launch Secret API to be UINT64 for base and size
>   OvmfPkg/AmdSev/SecretDxe: make secret location naming generic
> 
>  OvmfPkg/OvmfPkg.dec                                |  2 +-
>  OvmfPkg/AmdSev/SecretDxe/SecretDxe.inf             |  2 +-
>  ...aunchSecret.h => ConfidentialComputingSecret.h} | 14 +++++++-------
>  OvmfPkg/AmdSev/SecretDxe/SecretDxe.c               |  6 +++---
>  4 files changed, 12 insertions(+), 12 deletions(-)
>  rename OvmfPkg/Include/Guid/{SevLaunchSecret.h =>
> ConfidentialComputingSecret.h} (65%)
> 
> --
> 2.26.2

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

[Dov Murik]

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".

-Dov


> 
> -#ifndef SEV_LAUNCH_SECRET_H_
> -#define SEV_LAUNCH_SECRET_H_
> +#ifndef CONFIDENTIAL_COMPUTING_SECRET_H_
> +#define CONFIDENTIAL_COMPUTING_SECRET_H_
> 
>  #include <Uefi/UefiBaseType.h>
> 
> -#define SEV_LAUNCH_SECRET_GUID                          \
> +#define CONFIDENTIAL_COMPUTING_SECRET_GUID              \
>    { 0xadf956ad,                                         \
>      0xe98c,                                             \
>      0x484c,                                             \
> @@ -21,8 +21,8 @@
>  typedef struct {
>    UINT64 Base;
>    UINT64 Size;
> -} SEV_LAUNCH_SECRET_LOCATION;
> +} CONFIDENTIAL_COMPUTING_SECRET_LOCATION;
> 
> -extern EFI_GUID gSevLaunchSecretGuid;
> +extern EFI_GUID gConfidentialComputingSecretGuid;
> 
>  #endif // SEV_LAUNCH_SECRET_H_
> diff --git a/OvmfPkg/AmdSev/SecretDxe/SecretDxe.c b/OvmfPkg/AmdSev/SecretDxe/SecretDxe.c
> index 5385a6aea275..308022b5b25e 100644
> --- a/OvmfPkg/AmdSev/SecretDxe/SecretDxe.c
> +++ b/OvmfPkg/AmdSev/SecretDxe/SecretDxe.c
> @@ -6,9 +6,9 @@
>  **/
>  #include <PiDxe.h>
>  #include <Library/UefiBootServicesTableLib.h>
> -#include <Guid/SevLaunchSecret.h>
> +#include <Guid/ConfidentialComputingSecret.h>
> 
> -STATIC SEV_LAUNCH_SECRET_LOCATION mSecretDxeTable = {
> +STATIC CONFIDENTIAL_COMPUTING_SECRET_LOCATION mSecretDxeTable = {
>    FixedPcdGet32 (PcdSevLaunchSecretBase),
>    FixedPcdGet32 (PcdSevLaunchSecretSize),
>  };
> @@ -21,7 +21,7 @@ InitializeSecretDxe(
>    )
>  {
>    return gBS->InstallConfigurationTable (
> -                &gSevLaunchSecretGuid,
> +                &gConfidentialComputingSecretGuid,
>                  &mSecretDxeTable
>                  );
>  }
> -- 
> 2.26.2
> 

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

[James Bottomley]

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

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

[Yao, Jiewen]

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
> 

Re: [edk2-devel] [PATCH 0/2] Update SevSecret API to work for TDX

[Laszlo Ersek]

+Mike

On 12/16/20 02:41, James Bottomley wrote:
> This patch series changes the EFI configuration table information
> which is queried by the bootloader to make it more compatible with
> Intel TDX.  The first patch changes the ABI to make the table contain
> two 64 bit integers instead of two 32 bit ones.  The second patch is a
> cosmetic one to change the names of the GUIDs and tables to have a
> confidential computing prefix instead of a SEV Launch one.
> 
> The first patch *must* be applied before the next stable tag to avoid
> ABI breakage.  The second is purely cosmetic and doesn't change the
> code output.
> 
> Ultimately there will still need to be a TDX collector for the secret,
> which would feed the value into the SecretDxe, but these changes
> should ensure that no further changes would be required by the secret
> consumers.
> 
> James
> 
> ---
> 
> James Bottomley (2):
>   OvmfPkg: Change SEV Launch Secret API to be UINT64 for base and size
>   OvmfPkg/AmdSev/SecretDxe: make secret location naming generic
> 
>  OvmfPkg/OvmfPkg.dec                                |  2 +-
>  OvmfPkg/AmdSev/SecretDxe/SecretDxe.inf             |  2 +-
>  ...aunchSecret.h => ConfidentialComputingSecret.h} | 14 +++++++-------
>  OvmfPkg/AmdSev/SecretDxe/SecretDxe.c               |  6 +++---
>  4 files changed, 12 insertions(+), 12 deletions(-)
>  rename OvmfPkg/Include/Guid/{SevLaunchSecret.h => ConfidentialComputingSecret.h} (65%)
> 

series
Reviewed-by: Laszlo Ersek <lersek@redhat.com>

I tried merging this:

https://github.com/tianocore/edk2/pull/1235

but the Ubuntu builds all failed. I've checked two logs:

https://dev.azure.com/tianocore/edk2-ci/_build/results?buildId=16967&view=logs&j=cf2d8b26-a21c-5c68-abf4-b944c123e462&t=5ffbbe5c-1d3a-55f5-5ef3-8a0ef80d76a1&l=184
https://dev.azure.com/tianocore/edk2-ci/_build/results?buildId=16968&view=logs&j=47cf355a-6eb4-51a8-46a8-ff4028bfcac0&t=beedef5d-00d0-5a8c-fa35-57d7319988c2&l=182

They say,

INFO - /bin/sh: 1: qemu-system-aarch64: not found
INFO - /bin/sh: 1: qemu-system-x86_64: not found

I guess I won't be merging the three patch sets that I had planned for
this evening...

Laszlo

Re: [edk2-devel] [PATCH 0/2] Update SevSecret API to work for TDX

[Laszlo Ersek]

On 12/17/20 19:43, Laszlo Ersek wrote:

> I tried merging this:
> 
> https://github.com/tianocore/edk2/pull/1235
> 
> but the Ubuntu builds all failed. I've checked two logs:
> 
> https://dev.azure.com/tianocore/edk2-ci/_build/results?buildId=16967&view=logs&j=cf2d8b26-a21c-5c68-abf4-b944c123e462&t=5ffbbe5c-1d3a-55f5-5ef3-8a0ef80d76a1&l=184
> https://dev.azure.com/tianocore/edk2-ci/_build/results?buildId=16968&view=logs&j=47cf355a-6eb4-51a8-46a8-ff4028bfcac0&t=beedef5d-00d0-5a8c-fa35-57d7319988c2&l=182
> 
> They say,
> 
> INFO - /bin/sh: 1: qemu-system-aarch64: not found
> INFO - /bin/sh: 1: qemu-system-x86_64: not found

The "Install qemu" tasks earlier seem to complete:

https://dev.azure.com/tianocore/edk2-ci/_build/results?buildId=16967&view=logs&j=cf2d8b26-a21c-5c68-abf4-b944c123e462&t=a5c654c1-e049-5a30-61a9-da81b8ec031f
https://dev.azure.com/tianocore/edk2-ci/_build/results?buildId=16968&view=logs&j=47cf355a-6eb4-51a8-46a8-ff4028bfcac0&t=9a629c6e-a36d-5733-3aff-19ed2a42cf75

However, the qemu "4.2-3ubuntu6.10" package is a dummy package:

  https://packages.ubuntu.com/focal/qemu

and as shown under the link, it has no dependency on the packages with the actual qemu executables. So the latter do not get pulled in.

(Even the logs make that clear: "Need to get 14.3 kB of archives" -- obviously, a real QEMU won't fit in that, and no other packages get pulled in).

The meta-package that pulls in all system emulators is called "qemu-system":

  https://packages.ubuntu.com/focal/qemu-system

What I don't understand at this point is how the CI scripts could work previously.

... Aha! I do understand it now. Look at one of the last successful PRs:

  https://github.com/tianocore/edk2/pull/1232

The CI logs contain this message:

  "##[warning]Ubuntu-latest pipelines will use Ubuntu-20.04 soon. For more details, see https://github.com/actions/virtual-environments/issues/1816"

So let's check out:

  https://github.com/actions/virtual-environments/issues/1816

Okay... so it looks like I'm the victim of "Ubuntu-latest" switching to 20.04 ("focal") from 18.04 ("bionic"). Compare the "qemu" package in both:

  https://packages.ubuntu.com/bionic/qemu
  https://packages.ubuntu.com/focal/qemu

In the former, qemu depends on qemu-system (which depends further on the actual emulator subpackages), in the latter, qemu doesn't depend on anything.

According to <https://github.com/actions/virtual-environments/issues/1816>, we could change:

.azurepipelines/Ubuntu-GCC5.yml:    vm_image: 'ubuntu-latest'
.azurepipelines/Ubuntu-PatchCheck.yml:  vmImage: 'ubuntu-latest'
ArmVirtPkg/PlatformCI/.azurepipelines/Ubuntu-GCC5.yml:      vm_image: 'ubuntu-latest'
EmulatorPkg/PlatformCI/.azurepipelines/Ubuntu-GCC5.yml:      vm_image: 'ubuntu-latest'
OvmfPkg/PlatformCI/.azurepipelines/Ubuntu-GCC5.yml:      vm_image: 'ubuntu-latest'

to "ubuntu-18.04". But perhaps we should change:

ArmVirtPkg/PlatformCI/.azurepipelines/Ubuntu-GCC5.yml:        - bash: sudo apt-get install qemu
OvmfPkg/PlatformCI/.azurepipelines/Ubuntu-GCC5.yml:        - bash: sudo apt-get install qemu

to "qemu-system", instead.

Laszlo

Re: [edk2-devel] [PATCH 0/2] Update SevSecret API to work for TDX

[Laszlo Ersek]

On 12/17/20 20:23, Laszlo Ersek wrote:
> On 12/17/20 19:43, Laszlo Ersek wrote:
> 
>> I tried merging this:
>>
>> https://github.com/tianocore/edk2/pull/1235
>>
>> but the Ubuntu builds all failed. I've checked two logs:
>>
>> https://dev.azure.com/tianocore/edk2-ci/_build/results?buildId=16967&view=logs&j=cf2d8b26-a21c-5c68-abf4-b944c123e462&t=5ffbbe5c-1d3a-55f5-5ef3-8a0ef80d76a1&l=184
>> https://dev.azure.com/tianocore/edk2-ci/_build/results?buildId=16968&view=logs&j=47cf355a-6eb4-51a8-46a8-ff4028bfcac0&t=beedef5d-00d0-5a8c-fa35-57d7319988c2&l=182
>>
>> They say,
>>
>> INFO - /bin/sh: 1: qemu-system-aarch64: not found
>> INFO - /bin/sh: 1: qemu-system-x86_64: not found
> 
> The "Install qemu" tasks earlier seem to complete:
> 
> https://dev.azure.com/tianocore/edk2-ci/_build/results?buildId=16967&view=logs&j=cf2d8b26-a21c-5c68-abf4-b944c123e462&t=a5c654c1-e049-5a30-61a9-da81b8ec031f
> https://dev.azure.com/tianocore/edk2-ci/_build/results?buildId=16968&view=logs&j=47cf355a-6eb4-51a8-46a8-ff4028bfcac0&t=9a629c6e-a36d-5733-3aff-19ed2a42cf75
> 
> However, the qemu "4.2-3ubuntu6.10" package is a dummy package:
> 
>   https://packages.ubuntu.com/focal/qemu
> 
> and as shown under the link, it has no dependency on the packages with the actual qemu executables. So the latter do not get pulled in.
> 
> (Even the logs make that clear: "Need to get 14.3 kB of archives" -- obviously, a real QEMU won't fit in that, and no other packages get pulled in).
> 
> The meta-package that pulls in all system emulators is called "qemu-system":
> 
>   https://packages.ubuntu.com/focal/qemu-system
> 
> What I don't understand at this point is how the CI scripts could work previously.
> 
> ... Aha! I do understand it now. Look at one of the last successful PRs:
> 
>   https://github.com/tianocore/edk2/pull/1232
> 
> The CI logs contain this message:
> 
>   "##[warning]Ubuntu-latest pipelines will use Ubuntu-20.04 soon. For more details, see https://github.com/actions/virtual-environments/issues/1816"
> 
> So let's check out:
> 
>   https://github.com/actions/virtual-environments/issues/1816
> 
> Okay... so it looks like I'm the victim of "Ubuntu-latest" switching to 20.04 ("focal") from 18.04 ("bionic"). Compare the "qemu" package in both:
> 
>   https://packages.ubuntu.com/bionic/qemu
>   https://packages.ubuntu.com/focal/qemu
> 
> In the former, qemu depends on qemu-system (which depends further on the actual emulator subpackages), in the latter, qemu doesn't depend on anything.
> 
> According to <https://github.com/actions/virtual-environments/issues/1816>, we could change:
> 
> .azurepipelines/Ubuntu-GCC5.yml:    vm_image: 'ubuntu-latest'
> .azurepipelines/Ubuntu-PatchCheck.yml:  vmImage: 'ubuntu-latest'
> ArmVirtPkg/PlatformCI/.azurepipelines/Ubuntu-GCC5.yml:      vm_image: 'ubuntu-latest'
> EmulatorPkg/PlatformCI/.azurepipelines/Ubuntu-GCC5.yml:      vm_image: 'ubuntu-latest'
> OvmfPkg/PlatformCI/.azurepipelines/Ubuntu-GCC5.yml:      vm_image: 'ubuntu-latest'
> 
> to "ubuntu-18.04". But perhaps we should change:
> 
> ArmVirtPkg/PlatformCI/.azurepipelines/Ubuntu-GCC5.yml:        - bash: sudo apt-get install qemu
> OvmfPkg/PlatformCI/.azurepipelines/Ubuntu-GCC5.yml:        - bash: sudo apt-get install qemu
> 
> to "qemu-system", instead.

Well, no. I've just tried that:

  https://github.com/tianocore/edk2/pull/1236

and we have two problems with it:

(1) the emulator is now available, but it crashes:

https://dev.azure.com/tianocore/edk2-ci/_build/results?buildId=16975&view=logs&j=cf2d8b26-a21c-5c68-abf4-b944c123e462&t=5ffbbe5c-1d3a-55f5-5ef3-8a0ef80d76a1

"INFO - Segmentation fault (core dumped)"

(2) in spite of this failure, the mergify bot says, "All checks passed.
Auto close personal build." :/

So at the moment we can only go back to the 18.04LTS image. I'll try
that next.

Laszlo

Re: [edk2-devel] [PATCH 0/2] Update SevSecret API to work for TDX

[Laszlo Ersek]

On 12/16/20 02:41, James Bottomley wrote:
> This patch series changes the EFI configuration table information
> which is queried by the bootloader to make it more compatible with
> Intel TDX.  The first patch changes the ABI to make the table contain
> two 64 bit integers instead of two 32 bit ones.  The second patch is a
> cosmetic one to change the names of the GUIDs and tables to have a
> confidential computing prefix instead of a SEV Launch one.
> 
> The first patch *must* be applied before the next stable tag to avoid
> ABI breakage.  The second is purely cosmetic and doesn't change the
> code output.
> 
> Ultimately there will still need to be a TDX collector for the secret,
> which would feed the value into the SecretDxe, but these changes
> should ensure that no further changes would be required by the secret
> consumers.
> 
> James
> 
> ---
> 
> James Bottomley (2):
>   OvmfPkg: Change SEV Launch Secret API to be UINT64 for base and size
>   OvmfPkg/AmdSev/SecretDxe: make secret location naming generic
> 
>  OvmfPkg/OvmfPkg.dec                                |  2 +-
>  OvmfPkg/AmdSev/SecretDxe/SecretDxe.inf             |  2 +-
>  ...aunchSecret.h => ConfidentialComputingSecret.h} | 14 +++++++-------
>  OvmfPkg/AmdSev/SecretDxe/SecretDxe.c               |  6 +++---
>  4 files changed, 12 insertions(+), 12 deletions(-)
>  rename OvmfPkg/Include/Guid/{SevLaunchSecret.h => ConfidentialComputingSecret.h} (65%)
> 

Merged as commit range c487970ac89d..96201ae7bf97, via
<https://github.com/tianocore/edk2/pull/1235>.

Thanks
Laszlo