[edk2-devel] [PATCH V3 06/10] OvmfPkg: Add AmdSev.asm in ResetVector
Yao, Jiewen
jiewen.yao at intel.com
Wed Jul 28 15:22:02 UTC 2021
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 at edk2.groups.io <devel at edk2.groups.io> On Behalf Of Brijesh
> Singh via groups.io
> Sent: Wednesday, July 28, 2021 10:34 PM
> To: Yao, Jiewen <jiewen.yao at intel.com>; Xu, Min M <min.m.xu at intel.com>
> Cc: brijesh.singh at amd.com; devel at edk2.groups.io; Ard Biesheuvel
> <ardb+tianocore at kernel.org>; Justen, Jordan L <jordan.l.justen at intel.com>;
> Erdem Aktas <erdemaktas at google.com>; James Bottomley
> <jejb at linux.ibm.com>; Tom Lendacky <thomas.lendacky at 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 at intel.com>
> >> Sent: Wednesday, July 28, 2021 2:58 PM
> >> To: Yao, Jiewen <jiewen.yao at intel.com>
> >> Cc: Brijesh Singh <brijesh.singh at amd.com>; devel at edk2.groups.io; Ard
> >> Biesheuvel <ardb+tianocore at kernel.org>; Justen, Jordan L
> >> <jordan.l.justen at intel.com>; Erdem Aktas <erdemaktas at google.com>;
> James
> >> Bottomley <jejb at linux.ibm.com>; Tom Lendacky
> <thomas.lendacky at 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 at 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
>
>
>
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#78290): https://edk2.groups.io/g/devel/message/78290
Mute This Topic: https://groups.io/mt/84476064/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list