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

[Brijesh Singh via groups.io]

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 (#78287): https://edk2.groups.io/g/devel/message/78287
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]
-=-=-=-=-=-=-=-=-=-=-=-