Re: [PATCH] OvmfPkg/AmdSevDxe: Update ConfidentialComputing blob struct definition

[Michael Roth <michael.roth@amd.com>]

On Fri, May 13, 2022 at 08:32:38AM -0500, Tom Lendacky wrote:
> On 5/13/22 08:22, Michael Roth wrote:
> > The Confidential Computing blob defined here is intended to match the
> > definition defined by linux guest kernel. Previously, both definitions
> > relied on natural alignment, but that relies on both OVMF and kernel
> > being compiled as 64-bit. While there aren't currently any plans to
> > enable SNP support for 32-bit compilations, the kernel definition has
> > since been updated to use explicit padding/reserved fields to avoid
> > this dependency. Update OVMF to match that definition.
> > 
> > No functional changes (for currently-supported environments, at least).
> > 
> > Signed-off-by: Michael Roth <michael.roth@amd.com>
> 
> Minor nit comment below that can be ignored if desired.
> 
> Reviewed-by: Tom Lendacky <thomas.lendacky@amd.com>
> 
> > ---
> >   OvmfPkg/AmdSevDxe/AmdSevDxe.c                          | 2 ++
> >   OvmfPkg/Include/Guid/ConfidentialComputingSevSnpBlob.h | 6 ++++--
> >   2 files changed, 6 insertions(+), 2 deletions(-)
> > 
> > diff --git a/OvmfPkg/AmdSevDxe/AmdSevDxe.c b/OvmfPkg/AmdSevDxe/AmdSevDxe.c
> > index 662d3c4ccb..ee6d2528d9 100644
> > --- a/OvmfPkg/AmdSevDxe/AmdSevDxe.c
> > +++ b/OvmfPkg/AmdSevDxe/AmdSevDxe.c
> > @@ -27,8 +27,10 @@ STATIC CONFIDENTIAL_COMPUTING_SNP_BLOB_LOCATION  mSnpBootDxeTable = {
> >     0,
> >     (UINT64)(UINTN)FixedPcdGet32 (PcdOvmfSnpSecretsBase),
> >     FixedPcdGet32 (PcdOvmfSnpSecretsSize),
> > +  0,
> >     (UINT64)(UINTN)FixedPcdGet32 (PcdOvmfCpuidBase),
> >     FixedPcdGet32 (PcdOvmfCpuidSize),
> > +  0,
> >   };
> >   EFI_STATUS
> > diff --git a/OvmfPkg/Include/Guid/ConfidentialComputingSevSnpBlob.h b/OvmfPkg/Include/Guid/ConfidentialComputingSevSnpBlob.h
> > index b328310fd0..83620e31b8 100644
> > --- a/OvmfPkg/Include/Guid/ConfidentialComputingSevSnpBlob.h
> > +++ b/OvmfPkg/Include/Guid/ConfidentialComputingSevSnpBlob.h
> > @@ -18,14 +18,16 @@
> >       { 0x85, 0x54, 0x93, 0xd7, 0x77, 0x91, 0x2d, 0x42 }, \
> >     }
> > -typedef struct {
> > +typedef PACKED struct {
> >     UINT32    Header;
> >     UINT16    Version;
> > -  UINT16    Reserved1;
> > +  UINT16    Reserved;
> 
> Not to be picky, but I would have left this as Reserved1 and then made the
> below entries Reserved2 and Reserved3.

Hi Tom,

I updated those to match how the reserved fields are numbered in the
kernel since it seemed like it could cause confusion otherwise. I should
have noted that in the commit log though as it's a somewhat unrelated
change.

Thanks!

-Mike

> 
> Thanks,
> Tom
> 
> >     UINT64    SecretsPhysicalAddress;
> >     UINT32    SecretsSize;
> > +  UINT32    Reserved1;
> >     UINT64    CpuidPhysicalAddress;
> >     UINT32    CpuidLSize;
> > +  UINT32    Reserved2;
> >   } CONFIDENTIAL_COMPUTING_SNP_BLOB_LOCATION;
> >   extern EFI_GUID  gConfidentialComputingSevSnpBlobGuid;