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

["Laszlo Ersek" <lersek@redhat.com>]

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