From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-1.mimecast.com (us-smtp-1.mimecast.com [205.139.110.120]) by mx.groups.io with SMTP id smtpd.web10.29003.1574364485560920716 for ; Thu, 21 Nov 2019 11:28:05 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=S094VfRT; spf=pass (domain: redhat.com, ip: 205.139.110.120, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1574364484; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=G9dZ9fnrqIblT3DrvlwVruuC10zRahq8lmmXYE5+HEo=; b=S094VfRTpYFMgo05lZ5SEflmt3Y31rT7+8ImJm4uYCGQt//BVxbvENgEFTHsi3j2ghzZva dEN8N4McgkQIPyBzgvF09f0havGB2E1JcUoyMK1BgtWaxQhW0A8ue5O58to/H9mSN6Txm1 gcXwlR3LB46SqRU2IPhTWncfrETEHiM= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-220-2pqXd65MPM-XSQjk4EvEuw-1; Thu, 21 Nov 2019 14:28:00 -0500 Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.phx2.redhat.com [10.5.11.15]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id D9B1218557C3; Thu, 21 Nov 2019 19:27:57 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-197.ams2.redhat.com [10.36.116.197]) by smtp.corp.redhat.com (Postfix) with ESMTP id AA3BA5EE0F; Thu, 21 Nov 2019 19:27:55 +0000 (UTC) Subject: Re: [edk2-devel] [RFC PATCH v3 37/43] OvmfPkg: Reserve a page in memory for the SEV-ES AP reset vector To: devel@edk2.groups.io, thomas.lendacky@amd.com Cc: Jordan Justen , Ard Biesheuvel , Michael D Kinney , Liming Gao , Eric Dong , Ray Ni , Brijesh Singh References: <706f17aa0ea3ed3f57c1e933e93164a94ba1a0cf.1574280425.git.thomas.lendacky@amd.com> From: "Laszlo Ersek" Message-ID: <85a5bcf3-25e2-7298-bca6-cef09cdb5d47@redhat.com> Date: Thu, 21 Nov 2019 20:27:54 +0100 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: <706f17aa0ea3ed3f57c1e933e93164a94ba1a0cf.1574280425.git.thomas.lendacky@amd.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.15 X-MC-Unique: 2pqXd65MPM-XSQjk4EvEuw-1 X-Mimecast-Spam-Score: 0 Content-Language: en-US Content-Type: text/plain; charset=WINDOWS-1252 Content-Transfer-Encoding: quoted-printable On 11/20/19 21:06, Lendacky, Thomas wrote: > BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=3D2198 >=20 > Reserve a fixed area of memory for the SEV-ES AP reset vector and set > the fixed PCD, PcdSevEsResetRipBase, to this value. >=20 > A hypervisor is not allowed to update an SEV-ES guest's register state, > so when booting an SEV-ES guest AP, the hypervisor is not allowed to > set the RIP to the guest requested value. Instead an SEV-ES AP must be > re-directed from within the guest to the actual requested staring locatio= n > as specified in the INIT-SIPI-SIPI sequence. >=20 > Provide reset vector code that contains support to jump to the desired > RIP location after having been started. This is required for only the > very first AP reset. (1) I suggest replacing "Provide reset vector code" with "Provide *memory for* reset vector code" >=20 > This new reset vector code is used in place of the original code because (2) I suggest replacing "reset vector code" with "source file". > of the include path order set in OvmfPkg/ResetVector/ResetVector.inf > under "[BuildOptions]". >=20 > Cc: Jordan Justen > Cc: Laszlo Ersek > Cc: Ard Biesheuvel > Signed-off-by: Tom Lendacky > --- > OvmfPkg/OvmfPkgX64.fdf | 3 + > OvmfPkg/ResetVector/ResetVector.inf | 2 + > OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm | 94 ++++++++++++++++++++ > OvmfPkg/ResetVector/ResetVector.nasmb | 1 + > 4 files changed, 100 insertions(+) > create mode 100644 OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm >=20 > diff --git a/OvmfPkg/OvmfPkgX64.fdf b/OvmfPkg/OvmfPkgX64.fdf > index 973b19fdbf19..b7618b376430 100644 > --- a/OvmfPkg/OvmfPkgX64.fdf > +++ b/OvmfPkg/OvmfPkgX64.fdf > @@ -82,6 +82,9 @@ [FD.MEMFD] > 0x009000|0x002000 > gEfiMdeModulePkgTokenSpaceGuid.PcdSecGhcbBase|gEfiMdeModulePkgTokenSpace= Guid.PcdSecGhcbSize > =20 > +0x00B000|0x001000 > +gUefiCpuPkgTokenSpaceGuid.PcdSevEsResetRipBase|gUefiCpuPkgTokenSpaceGuid= .PcdSevEsResetRipSize > + > 0x010000|0x010000 > gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPeiTempRamBase|gUefiOvmfPkgTokenSpa= ceGuid.PcdOvmfSecPeiTempRamSize > =20 > diff --git a/OvmfPkg/ResetVector/ResetVector.inf b/OvmfPkg/ResetVector/Re= setVector.inf > index 266c5fc5c8b3..284f03c4fb1e 100644 > --- a/OvmfPkg/ResetVector/ResetVector.inf > +++ b/OvmfPkg/ResetVector/ResetVector.inf > @@ -36,6 +36,8 @@ [BuildOptions] > [Pcd] > gEfiMdeModulePkgTokenSpaceGuid.PcdSecGhcbBase > gEfiMdeModulePkgTokenSpaceGuid.PcdSecGhcbSize > + gUefiCpuPkgTokenSpaceGuid.PcdSevEsResetRipBase > + gUefiCpuPkgTokenSpaceGuid.PcdSevEsResetRipSize (3) "RipSize" looks quite strange; how about: - PcdSevEsResetVectorBase - PcdSevEsResetVectorSize (i.e. s/Rip/Vector/) (4) I think the module will not actually use the "size" PCD, so we can remove it from the INF file. > gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPageTablesBase > gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPageTablesSize > gUefiOvmfPkgTokenSpaceGuid.PcdOvmfSecPeiTempRamBase > diff --git a/OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm b/OvmfPkg/Reset= Vector/Ia16/ResetVectorVtf0.asm > new file mode 100644 > index 000000000000..2a3c9bafb451 > --- /dev/null > +++ b/OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm > @@ -0,0 +1,94 @@ > +;-----------------------------------------------------------------------= ------- > +; @file > +; First code executed by processor after resetting. > +; Derived from UefiCpuPkg/ResetVector/Vtf0/Ia16/ResetVectorVtf0.asm > +; > +; Copyright (c) 2008 - 2014, Intel Corporation. All rights reserved.
> +; SPDX-License-Identifier: BSD-2-Clause-Patent > +; > +;-----------------------------------------------------------------------= ------- > + > +BITS 16 > + > +ALIGN 16 > + > +; > +; Pad the image size to 4k when page tables are in VTF0 > +; > +; If the VTF0 image has page tables built in, then we need to make > +; sure the end of VTF0 is 4k above where the page tables end. > +; > +; This is required so the page tables will be 4k aligned when VTF0 is > +; located just below 0x100000000 (4GB) in the firmware device. > +; > +%ifdef ALIGN_TOP_TO_4K_FOR_PAGING > + TIMES (0x1000 - ($ - EndOfPageTables) - 0x20) DB 0 > +%endif > + > +; > +; SEV-ES Processor Reset support > +; > +; sevEsResetBlock: > +; For the initial boot of an AP under SEV-ES, the "reset" RIP must be > +; programmed to the RAM area defined by SEV_ES_RESET_IP. A known offse= t > +; and GUID will be used to locate this block in the firmware and extra= ct > +; the build time RIP value. The GUID must always be 48 bytes from the > +; end of the firmware. > +; > +; 0xffffffca (-0x36) - IP value > +; 0xffffffcc (-0x34) - CS selector value (5) I think the documentation of these four bytes is incorrect. (Similarly in the previous patch, in the commit message.) We populate these bytes with the *linear address* of the "reset trampoline" where the host will have to boot the AP for the first time. It's not expressed in real mode CS:IP notation / meaning. (6) Which brings me to my main point... Value 0x00B000 (for the "base" PCD) is relative to the base address of FD.MEMFD, namely 8MB (see MEMFD_BASE_ADDRESS). IOW, the ultimate value of SEV_ES_RESET_IP will be 0x0x0080_B000. That address is larger than 1MB. Therefore, the host will have to launch the AP (for the first time) above 1MB (in guest-phys address space). How can that work, if the AP is supposed to start in real mode? The linear address 0x0x0080_B000 cannot be expressed in 16-bit real mode CS:IP notation at all. Does the hypervisor start the AP in "big real mode" (16-bit mode with 4GB segment limits, and CS containing a segment selector, not the actual segment base)? ... I guess that would answer my question (6) -- and the last few patches in this series, before this one, *do* suggest "big real mode" to me --, but the documentation around (5), and in the commit message of the previous patch, still doesn't match that. ... AFAICS I'm only requesting small cleanups for this patch (naming, documentation, PCD listing), thus, conditional on everything addressed, I feel comfortable giving Reviewed-by: Laszlo Ersek in advance. Thanks! Laszlo > +; 0xffffffce (-0x32) - Size of SEV-ES reset block > +; 0xffffffd0 (-0x30) - SEV-ES reset block GUID > +; (00f771de-1a7e-4fcb-890e-68c77e2fb44e) > +; > + > +TIMES (32 - (sevEsResetBlockEnd - sevEsResetBlockStart)) DB 0 > + > +sevEsResetBlockStart: > + DD SEV_ES_RESET_IP > + DW sevEsResetBlockEnd - sevEsResetBlockStart > + DB 0xDE, 0x71, 0xF7, 0x00, 0x7E, 0x1A, 0xCB, 0x4F > + DB 0x89, 0x0E, 0x68, 0xC7, 0x7E, 0x2F, 0xB4, 0x4E > +sevEsResetBlockEnd: > + > +ALIGN 16 > + > +applicationProcessorEntryPoint: > +; > +; Application Processors entry point > +; > +; GenFv generates code aligned on a 4k boundary which will jump to this > +; location. (0xffffffe0) This allows the Local APIC Startup IPI to be > +; used to wake up the application processors. > +; > + jmp EarlyApInitReal16 > + > +ALIGN 8 > + > + DD 0 > + > +; > +; The VTF signature > +; > +; VTF-0 means that the VTF (Volume Top File) code does not require > +; any fixups. > +; > +vtfSignature: > + DB 'V', 'T', 'F', 0 > + > +ALIGN 16 > + > +resetVector: > +; > +; Reset Vector > +; > +; This is where the processor will begin execution > +; > + nop > + nop > + jmp EarlyBspInitReal16 > + > +ALIGN 16 > + > +fourGigabytes: > + > diff --git a/OvmfPkg/ResetVector/ResetVector.nasmb b/OvmfPkg/ResetVector/= ResetVector.nasmb > index 708bbda6208f..2bf14681099e 100644 > --- a/OvmfPkg/ResetVector/ResetVector.nasmb > +++ b/OvmfPkg/ResetVector/ResetVector.nasmb > @@ -81,5 +81,6 @@ > =20 > %include "Main.asm" > =20 > + %define SEV_ES_RESET_IP FixedPcdGet32 (PcdSevEsResetRipBase) > %include "Ia16/ResetVectorVtf0.asm" > =20 >=20