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

["Laszlo Ersek" <lersek@redhat.com>]

On 05/05/21 09:10, Dov Murik wrote:
> 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".)

Please consider the SIGNATURE_32() macro.

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

A version scalar is good for compatible changes (= only appending new
fields). Incompatible changes require GUID changes.

(I'll review this patch myself from the scratch later; just making some
quick comments-on-comments for the time being.)

Thanks
Laszlo

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