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 <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.
It would be nice to use a %define then, I think. Macro names are easier to grep for than magic constants. Thanks! Laszlo -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#48425): https://edk2.groups.io/g/devel/message/48425 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] -=-=-=-=-=-=-=-=-=-=-=-
