On 10/2/19 9:54 AM, Laszlo Ersek wrote: > On 09/19/19 21:53, Lendacky, Thomas wrote: >> From: Tom Lendacky <thomas.lenda...@amd.com> >> >> 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 <jordan.l.jus...@intel.com> >> Cc: Laszlo Ersek <ler...@redhat.com> >> Cc: Ard Biesheuvel <ard.biesheu...@linaro.org> >> Signed-off-by: Tom Lendacky <thomas.lenda...@amd.com> >> --- >> 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.<BR> >> +; 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.<BR> >> +; Copyright (c) 2019, AMD Inc. All rights reserved.<BR> >> ; 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. Thanks, Tom > > Thanks > Laszlo > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#48387): https://edk2.groups.io/g/devel/message/48387 Mute This Topic: https://groups.io/mt/34203584/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
