Re: [PATCH V3 06/10] OvmfPkg: Add AmdSev.asm in ResetVector


Yao, Jiewen
 

comment below

thank you!
Yao, Jiewen


在 2021年7月29日,下午12:29,Brijesh Singh via groups.io <brijesh.singh=amd.com@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@intel.com>
Sent: Thursday, July 29, 2021 7:49 AM
To: Brijesh Singh <brijesh.singh@amd.com>; devel@edk2.groups.io; Xu, Min M
<min.m.xu@intel.com>
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>; Justen, Jordan L
<jordan.l.justen@intel.com>; Erdem Aktas <erdemaktas@google.com>; James
Bottomley <jejb@linux.ibm.com>; Tom Lendacky <thomas.lendacky@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@amd.com>
Sent: Thursday, July 29, 2021 2:59 AM
To: Yao, Jiewen <jiewen.yao@intel.com>; devel@edk2.groups.io; Xu, Min
M <min.m.xu@intel.com>
Cc: brijesh.singh@amd.com; Ard Biesheuvel <ardb+tianocore@kernel.org>;
Justen, Jordan L <jordan.l.justen@intel.com>; Erdem Aktas
<erdemaktas@google.com>; James Bottomley <jejb@linux.ibm.com>; Tom
Lendacky <thomas.lendacky@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@edk2.groups.io <devel@edk2.groups.io> On Behalf Of
Brijesh Singh via groups.io
Sent: Wednesday, July 28, 2021 11:59 PM
To: Yao, Jiewen <jiewen.yao@intel.com>; devel@edk2.groups.io; Xu,
Min M <min.m.xu@intel.com>
Cc: brijesh.singh@amd.com; Ard Biesheuvel
<ardb+tianocore@kernel.org>; Justen, Jordan L
<jordan.l.justen@intel.com>; Erdem Aktas <erdemaktas@google.com>;
James Bottomley <jejb@linux.ibm.com>; Tom Lendacky
<thomas.lendacky@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@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.yao@intel.com>; Xu, Min M
<min.m.xu@intel.com>
Cc: brijesh.singh@amd.com; devel@edk2.groups.io; Ard Biesheuvel
<ardb+tianocore@kernel.org>; Justen, Jordan L
<jordan.l.justen@intel.com>;
Erdem Aktas <erdemaktas@google.com>; James Bottomley
<jejb@linux.ibm.com>; Tom Lendacky <thomas.lendacky@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@intel.com>
Sent: Wednesday, July 28, 2021 2:58 PM
To: Yao, Jiewen <jiewen.yao@intel.com>
Cc: Brijesh Singh <brijesh.singh@amd.com>;
devel@edk2.groups.io; Ard Biesheuvel
<ardb+tianocore@kernel.org>; Justen, Jordan L
<jordan.l.justen@intel.com>; Erdem Aktas
<erdemaktas@google.com>;
James
Bottomley <jejb@linux.ibm.com>; Tom Lendacky
<thomas.lendacky@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@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







Join devel@edk2.groups.io to automatically receive all group messages.