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

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

I would say it is NOT the best software practice to define 2 enable fields to indicate one type.

What if some software error, set both TdxEnable and SevEnable at same time?
How do you detect such error?

If some code may check the SEV only, and some code may check TDX only, then both code can run. That will be a disaster. The code is hard to maintain and hard to debug.

Another consideration is: what if someone wants to set the third type?
Do we want to reserve the 3rd byte? To indicate the third type? It is not scalable.

The best software practice it to just define one field as enumeration. So any software can only set Tdx or Sev. The result is consistent, no matter you check the SEV at first or TDX at first.
If there is 3rd bytes, we just need assign the 3rd value to it, without impact any other field.


I think we can add "must zero" in the comment. But it does not mean there will be no error during development.

UNION is not a type safe structure. Usually, the consumer of UNION shall refer to a common field to know what the type of union is - I treat that as header.

Since we are defining a common data structure, I think we can do some clean up.
Or if you have concern to change SEC_SEV_ES_WORK_AREA, we can define a new CC WORK_AREA without touching SEV_SEV_ES_WORKING_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 11:59 PM
> To: Yao, Jiewen <jiewen.yao@intel.com>; devel@edk2.groups.io; Xu, Min M
> <min.m.xu@intel.com>
> Cc: brijesh.singh@amd.com; 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 Yao Jiewen,
> 
> I guess I am still trying to figure out why we need the header in the
> work area. Can't we do something like this:
> 
> typedef struct {
> 	UINT8    SevEsEnabled;
> 
> 	// If Es is enabled then this field must be zeroed
> 	UINT8    MustBeZero;
> 
> 	UINT8    Reserved1[6];
> 
> 	UINT64   RandomData;
> 
> 	UINT64   EncryptionMask;
> } SEC_SEV_ES_WORK_AREA;
> 
> typedef struct {
> 	// If Tdx is enabled then it must be zeroed
> 	UINT8    MustBeZero
> 
> 	UINT8    TdxEnabled;
> 
> 	UINT8    Reserved2[6];
> 	....
> 
> } TX_WORK_AREA;
> 
> typedef union {
> 	SEC_SEV_ES_WORK_AREA SevEsWorkArea;
> 	TDX_WORK_AREA	     TdxWorkArea;
> } CC_WORK_AREA;
> 
> I am trying to minimize the changes to the existing code. The SEV and
> TDX probe logic should ensure that if the feature is detected, then it
> must clear the MustBeZero'ed field.
> 
> Basically, we already have a 64-bit value reserved in the SevEsWork area
> and currently only one byte is used and second byte can be detected for
> the TDX. Basically the which encryption technology is active the
> definition of the work area will change.
> 
> Am I missing something ?
> 
> Thanks
> 
> On 7/28/21 10:22 AM, Yao, Jiewen wrote:
> > 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
> >>
> >>
> >>
> >>
> >
> 
> 
> 
>