Re: [edk2-devel] [PATCH V3 06/10] OvmfPkg: Add AmdSev.asm in ResetVector

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

Hi Brijesh
Thanks!

I think if we want to reuse this, we need rename the data structure.

First, we should use a generic name.

Second, I don’t think it is good idea to define two *enable* fields. Since only one of them should be enabled, we should use 1 field as enumeration.

Third, we should hide the SEV specific and TDX specific definition in CC common work area.

If we agree to use a common work area, I recommend below:

typedef struct {
   UINT8    HeaderVersion; // 0
   UINT8    HeadLength; // 4
   UINT8    Type; // 0 - legacy, 1 - SEV, 2 - TDX
   UINT8    SubType; // Type specific sub type, if needed.
} CC_COMMON_WORK_AREA_HEADER;

typedef struct {
   CC_COMMON_WORK_AREA_HEADER Header;
   // reset is valid if Type == 1
   UINT8    Reserved1[4];
   UINT64   RandomData;
   UINT64   EncryptionMask;
} SEC_SEV_ES_WORK_AREA;

typedef struct {
   CC_COMMON_WORK_AREA_HEADER Header;
   // reset is valid if Type == 2
   UINT8    TdxSpecific[];  // TBD
} TDX_WORK_AREA;

Thank you
Yao Jiewen

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Brijesh
> Singh via groups.io
> Sent: Wednesday, July 28, 2021 10:34 PM
> To: Yao, Jiewen <jiewen.yao@intel.com>; Xu, Min M <min.m.xu@intel.com>
> Cc: brijesh.singh@amd.com; devel@edk2.groups.io; Ard Biesheuvel
> <ardb+tianocore@kernel.org>; Justen, Jordan L <jordan.l.justen@intel.com>;
> Erdem Aktas <erdemaktas@google.com>; James Bottomley
> <jejb@linux.ibm.com>; Tom Lendacky <thomas.lendacky@amd.com>
> Subject: Re: [edk2-devel] [PATCH V3 06/10] OvmfPkg: Add AmdSev.asm in
> ResetVector
> 
> Hi Jiewen and Min,
> 
> See my comments below.
> 
> 
> On 7/28/21 2:54 AM, Yao, Jiewen wrote:
> > Yes. I am thinking the same thing.
> >
> > [CC Flag memory location]
> > 1) A general purpose register, such as EBP.
> >
> > 2) A global variable, such as
> > .data
> > TeeFlags: DD 0
> >
> > 3) A fixed region in stack, such as
> > dword[STACK_TOP - 4]
> >
> > 4) A new CC common fixed region, such as
> > dword[CC_COMMON_FLAGS]
> >
> > 5) A fixed region piggyback on existing CC working area, such as
> > dword[CC_WORKING_AREA]
> >
> > Hi Brijesh/Min
> > Any preference?
> >
> > [CC Indicator Flags]
> > Proposal: UINT8[4]
> >
> > Byte [0] Version: 0
> > byte [1] Length: 4
> > byte [2] Type:
> > 	0: legacy
> > 	1: SEV
> > 	2: TDX
> > byte [3] Sub Type:
> > 	If Type is 0 (legacy), then
> > 		0: legacy
> > 	If Type is 1 (SEV), then
> > 		0: SEV
> > 		1: SEV-ES
> > 		2: SEV-SNP
> > 	If Type is 2 (TDX), then
> > 		0: TDX 1.0
> >
> > Thank you
> > Yao Jiewen
> >
> >
> >> -----Original Message-----
> >> From: Xu, Min M <min.m.xu@intel.com>
> >> Sent: Wednesday, July 28, 2021 2:58 PM
> >> To: Yao, Jiewen <jiewen.yao@intel.com>
> >> Cc: Brijesh Singh <brijesh.singh@amd.com>; devel@edk2.groups.io; Ard
> >> Biesheuvel <ardb+tianocore@kernel.org>; Justen, Jordan L
> >> <jordan.l.justen@intel.com>; Erdem Aktas <erdemaktas@google.com>;
> James
> >> Bottomley <jejb@linux.ibm.com>; Tom Lendacky
> <thomas.lendacky@amd.com>
> >> Subject: RE: [PATCH V3 06/10] OvmfPkg: Add AmdSev.asm in ResetVector
> >>
> >> On July 28, 2021 2:05 PM, Yao, Jiewen wrote:
> >>> It does not necessary to be a working area.
> >>>
> >>> We just need a common TEE flag to indicate if the system run in legacy, SEV,
> >> or
> >>> TDX, right?
> >> Right. We need somewhere to store this flag, either in a Register or in
> Memory.
> >> If it is memory, then in Tdx the memory region should be initialized by host
> VMM.
> >>>
> >>> thank you!
> >>> Yao, Jiewen
> >>>
> >>>
> >>>> 在 2021年7月28日，下午1:07，Xu, Min M <min.m.xu@intel.com> 写
> 道：
> >>>>
> >>>> On July 27, 2021 8:46 PM, Yao, Jiewen wrote:
> >>>>> HI Min
> >>>>> I agree with Brijesh.
> >>>>>
> >>>>> The basic rule is: SEV file shall never refer to TDX data structure.
> >>>>> TDX file shall never refer to SEV data structure.
> >>>>> These code should be isolated clearly.
> >>>>>
> >>>>> Do we still need that logic if we follow the new pattern?
> >>>> I have replied to Brijesh's mail about the concern of the new pattern.
> >>>>
> >>>> I have some concern in the current pattern:
> >>>> ====================
> >>>>     OneTimeCall   PreMainFunctionHookSev
> >>>>     OneTimeCall   PreMainFunctionHookTdx
> >>>> MainFunction:
> >>>>     XXXXXX
> >>>>     OneTimeCall   PostMainFunctionHookSev
> >>>>     OneTimeCall   PostMainFunctionHookTdx
> >>>> ====================
> >>>> The TEE function need implement a TEE check function (such as IsSev, or
> >> IsTdx).
> >>>>
> >>>> Tdx call CPUID(0x21) to determine if it is tdx guest in the very
> >>>> beginning of ResetVector. Then 'TDXG' is set in TDX_WORK_AREA. SEV
> does
> >>> the similar work which call CheckSevFeatures to set SEV_ES_WORK_AREA to
> 1.
> >>>>
> >>>> After that both TDX and SEV read the above WORK_AREA to check if it is
> TDX
> >>> or SEV or legacy guest.
> >>>>
> >>>> In Tdx the access to SEV_ES_WORK_AREA will trigger error because
> >>> SEV_ES_WORK_AREA is *NOT* initialized by host VMM.
> >>>> In SEV-SNP I am afraid the access to TDX_WORK_AREA will trigger error
> too.
> >>>>
> >>>> I am wondering if TDX and SEV can use the same memory region (for
> >> example,
> >>> TEE_WORK_AREA) as the work area?
> >>>> So that this work area is guaranteed to be initialized in both TDX and
> >>>> SEV. Structure of the TEE_WORK_AREA may look like this:
> >>>>   typedef struct {
> >>>>       UINT8  Flag[4];         'TDXG' or 'SEVG' or all-0
> >>>>       UINT8  Others[];
> >>>>   } TEE_WORK_AREA;
> >>>>>
> 
> Are we reserving a new page for the TDX_WORK_AREA ? I am wondering why
> can't we use the SEV_ES_WORK_AREA instead of wasting space in the MEMFD.
> 
> The SEV_ES_WORK_AREA layout looks like this:
> 
> typedef struct _SEC_SEV_ES_WORK_AREA {
>    UINT8    SevEsEnabled;
>    UINT8    Reserved1[7];
> 
>    UINT64   RandomData;
> 
>    UINT64   EncryptionMask;
> } SEC_SEV_ES_WORK_AREA;
> 
> There is reserved bit after the SevEsEnabled and one byte can be used
> for the TdxEnabled;
> 
> typedef struct _SEC_SEV_ES_WORK_AREA {
>    UINT8    SevEsEnabled;
>    UINT8    TdxEnabled;
>    UINT8    Reserved2[6];
> 
>    UINT64   RandomData;
> 
>    UINT64   EncryptionMask;
> } SEC_SEV_ES_WORK_AREA;
> 
> The SEV_ES_WORK_AREA can be treated as a TEE_WORK_AREA and we can be
> pull out from MemEncrypSevLib.h to CcWorkAreaLib.h and rename the
> structure (if needed).
> 
> Both the SEV-SNP and TEE host-VMM accepts the TEE_WORK_AREA before
> booting the guest to ensure that its safe to access the memory without
> going through the accept/validation process.
> 
> In case of the TDX, the reset vector code sets the TdxEnabled on the
> entry. In case of the SEV, the workarea is valid from SEC to PEI phase
> only and it gets reused for other purposes. The PEI phase set the Pcd's
> (such as SevEsEnabled or SevEnabled etc) so that Dxe or other EDK2 core
> does not need to know anything about the workarea and they simply can
> read the PCDs.
> 
> -Brijesh
> 
> 
> 
>