From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: redhat.com, ip: 209.132.183.28, mailfrom: lersek@redhat.com) Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) by groups.io with SMTP; Tue, 24 Sep 2019 06:42:21 -0700 Received: from smtp.corp.redhat.com (int-mx07.intmail.prod.int.phx2.redhat.com [10.5.11.22]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 687C2A26674; Tue, 24 Sep 2019 13:42:20 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-118.rdu2.redhat.com [10.10.120.118]) by smtp.corp.redhat.com (Postfix) with ESMTP id F3E421001B05; Tue, 24 Sep 2019 13:42:17 +0000 (UTC) Subject: Re: [edk2-devel] [RFC PATCH v2 04/44] OvmfPkg/ResetVector: Add support for a 32-bit SEV check To: devel@edk2.groups.io, thomas.lendacky@amd.com Cc: Jordan Justen , Ard Biesheuvel , Michael D Kinney , Liming Gao , Eric Dong , Ray Ni , "Singh, Brijesh" References: <54ebf48fe05c20a1181a3dc90496e4835912ebf2.1568922728.git.thomas.lendacky@amd.com> From: "Laszlo Ersek" Message-ID: Date: Tue, 24 Sep 2019 15:42:16 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <54ebf48fe05c20a1181a3dc90496e4835912ebf2.1568922728.git.thomas.lendacky@amd.com> X-Scanned-By: MIMEDefang 2.84 on 10.5.11.22 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.6.2 (mx1.redhat.com [10.5.110.68]); Tue, 24 Sep 2019 13:42:20 +0000 (UTC) Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 09/19/19 21:52, Lendacky, Thomas wrote: > From: Tom Lendacky > > 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 > Cc: Laszlo Ersek > Cc: Ard Biesheuvel > Signed-off-by: Tom Lendacky > --- > 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 , 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