[edk2-devel] [PATCH V3 06/10] OvmfPkg: Add AmdSev.asm in ResetVector
Min Xu
min.m.xu at intel.com
Thu Jul 29 02:44:52 UTC 2021
Jiewen & Singh
>From the discussion I am thinking we have below rules to follow to the design the structure
of TEE_WORK_AREA:
1. Design should be flexible but not too complicated
2. Reuse the current SEV_ES_WORK_AREA (PcdSevEsWorkAreaBase) as TEE_WORK_AREA
3. TEE_WORK_AREA should be initialized to all-0 at the beginning of ResetVecotr
4. Reduce the changes to exiting code if possible
So I try to make below conclusions below: (Please review)
1. SEV_ES_WORK_AREA is used as the TEE_WORK_AREA by both TDX and SEV, maybe in
the future it can be used by other CC technologies.
2. In MEMFD, add below initial value. So that TEE_WORK_AREA is guaranteed to be cleared
in legacy guest. In TDX this memory region is initialized to be all-0 by host VMM. In SEV the memory
region is cleared as well.
0x00B000|0x001000
gUefiCpuPkgTokenSpaceGuid.PcdSevEsWorkAreaBase|gUefiCpuPkgTokenSpaceGuid.PcdSevEsWorkAreaSize
DATA = {
0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00
}
3. The structure of TEE_WORK_AREA
The current SEV_ES_WORK_AREA is defined as below:
typedef struct {
UINT8 SevEsEnabled;
UINT8 Reserved1[7];
[Others...]
} SEC_SEV_ES_WORK_AREA;
So I think the TEE_WORK_AREA can be:
Byte[0] Type:
0: legacy 1: SEV 2: TDX
Byte[1] Subtype:
If Type is 0, then it is 0
If Type is 1, then it is up to SEV's definition
If Type is 2, then it is up to TDX's definition
Byte[] other bytes are defined based on the Type/Subtype
I check the code in SecMain.c.
SevEsIsEnabled() need updated to check SevEsWorkarea->SevEsEnabled == 1, not non-0.
@Brijesh Singh Is there any other code need update?
> -----Original Message-----
> From: Yao, Jiewen <jiewen.yao at intel.com>
> Sent: Thursday, July 29, 2021 7:49 AM
> To: Brijesh Singh <brijesh.singh at amd.com>; devel at edk2.groups.io; Xu, Min M
> <min.m.xu at intel.com>
> Cc: 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
>
> Comment below:
>
> > -----Original Message-----
> > From: Brijesh Singh <brijesh.singh at amd.com>
> > Sent: Thursday, July 29, 2021 2:59 AM
> > To: Yao, Jiewen <jiewen.yao at intel.com>; devel at edk2.groups.io; Xu, Min
> > M <min.m.xu at intel.com>
> > Cc: brijesh.singh at amd.com; 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
> >
> >
> >
> > On 7/28/21 11:26 AM, Yao, Jiewen wrote:
> > > 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?
> >
> > Hmm, aren't we saying it is a software bug. In that case another bug
> > can also mess up the structure header.
> [Jiewen] Yes. I treat it as a software implementation bug.
> The key thing in software design is to avoid the developer making mistake, help
> debug issues, help maintain the code easily.
>
>
> >
> > >
> > > 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 was trying to see if we can make it work without requiring any
> > changes to UefiCpuPkg etc (which uses the EsWorkArea).
> [Jiewen] I agree with you.
> And I think the priority should be:
> 1) make a good design following the best practice.
> 2) minimize the change.
>
> I don’t think two *enable* fields can satisfy #1.
> And I am open on how to do #2. (See rest comment below)
>
>
>
> >
> >
> > >
> > > 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.
> > >
> >
> > In your below structure, I assume the SEV or TDX probe will set the
> > Type only when it detects that feature is active. But which layer of
> > the software is going to set the type to zero to indicate its a legacy guest ?
> [Jiewen] Good question.
> I expect some initialization function, such as InitCcWorkAreaSev,
> InitCcWorkAreaTdx.
> The default value Legacy shall be override in InitCcWorkArea after capability
> check.
> PreMainFunctionHookXXX is common patter for all function. It just checks the
> CcWorkArea.
>
>
>
> >
> > 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;
> >
> > i.e In this call sequence:
> >
> > OneTimeCall PreMainFunctionHookSev
> > OneTimeCall PreMainFunctionHookTdx
> >
> > ....
> >
> > The PreMainFunctionHookSev will detect whether SEV is active or not.
> > If its active then set the type = MEM_ENCRYPT_SEV; and similarly the
> > Tdx hook will set the type=MEM_ENCRYPT_TDX. What if neither TDX or SEV
> > is enabled. At this time we will depend on hypervisor to ensure that
> > value at that memory is zero.
> [Jiewen] I think we just let InitCcWorkAreaSev and InitCcWorkAreaTdx override
> the default value.
> InitCcWorkArea{Sev,Tdx}:
> // Detect Hardware Capability
> // If discovered, then set Type = {SEV,TDX}
> // Else, leave it as is
>
>
>
>
> >
> > Additionally, do you see a need for the HeadLength field in this
> > structure ? In asm it is preferred if we can access the actual
> > workarea pointer at the fixed location instead of first reading the
> > HeadLength to determine how much it need to skip.
> [Jiewen] You are right.
> Length/Version is NOT absolutely necessary, if the header is simple enough. If
> you think we don’t need them, I am OK to remove them.
> I think only "Type" is mandatory, which tells us the category.
> I think "SubType" is useful, because we might want to know if it is SEV, or SEV-ES,
> or SEV-SNP, and if it is TDX 1.0, or TDX.future. Please let me know your thought.
>
>
> One question on SEC_SEV_ES_WORK_AREA. Since you have SEV/SEV-ES/SEV-
> SNP, is this SEV_ES_WORK_AREA only for SEV_ES? Or it is also used in SEV (no
> SEV_ES), and SEV_SNP ?
> For example, when we see SevEsEnable=0, how do we know it is SEV (no SEV_ES)
> or legacy (no SEV, no SEV_ES)? Or we don’t need care in SEV (no SEV_ES) case.
>
>
>
>
> >
> >
> > > 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 11:59 PM
> > >> To: Yao, Jiewen <jiewen.yao at intel.com>; devel at edk2.groups.io; Xu,
> > >> Min M <min.m.xu at intel.com>
> > >> Cc: brijesh.singh at amd.com; 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 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 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 (#78334): https://edk2.groups.io/g/devel/message/78334
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