From: Tom Lendacky <thomas.lendacky@amd.com>
BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=2198
During BSP startup, the reset vector code will issue a CPUID instruction
while in 32-bit mode. When running as an SEV-ES guest, this will trigger
a #VC exception.
Add exception handling support to the early reset vector code to catch
these exceptions. Also, since the guest is in 32-bit mode at this point,
writes to the GHCB will be encrypted and thus not able to be read by the
hypervisor, so use the GHCB CPUID request/response protocol to obtain the
requested CPUID function values and provide these to the guest.
The exception handling support is active during the SEV check and uses the
OVMF temporary RAM space for a stack. After the SEV check is complete, the
exception handling support is removed and the stack pointer cleared.
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
---
OvmfPkg/ResetVector/ResetVector.inf | 2 +
OvmfPkg/ResetVector/Ia32/PageTables64.asm | 177 +++++++++++++++++++++-
OvmfPkg/ResetVector/ResetVector.nasmb | 1 +
3 files changed, 179 insertions(+), 1 deletion(-)
diff --git a/OvmfPkg/ResetVector/ResetVector.inf b/OvmfPkg/ResetVector/ResetVector.inf
index b0ddfa5832a2..960b47cd0797 100644
--- a/OvmfPkg/ResetVector/ResetVector.inf
+++ b/OvmfPkg/ResetVector/ResetVector.inf
@@ -35,3 +35,5 @@ [BuildOptions]
[Pcd]
gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPageTablesBase
gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPageTablesSize
+ gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPeiTempRamBase
+ gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPeiTempRamSize
diff --git a/OvmfPkg/ResetVector/Ia32/PageTables64.asm b/OvmfPkg/ResetVector/Ia32/PageTables64.asm
index abad009f20f5..40f7814c1134 100644
--- a/OvmfPkg/ResetVector/Ia32/PageTables64.asm
+++ b/OvmfPkg/ResetVector/Ia32/PageTables64.asm
@@ -33,10 +33,21 @@ BITS 32
; Check if Secure Encrypted Virtualization (SEV) feature is enabled
;
-; If SEV is enabled then EAX will be at least 32
+; 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.
;
CheckSevFeature:
+ ;
+ ; Set up exception handlers to check for SEV-ES
+ ; Load temporary RAM stack based on PCDs
+ ; Establish exception handlers
+ ;
+ mov esp, SEV_TOP_OF_STACK
+ mov eax, ADDR_OF(Idtr)
+ lidt [cs:eax]
+
; Check if we have a valid (0x8000_001F) CPUID leaf
mov eax, 0x80000000
cpuid
@@ -73,6 +84,15 @@ NoSev:
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 CheckSevFeature
;
@@ -146,3 +166,158 @@ pageTableEntriesLoop:
mov cr3, eax
OneTimeCallRet SetCr3ForPageTables64
+
+SevEsIdtCommon:
+ hlt
+ jmp SevEsIdtCommon
+ iret
+
+SevEsIdtVmmComm:
+ ;
+ ; If we're here, then we are an SEV-ES guest and this
+ ; was triggered by a CPUID instruction
+ ;
+ pop ecx ; Error code
+ cmp ecx, 0x72 ; Be sure it was CPUID
+ jne SevEsIdtCommon
+
+ ;
+ ; Set up local variable room on the stack
+ ; CPUID function : + 28
+ ; CPUID 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, 32
+
+ ; Save CPUID function and initial register request
+ mov [esp + 28], eax
+ xor eax, eax
+ mov [esp + 24], eax
+
+ ; Save current GHCB MSR value
+ mov ecx, 0xc0010130
+ rdmsr
+ mov [esp + 20], eax
+ mov [esp + 16], 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 + 24]
+ cmp eax, 4
+ jge VmmDone
+
+ shl eax, 30
+ or eax, 0x004
+ mov edx, [esp + 28]
+ mov ecx, 0xc0010130
+ wrmsr
+
+ ; Issue VMGEXIT (rep; vmmcall)
+ db 0xf3
+ db 0x0f
+ db 0x01
+ db 0xd9
+
+ ;
+ ; 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, 0x005
+ jne SevEsIdtCommon
+
+ ; Save returned value
+ shr eax, 30
+ and eax, 0x3
+ shl eax, 2
+ mov ecx, esp
+ add ecx, eax
+ mov [ecx], edx
+
+ ; Next register
+ inc word [esp + 24]
+
+ 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 + 20]
+ mov edx, [esp + 16]
+ mov ecx, 0xc0010130
+ wrmsr
+
+ mov eax, [esp + 0]
+ mov ebx, [esp + 4]
+ mov ecx, [esp + 8]
+ mov edx, [esp + 12]
+
+ add esp, 32
+ add word [esp], 2 ; Skip over the CPUID instruction
+ 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
+;
+%rep 29
+ dw (ADDR_OF(SevEsIdtCommon) & 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(SevEsIdtCommon) >> 16) ; 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
+;
+%rep 2
+ dw (ADDR_OF(SevEsIdtCommon) & 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(SevEsIdtCommon) >> 16) ; Offset high bits 31..16
+%endrep
+IDT_END:
diff --git a/OvmfPkg/ResetVector/ResetVector.nasmb b/OvmfPkg/ResetVector/ResetVector.nasmb
index 75cfe16654b1..3b213cd05ab2 100644
--- a/OvmfPkg/ResetVector/ResetVector.nasmb
+++ b/OvmfPkg/ResetVector/ResetVector.nasmb
@@ -55,6 +55,7 @@
%define PT_ADDR(Offset) (FixedPcdGet32 (PcdOvmfSecPageTablesBase) + (Offset))
%include "Ia32/Flat32ToFlat64.asm"
+ %define SEV_TOP_OF_STACK (FixedPcdGet32 (PcdOvmfSecPeiTempRamBase) + FixedPcdGet32 (PcdOvmfSecPeiTempRamSize))
%include "Ia32/PageTables64.asm"
%endif
--
2.17.1
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#47638): https://edk2.groups.io/g/devel/message/47638
Mute This Topic: https://groups.io/mt/34203539/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
On 09/19/19 21:52, Lendacky, Thomas wrote:
> From: Tom Lendacky <thomas.lendacky@amd.com>
>
> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=2198
>
> During BSP startup, the reset vector code will issue a CPUID instruction
> while in 32-bit mode. When running as an SEV-ES guest, this will trigger
> a #VC exception.
(1) In the assembly source code, please annotate both CPUID instructions
under CheckSevFeature, such as
; raises #VC when in an SEV-ES guest
>
> Add exception handling support to the early reset vector code to catch
> these exceptions. Also, since the guest is in 32-bit mode at this point,
> writes to the GHCB will be encrypted and thus not able to be read by the
> hypervisor, so use the GHCB CPUID request/response protocol to obtain the
> requested CPUID function values and provide these to the guest.
>
> The exception handling support is active during the SEV check and uses the
> OVMF temporary RAM space for a stack. After the SEV check is complete, the
> exception handling support is removed and the stack pointer cleared.
>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
> ---
> OvmfPkg/ResetVector/ResetVector.inf | 2 +
> OvmfPkg/ResetVector/Ia32/PageTables64.asm | 177 +++++++++++++++++++++-
> OvmfPkg/ResetVector/ResetVector.nasmb | 1 +
> 3 files changed, 179 insertions(+), 1 deletion(-)
>
> diff --git a/OvmfPkg/ResetVector/ResetVector.inf b/OvmfPkg/ResetVector/ResetVector.inf
> index b0ddfa5832a2..960b47cd0797 100644
> --- a/OvmfPkg/ResetVector/ResetVector.inf
> +++ b/OvmfPkg/ResetVector/ResetVector.inf
> @@ -35,3 +35,5 @@ [BuildOptions]
> [Pcd]
> gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPageTablesBase
> gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPageTablesSize
> + gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPeiTempRamBase
> + gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPeiTempRamSize
> diff --git a/OvmfPkg/ResetVector/Ia32/PageTables64.asm b/OvmfPkg/ResetVector/Ia32/PageTables64.asm
> index abad009f20f5..40f7814c1134 100644
> --- a/OvmfPkg/ResetVector/Ia32/PageTables64.asm
> +++ b/OvmfPkg/ResetVector/Ia32/PageTables64.asm
> @@ -33,10 +33,21 @@ BITS 32
>
> ; Check if Secure Encrypted Virtualization (SEV) feature is enabled
> ;
> -; If SEV is enabled then EAX will be at least 32
> +; 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.
> ;
> CheckSevFeature:
> + ;
> + ; Set up exception handlers to check for SEV-ES
> + ; Load temporary RAM stack based on PCDs
> + ; Establish exception handlers
> + ;
> + mov esp, SEV_TOP_OF_STACK
(2) Can we %define SEV_TOP_OF_STACK in this file, or does it have to be
in "ResetVector.nasmb"?
(3) Do we have an estimate how much stack we need? This would be a
constraint on PcdOvmfSecPeiTempRamSize. The limit would be nice to
document (perhaps in a comment somewhere).
> + mov eax, ADDR_OF(Idtr)
> + lidt [cs:eax]
> +
> ; Check if we have a valid (0x8000_001F) CPUID leaf
> mov eax, 0x80000000
> cpuid
> @@ -73,6 +84,15 @@ NoSev:
> xor eax, eax
>
> SevExit:
> + ;
> + ; Clear exception handlers and stack
> + ;
> + push eax
> + mov eax, ADDR_OF(IdtrClear)
> + lidt [cs:eax]
> + pop eax
> + mov esp, 0
> +
I'm not sure the resultant IDT and ESP contents are the same as before
(pre-patch), but I guess these values should be OK too.
> OneTimeCallRet CheckSevFeature
>
> ;
> @@ -146,3 +166,158 @@ pageTableEntriesLoop:
> mov cr3, eax
>
> OneTimeCallRet SetCr3ForPageTables64
> +
> +SevEsIdtCommon:
> + hlt
> + jmp SevEsIdtCommon
> + iret
> +
> +SevEsIdtVmmComm:
> + ;
> + ; If we're here, then we are an SEV-ES guest and this
> + ; was triggered by a CPUID instruction
> + ;
> + pop ecx ; Error code
> + cmp ecx, 0x72 ; Be sure it was CPUID
> + jne SevEsIdtCommon
(4) Can you steal the DebugLog / PrintStringSi code from
"OvmfPkg/QemuVideoDxe/VbeShim.asm", and print a simple message to the
QEMU debug port, whenever you jump to SevEsIdtCommon?
This is basically an ASSERT(). It should have a message, if possible.
(I'm not sure if the OUT instruction will work with SEV-ES at this
stage, i.e. in 32-bit mode.)
> +
> + ;
> + ; Set up local variable room on the stack
> + ; CPUID function : + 28
> + ; CPUID 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, 32
(5) Can we %define macros for these offsets? (Including the size
constant 32.)
> +
> + ; Save CPUID function and initial register request
> + mov [esp + 28], eax
> + xor eax, eax
> + mov [esp + 24], eax
(6) The comment "CPUID register" (at offset 24), and the other comment
"initial register", are pretty confusing. Can you please document:
- the mapping: 0->EAX, ... 3->EDX,
- and the fact that the dword at [esp + 24] is the loop variable?
> +
> + ; Save current GHCB MSR value
> + mov ecx, 0xc0010130
> + rdmsr
> + mov [esp + 20], eax
> + mov [esp + 16], 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 + 24]
> + cmp eax, 4
> + jge VmmDone
> +
> + shl eax, 30
> + or eax, 0x004
(7) Please %define GHCBInfoCpuIdRequest or something similar for the
value 4.
> + mov edx, [esp + 28]
> + mov ecx, 0xc0010130
> + wrmsr
> +
> + ; Issue VMGEXIT (rep; vmmcall)
> + db 0xf3
> + db 0x0f
> + db 0x01
> + db 0xd9
(8) Can you please file an RFE at <https://bugzilla.nasm.us/>, for
supporting this instruction, and add the link here, as a comment? I've
been fighting an uphill battle against DB-encoded instructions in edk2
assembly code.
> +
> + ;
> + ; 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, 0x005
(9) Please %define GHCBInfoCpuIdResponse for value 5.
> + jne SevEsIdtCommon
(10) Please see (4). The message could be, "no GHCBInfoCpuIdResponse
received", or similar.
> +
> + ; Save returned value
> + shr eax, 30
> + and eax, 0x3
(11) Do we need the AND after the SHR? I think the new high order bits
from the SHR should be zero.
> + shl eax, 2
> + mov ecx, esp
> + add ecx, eax
> + mov [ecx], edx
(12) The beauty of the lean and mean x86 instruction set:
mov [esp + eax * 4], edx
(I tested just this one instruction with nasm + ndisasm; the encoding is
0x89, 0x14, 0x84.)
> +
> + ; Next register
> + inc word [esp + 24]
> +
> + 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 + 20]
> + mov edx, [esp + 16]
> + mov ecx, 0xc0010130
> + wrmsr
> +
> + mov eax, [esp + 0]
> + mov ebx, [esp + 4]
> + mov ecx, [esp + 8]
> + mov edx, [esp + 12]
> +
> + add esp, 32
(13) Please see (5).
> + add word [esp], 2 ; Skip over the CPUID instruction
OK, this seems safe. CPUID has only one encoding (0x0F, 0xA2), and we
checked SW_EXITCODE = 0x72 at the top.
> + 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
> +;
> +%rep 29
> + dw (ADDR_OF(SevEsIdtCommon) & 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(SevEsIdtCommon) >> 16) ; 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
> +;
> +%rep 2
> + dw (ADDR_OF(SevEsIdtCommon) & 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(SevEsIdtCommon) >> 16) ; Offset high bits 31..16
> +%endrep
> +IDT_END:
(14) Above, we have two explicit jumps to SevEsIdtCommon, and I've asked
for meaningful assertion messages there.
For the uninteresting exception vectors 0-28 and 30-31, can we use a
handler that is not directly SevEsIdtCommon, but logs a message, and
then jumps to SevEsIdtCommon? (It could even be implemented as a
"prefix" of SevEsIdtCommon, and then no jump would be required.)
> diff --git a/OvmfPkg/ResetVector/ResetVector.nasmb b/OvmfPkg/ResetVector/ResetVector.nasmb
> index 75cfe16654b1..3b213cd05ab2 100644
> --- a/OvmfPkg/ResetVector/ResetVector.nasmb
> +++ b/OvmfPkg/ResetVector/ResetVector.nasmb
> @@ -55,6 +55,7 @@
>
> %define PT_ADDR(Offset) (FixedPcdGet32 (PcdOvmfSecPageTablesBase) + (Offset))
> %include "Ia32/Flat32ToFlat64.asm"
> + %define SEV_TOP_OF_STACK (FixedPcdGet32 (PcdOvmfSecPeiTempRamBase) + FixedPcdGet32 (PcdOvmfSecPeiTempRamSize))
> %include "Ia32/PageTables64.asm"
> %endif
>
>
(I've commented on this under (2).)
Thanks!
Laszlo
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#47952): https://edk2.groups.io/g/devel/message/47952
Mute This Topic: https://groups.io/mt/34203539/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
On 09/24/19 15:42, Laszlo Ersek wrote: > On 09/19/19 21:52, Lendacky, Thomas wrote: >> + mov esp, SEV_TOP_OF_STACK > (3) Do we have an estimate how much stack we need? This would be a > constraint on PcdOvmfSecPeiTempRamSize. The limit would be nice to > document (perhaps in a comment somewhere). Ah I've just been reminded by [RFC PATCH v2 06/44]: we could use "%if" + "%error" to catch (at compile time) if the stack is too small. (Not sure if this is overly useful; it might not be, if the stack demand is negligible.) Thanks Laszlo -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#47953): https://edk2.groups.io/g/devel/message/47953 Mute This Topic: https://groups.io/mt/34203539/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
On 9/24/19 8:42 AM, Laszlo Ersek wrote: > On 09/19/19 21:52, Lendacky, Thomas wrote: >> From: Tom Lendacky <thomas.lendacky@amd.com> >> >> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=2198 >> >> During BSP startup, the reset vector code will issue a CPUID instruction >> while in 32-bit mode. When running as an SEV-ES guest, this will trigger >> a #VC exception. > > (1) In the assembly source code, please annotate both CPUID instructions > under CheckSevFeature, such as > > ; raises #VC when in an SEV-ES guest Will do. > >> >> Add exception handling support to the early reset vector code to catch >> these exceptions. Also, since the guest is in 32-bit mode at this point, >> writes to the GHCB will be encrypted and thus not able to be read by the >> hypervisor, so use the GHCB CPUID request/response protocol to obtain the >> requested CPUID function values and provide these to the guest. >> >> The exception handling support is active during the SEV check and uses the >> OVMF temporary RAM space for a stack. After the SEV check is complete, the >> exception handling support is removed and the stack pointer cleared. >> >> Cc: Jordan Justen <jordan.l.justen@intel.com> >> Cc: Laszlo Ersek <lersek@redhat.com> >> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org> >> Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com> >> --- >> OvmfPkg/ResetVector/ResetVector.inf | 2 + >> OvmfPkg/ResetVector/Ia32/PageTables64.asm | 177 +++++++++++++++++++++- >> OvmfPkg/ResetVector/ResetVector.nasmb | 1 + >> 3 files changed, 179 insertions(+), 1 deletion(-) >> >> diff --git a/OvmfPkg/ResetVector/ResetVector.inf b/OvmfPkg/ResetVector/ResetVector.inf >> index b0ddfa5832a2..960b47cd0797 100644 >> --- a/OvmfPkg/ResetVector/ResetVector.inf >> +++ b/OvmfPkg/ResetVector/ResetVector.inf >> @@ -35,3 +35,5 @@ [BuildOptions] >> [Pcd] >> gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPageTablesBase >> gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPageTablesSize >> + gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPeiTempRamBase >> + gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPeiTempRamSize >> diff --git a/OvmfPkg/ResetVector/Ia32/PageTables64.asm b/OvmfPkg/ResetVector/Ia32/PageTables64.asm >> index abad009f20f5..40f7814c1134 100644 >> --- a/OvmfPkg/ResetVector/Ia32/PageTables64.asm >> +++ b/OvmfPkg/ResetVector/Ia32/PageTables64.asm >> @@ -33,10 +33,21 @@ BITS 32 >> >> ; Check if Secure Encrypted Virtualization (SEV) feature is enabled >> ; >> -; If SEV is enabled then EAX will be at least 32 >> +; 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. >> ; >> CheckSevFeature: >> + ; >> + ; Set up exception handlers to check for SEV-ES >> + ; Load temporary RAM stack based on PCDs >> + ; Establish exception handlers >> + ; >> + mov esp, SEV_TOP_OF_STACK > > (2) Can we %define SEV_TOP_OF_STACK in this file, or does it have to be > in "ResetVector.nasmb"? It looks like it has to be in ResetVector.nasmb for some reason. If I move it into this file, it fails: Ia32/PageTables64.asm:76: error: expecting `)' > > (3) Do we have an estimate how much stack we need? This would be a > constraint on PcdOvmfSecPeiTempRamSize. The limit would be nice to > document (perhaps in a comment somewhere). It's a fairly small amount of stack space, about 44 bytes (12 bytes for the IRET frame and 32 bytes for the #VC "local" variables). The temporary RAM size is 64K, so we're fine. I'll document the stack usage at the start of the SevEsIdtVmmComm exception handler. > >> + mov eax, ADDR_OF(Idtr) >> + lidt [cs:eax] >> + >> ; Check if we have a valid (0x8000_001F) CPUID leaf >> mov eax, 0x80000000 >> cpuid >> @@ -73,6 +84,15 @@ NoSev: >> xor eax, eax >> >> SevExit: >> + ; >> + ; Clear exception handlers and stack >> + ; >> + push eax >> + mov eax, ADDR_OF(IdtrClear) >> + lidt [cs:eax] >> + pop eax >> + mov esp, 0 >> + > > I'm not sure the resultant IDT and ESP contents are the same as before > (pre-patch), but I guess these values should be OK too. At this stage of the boot, both the IDT and ESP values are zero. They haven't been set up this early in the boot. > >> OneTimeCallRet CheckSevFeature >> >> ; >> @@ -146,3 +166,158 @@ pageTableEntriesLoop: >> mov cr3, eax >> >> OneTimeCallRet SetCr3ForPageTables64 >> + >> +SevEsIdtCommon: >> + hlt >> + jmp SevEsIdtCommon >> + iret >> + >> +SevEsIdtVmmComm: >> + ; >> + ; If we're here, then we are an SEV-ES guest and this >> + ; was triggered by a CPUID instruction >> + ; >> + pop ecx ; Error code >> + cmp ecx, 0x72 ; Be sure it was CPUID >> + jne SevEsIdtCommon > > (4) Can you steal the DebugLog / PrintStringSi code from > "OvmfPkg/QemuVideoDxe/VbeShim.asm", and print a simple message to the > QEMU debug port, whenever you jump to SevEsIdtCommon? > > This is basically an ASSERT(). It should have a message, if possible. > (I'm not sure if the OUT instruction will work with SEV-ES at this > stage, i.e. in 32-bit mode.) Right, there isn't a way to share the OUT instruction information with the hypervisor at this point because we are running in 32-bit mode (this might be nice to add to the GHCB protocol, similar to the CPUID support, I'll have to think about that). Since we can't output a message, I can do something along the line of using the GHCB protocol to request the hypervisor to terminate the guest. Would that be better? > >> + >> + ; >> + ; Set up local variable room on the stack >> + ; CPUID function : + 28 >> + ; CPUID 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, 32 > > (5) Can we %define macros for these offsets? (Including the size > constant 32.) Will do. > >> + >> + ; Save CPUID function and initial register request >> + mov [esp + 28], eax >> + xor eax, eax >> + mov [esp + 24], eax > > (6) The comment "CPUID register" (at offset 24), and the other comment > "initial register", are pretty confusing. Can you please document: > > - the mapping: 0->EAX, ... 3->EDX, > - and the fact that the dword at [esp + 24] is the loop variable? Yup, can do. > >> + >> + ; Save current GHCB MSR value >> + mov ecx, 0xc0010130 >> + rdmsr >> + mov [esp + 20], eax >> + mov [esp + 16], 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 + 24] >> + cmp eax, 4 >> + jge VmmDone >> + >> + shl eax, 30 >> + or eax, 0x004 > > (7) Please %define GHCBInfoCpuIdRequest or something similar for the > value 4. Ok. I'll remove all of the hardcoded values and %define them. > >> + mov edx, [esp + 28] >> + mov ecx, 0xc0010130 >> + wrmsr >> + >> + ; Issue VMGEXIT (rep; vmmcall) >> + db 0xf3 >> + db 0x0f >> + db 0x01 >> + db 0xd9 > > (8) Can you please file an RFE at <https://bugzilla.nasm.us/>, for > supporting this instruction, and add the link here, as a comment? I've > been fighting an uphill battle against DB-encoded instructions in edk2 > assembly code. Yes, let me look into that. > >> + >> + ; >> + ; 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, 0x005 > > (9) Please %define GHCBInfoCpuIdResponse for value 5. > >> + jne SevEsIdtCommon > > (10) Please see (4). The message could be, "no GHCBInfoCpuIdResponse > received", or similar. Sure, if I find a way to issue messages at this point. > >> + >> + ; Save returned value >> + shr eax, 30 >> + and eax, 0x3 > > (11) Do we need the AND after the SHR? I think the new high order bits > from the SHR should be zero. No, we don't actually need the AND, just a habit to show the size of the area. But since we're at the top of the register range (bits 30 and 31) it's not really necessary. I'll remove it. > >> + shl eax, 2 >> + mov ecx, esp >> + add ecx, eax >> + mov [ecx], edx > > (12) The beauty of the lean and mean x86 instruction set: > > mov [esp + eax * 4], edx > > (I tested just this one instruction with nasm + ndisasm; the encoding is > 0x89, 0x14, 0x84.) Ah, yes. Will do. > >> + >> + ; Next register >> + inc word [esp + 24] >> + >> + 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 + 20] >> + mov edx, [esp + 16] >> + mov ecx, 0xc0010130 >> + wrmsr >> + >> + mov eax, [esp + 0] >> + mov ebx, [esp + 4] >> + mov ecx, [esp + 8] >> + mov edx, [esp + 12] >> + >> + add esp, 32 > > (13) Please see (5). > >> + add word [esp], 2 ; Skip over the CPUID instruction > > OK, this seems safe. CPUID has only one encoding (0x0F, 0xA2), and we > checked SW_EXITCODE = 0x72 at the top. In this case, yes. Since we're covering a small piece of code and we generated the instructions I think we can rely on that. Later on, that's where the instruction parsing comes in. > >> + 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 >> +; >> +%rep 29 >> + dw (ADDR_OF(SevEsIdtCommon) & 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(SevEsIdtCommon) >> 16) ; 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 >> +; >> +%rep 2 >> + dw (ADDR_OF(SevEsIdtCommon) & 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(SevEsIdtCommon) >> 16) ; Offset high bits 31..16 >> +%endrep >> +IDT_END: > > (14) Above, we have two explicit jumps to SevEsIdtCommon, and I've asked > for meaningful assertion messages there. > > For the uninteresting exception vectors 0-28 and 30-31, can we use a > handler that is not directly SevEsIdtCommon, but logs a message, and > then jumps to SevEsIdtCommon? (It could even be implemented as a > "prefix" of SevEsIdtCommon, and then no jump would be required.) If I come up with a way to get a message out, we can look at that. I'll need to see if it is too late to update the GHCB protocol specification to add an OUT protocol to support messages when in 32-bit. I think I can actually set just the #VC exception entry so that any other exception just causes the guest to die, similar to what would happen if an exception were to occur at this stage today. Thanks for the thorough review! Tom > >> diff --git a/OvmfPkg/ResetVector/ResetVector.nasmb b/OvmfPkg/ResetVector/ResetVector.nasmb >> index 75cfe16654b1..3b213cd05ab2 100644 >> --- a/OvmfPkg/ResetVector/ResetVector.nasmb >> +++ b/OvmfPkg/ResetVector/ResetVector.nasmb >> @@ -55,6 +55,7 @@ >> >> %define PT_ADDR(Offset) (FixedPcdGet32 (PcdOvmfSecPageTablesBase) + (Offset)) >> %include "Ia32/Flat32ToFlat64.asm" >> + %define SEV_TOP_OF_STACK (FixedPcdGet32 (PcdOvmfSecPeiTempRamBase) + FixedPcdGet32 (PcdOvmfSecPeiTempRamSize)) >> %include "Ia32/PageTables64.asm" >> %endif >> >> > > (I've commented on this under (2).) > > Thanks! > Laszlo > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#47990): https://edk2.groups.io/g/devel/message/47990 Mute This Topic: https://groups.io/mt/34203539/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
On 09/24/19 20:57, Lendacky, Thomas wrote: > On 9/24/19 8:42 AM, Laszlo Ersek wrote: >> On 09/19/19 21:52, Lendacky, Thomas wrote: >>> + mov esp, SEV_TOP_OF_STACK >> >> (2) Can we %define SEV_TOP_OF_STACK in this file, or does it have to be >> in "ResetVector.nasmb"? > > It looks like it has to be in ResetVector.nasmb for some reason. If I move > it into this file, it fails: > > Ia32/PageTables64.asm:76: error: expecting `)' Huh. Strange. Not a problem to lose sleep over, though. >>> +SevEsIdtCommon: >>> + hlt >>> + jmp SevEsIdtCommon >>> + iret >>> + >>> +SevEsIdtVmmComm: >>> + ; >>> + ; If we're here, then we are an SEV-ES guest and this >>> + ; was triggered by a CPUID instruction >>> + ; >>> + pop ecx ; Error code >>> + cmp ecx, 0x72 ; Be sure it was CPUID >>> + jne SevEsIdtCommon >> >> (4) Can you steal the DebugLog / PrintStringSi code from >> "OvmfPkg/QemuVideoDxe/VbeShim.asm", and print a simple message to the >> QEMU debug port, whenever you jump to SevEsIdtCommon? >> >> This is basically an ASSERT(). It should have a message, if possible. >> (I'm not sure if the OUT instruction will work with SEV-ES at this >> stage, i.e. in 32-bit mode.) > > Right, there isn't a way to share the OUT instruction information with > the hypervisor at this point because we are running in 32-bit mode (this > might be nice to add to the GHCB protocol, similar to the CPUID support, > I'll have to think about that). > > Since we can't output a message, I can do something along the line of > using the GHCB protocol to request the hypervisor to terminate the guest. > Would that be better? I think that would be great; from publication#56421, it looks like the guest can pass a (reason code set, reason code) pair along with the termination request, and QEMU could display that. We could assign reason code set 1 to QEMU (if a reason code set has not been assigned yet), and then we could use different reason codes (in code set 1) for the various SevEsIdtCommon jumps. (I don't think we need to distinguish the exception vectors from each other -- as long as a user can tell us that the guest died due to *some* exception, we can write debug patches to tell those apart; we'll know where to start.) Thanks! Laszlo -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#48035): https://edk2.groups.io/g/devel/message/48035 Mute This Topic: https://groups.io/mt/34203539/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
On 09/24/19 20:57, Lendacky, Thomas wrote: > On 9/24/19 8:42 AM, Laszlo Ersek wrote: >> On 09/19/19 21:52, Lendacky, Thomas wrote: >>> + ; Issue VMGEXIT (rep; vmmcall) >>> + db 0xf3 >>> + db 0x0f >>> + db 0x01 >>> + db 0xd9 >> >> (8) Can you please file an RFE at <https://bugzilla.nasm.us/>, for >> supporting this instruction, and add the link here, as a comment? I've >> been fighting an uphill battle against DB-encoded instructions in edk2 >> assembly code. > > Yes, let me look into that. Actually, from peeking ahead at patch "MdePkg/BaseLib: Add support for the VMGEXIT instruction", it looks like "rep; vmmcall" is already understood by NASM. Can you use that here? ... In case that sequence of mnemonics is specific to NASM's 64-bit mode: can you bracket it with BITS 64 / BITS 32, just so we can avoid the DBs? Something like (if necessary): ; Issue VMGEXIT BITS 64 rep; vmmcall BITS 32 Hmmm why don't I try this out myself... ... So, first, the semicolon (;) seems wrong in the NASM source. It turns vmmcall into a comment, and NASM assembles only the REP prefix (to a single 0xF3 byte). Second, when I remove the semicolon, NASM indeed complains in 32-bit mode "error: instruction not supported in 32-bit mode". But the following does work: BITS 64 rep vmmcall BITS 32 and for it, NASM generates the bytes seen above (f3 0f 01 d9). So I suggest: - using this pattern in the present patch - using this pattern in the Ia32/VmgExit.nasm source file in the MdePkg/BaseLib patch - removing the semicolon in the X64/VmgExit.nasm source file in the MdePkg/BaseLib patch Thanks! Laszlo -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#48292): https://edk2.groups.io/g/devel/message/48292 Mute This Topic: https://groups.io/mt/34203539/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
On 9/30/19 2:29 PM, Laszlo Ersek via Groups.Io wrote: > On 09/24/19 20:57, Lendacky, Thomas wrote: >> On 9/24/19 8:42 AM, Laszlo Ersek wrote: >>> On 09/19/19 21:52, Lendacky, Thomas wrote: > >>>> + ; Issue VMGEXIT (rep; vmmcall) >>>> + db 0xf3 >>>> + db 0x0f >>>> + db 0x01 >>>> + db 0xd9 >>> >>> (8) Can you please file an RFE at <https://bugzilla.nasm.us/>, for >>> supporting this instruction, and add the link here, as a comment? I've >>> been fighting an uphill battle against DB-encoded instructions in edk2 >>> assembly code. >> >> Yes, let me look into that. > > Actually, from peeking ahead at patch "MdePkg/BaseLib: Add support for > the VMGEXIT instruction", it looks like "rep; vmmcall" is already > understood by NASM. > > Can you use that here? > > ... In case that sequence of mnemonics is specific to NASM's 64-bit > mode: can you bracket it with BITS 64 / BITS 32, just so we can avoid > the DBs? Something like (if necessary): > > ; Issue VMGEXIT > BITS 64 > rep; vmmcall > BITS 32 > > > Hmmm why don't I try this out myself... > > ... So, first, the semicolon (;) seems wrong in the NASM source. It > turns vmmcall into a comment, and NASM assembles only the REP prefix (to > a single 0xF3 byte). Yeah, missed that. I'm so used to the GCC/GAS syntax. > > Second, when I remove the semicolon, NASM indeed complains in 32-bit > mode "error: instruction not supported in 32-bit mode". Right, that was why I moved to the DBs. > > But the following does work: > > BITS 64 > rep vmmcall > BITS 32 > > and for it, NASM generates the bytes seen above (f3 0f 01 d9). Excellent, I'll use that. > > So I suggest: > - using this pattern in the present patch > - using this pattern in the Ia32/VmgExit.nasm source file in the > MdePkg/BaseLib patch > - removing the semicolon in the X64/VmgExit.nasm source file in the > MdePkg/BaseLib patch > Will do. Thanks, Tom > Thanks! > Laszlo > > > > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#48298): https://edk2.groups.io/g/devel/message/48298 Mute This Topic: https://groups.io/mt/34203539/1787277 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org] -=-=-=-=-=-=-=-=-=-=-=-
© 2016 - 2025 Red Hat, Inc.