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


Min Xu
 

RFC: https://bugzilla.tianocore.org/show_bug.cgi?id=3429

AmdSev.asm includes below routines:
- CheckSevFeatures
Check if Secure Encrypted Virtualization (SEV) features are enabled.
- PreSetCr3ForPageTables64Sev
It is called before SetCr3ForPageTables64 in SEV guests.
- PostSetCr3PageTables64Sev
It is called after SetCr3PageTables64 in SEV guests.
- PostJump64BitAndLandHereSev
It is called after Jump64BitAndLandHere in SEV guests.
- #VC exception handling routines

These routines are extracted from PageTables64.asm and Flat32ToFlat64.asm
Need AMD engineers' help to review/validate the patch so that there is
no regression. Thanks in advance!

Note:
In above Pre/Post routines, dword[TDX_WORK_AREA] should be checked
to see if it is 'TDXG' (Tdx guests). This is because some memory region
for example, byte[SEV_ES_WORK_AREA] cannot be accessed in Tdx guests.
Tdx requires that any memory region to be accessed should be accepted
first or initialized by host VMM before Td guest is launched.

Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Brijesh Singh <brijesh.singh@amd.com>
Cc: Erdem Aktas <erdemaktas@google.com>
Cc: James Bottomley <jejb@linux.ibm.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Signed-off-by: Min Xu <min.m.xu@intel.com>
---
OvmfPkg/ResetVector/Ia32/AmdSev.asm | 526 ++++++++++++++++++++++++++++
1 file changed, 526 insertions(+)
create mode 100644 OvmfPkg/ResetVector/Ia32/AmdSev.asm

diff --git a/OvmfPkg/ResetVector/Ia32/AmdSev.asm b/OvmfPkg/ResetVector/Ia32/AmdSev.asm
new file mode 100644
index 000000000000..962b7e169c61
--- /dev/null
+++ b/OvmfPkg/ResetVector/Ia32/AmdSev.asm
@@ -0,0 +1,526 @@
+;------------------------------------------------------------------------------
+; @file
+; AMD SEV routines
+;
+; Copyright (c) 2020, Advanced Micro Devices, Inc. All rights reserved.<BR>
+; Copyright (c) 2021, Intel Corporation. All rights reserved.<BR>
+;
+; SPDX-License-Identifier: BSD-2-Clause-Patent
+;
+;------------------------------------------------------------------------------
+
+
+BITS 32
+;
+; SEV-ES #VC exception handler support
+;
+; #VC handler local variable locations
+;
+%define VC_CPUID_RESULT_EAX 0
+%define VC_CPUID_RESULT_EBX 4
+%define VC_CPUID_RESULT_ECX 8
+%define VC_CPUID_RESULT_EDX 12
+%define VC_GHCB_MSR_EDX 16
+%define VC_GHCB_MSR_EAX 20
+%define VC_CPUID_REQUEST_REGISTER 24
+%define VC_CPUID_FUNCTION 28
+
+; #VC handler total local variable size
+;
+%define VC_VARIABLE_SIZE 32
+
+; #VC handler GHCB CPUID request/response protocol values
+;
+%define GHCB_CPUID_REQUEST 4
+%define GHCB_CPUID_RESPONSE 5
+%define GHCB_CPUID_REGISTER_SHIFT 30
+%define CPUID_INSN_LEN 2
+
+;
+; Check if Secure Encrypted Virtualization (SEV) features are enabled.
+;
+; Register usage is tight in this routine, so multiple calls for the
+; same CPUID and MSR data are performed to keep things simple.
+;
+; Modified: EAX, EBX, ECX, EDX, ESP
+;
+; If SEV is enabled then EAX will be at least 32.
+; If SEV is disabled then EAX will be zero.
+;
+CheckSevFeatures:
+ ; Set the first byte of the workarea to zero to communicate to the SEC
+ ; phase that SEV-ES is not enabled. If SEV-ES is enabled, the CPUID
+ ; instruction will trigger a #VC exception where the first byte of the
+ ; workarea will be set to one or, if CPUID is not being intercepted,
+ ; the MSR check below will set the first byte of the workarea to one.
+ mov byte[SEV_ES_WORK_AREA], 0
+
+ ;
+ ; Set up exception handlers to check for SEV-ES
+ ; Load temporary RAM stack based on PCDs (see SevEsIdtVmmComm for
+ ; stack usage)
+ ; Establish exception handlers
+ ;
+ mov esp, SEV_ES_VC_TOP_OF_STACK
+ mov eax, ADDR_OF(Idtr)
+ lidt [cs:eax]
+
+ ; Check if we have a valid (0x8000_001F) CPUID leaf
+ ; CPUID raises a #VC exception if running as an SEV-ES guest
+ mov eax, 0x80000000
+ cpuid
+
+ ; This check should fail on Intel or Non SEV AMD CPUs. In future if
+ ; Intel CPUs supports this CPUID leaf then we are guranteed to have exact
+ ; same bit definition.
+ cmp eax, 0x8000001f
+ jl NoSev
+
+ ; Check for SEV memory encryption feature:
+ ; CPUID Fn8000_001F[EAX] - Bit 1
+ ; CPUID raises a #VC exception if running as an SEV-ES guest
+ mov eax, 0x8000001f
+ cpuid
+ bt eax, 1
+ jnc NoSev
+
+ ; Check if SEV memory encryption is enabled
+ ; MSR_0xC0010131 - Bit 0 (SEV enabled)
+ mov ecx, 0xc0010131
+ rdmsr
+ bt eax, 0
+ jnc NoSev
+
+ ; Check for SEV-ES memory encryption feature:
+ ; CPUID Fn8000_001F[EAX] - Bit 3
+ ; CPUID raises a #VC exception if running as an SEV-ES guest
+ mov eax, 0x8000001f
+ cpuid
+ bt eax, 3
+ jnc GetSevEncBit
+
+ ; Check if SEV-ES is enabled
+ ; MSR_0xC0010131 - Bit 1 (SEV-ES enabled)
+ mov ecx, 0xc0010131
+ rdmsr
+ bt eax, 1
+ jnc GetSevEncBit
+
+ ; Set the first byte of the workarea to one to communicate to the SEC
+ ; phase that SEV-ES is enabled.
+ mov byte[SEV_ES_WORK_AREA], 1
+
+GetSevEncBit:
+ ; Get pte bit position to enable memory encryption
+ ; CPUID Fn8000_001F[EBX] - Bits 5:0
+ ;
+ and ebx, 0x3f
+ mov eax, ebx
+
+ ; The encryption bit position is always above 31
+ sub ebx, 32
+ jns SevSaveMask
+
+ ; Encryption bit was reported as 31 or below, enter a HLT loop
+SevEncBitLowHlt:
+ cli
+ hlt
+ jmp SevEncBitLowHlt
+
+SevSaveMask:
+ xor edx, edx
+ bts edx, ebx
+
+ mov dword[SEV_ES_WORK_AREA_ENC_MASK], 0
+ mov dword[SEV_ES_WORK_AREA_ENC_MASK + 4], edx
+ jmp SevExit
+
+NoSev:
+ ;
+ ; Perform an SEV-ES sanity check by seeing if a #VC exception occurred.
+ ;
+ cmp byte[SEV_ES_WORK_AREA], 0
+ jz NoSevPass
+
+ ;
+ ; A #VC was received, yet CPUID indicates no SEV-ES support, something
+ ; isn't right.
+ ;
+NoSevEsVcHlt:
+ cli
+ hlt
+ jmp NoSevEsVcHlt
+
+NoSevPass:
+ xor eax, eax
+
+SevExit:
+ ;
+ ; Clear exception handlers and stack
+ ;
+ push eax
+ mov eax, ADDR_OF(IdtrClear)
+ lidt [cs:eax]
+ pop eax
+ mov esp, 0
+
+ OneTimeCallRet CheckSevFeatures
+
+;
+; Called before SetCr3ForPageTables64 in SEV guests
+;
+; Modified: EAX, EBX, ECX, EDX, ESP
+;
+PreSetCr3ForPageTables64Sev:
+ ; In Tdx TDX_WORK_AREA was set to 'TDXG'.
+ ; CheckSevFeatures cannot be called in Tdx guest because SEV_ES_WORK_AREA
+ ; cannot be accessed in this situation. Any memory region to be accessed
+ ; in Td guest should be accepted first.
+ cmp dword[TDX_WORK_AREA], 0x47584454 ; 'TDXG'
+ jz ExitPreSetCr3ForPageTables64Sev
+
+ OneTimeCall CheckSevFeatures
+
+ExitPreSetCr3ForPageTables64Sev:
+ OneTimeCallRet PreSetCr3ForPageTables64Sev
+
+;
+; It is called in SEV after SetCr3PageTables64
+;
+; Modified: EAX, EBX, ECX, EDX, ESP
+;
+PostSetCr3PageTables64Sev:
+ ; In Tdx TDX_WORK_AREA was set to 'TDXG'.
+ ; CheckSevFeatures cannot be called in Tdx because SEV_ES_WORK_AREA
+ ; cannot be accessed in this situation. Any memory region to be accessed
+ ; in Td guest should be accepted first.
+ cmp dword[TDX_WORK_AREA], 0x47584454 ; 'TDXG'
+ jz ExitPostSetCr3PageTables64Sev
+
+ mov eax, cr4
+ bts eax, 5 ; enable PAE
+ mov cr4, eax
+
+ mov ecx, 0xc0000080
+ rdmsr
+ bts eax, 8 ; set LME
+ wrmsr
+
+ ;
+ ; SEV-ES mitigation check support
+ ;
+ xor ebx, ebx
+
+ cmp byte[SEV_ES_WORK_AREA], 0
+ jz EnablePaging
+
+ ;
+ ; SEV-ES is active, perform a quick sanity check against the reported
+ ; encryption bit position. This is to help mitigate against attacks where
+ ; the hypervisor reports an incorrect encryption bit position.
+ ;
+ ; This is the first step in a two step process. Before paging is enabled
+ ; writes to memory are encrypted. Using the RDRAND instruction (available
+ ; on all SEV capable processors), write 64-bits of random data to the
+ ; SEV_ES_WORK_AREA and maintain the random data in registers (register
+ ; state is protected under SEV-ES). This will be used in the second step.
+ ;
+RdRand1:
+ rdrand ecx
+ jnc RdRand1
+ mov dword[SEV_ES_WORK_AREA_RDRAND], ecx
+RdRand2:
+ rdrand edx
+ jnc RdRand2
+ mov dword[SEV_ES_WORK_AREA_RDRAND + 4], edx
+
+ ;
+ ; Use EBX instead of the SEV_ES_WORK_AREA memory to determine whether to
+ ; perform the second step.
+ ;
+ mov ebx, 1
+
+EnablePaging:
+ mov eax, cr0
+ bts eax, 31 ; set PG
+ mov cr0, eax ; enable paging
+
+ExitPostSetCr3PageTables64Sev:
+
+ OneTimeCallRet PostSetCr3PageTables64Sev
+
+;
+; Start of #VC exception handling routines
+;
+
+SevEsIdtNotCpuid:
+ ;
+ ; Use VMGEXIT to request termination.
+ ; 1 - #VC was not for CPUID
+ ;
+ mov eax, 1
+ jmp SevEsIdtTerminate
+
+SevEsIdtNoCpuidResponse:
+ ;
+ ; Use VMGEXIT to request termination.
+ ; 2 - GHCB_CPUID_RESPONSE not received
+ ;
+ mov eax, 2
+
+SevEsIdtTerminate:
+ ;
+ ; Use VMGEXIT to request termination. At this point the reason code is
+ ; located in EAX, so shift it left 16 bits to the proper location.
+ ;
+ ; EAX[11:0] => 0x100 - request termination
+ ; EAX[15:12] => 0x1 - OVMF
+ ; EAX[23:16] => 0xXX - REASON CODE
+ ;
+ shl eax, 16
+ or eax, 0x1100
+ xor edx, edx
+ mov ecx, 0xc0010130
+ wrmsr
+ ;
+ ; Issue VMGEXIT - NASM doesn't support the vmmcall instruction in 32-bit
+ ; mode, so work around this by temporarily switching to 64-bit mode.
+ ;
+BITS 64
+ rep vmmcall
+BITS 32
+
+ ;
+ ; We shouldn't come back from the VMGEXIT, but if we do, just loop.
+ ;
+SevEsIdtHlt:
+ hlt
+ jmp SevEsIdtHlt
+ iret
+
+ ;
+ ; Total stack usage for the #VC handler is 44 bytes:
+ ; - 12 bytes for the exception IRET (after popping error code)
+ ; - 32 bytes for the local variables.
+ ;
+SevEsIdtVmmComm:
+ ;
+ ; If we're here, then we are an SEV-ES guest and this
+ ; was triggered by a CPUID instruction
+ ;
+ ; Set the first byte of the workarea to one to communicate that
+ ; a #VC was taken.
+ mov byte[SEV_ES_WORK_AREA], 1
+
+ pop ecx ; Error code
+ cmp ecx, 0x72 ; Be sure it was CPUID
+ jne SevEsIdtNotCpuid
+
+ ; Set up local variable room on the stack
+ ; CPUID function : + 28
+ ; CPUID request register : + 24
+ ; GHCB MSR (EAX) : + 20
+ ; GHCB MSR (EDX) : + 16
+ ; CPUID result (EDX) : + 12
+ ; CPUID result (ECX) : + 8
+ ; CPUID result (EBX) : + 4
+ ; CPUID result (EAX) : + 0
+ sub esp, VC_VARIABLE_SIZE
+
+ ; Save the CPUID function being requested
+ mov [esp + VC_CPUID_FUNCTION], eax
+
+ ; The GHCB CPUID protocol uses the following mapping to request
+ ; a specific register:
+ ; 0 => EAX, 1 => EBX, 2 => ECX, 3 => EDX
+ ;
+ ; Set EAX as the first register to request. This will also be used as a
+ ; loop variable to request all register values (EAX to EDX).
+ xor eax, eax
+ mov [esp + VC_CPUID_REQUEST_REGISTER], eax
+
+ ; Save current GHCB MSR value
+ mov ecx, 0xc0010130
+ rdmsr
+ mov [esp + VC_GHCB_MSR_EAX], eax
+ mov [esp + VC_GHCB_MSR_EDX], edx
+
+NextReg:
+ ;
+ ; Setup GHCB MSR
+ ; GHCB_MSR[63:32] = CPUID function
+ ; GHCB_MSR[31:30] = CPUID register
+ ; GHCB_MSR[11:0] = CPUID request protocol
+ ;
+ mov eax, [esp + VC_CPUID_REQUEST_REGISTER]
+ cmp eax, 4
+ jge VmmDone
+
+ shl eax, GHCB_CPUID_REGISTER_SHIFT
+ or eax, GHCB_CPUID_REQUEST
+ mov edx, [esp + VC_CPUID_FUNCTION]
+ mov ecx, 0xc0010130
+ wrmsr
+
+ ;
+ ; Issue VMGEXIT - NASM doesn't support the vmmcall instruction in 32-bit
+ ; mode, so work around this by temporarily switching to 64-bit mode.
+ ;
+BITS 64
+ rep vmmcall
+BITS 32
+
+ ;
+ ; Read GHCB MSR
+ ; GHCB_MSR[63:32] = CPUID register value
+ ; GHCB_MSR[31:30] = CPUID register
+ ; GHCB_MSR[11:0] = CPUID response protocol
+ ;
+ mov ecx, 0xc0010130
+ rdmsr
+ mov ecx, eax
+ and ecx, 0xfff
+ cmp ecx, GHCB_CPUID_RESPONSE
+ jne SevEsIdtNoCpuidResponse
+
+ ; Save returned value
+ shr eax, GHCB_CPUID_REGISTER_SHIFT
+ mov [esp + eax * 4], edx
+
+ ; Next register
+ inc word [esp + VC_CPUID_REQUEST_REGISTER]
+
+ jmp NextReg
+
+VmmDone:
+ ;
+ ; At this point we have all CPUID register values. Restore the GHCB MSR,
+ ; set the return register values and return.
+ ;
+ mov eax, [esp + VC_GHCB_MSR_EAX]
+ mov edx, [esp + VC_GHCB_MSR_EDX]
+ mov ecx, 0xc0010130
+ wrmsr
+
+ mov eax, [esp + VC_CPUID_RESULT_EAX]
+ mov ebx, [esp + VC_CPUID_RESULT_EBX]
+ mov ecx, [esp + VC_CPUID_RESULT_ECX]
+ mov edx, [esp + VC_CPUID_RESULT_EDX]
+
+ add esp, VC_VARIABLE_SIZE
+
+ ; Update the EIP value to skip over the now handled CPUID instruction
+ ; (the CPUID instruction has a length of 2)
+ add word [esp], CPUID_INSN_LEN
+ iret
+
+ALIGN 2
+
+Idtr:
+ dw IDT_END - IDT_BASE - 1 ; Limit
+ dd ADDR_OF(IDT_BASE) ; Base
+
+IdtrClear:
+ dw 0 ; Limit
+ dd 0 ; Base
+
+ALIGN 16
+
+;
+; The Interrupt Descriptor Table (IDT)
+; This will be used to determine if SEV-ES is enabled. Upon execution
+; of the CPUID instruction, a VMM Communication Exception will occur.
+; This will tell us if SEV-ES is enabled. We can use the current value
+; of the GHCB MSR to determine the SEV attributes.
+;
+IDT_BASE:
+;
+; Vectors 0 - 28 (No handlers)
+;
+%rep 29
+ dw 0 ; Offset low bits 15..0
+ dw 0x10 ; Selector
+ db 0 ; Reserved
+ db 0x8E ; Gate Type (IA32_IDT_GATE_TYPE_INTERRUPT_32)
+ dw 0 ; Offset high bits 31..16
+%endrep
+;
+; Vector 29 (VMM Communication Exception)
+;
+ dw (ADDR_OF(SevEsIdtVmmComm) & 0xffff) ; Offset low bits 15..0
+ dw 0x10 ; Selector
+ db 0 ; Reserved
+ db 0x8E ; Gate Type (IA32_IDT_GATE_TYPE_INTERRUPT_32)
+ dw (ADDR_OF(SevEsIdtVmmComm) >> 16) ; Offset high bits 31..16
+;
+; Vectors 30 - 31 (No handlers)
+;
+%rep 2
+ dw 0 ; Offset low bits 15..0
+ dw 0x10 ; Selector
+ db 0 ; Reserved
+ db 0x8E ; Gate Type (IA32_IDT_GATE_TYPE_INTERRUPT_32)
+ dw 0 ; Offset high bits 31..16
+%endrep
+IDT_END:
+
+BITS 64
+
+;
+; Called after Jump64BitAndLandHere
+;
+PostJump64BitAndLandHereSev:
+
+ ;
+ ; If it is Tdx guest, jump to exit point directly.
+ ; This is because following code may access the memory region which has
+ ; not been accepted. It is not allowed in Tdx guests.
+ ;
+ mov eax, dword[TDX_WORK_AREA]
+ cmp eax, 0x47584454 ; 'TDXG'
+ jz GoodCompare
+
+ ;
+ ; Check if the second step of the SEV-ES mitigation is to be performed.
+ ;
+ test ebx, ebx
+ jz InsnCompare
+
+ ;
+ ; SEV-ES is active, perform the second step of the encryption bit postion
+ ; mitigation check. The ECX and EDX register contain data from RDRAND that
+ ; was stored to memory in encrypted form. If the encryption bit position is
+ ; valid, the contents of ECX and EDX will match the memory location.
+ ;
+ cmp dword[SEV_ES_WORK_AREA_RDRAND], ecx
+ jne SevEncBitHlt
+ cmp dword[SEV_ES_WORK_AREA_RDRAND + 4], edx
+ jne SevEncBitHlt
+
+ ;
+ ; If SEV or SEV-ES is active, perform a quick sanity check against
+ ; the reported encryption bit position. This is to help mitigate
+ ; against attacks where the hypervisor reports an incorrect encryption
+ ; bit position. If SEV is not active, this check will always succeed.
+ ;
+ ; The cmp instruction compares the first four bytes of the cmp instruction
+ ; itself (which will be read decrypted if SEV or SEV-ES is active and the
+ ; encryption bit position is valid) against the immediate within the
+ ; instruction (an instruction fetch is always decrypted correctly by
+ ; hardware) based on RIP relative addressing.
+ ;
+InsnCompare:
+ cmp dword[rel InsnCompare], 0xFFF63D81
+ je GoodCompare
+
+ ;
+ ; The hypervisor provided an incorrect encryption bit position, do not
+ ; proceed.
+ ;
+SevEncBitHlt:
+ cli
+ hlt
+ jmp SevEncBitHlt
+
+GoodCompare:
+ OneTimeCallRet PostJump64BitAndLandHereSev
--
2.29.2.windows.2


Brijesh Singh
 

Hi Min,

This refactoring is already done by the SNP patch series.

https://edk2.groups.io/g/devel/message/77336?p=,,,20,0,0,0::Created,,posterid%3A5969970,20,2,20,83891510

It appears that you are also pulling in some of TDX logic inside the
AMDSev.asm such as

;
+PostJump64BitAndLandHereSev:
+
+ ;
+ ; If it is Tdx guest, jump to exit point directly.
+ ; This is because following code may access the memory region which has
+ ; not been accepted. It is not allowed in Tdx guests.
+ ;
+ mov eax, dword[TDX_WORK_AREA]
+ cmp eax, 0x47584454 ; 'TDXG'
+ jz GoodCompare

Why we are referring the TDX workarea inside the AmdSev.asm ?

I will take out my refactoring patch outside of the SNP series and
submit it so that you can build on top of. This will simplify review
process.

thanks

On 7/27/21 12:42 AM, Min Xu wrote:
RFC: https://bugzilla.tianocore.org/show_bug.cgi?id=3429

AmdSev.asm includes below routines:
- CheckSevFeatures
Check if Secure Encrypted Virtualization (SEV) features are enabled.
- PreSetCr3ForPageTables64Sev
It is called before SetCr3ForPageTables64 in SEV guests.
- PostSetCr3PageTables64Sev
It is called after SetCr3PageTables64 in SEV guests.
- PostJump64BitAndLandHereSev
It is called after Jump64BitAndLandHere in SEV guests.
- #VC exception handling routines

These routines are extracted from PageTables64.asm and Flat32ToFlat64.asm
Need AMD engineers' help to review/validate the patch so that there is
no regression. Thanks in advance!

Note:
In above Pre/Post routines, dword[TDX_WORK_AREA] should be checked
to see if it is 'TDXG' (Tdx guests). This is because some memory region
for example, byte[SEV_ES_WORK_AREA] cannot be accessed in Tdx guests.
Tdx requires that any memory region to be accessed should be accepted
first or initialized by host VMM before Td guest is launched.

Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Brijesh Singh <brijesh.singh@amd.com>
Cc: Erdem Aktas <erdemaktas@google.com>
Cc: James Bottomley <jejb@linux.ibm.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Signed-off-by: Min Xu <min.m.xu@intel.com>
---
OvmfPkg/ResetVector/Ia32/AmdSev.asm | 526 ++++++++++++++++++++++++++++
1 file changed, 526 insertions(+)
create mode 100644 OvmfPkg/ResetVector/Ia32/AmdSev.asm

diff --git a/OvmfPkg/ResetVector/Ia32/AmdSev.asm b/OvmfPkg/ResetVector/Ia32/AmdSev.asm
new file mode 100644
index 000000000000..962b7e169c61
--- /dev/null
+++ b/OvmfPkg/ResetVector/Ia32/AmdSev.asm
@@ -0,0 +1,526 @@
+;------------------------------------------------------------------------------
+; @file
+; AMD SEV routines
+;
+; Copyright (c) 2020, Advanced Micro Devices, Inc. All rights reserved.<BR>
+; Copyright (c) 2021, Intel Corporation. All rights reserved.<BR>
+;
+; SPDX-License-Identifier: BSD-2-Clause-Patent
+;
+;------------------------------------------------------------------------------
+
+
+BITS 32
+;
+; SEV-ES #VC exception handler support
+;
+; #VC handler local variable locations
+;
+%define VC_CPUID_RESULT_EAX 0
+%define VC_CPUID_RESULT_EBX 4
+%define VC_CPUID_RESULT_ECX 8
+%define VC_CPUID_RESULT_EDX 12
+%define VC_GHCB_MSR_EDX 16
+%define VC_GHCB_MSR_EAX 20
+%define VC_CPUID_REQUEST_REGISTER 24
+%define VC_CPUID_FUNCTION 28
+
+; #VC handler total local variable size
+;
+%define VC_VARIABLE_SIZE 32
+
+; #VC handler GHCB CPUID request/response protocol values
+;
+%define GHCB_CPUID_REQUEST 4
+%define GHCB_CPUID_RESPONSE 5
+%define GHCB_CPUID_REGISTER_SHIFT 30
+%define CPUID_INSN_LEN 2
+
+;
+; Check if Secure Encrypted Virtualization (SEV) features are enabled.
+;
+; Register usage is tight in this routine, so multiple calls for the
+; same CPUID and MSR data are performed to keep things simple.
+;
+; Modified: EAX, EBX, ECX, EDX, ESP
+;
+; If SEV is enabled then EAX will be at least 32.
+; If SEV is disabled then EAX will be zero.
+;
+CheckSevFeatures:
+ ; Set the first byte of the workarea to zero to communicate to the SEC
+ ; phase that SEV-ES is not enabled. If SEV-ES is enabled, the CPUID
+ ; instruction will trigger a #VC exception where the first byte of the
+ ; workarea will be set to one or, if CPUID is not being intercepted,
+ ; the MSR check below will set the first byte of the workarea to one.
+ mov byte[SEV_ES_WORK_AREA], 0
+
+ ;
+ ; Set up exception handlers to check for SEV-ES
+ ; Load temporary RAM stack based on PCDs (see SevEsIdtVmmComm for
+ ; stack usage)
+ ; Establish exception handlers
+ ;
+ mov esp, SEV_ES_VC_TOP_OF_STACK
+ mov eax, ADDR_OF(Idtr)
+ lidt [cs:eax]
+
+ ; Check if we have a valid (0x8000_001F) CPUID leaf
+ ; CPUID raises a #VC exception if running as an SEV-ES guest
+ mov eax, 0x80000000
+ cpuid
+
+ ; This check should fail on Intel or Non SEV AMD CPUs. In future if
+ ; Intel CPUs supports this CPUID leaf then we are guranteed to have exact
+ ; same bit definition.
+ cmp eax, 0x8000001f
+ jl NoSev
+
+ ; Check for SEV memory encryption feature:
+ ; CPUID Fn8000_001F[EAX] - Bit 1
+ ; CPUID raises a #VC exception if running as an SEV-ES guest
+ mov eax, 0x8000001f
+ cpuid
+ bt eax, 1
+ jnc NoSev
+
+ ; Check if SEV memory encryption is enabled
+ ; MSR_0xC0010131 - Bit 0 (SEV enabled)
+ mov ecx, 0xc0010131
+ rdmsr
+ bt eax, 0
+ jnc NoSev
+
+ ; Check for SEV-ES memory encryption feature:
+ ; CPUID Fn8000_001F[EAX] - Bit 3
+ ; CPUID raises a #VC exception if running as an SEV-ES guest
+ mov eax, 0x8000001f
+ cpuid
+ bt eax, 3
+ jnc GetSevEncBit
+
+ ; Check if SEV-ES is enabled
+ ; MSR_0xC0010131 - Bit 1 (SEV-ES enabled)
+ mov ecx, 0xc0010131
+ rdmsr
+ bt eax, 1
+ jnc GetSevEncBit
+
+ ; Set the first byte of the workarea to one to communicate to the SEC
+ ; phase that SEV-ES is enabled.
+ mov byte[SEV_ES_WORK_AREA], 1
+
+GetSevEncBit:
+ ; Get pte bit position to enable memory encryption
+ ; CPUID Fn8000_001F[EBX] - Bits 5:0
+ ;
+ and ebx, 0x3f
+ mov eax, ebx
+
+ ; The encryption bit position is always above 31
+ sub ebx, 32
+ jns SevSaveMask
+
+ ; Encryption bit was reported as 31 or below, enter a HLT loop
+SevEncBitLowHlt:
+ cli
+ hlt
+ jmp SevEncBitLowHlt
+
+SevSaveMask:
+ xor edx, edx
+ bts edx, ebx
+
+ mov dword[SEV_ES_WORK_AREA_ENC_MASK], 0
+ mov dword[SEV_ES_WORK_AREA_ENC_MASK + 4], edx
+ jmp SevExit
+
+NoSev:
+ ;
+ ; Perform an SEV-ES sanity check by seeing if a #VC exception occurred.
+ ;
+ cmp byte[SEV_ES_WORK_AREA], 0
+ jz NoSevPass
+
+ ;
+ ; A #VC was received, yet CPUID indicates no SEV-ES support, something
+ ; isn't right.
+ ;
+NoSevEsVcHlt:
+ cli
+ hlt
+ jmp NoSevEsVcHlt
+
+NoSevPass:
+ xor eax, eax
+
+SevExit:
+ ;
+ ; Clear exception handlers and stack
+ ;
+ push eax
+ mov eax, ADDR_OF(IdtrClear)
+ lidt [cs:eax]
+ pop eax
+ mov esp, 0
+
+ OneTimeCallRet CheckSevFeatures
+
+;
+; Called before SetCr3ForPageTables64 in SEV guests
+;
+; Modified: EAX, EBX, ECX, EDX, ESP
+;
+PreSetCr3ForPageTables64Sev:
+ ; In Tdx TDX_WORK_AREA was set to 'TDXG'.
+ ; CheckSevFeatures cannot be called in Tdx guest because SEV_ES_WORK_AREA
+ ; cannot be accessed in this situation. Any memory region to be accessed
+ ; in Td guest should be accepted first.
+ cmp dword[TDX_WORK_AREA], 0x47584454 ; 'TDXG'
+ jz ExitPreSetCr3ForPageTables64Sev
+
+ OneTimeCall CheckSevFeatures
+
+ExitPreSetCr3ForPageTables64Sev:
+ OneTimeCallRet PreSetCr3ForPageTables64Sev
+
+;
+; It is called in SEV after SetCr3PageTables64
+;
+; Modified: EAX, EBX, ECX, EDX, ESP
+;
+PostSetCr3PageTables64Sev:
+ ; In Tdx TDX_WORK_AREA was set to 'TDXG'.
+ ; CheckSevFeatures cannot be called in Tdx because SEV_ES_WORK_AREA
+ ; cannot be accessed in this situation. Any memory region to be accessed
+ ; in Td guest should be accepted first.
+ cmp dword[TDX_WORK_AREA], 0x47584454 ; 'TDXG'
+ jz ExitPostSetCr3PageTables64Sev
+
+ mov eax, cr4
+ bts eax, 5 ; enable PAE
+ mov cr4, eax
+
+ mov ecx, 0xc0000080
+ rdmsr
+ bts eax, 8 ; set LME
+ wrmsr
+
+ ;
+ ; SEV-ES mitigation check support
+ ;
+ xor ebx, ebx
+
+ cmp byte[SEV_ES_WORK_AREA], 0
+ jz EnablePaging
+
+ ;
+ ; SEV-ES is active, perform a quick sanity check against the reported
+ ; encryption bit position. This is to help mitigate against attacks where
+ ; the hypervisor reports an incorrect encryption bit position.
+ ;
+ ; This is the first step in a two step process. Before paging is enabled
+ ; writes to memory are encrypted. Using the RDRAND instruction (available
+ ; on all SEV capable processors), write 64-bits of random data to the
+ ; SEV_ES_WORK_AREA and maintain the random data in registers (register
+ ; state is protected under SEV-ES). This will be used in the second step.
+ ;
+RdRand1:
+ rdrand ecx
+ jnc RdRand1
+ mov dword[SEV_ES_WORK_AREA_RDRAND], ecx
+RdRand2:
+ rdrand edx
+ jnc RdRand2
+ mov dword[SEV_ES_WORK_AREA_RDRAND + 4], edx
+
+ ;
+ ; Use EBX instead of the SEV_ES_WORK_AREA memory to determine whether to
+ ; perform the second step.
+ ;
+ mov ebx, 1
+
+EnablePaging:
+ mov eax, cr0
+ bts eax, 31 ; set PG
+ mov cr0, eax ; enable paging
+
+ExitPostSetCr3PageTables64Sev:
+
+ OneTimeCallRet PostSetCr3PageTables64Sev
+
+;
+; Start of #VC exception handling routines
+;
+
+SevEsIdtNotCpuid:
+ ;
+ ; Use VMGEXIT to request termination.
+ ; 1 - #VC was not for CPUID
+ ;
+ mov eax, 1
+ jmp SevEsIdtTerminate
+
+SevEsIdtNoCpuidResponse:
+ ;
+ ; Use VMGEXIT to request termination.
+ ; 2 - GHCB_CPUID_RESPONSE not received
+ ;
+ mov eax, 2
+
+SevEsIdtTerminate:
+ ;
+ ; Use VMGEXIT to request termination. At this point the reason code is
+ ; located in EAX, so shift it left 16 bits to the proper location.
+ ;
+ ; EAX[11:0] => 0x100 - request termination
+ ; EAX[15:12] => 0x1 - OVMF
+ ; EAX[23:16] => 0xXX - REASON CODE
+ ;
+ shl eax, 16
+ or eax, 0x1100
+ xor edx, edx
+ mov ecx, 0xc0010130
+ wrmsr
+ ;
+ ; Issue VMGEXIT - NASM doesn't support the vmmcall instruction in 32-bit
+ ; mode, so work around this by temporarily switching to 64-bit mode.
+ ;
+BITS 64
+ rep vmmcall
+BITS 32
+
+ ;
+ ; We shouldn't come back from the VMGEXIT, but if we do, just loop.
+ ;
+SevEsIdtHlt:
+ hlt
+ jmp SevEsIdtHlt
+ iret
+
+ ;
+ ; Total stack usage for the #VC handler is 44 bytes:
+ ; - 12 bytes for the exception IRET (after popping error code)
+ ; - 32 bytes for the local variables.
+ ;
+SevEsIdtVmmComm:
+ ;
+ ; If we're here, then we are an SEV-ES guest and this
+ ; was triggered by a CPUID instruction
+ ;
+ ; Set the first byte of the workarea to one to communicate that
+ ; a #VC was taken.
+ mov byte[SEV_ES_WORK_AREA], 1
+
+ pop ecx ; Error code
+ cmp ecx, 0x72 ; Be sure it was CPUID
+ jne SevEsIdtNotCpuid
+
+ ; Set up local variable room on the stack
+ ; CPUID function : + 28
+ ; CPUID request register : + 24
+ ; GHCB MSR (EAX) : + 20
+ ; GHCB MSR (EDX) : + 16
+ ; CPUID result (EDX) : + 12
+ ; CPUID result (ECX) : + 8
+ ; CPUID result (EBX) : + 4
+ ; CPUID result (EAX) : + 0
+ sub esp, VC_VARIABLE_SIZE
+
+ ; Save the CPUID function being requested
+ mov [esp + VC_CPUID_FUNCTION], eax
+
+ ; The GHCB CPUID protocol uses the following mapping to request
+ ; a specific register:
+ ; 0 => EAX, 1 => EBX, 2 => ECX, 3 => EDX
+ ;
+ ; Set EAX as the first register to request. This will also be used as a
+ ; loop variable to request all register values (EAX to EDX).
+ xor eax, eax
+ mov [esp + VC_CPUID_REQUEST_REGISTER], eax
+
+ ; Save current GHCB MSR value
+ mov ecx, 0xc0010130
+ rdmsr
+ mov [esp + VC_GHCB_MSR_EAX], eax
+ mov [esp + VC_GHCB_MSR_EDX], edx
+
+NextReg:
+ ;
+ ; Setup GHCB MSR
+ ; GHCB_MSR[63:32] = CPUID function
+ ; GHCB_MSR[31:30] = CPUID register
+ ; GHCB_MSR[11:0] = CPUID request protocol
+ ;
+ mov eax, [esp + VC_CPUID_REQUEST_REGISTER]
+ cmp eax, 4
+ jge VmmDone
+
+ shl eax, GHCB_CPUID_REGISTER_SHIFT
+ or eax, GHCB_CPUID_REQUEST
+ mov edx, [esp + VC_CPUID_FUNCTION]
+ mov ecx, 0xc0010130
+ wrmsr
+
+ ;
+ ; Issue VMGEXIT - NASM doesn't support the vmmcall instruction in 32-bit
+ ; mode, so work around this by temporarily switching to 64-bit mode.
+ ;
+BITS 64
+ rep vmmcall
+BITS 32
+
+ ;
+ ; Read GHCB MSR
+ ; GHCB_MSR[63:32] = CPUID register value
+ ; GHCB_MSR[31:30] = CPUID register
+ ; GHCB_MSR[11:0] = CPUID response protocol
+ ;
+ mov ecx, 0xc0010130
+ rdmsr
+ mov ecx, eax
+ and ecx, 0xfff
+ cmp ecx, GHCB_CPUID_RESPONSE
+ jne SevEsIdtNoCpuidResponse
+
+ ; Save returned value
+ shr eax, GHCB_CPUID_REGISTER_SHIFT
+ mov [esp + eax * 4], edx
+
+ ; Next register
+ inc word [esp + VC_CPUID_REQUEST_REGISTER]
+
+ jmp NextReg
+
+VmmDone:
+ ;
+ ; At this point we have all CPUID register values. Restore the GHCB MSR,
+ ; set the return register values and return.
+ ;
+ mov eax, [esp + VC_GHCB_MSR_EAX]
+ mov edx, [esp + VC_GHCB_MSR_EDX]
+ mov ecx, 0xc0010130
+ wrmsr
+
+ mov eax, [esp + VC_CPUID_RESULT_EAX]
+ mov ebx, [esp + VC_CPUID_RESULT_EBX]
+ mov ecx, [esp + VC_CPUID_RESULT_ECX]
+ mov edx, [esp + VC_CPUID_RESULT_EDX]
+
+ add esp, VC_VARIABLE_SIZE
+
+ ; Update the EIP value to skip over the now handled CPUID instruction
+ ; (the CPUID instruction has a length of 2)
+ add word [esp], CPUID_INSN_LEN
+ iret
+
+ALIGN 2
+
+Idtr:
+ dw IDT_END - IDT_BASE - 1 ; Limit
+ dd ADDR_OF(IDT_BASE) ; Base
+
+IdtrClear:
+ dw 0 ; Limit
+ dd 0 ; Base
+
+ALIGN 16
+
+;
+; The Interrupt Descriptor Table (IDT)
+; This will be used to determine if SEV-ES is enabled. Upon execution
+; of the CPUID instruction, a VMM Communication Exception will occur.
+; This will tell us if SEV-ES is enabled. We can use the current value
+; of the GHCB MSR to determine the SEV attributes.
+;
+IDT_BASE:
+;
+; Vectors 0 - 28 (No handlers)
+;
+%rep 29
+ dw 0 ; Offset low bits 15..0
+ dw 0x10 ; Selector
+ db 0 ; Reserved
+ db 0x8E ; Gate Type (IA32_IDT_GATE_TYPE_INTERRUPT_32)
+ dw 0 ; Offset high bits 31..16
+%endrep
+;
+; Vector 29 (VMM Communication Exception)
+;
+ dw (ADDR_OF(SevEsIdtVmmComm) & 0xffff) ; Offset low bits 15..0
+ dw 0x10 ; Selector
+ db 0 ; Reserved
+ db 0x8E ; Gate Type (IA32_IDT_GATE_TYPE_INTERRUPT_32)
+ dw (ADDR_OF(SevEsIdtVmmComm) >> 16) ; Offset high bits 31..16
+;
+; Vectors 30 - 31 (No handlers)
+;
+%rep 2
+ dw 0 ; Offset low bits 15..0
+ dw 0x10 ; Selector
+ db 0 ; Reserved
+ db 0x8E ; Gate Type (IA32_IDT_GATE_TYPE_INTERRUPT_32)
+ dw 0 ; Offset high bits 31..16
+%endrep
+IDT_END:
+
+BITS 64
+
+;
+; Called after Jump64BitAndLandHere
+;
+PostJump64BitAndLandHereSev:
+
+ ;
+ ; If it is Tdx guest, jump to exit point directly.
+ ; This is because following code may access the memory region which has
+ ; not been accepted. It is not allowed in Tdx guests.
+ ;
+ mov eax, dword[TDX_WORK_AREA]
+ cmp eax, 0x47584454 ; 'TDXG'
+ jz GoodCompare
+
+ ;
+ ; Check if the second step of the SEV-ES mitigation is to be performed.
+ ;
+ test ebx, ebx
+ jz InsnCompare
+
+ ;
+ ; SEV-ES is active, perform the second step of the encryption bit postion
+ ; mitigation check. The ECX and EDX register contain data from RDRAND that
+ ; was stored to memory in encrypted form. If the encryption bit position is
+ ; valid, the contents of ECX and EDX will match the memory location.
+ ;
+ cmp dword[SEV_ES_WORK_AREA_RDRAND], ecx
+ jne SevEncBitHlt
+ cmp dword[SEV_ES_WORK_AREA_RDRAND + 4], edx
+ jne SevEncBitHlt
+
+ ;
+ ; If SEV or SEV-ES is active, perform a quick sanity check against
+ ; the reported encryption bit position. This is to help mitigate
+ ; against attacks where the hypervisor reports an incorrect encryption
+ ; bit position. If SEV is not active, this check will always succeed.
+ ;
+ ; The cmp instruction compares the first four bytes of the cmp instruction
+ ; itself (which will be read decrypted if SEV or SEV-ES is active and the
+ ; encryption bit position is valid) against the immediate within the
+ ; instruction (an instruction fetch is always decrypted correctly by
+ ; hardware) based on RIP relative addressing.
+ ;
+InsnCompare:
+ cmp dword[rel InsnCompare], 0xFFF63D81
+ je GoodCompare
+
+ ;
+ ; The hypervisor provided an incorrect encryption bit position, do not
+ ; proceed.
+ ;
+SevEncBitHlt:
+ cli
+ hlt
+ jmp SevEncBitHlt
+
+GoodCompare:
+ OneTimeCallRet PostJump64BitAndLandHereSev


Min Xu
 

On July 27, 2021 6:57 PM, Brijesh Singh wrote:
Hi Min,

This refactoring is already done by the SNP patch series.

https://edk2.groups.io/g/devel/message/77336?p=,,,20,0,0,0::Created,,post
erid%3A5969970,20,2,20,83891510

It appears that you are also pulling in some of TDX logic inside the
AMDSev.asm such as

;
+PostJump64BitAndLandHereSev:
+
+ ;
+ ; If it is Tdx guest, jump to exit point directly.
+ ; This is because following code may access the memory region which has
+ ; not been accepted. It is not allowed in Tdx guests.
+ ;
+ mov eax, dword[TDX_WORK_AREA]
+ cmp eax, 0x47584454 ; 'TDXG'
+ jz GoodCompare

Why we are referring the TDX workarea inside the AmdSev.asm ?
See my explanation in the above comments. In Tdx guests memory region cannot
be accessed unless it is accepted by guest or initialized by the host VMM. In
PostJump64BitAndLandHereSev there is access to dword[SEV_ES_WORK_AREA_RDRAND]
which is not initialized by host VMM. If this code will not be executed in
Tdx guest, then the above check is not needed. I need your help to confirm it.

There are similar Tdx check in my patch of AmdSev.asm. For example in CheckSevFeatures
byte[SEV_ES_WORK_AREA] is used to record the SEV-ES flag. This memory region is
not initialized by host VMM either. So in Tdx it will trigger error.

Another solution is that the memory region used by SEV in ResetVector are added
Into Tdx metadata so that host VMM will initialize those memory region when
It creates the Td guest. What's your opinion?

I will take out my refactoring patch outside of the SNP series and submit it so
that you can build on top of. This will simplify review process.
Thank you very much for the refactoring. I will refine my patch based on it.

thanks


Brijesh Singh
 

On 7/27/21 6:51 AM, Xu, Min M wrote:
On July 27, 2021 6:57 PM, Brijesh Singh wrote:
Hi Min,

This refactoring is already done by the SNP patch series.

https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fedk2.groups.io%2Fg%2Fdevel%2Fmessage%2F77336%3Fp%3D%2C%2C%2C20%2C0%2C0%2C0%3A%3ACreated%2C%2Cpost&;data=04%7C01%7Cbrijesh.singh%40amd.com%7C22b61f2ff5bb48348b0608d950f4d7c5%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637629834792320372%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=tMGpR4a2uZTTR%2FsciTN0oeca2mZ32GfX3K78lA5BWas%3D&amp;reserved=0
erid%3A5969970,20,2,20,83891510

It appears that you are also pulling in some of TDX logic inside the
AMDSev.asm such as

;
+PostJump64BitAndLandHereSev:
+
+ ;
+ ; If it is Tdx guest, jump to exit point directly.
+ ; This is because following code may access the memory region which has
+ ; not been accepted. It is not allowed in Tdx guests.
+ ;
+ mov eax, dword[TDX_WORK_AREA]
+ cmp eax, 0x47584454 ; 'TDXG'
+ jz GoodCompare

Why we are referring the TDX workarea inside the AmdSev.asm ?
See my explanation in the above comments. In Tdx guests memory region cannot
be accessed unless it is accepted by guest or initialized by the host VMM. In
PostJump64BitAndLandHereSev there is access to dword[SEV_ES_WORK_AREA_RDRAND]
which is not initialized by host VMM. If this code will not be executed in
Tdx guest, then the above check is not needed. I need your help to confirm it.

There are similar Tdx check in my patch of AmdSev.asm. For example in CheckSevFeatures
byte[SEV_ES_WORK_AREA] is used to record the SEV-ES flag. This memory region is
not initialized by host VMM either. So in Tdx it will trigger error.

Another solution is that the memory region used by SEV in ResetVector are added
Into Tdx metadata so that host VMM will initialize those memory region when
It creates the Td guest. What's your opinion?
I am not full versed on TDX yet and sorry I am not able to follow you
question completely to provide any advice. With SEV and SEV-ES, a guest
can access the memory without going through the validation process, but
with the SEV-SNP, the page need to be validated (aka accepted) before
the access. In SNP series, we ensure that the data pages used in the
reset vector are pre-validated during the VM creation time -- this
allows us to access the pages without going through accept process. If I
follow you correctly on your metadata comment then it is similar to
saying is pre-validate these range of pages used in the reset vector
code (that include GHCB page, Page table pages etc), right ? 

For SEV-SNP, see this patch

https://edk2.groups.io/g/devel/message/77342?p=,,,20,0,0,0::Created,,posterid%3A5969970,20,2,20,83891520

A VMM (qemu) looks for the range of page it need to prevalidate before
the boot, the range is provided through the GUID (SevSnpBootBlock).

I will take out my refactoring patch outside of the SNP series and submit it so
that you can build on top of. This will simplify review process.
Thank you very much for the refactoring. I will refine my patch based on it.
thanks


Yao, Jiewen
 

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?

Thank you
Yao Jiewen

-----Original Message-----
From: Brijesh Singh <brijesh.singh@amd.com>
Sent: Tuesday, July 27, 2021 8:31 PM
To: Xu, Min M <min.m.xu@intel.com>; devel@edk2.groups.io
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>; Yao,
Jiewen <jiewen.yao@intel.com>; Tom Lendacky <thomas.lendacky@amd.com>
Subject: Re: [PATCH V3 06/10] OvmfPkg: Add AmdSev.asm in ResetVector


On 7/27/21 6:51 AM, Xu, Min M wrote:
On July 27, 2021 6:57 PM, Brijesh Singh wrote:
Hi Min,

This refactoring is already done by the SNP patch series.

https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fedk2.gr
oups.io%2Fg%2Fdevel%2Fmessage%2F77336%3Fp%3D%2C%2C%2C20%2C0%2
C0%2C0%3A%3ACreated%2C%2Cpost&amp;data=04%7C01%7Cbrijesh.singh%4
0amd.com%7C22b61f2ff5bb48348b0608d950f4d7c5%7C3dd8961fe4884e608e1
1a82d994e183d%7C0%7C0%7C637629834792320372%7CUnknown%7CTWFpb
GZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6M
n0%3D%7C1000&amp;sdata=tMGpR4a2uZTTR%2FsciTN0oeca2mZ32GfX3K78lA
5BWas%3D&amp;reserved=0
erid%3A5969970,20,2,20,83891510

It appears that you are also pulling in some of TDX logic inside the
AMDSev.asm such as

;
+PostJump64BitAndLandHereSev:
+
+ ;
+ ; If it is Tdx guest, jump to exit point directly.
+ ; This is because following code may access the memory region which has
+ ; not been accepted. It is not allowed in Tdx guests.
+ ;
+ mov eax, dword[TDX_WORK_AREA]
+ cmp eax, 0x47584454 ; 'TDXG'
+ jz GoodCompare

Why we are referring the TDX workarea inside the AmdSev.asm ?
See my explanation in the above comments. In Tdx guests memory region
cannot
be accessed unless it is accepted by guest or initialized by the host VMM. In
PostJump64BitAndLandHereSev there is access to
dword[SEV_ES_WORK_AREA_RDRAND]
which is not initialized by host VMM. If this code will not be executed in
Tdx guest, then the above check is not needed. I need your help to confirm it.

There are similar Tdx check in my patch of AmdSev.asm. For example in
CheckSevFeatures
byte[SEV_ES_WORK_AREA] is used to record the SEV-ES flag. This memory
region is
not initialized by host VMM either. So in Tdx it will trigger error.

Another solution is that the memory region used by SEV in ResetVector are
added
Into Tdx metadata so that host VMM will initialize those memory region when
It creates the Td guest. What's your opinion?
I am not full versed on TDX yet and sorry I am not able to follow you
question completely to provide any advice. With SEV and SEV-ES, a guest
can access the memory without going through the validation process, but
with the SEV-SNP, the page need to be validated (aka accepted) before
the access. In SNP series, we ensure that the data pages used in the
reset vector are pre-validated during the VM creation time -- this
allows us to access the pages without going through accept process. If I
follow you correctly on your metadata comment then it is similar to
saying is pre-validate these range of pages used in the reset vector
code (that include GHCB page, Page table pages etc), right ?

For SEV-SNP, see this patch

https://edk2.groups.io/g/devel/message/77342?p=,,,20,0,0,0::Created,,posteri
d%3A5969970,20,2,20,83891520

A VMM (qemu) looks for the range of page it need to prevalidate before
the boot, the range is provided through the GUID (SevSnpBootBlock).

I will take out my refactoring patch outside of the SNP series and submit it so
that you can build on top of. This will simplify review process.
Thank you very much for the refactoring. I will refine my patch based on it.
thanks


Min Xu
 

On July 27, 2021 8:31 PM, Brijesh Singh wrote:
On 7/27/21 6:51 AM, Xu, Min M wrote:
On July 27, 2021 6:57 PM, Brijesh Singh wrote:
Hi Min,

This refactoring is already done by the SNP patch series.

https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fedk
2.groups.io%2Fg%2Fdevel%2Fmessage%2F77336%3Fp%3D%2C%2C%2C20%
2C0%2C0%2
C0%3A%3ACreated%2C%2Cpost&amp;data=04%7C01%7Cbrijesh.singh%40a
md.com%
7C22b61f2ff5bb48348b0608d950f4d7c5%7C3dd8961fe4884e608e11a82d994
e183d
%7C0%7C0%7C637629834792320372%7CUnknown%7CTWFpbGZsb3d8ey
JWIjoiMC4wLjA
wMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;s
data=
tMGpR4a2uZTTR%2FsciTN0oeca2mZ32GfX3K78lA5BWas%3D&amp;reserved
=0
erid%3A5969970,20,2,20,83891510

It appears that you are also pulling in some of TDX logic inside the
AMDSev.asm such as

;
+PostJump64BitAndLandHereSev:
+
+ ;
+ ; If it is Tdx guest, jump to exit point directly.
+ ; This is because following code may access the memory region which
has
+ ; not been accepted. It is not allowed in Tdx guests.
+ ;
+ mov eax, dword[TDX_WORK_AREA]
+ cmp eax, 0x47584454 ; 'TDXG'
+ jz GoodCompare

Why we are referring the TDX workarea inside the AmdSev.asm ?
See my explanation in the above comments. In Tdx guests memory region
cannot be accessed unless it is accepted by guest or initialized by
the host VMM. In PostJump64BitAndLandHereSev there is access to
dword[SEV_ES_WORK_AREA_RDRAND] which is not initialized by host
VMM.
If this code will not be executed in Tdx guest, then the above check is not
needed. I need your help to confirm it.

There are similar Tdx check in my patch of AmdSev.asm. For example in
CheckSevFeatures byte[SEV_ES_WORK_AREA] is used to record the SEV-ES
flag. This memory region is not initialized by host VMM either. So in Tdx it
will trigger error.

Another solution is that the memory region used by SEV in ResetVector
are added Into Tdx metadata so that host VMM will initialize those
memory region when It creates the Td guest. What's your opinion?
I am not full versed on TDX yet and sorry I am not able to follow you
question completely to provide any advice. With SEV and SEV-ES, a guest can
access the memory without going through the validation process, but with
the SEV-SNP, the page need to be validated (aka accepted) before the access.
TDX has the same requirement.
In SNP series, we ensure that the data pages used in the reset vector are pre-
validated during the VM creation time -- this allows us to access the pages
without going through accept process. If I follow you correctly on your
metadata comment then it is similar to saying is pre-validate these range of
pages used in the reset vector code (that include GHCB page, Page table
pages etc), right ?
That's right. Tdx metadata describes the memory region which host VMM initialized
during the VM creation time.

In the current patch-set, below memory region are described in Tdx metadata.
- TdMailbox (PcdOvmfSecGhcbBackupBase)
- TdHob(PcdOvmfSecGhcbBase)
- TdExtraPage(PcdOvmfSecGhcbPageTableBase)
- OvmfPageTable (PcdOvmfSecPageTablesBase)
These memory regions are initialized by host VMM so they can be accessed in ResetVector in Tdx guests.

In the SEV codes, I find some memory is accessed as well. CheckSevFeatures is the example.
In CheckSevFeatures byte[SEV_ES_WORK_AREA] (PcdSevEsWorkAreaBase) is used to record/check
if it is SEV. So if this function is called in Tdx guest, then error is triggered.

What I am concerned is that, 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 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 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;


For SEV-SNP, see this patch

https://edk2.groups.io/g/devel/message/77342?p=,,,20,0,0,0::Created,,post
erid%3A5969970,20,2,20,83891520

A VMM (qemu) looks for the range of page it need to prevalidate before the
boot, the range is provided through the GUID (SevSnpBootBlock).

I will take out my refactoring patch outside of the SNP series and
submit it so that you can build on top of. This will simplify review process.
Thank you very much for the refactoring. I will refine my patch based on it.
thanks


Min Xu
 

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;


Yao, Jiewen
 

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?

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;


Min Xu
 

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;


Yao, Jiewen
 

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;


Min Xu
 

I noticed SEV has the memory region of SEV_ES_WORK_AREA (gUefiCpuPkgTokenSpaceGuid.PcdSevEsWorkAreaBase) in MEMFD
The definition is below:
typedef struct _SEC_SEV_ES_WORK_AREA {
UINT8 SevEsEnabled;
UINT8 Reserved1[7];
UINT64 RandomData;
UINT64 EncryptionMask;
} SEC_SEV_ES_WORK_AREA;

In ResetVector SEV flag is recorded in the first byte of SEV_ES_WORK_AREA. In SecMain.c this flag is read to determine SEV.

I am thinking whether this memory region can be used by both TDX and SEV. (This is option 5)

SEV_ES_WORK_AREA will be added in tdx metadata so that it is initialized by host VMM and can be accessed in Tdx guest.
In SEV guest I believe SEV_ES_WORK_AREA can be accessed without any error.
The first 8 bytes of SEV_ES_WORK_AREA can be redefined as [CC Indicator Flags].

-----Original Message-----
From: Yao, Jiewen <jiewen.yao@intel.com>
Sent: Wednesday, July 28, 2021 3:55 PM
To: Xu, Min M <min.m.xu@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

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;


Brijesh Singh
 

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


Yao, Jiewen
 

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




Brijesh Singh
 

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



Yao, Jiewen
 

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?

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 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.

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






Brijesh Singh
 

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.

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).


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 ?

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.

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.


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





Yao, Jiewen
 

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






Min Xu
 

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@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






Brijesh Singh
 

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 ?



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. 


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.

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





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