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

["Lendacky, Thomas" <thomas.lendacky@amd.com>]

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.

Thanks,
Tom

> 
> Thanks
> Laszlo
>