[edk2-devel] [PATCH V3 06/10] OvmfPkg: Add AmdSev.asm in ResetVector
Yao, Jiewen
jiewen.yao at intel.com
Thu Jul 29 05:17:08 UTC 2021
comment below
thank you!
Yao, Jiewen
> 在 2021年7月29日，下午12:29，Brijesh Singh via groups.io <brijesh.singh=amd.com at groups.io> 写道：
>
>
>> On 7/28/21 9:44 PM, Xu, Min M wrote:
>> 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
>> }
>
> Hmm, I thought the contents of the data pages are controlled by the host
> VMM. If the backing pages are not zero filled then there is no guarantee
> that memory will be zero. To verify it:
>
> 1. I applied your above change in OvmfPkgX86.fdt. I modified the DATA
> values from 0x00 -> 0xCC
>
> 2. Modified the SecMain.c to dump the SevEsWorkArea on entry
>
> And dump does not contain the 0xcc.
>
> And to confirm further, I attached to the qemu with the GDB before the
> booting the OVMF, and modified the SevEsWorkArea with some garbage
> number and this time the dump printed garbage value I put through the
> debugger.
>
> In summary, the OVMF to zero the workarea memory on the entry and we
> cannot rely on the DATA={0x00, 0x00...} to zero the CCWorkArea.
>
> Did I miss something ?
[jiewen] I am not sure how SEV works.
For TDX, this memory is private memory. VMM or QEMU cannot modify it *after* launch.
VMM or QEMU may modify it *before* launch. That will be detected by attestation later if it is added memory. That will be zeroed with accept page if it is auged memory.
So in TDX, I have no concern.
Anyway, I think the logic can be:
=====
CcWorkArea.Type = 0;
InitCcWorkAreaSev(); // set Type=1 if SEV
InitCcWorkAreaTdx(); // set Type=2 if TDX
=====
>
>
>>
>> 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 personally like Yao Jiewen's struct definition, but I can also live
> with this one as well :). The only question I had was with his proposal
> was what if we remove the Length and Version fields. If the header
> length was fixed then life would be much easier in the ASM code.
[jiewen] I am ok to remove version and length. The we need very carefully maintain the compatibility.
How about below:
typedef struct {
UINT8 Type;
UINT8 SubType;
UINT8 Reserved[6];
} CC_WORK_AREA_HEAD;
That almost aligns with existing SEV_ES.
>
>
>> 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?
>
> As noted before, the SevEsWorkAreas is used to pass the information from
> the Sec to PEI phase. The workarea gets reused for totally different
> purpose after the PEI phase.
[jiewen] Sorry. I am not aware of that.
Any documentation?
Is that SEV specific purpose? Or generic CC purpose?
>
> thanks
>
>>> -----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 (#78342): https://edk2.groups.io/g/devel/message/78342
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