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 <j...@linux.ibm.com>
>> Cc: Min Xu <min.m...@intel.com>
>> Cc: Jiewen Yao <jiewen....@intel.com>
>> Cc: Tom Lendacky <thomas.lenda...@amd.com>
>> Cc: Jordan Justen <jordan.l.jus...@intel.com>
>> Cc: Ard Biesheuvel <ardb+tianoc...@kernel.org>
>> Cc: Laszlo Ersek <ler...@redhat.com>
>> Cc: Erdem Aktas <erdemak...@google.com>
>> Signed-off-by: Brijesh Singh <brijesh.si...@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
>>
>
>
>
>
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#74774): https://edk2.groups.io/g/devel/message/74774
Mute This Topic: https://groups.io/mt/82479084/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
