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....@intel.com>; Xu, Min M <min.m...@intel.com>
Cc: brijesh.si...@amd.com; devel@edk2.groups.io; Ard Biesheuvel
<ardb+tianoc...@kernel.org>; Justen, Jordan L <jordan.l.jus...@intel.com>;
Erdem Aktas <erdemak...@google.com>; James Bottomley
<j...@linux.ibm.com>; Tom Lendacky <thomas.lenda...@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...@intel.com>
Sent: Wednesday, July 28, 2021 2:58 PM
To: Yao, Jiewen <jiewen....@intel.com>
Cc: Brijesh Singh <brijesh.si...@amd.com>; devel@edk2.groups.io; Ard
Biesheuvel <ardb+tianoc...@kernel.org>; Justen, Jordan L
<jordan.l.jus...@intel.com>; Erdem Aktas <erdemak...@google.com>;
James
Bottomley <j...@linux.ibm.com>; Tom Lendacky
<thomas.lenda...@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...@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 (#78297): https://edk2.groups.io/g/devel/message/78297
Mute This Topic: https://groups.io/mt/84476064/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
