[edk2-devel] [RFC PATCH v2 37/44] OvmfPkg: Add support for SEV-ES AP reset vector re-directing
Laszlo Ersek
lersek at redhat.com
Thu Oct 3 09:09:08 UTC 2019
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.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?
>
> Yes, this is due to the BuildOptions include path order. I'll update the
> commit message.
>
>>
>>>
>>> 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.
>
> 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/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list