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; Wed, 02 Oct 2019 07:54:08 -0700 Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.phx2.redhat.com [10.5.11.14]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id CE14618C890B; Wed, 2 Oct 2019 14:54:07 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-71.rdu2.redhat.com [10.10.120.71]) by smtp.corp.redhat.com (Postfix) with ESMTP id F3CE05D9D3; Wed, 2 Oct 2019 14:54:04 +0000 (UTC) Subject: Re: [edk2-devel] [RFC PATCH v2 37/44] OvmfPkg: Add support for SEV-ES AP reset vector re-directing 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: <61fd6897ca552edbb39ab2f2ee0cce0b505ee71c.1568922729.git.thomas.lendacky@amd.com> From: "Laszlo Ersek" Message-ID: Date: Wed, 2 Oct 2019 16:54:04 +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: <61fd6897ca552edbb39ab2f2ee0cce0b505ee71c.1568922729.git.thomas.lendacky@amd.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.14 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.6.2 (mx1.redhat.com [10.5.110.70]); Wed, 02 Oct 2019 14:54:08 +0000 (UTC) Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit 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? > > 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. > +; > +;------------------------------------------------------------------------------ > + > +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. (4) Can we use "standardProcessorSevEsReset" in place of the constant 0xFFD0 somehow? Thanks Laszlo