Re: [edk2-devel] [PATCH v6 00/29] Add AMD Secure Nested Paging (SEV-SNP) support

["Yao, Jiewen" <jiewen.yao@intel.com>]

I do not see any conflict here.

You can use below definition
 enum {
   CC_ATTR_AMD_SEV = 0x0001,
   CC_ATTR_AMD_SEV_ES =  0x0101,
   CC_ATTR_AMD_SEV_SNP =  0x0201,
   CC_ATTR_INTEL_TDX =  0x0002,
 } ConfidentialComputingAttr;

BTW: Please remove SGX, we don’t need it here.

Thank you
Yao Jiewen

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Brijesh
> Singh via groups.io
> Sent: Thursday, September 9, 2021 6:51 PM
> To: Xu, Min M <min.m.xu@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>;
> devel@edk2.groups.io
> Cc: James Bottomley <jejb@linux.ibm.com>; Tom Lendacky
> <thomas.lendacky@amd.com>; Justen, Jordan L <jordan.l.justen@intel.com>;
> Ard Biesheuvel <ardb+tianocore@kernel.org>; Erdem Aktas
> <erdemaktas@google.com>; Michael Roth <Michael.Roth@amd.com>; Gerd
> Hoffmann <kraxel@redhat.com>
> Subject: Re: [edk2-devel] [PATCH v6 00/29] Add AMD Secure Nested Paging
> (SEV-SNP) support
> 
> Hi Min,
> 
> On 9/8/21 7:31 PM, Xu, Min M wrote:
> > On September 9, 2021 3:46 AM, Brijesh Singh wrote:
> >> Thank you so much Yao for reviewing the patches. Based on some comments
> >> from Gerd I may update code around the reset vector area (mainly use the
> >> metadata format etc). For your comments regarding the introducing a new
> >> PcdConfidentialComputingCategory I will look to see what I can come up
> with
> >> and in UefiCpuPkg I will try to move all the SEV specific functions in new files
> >> (where applicable).
> >>
> > Hi, Brijesh
> > if you are considering to introduce a new PcdConfidentialComputingCategory
> > as Jiewen suggested below:
> >>> 0008-UefiCpuPkg-Define-the-SEV-SNP-specific-dynamic-PCDs
> >>> I really don't like the idea to use BOOL PcdSevEsIsEnabled and
> >> PcdSevSnpIsEnabled.
> >>> Can we define *one* PCD - such as PcdConfidentialComputingCategory?
> >>> We can assign range 0x0000~0xFFFF to AMD SEV, 0x10000~0x1FFFF to Intel
> >> TDX.
> >>> Then SEV=0x0000, SEV-ES=0x0001, SEV-SNP=0x0002, and TDX=0x10000
> >> later.
> >>> I really don't want to keep adding PCD endlessly in the future, like
> >> PcdSevXXXIsEnabled, PcdSevYYYIsEnabled, PcdTdxIsEnabled,
> >> PcdTdx20Enabled, PcdTdx30Enabled, ......
> > I also have some suggestions.
> >
> > As we have below definition in WorkArea.h
> >   typedef struct _CONFIDENTIAL_COMPUTING_WORK_AREA_HEADER {
> >     UINT8                   GuestType;
> >     UINT8                   Reserved1[3];
> >   } CONFIDENTIAL_COMPUTING_WORK_AREA_HEADER;
> >
> > Can we update above CONFIDENTIAL_COMPUTING_WORK_AREA_HEADER to
> below:
> >   typedef struct _CONFIDENTIAL_COMPUTING_WORK_AREA_HEADER {
> >     UINT8                   GuestType;
> >     UINT8                   SubType;               // subtype which indicates SEV-ES, SEV-NP,
> or TDX 1.0, TDX 2.0 etc.
> >     UINT8                   Reserved1[2];
> >   } CONFIDENTIAL_COMPUTING_WORK_AREA_HEADER;
> >
> > The PcdConfidentialComputingCategory can be defined as UINT32, like below:
> >   ## This dynamic PCD indicates the Confidential Computing Category
> >   #  [7:0]   Confidential Computing Category  (0 - Non-Cc, 1 - AmdSev, 2 -
> IntelTdx)
> >   #  [15:8]  Sub-Category (defined by each vendor, SEV-ES, SEV-SNP, or TDX-1.0,
> TDX-2.0, etc)
> >   #  [31:16] Reserved
> >   # @Prompt Confidential Computing Category
> >
> gUefiCpuPkgTokenSpaceGuid.PcdConfidentialComputingCategory|0|UINT32|0x
> 60000018
> >
> > So that we simply copy the
> CONFIDENTIAL_COMPUTING_WORK_AREA_HEADER to
> PcdConfidentialComputingCategory.
> > What's your thought?
> 
> I am not sure if its a good idea to pack a header like above in a 32-bit
> PCD. The caller need to unpack the 32-bit number and perform a bitshit
> etc. Additionally we also need to check for reserved bits being set to
> zero etc. I am more inclined toward something like this:
> 
> enum {
> 
>    /* The guest is running with memory encryption disabled. */
> 
>     CC_ATTR_UNDEF,
> 
>   /* The guest is running with active AMD SEV memory encryption. */
> 
>    CC_ATTR_AMD_SEV,
> 
>   /* The guest is running with active AMD SEV-ES memory encryption. */
> 
>   CC_ATTR_AMD_SEV_ES,
> 
>   /* The guest is running with active AMD SEV-SNP memory encryption. */
> 
>   CC_ATTR_AMD_SEV_SNP,
> 
>  /* The guest is running with active Intel TDX memory encryption. */
> 
>  CC_ATTR_INTEL_TDX,
> 
>  /* The guest is running with active Intel SGX memory encryption. */
> 
>  CC_ATTR_INTEL_SGX,
> 
> } ConfidentialComputingAttr;
> 
> The PcdConfidentialComputingAttr will be set to zero. The OVMF will set
> this dynamic PCD in PEI phase. The UefiCpuPkg will provide a new
> function that can be used by anyone to check the CC guest type.
> 
> BOOLEAN
> 
> CcPlatformHas(enum ConfidentialComputingAttr attr)
> 
> {
> 
>     UINT32 Val;
> 
>     Val = PcdGet32 (PcdConfidentialComputingAttr);
> 
>     return val == attr;
> 
> }
> 
> 
> > Thanks!
> > Min
> 
> 
> 
>