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; Thu, 03 Oct 2019 02:09:12 -0700 Received: from smtp.corp.redhat.com (int-mx06.intmail.prod.int.phx2.redhat.com [10.5.11.16]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id BFACE10CC1F1; Thu, 3 Oct 2019 09:09:11 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-154.rdu2.redhat.com [10.10.120.154]) by smtp.corp.redhat.com (Postfix) with ESMTP id 9E6805C21A; Thu, 3 Oct 2019 09:09:09 +0000 (UTC) Subject: Re: [edk2-devel] [RFC PATCH v2 37/44] OvmfPkg: Add support for SEV-ES AP reset vector re-directing To: "Lendacky, Thomas" , "devel@edk2.groups.io" Cc: Jordan Justen , Ard Biesheuvel , Michael D Kinney , Liming Gao , Eric Dong , Ray Ni , "Singh, Brijesh" References: <61fd6897ca552edbb39ab2f2ee0cce0b505ee71c.1568922729.git.thomas.lendacky@amd.com> <1cfba29a-5776-c48c-d934-89af2e8bafed@amd.com> From: "Laszlo Ersek" Message-ID: Date: Thu, 3 Oct 2019 11:09:08 +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: <1cfba29a-5776-c48c-d934-89af2e8bafed@amd.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.16 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.6.2 (mx1.redhat.com [10.5.110.65]); Thu, 03 Oct 2019 09:09:12 +0000 (UTC) Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 10/02/19 19:33, Lendacky, Thomas wrote: > On 10/2/19 9:54 AM, Laszlo Ersek wrote: >> On 09/19/19 21:53, Lendacky, Thomas wrote: >>> From: Tom Lendacky >>> >>> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=2198 >>> >>> A hypervisor is not allowed to update an SEV-ES guests 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 location >>> as specified in the INIT-SIPI-SIPI sequence. >>> >>> 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) In the commit message, can you mention the build mechanism by which >> this file overrides the original in UefiCpuPkg? >> >> Is it due to include path order? > > Yes, this is due to the BuildOptions include path order. I'll update the > commit message. > >> >>> >>> Cc: Jordan Justen >>> Cc: Laszlo Ersek >>> Cc: Ard Biesheuvel >>> Signed-off-by: Tom Lendacky >>> --- >>> OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm | 80 ++++++++++++++++++++ >>> 1 file changed, 80 insertions(+) >>> create mode 100644 OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm >>> >>> diff --git a/OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm b/OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm >>> new file mode 100644 >>> index 000000000000..1ac8b7ca7e85 >>> --- /dev/null >>> +++ b/OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm >>> @@ -0,0 +1,80 @@ >>> +;------------------------------------------------------------------------------ >>> +; @file >>> +; First code executed by processor after resetting. >>> +; Derived from UefiCpuPkg/ResetVector/Vtf0/Ia16/ResetVectorVtf0.asm >>> +; >>> +; Copyright (c) 2019, AMD Inc. All rights reserved.
>>> +; SPDX-License-Identifier: BSD-2-Clause-Patent >> >> (2) Thanks for the "derived from" hint -- but in that case, you should >> probably keep the original copyright notice too. > > Ok, will do. > >> >>> +; >>> +;------------------------------------------------------------------------------ >>> + >>> +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 >>> +; >>> +; standardProcessorSevEsReset: (0xffffffd0) >>> +; When using the Application Processors entry point, always perform a >>> +; far jump to the RIP/CS value contained at this location. This will >>> +; default to EarlyBspInitReal16 unless specifically overridden. >>> + >>> +standardProcessorSevEsReset: >>> + DW 0x0000 >>> + DW 0x0000 >>> + >>> +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 >>> +; >>> + cmp dword [CS:0xFFD0], 0 >>> + je EarlyBspInitReal16 >>> + jmp far [CS:0xFFD0] >>> + >>> +ALIGN 16 >>> + >>> +fourGigabytes: >>> + >>> >> >> It's worth looking at this patch as a diff against the original: >> >>> --- UefiCpuPkg/ResetVector/Vtf0/Ia16/ResetVectorVtf0.asm 2019-09-25 17:09:42.856850422 +0200 >>> +++ OvmfPkg/ResetVector/Ia16/ResetVectorVtf0.asm 2019-10-02 16:40:55.906630335 +0200 >>> @@ -1,8 +1,9 @@ >>> ;------------------------------------------------------------------------------ >>> ; @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.
>>> +; Copyright (c) 2019, AMD Inc. All rights reserved.
>>> ; SPDX-License-Identifier: BSD-2-Clause-Patent >>> ; >>> ;------------------------------------------------------------------------------ >>> @@ -24,6 +25,20 @@ >>> TIMES (0x1000 - ($ - EndOfPageTables) - 0x20) DB 0 >>> %endif >>> >>> +; >>> +; SEV-ES Processor Reset support >>> +; >>> +; standardProcessorSevEsReset: (0xffffffd0) >>> +; When using the Application Processors entry point, always perform a >>> +; far jump to the RIP/CS value contained at this location. This will >>> +; default to EarlyBspInitReal16 unless specifically overridden. >>> + >>> +standardProcessorSevEsReset: >>> + DW 0x0000 >>> + DW 0x0000 >>> + >>> +ALIGN 16 >>> + >>> applicationProcessorEntryPoint: >>> ; >>> ; Application Processors entry point >>> @@ -55,9 +70,9 @@ >>> ; >>> ; This is where the processor will begin execution >>> ; >>> - nop >>> - nop >>> - jmp EarlyBspInitReal16 >>> + cmp dword [CS:0xFFD0], 0 >>> + je EarlyBspInitReal16 >>> + jmp far [CS:0xFFD0] >>> >>> ALIGN 16 >>> >> >> (3) Can't we / shouldn't we implement this change in the original, >> actually? The new code doesn't seem to hurt if it's not activated, and >> it doesn't complicate the code much. > > If there are no objections, that can be done. It did concern me that there > are a couple of nop instructions before the jmp (which replaced a WBINVD) > and that there might be a reason for them being there. Based on that, I > just created the new file specific for OVMF. > >> >> (4) Can we use "standardProcessorSevEsReset" in place of the constant >> 0xFFD0 somehow? > > I'll look into that. I know "CS:standardProcessorSevEsReset" won't work > and I tried a bunch of different things, but there may be another way. It would be nice to use a %define then, I think. Macro names are easier to grep for than magic constants. Thanks! Laszlo