[edk2-devel] [RFC PATCH v2 37/44] OvmfPkg: Add support for SEV-ES AP reset vector re-directing

[Laszlo Ersek]

On 09/19/19 21:53, Lendacky, Thomas wrote:
> From: Tom Lendacky <thomas.lendacky at 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?

>
> Cc: Jordan Justen <jordan.l.justen at intel.com>
> Cc: Laszlo Ersek <lersek at redhat.com>
> Cc: Ard Biesheuvel <ard.biesheuvel at linaro.org>
> Signed-off-by: Tom Lendacky <thomas.lendacky at 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.

> +;
> +;------------------------------------------------------------------------------
> +
> +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.

(4) Can we use "standardProcessorSevEsReset" in place of the constant
0xFFD0 somehow?

Thanks
Laszlo

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#48374): https://edk2.groups.io/g/devel/message/48374
Mute This Topic: https://groups.io/mt/34203584/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-