Re: [edk2-devel] [PATCH RFC v2 27/28] OvmfPkg/AmdSev: Expose the SNP reserved pages through configuration table

["Dov Murik" <dovmurik@linux.vnet.ibm.com>]

Hi Brijesh,

On 30/04/2021 14:51, Brijesh Singh wrote:
> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3275
> 
> Now that both the secrets and cpuid pages are reserved in the HOB,
> extract the location details through fixed PCD and make it available
> to the guest OS through the configuration table.
> 
> Cc: James Bottomley <jejb@linux.ibm.com>
> Cc: Min Xu <min.m.xu@intel.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Tom Lendacky <thomas.lendacky@amd.com>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Erdem Aktas <erdemaktas@google.com>
> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
> ---
>  OvmfPkg/AmdSev/SecretDxe/SecretDxe.c               | 21 ++++++++++++++++++++
>  OvmfPkg/AmdSev/SecretDxe/SecretDxe.inf             |  4 ++++
>  OvmfPkg/Include/Guid/ConfidentialComputingSecret.h | 17 ++++++++++++++++
>  OvmfPkg/OvmfPkg.dec                                |  1 +
>  4 files changed, 43 insertions(+)
> 
> diff --git a/OvmfPkg/AmdSev/SecretDxe/SecretDxe.c b/OvmfPkg/AmdSev/SecretDxe/SecretDxe.c
> index 308022b5b2..08b6d9bddf 100644
> --- a/OvmfPkg/AmdSev/SecretDxe/SecretDxe.c
> +++ b/OvmfPkg/AmdSev/SecretDxe/SecretDxe.c
> @@ -6,6 +6,7 @@
>  **/
>  #include <PiDxe.h>
>  #include <Library/UefiBootServicesTableLib.h>
> +#include <Library/MemEncryptSevLib.h>
>  #include <Guid/ConfidentialComputingSecret.h>
> 
>  STATIC CONFIDENTIAL_COMPUTING_SECRET_LOCATION mSecretDxeTable = {
> @@ -13,6 +14,15 @@ STATIC CONFIDENTIAL_COMPUTING_SECRET_LOCATION mSecretDxeTable = {
>    FixedPcdGet32 (PcdSevLaunchSecretSize),
>  };
> 
> +STATIC CONFIDENTIAL_COMPUTING_BLOB_LOCATION mSnpBootDxeTable = {
> +  0x414d4445,     // AMDE

(nit: I believe this UINT32 will appear in memory as the string "EDMA".)



> +  1,

Not sure what's the official stance regarding a version field here. Maybe it's better to generate a new GUID whenever there's a struct change.


-Dov


> +  (UINT64)(UINTN) FixedPcdGet32 (PcdSevLaunchSecretBase),
> +  FixedPcdGet32 (PcdSevLaunchSecretSize),
> +  (UINT64)(UINTN) FixedPcdGet32 (PcdOvmfSnpCpuidBase),
> +  FixedPcdGet32 (PcdOvmfSnpCpuidSize),
> +};
> +
>  EFI_STATUS
>  EFIAPI
>  InitializeSecretDxe(
> @@ -20,6 +30,17 @@ InitializeSecretDxe(
>    IN EFI_SYSTEM_TABLE     *SystemTable
>    )
>  {
> +  //
> +  // If its SEV-SNP active guest then install the CONFIDENTIAL_COMPUTING_BLOB.
> +  // It contains the location for both the Secrets and CPUID page.
> +  //
> +  if (MemEncryptSevSnpIsEnabled ()) {
> +    return gBS->InstallConfigurationTable (
> +                  &gConfidentialComputingBlobGuid,
> +                  &mSnpBootDxeTable
> +                  );
> +  }
> +
>    return gBS->InstallConfigurationTable (
>                  &gConfidentialComputingSecretGuid,
>                  &mSecretDxeTable
> diff --git a/OvmfPkg/AmdSev/SecretDxe/SecretDxe.inf b/OvmfPkg/AmdSev/SecretDxe/SecretDxe.inf
> index 40bda7ff84..d15194b368 100644
> --- a/OvmfPkg/AmdSev/SecretDxe/SecretDxe.inf
> +++ b/OvmfPkg/AmdSev/SecretDxe/SecretDxe.inf
> @@ -23,13 +23,17 @@
>    MdePkg/MdePkg.dec
> 
>  [LibraryClasses]
> +  MemEncryptSevLib
>    UefiBootServicesTableLib
>    UefiDriverEntryPoint
> 
>  [Guids]
>    gConfidentialComputingSecretGuid
> +  gConfidentialComputingBlobGuid
> 
>  [FixedPcd]
> +  gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSnpCpuidBase
> +  gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSnpCpuidSize
>    gUefiOvmfPkgTokenSpaceGuid.PcdSevLaunchSecretBase
>    gUefiOvmfPkgTokenSpaceGuid.PcdSevLaunchSecretSize
> 
> diff --git a/OvmfPkg/Include/Guid/ConfidentialComputingSecret.h b/OvmfPkg/Include/Guid/ConfidentialComputingSecret.h
> index 7026fc5b08..0d7f1b8818 100644
> --- a/OvmfPkg/Include/Guid/ConfidentialComputingSecret.h
> +++ b/OvmfPkg/Include/Guid/ConfidentialComputingSecret.h
> @@ -18,11 +18,28 @@
>      { 0xae, 0x11, 0xb5, 0x1c, 0x7d, 0x33, 0x64, 0x47 }, \
>    }
> 
> +#define CONFIDENTIAL_COMPUTING_BLOB_GUID                \
> +  { 0x067b1f5f,                                         \
> +    0xcf26,                                             \
> +    0x44c5,                                             \
> +    { 0x85, 0x54, 0x93, 0xd7, 0x77, 0x91, 0x2d, 0x42 }, \
> +  }
> +
>  typedef struct {
>    UINT64 Base;
>    UINT64 Size;
>  } CONFIDENTIAL_COMPUTING_SECRET_LOCATION;
> 
> +typedef struct {
> +  UINT32  Header;
> +  UINT16  Version;
> +  UINT64  SecretsPhysicalAddress;
> +  UINT32  SecretsSize;
> +  UINT64  CpuidPhysicalAddress;
> +  UINT32  CpuidLSize;
> +} CONFIDENTIAL_COMPUTING_BLOB_LOCATION;
> +
>  extern EFI_GUID gConfidentialComputingSecretGuid;
> +extern EFI_GUID gConfidentialComputingBlobGuid;
> 
>  #endif // SEV_LAUNCH_SECRET_H_
> diff --git a/OvmfPkg/OvmfPkg.dec b/OvmfPkg/OvmfPkg.dec
> index d1bfe49731..f38c5e476a 100644
> --- a/OvmfPkg/OvmfPkg.dec
> +++ b/OvmfPkg/OvmfPkg.dec
> @@ -126,6 +126,7 @@
>    gQemuKernelLoaderFsMediaGuid          = {0x1428f772, 0xb64a, 0x441e, {0xb8, 0xc3, 0x9e, 0xbd, 0xd7, 0xf8, 0x93, 0xc7}}
>    gGrubFileGuid                         = {0xb5ae312c, 0xbc8a, 0x43b1, {0x9c, 0x62, 0xeb, 0xb8, 0x26, 0xdd, 0x5d, 0x07}}
>    gConfidentialComputingSecretGuid      = {0xadf956ad, 0xe98c, 0x484c, {0xae, 0x11, 0xb5, 0x1c, 0x7d, 0x33, 0x64, 0x47}}
> +  gConfidentialComputingBlobGuid        = {0x067b1f5f, 0xcf26, 0x44c5, {0x85, 0x54, 0x93, 0xd7, 0x77, 0x91, 0x2d, 0x42}}
> 
>  [Ppis]
>    # PPI whose presence in the PPI database signals that the TPM base address
>