Re: [edk2-devel] [PATCH 4/4] UefiCpuPkg/CpuExceptionHandler: Revert binary patching in standard CpuExceptionHandlerLib

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

On 5/5/20 5:15 PM, Laszlo Ersek via groups.io wrote:
> On 05/01/20 22:17, Lendacky, Thomas wrote:
>> BZ: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D2340&data=02%7C01%7Cthomas.lendacky%40amd.com%7Cd2ec699d2c644a55724008d7f141d96f%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637243137431443098&sdata=oTTju7144KZc8VCmQqu74UilIOzQyji9jlO%2BMJeZYyU%3D&reserved=0
>>
>> Now that an XCODE5 specific CpuExceptionHandlerLib library is in place,
>> revert the changes made to the ExceptionHandlerAsm.nasm in commit
>> 2db0ccc2d7fe ("UefiCpuPkg: Update CpuExceptionHandlerLib pass XCODE5 tool
>> chain") so that binary patching of flash code is not performed.
>>
>> Cc: Eric Dong <eric.dong@intel.com>
>> Cc: Ray Ni <ray.ni@intel.com>
>> Cc: Laszlo Ersek <lersek@redhat.com>
>> Cc: Liming Gao <liming.gao@intel.com>
>> Signed-off-by: Tom Lendacky <thomas.lendacky@amd.com>
>> ---
>>   .../X64/ExceptionHandlerAsm.nasm              | 25 +++++--------------
>>   1 file changed, 6 insertions(+), 19 deletions(-)
>>
>> diff --git a/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ExceptionHandlerAsm.nasm b/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ExceptionHandlerAsm.nasm
>> index 19198f273137..3814f9de3703 100644
>> --- a/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ExceptionHandlerAsm.nasm
>> +++ b/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ExceptionHandlerAsm.nasm
>> @@ -34,7 +34,7 @@ AsmIdtVectorBegin:
>>       db      0x6a        ; push  #VectorNum
>>       db      ($ - AsmIdtVectorBegin) / ((AsmIdtVectorEnd - AsmIdtVectorBegin) / 32) ; VectorNum
>>       push    rax
>> -    mov     rax, strict qword 0 ;    mov     rax, ASM_PFX(CommonInterruptEntry)
>> +    mov     rax, ASM_PFX(CommonInterruptEntry)
>>       jmp     rax
>>   %endrep
>>   AsmIdtVectorEnd:
>> @@ -44,8 +44,7 @@ HookAfterStubHeaderBegin:
>>   @VectorNum:
>>       db      0          ; 0 will be fixed
>>       push    rax
>> -    mov     rax, strict qword 0 ;     mov     rax, HookAfterStubHeaderEnd
>> -JmpAbsoluteAddress:
>> +    mov     rax, HookAfterStubHeaderEnd
>>       jmp     rax
>>   HookAfterStubHeaderEnd:
>>       mov     rax, rsp
>> @@ -257,7 +256,8 @@ HasErrorCode:
>>       ; and make sure RSP is 16-byte aligned
>>       ;
>>       sub     rsp, 4 * 8 + 8
>> -    call    ASM_PFX(CommonExceptionHandler)
>> +    mov     rax, ASM_PFX(CommonExceptionHandler)
>> +    call    rax
>>       add     rsp, 4 * 8 + 8
>>
>>       cli
>> @@ -365,24 +365,11 @@ DoIret:
>>   ; comments here for definition of address map
>>   global ASM_PFX(AsmGetTemplateAddressMap)
>>   ASM_PFX(AsmGetTemplateAddressMap):
>> -    lea     rax, [AsmIdtVectorBegin]
>> +    mov     rax, AsmIdtVectorBegin
>>       mov     qword [rcx], rax
>>       mov     qword [rcx + 0x8],  (AsmIdtVectorEnd - AsmIdtVectorBegin) / 32
>> -    lea     rax, [HookAfterStubHeaderBegin]
>> +    mov     rax, HookAfterStubHeaderBegin
>>       mov     qword [rcx + 0x10], rax
>> -
>> -; Fix up CommonInterruptEntry address
>> -    lea    rax, [ASM_PFX(CommonInterruptEntry)]
>> -    lea    rcx, [AsmIdtVectorBegin]
>> -%rep  32
>> -    mov    qword [rcx + (JmpAbsoluteAddress - 8 - HookAfterStubHeaderBegin)], rax
>> -    add    rcx, (AsmIdtVectorEnd - AsmIdtVectorBegin) / 32
>> -%endrep
>> -; Fix up HookAfterStubHeaderEnd
>> -    lea    rax, [HookAfterStubHeaderEnd]
>> -    lea    rcx, [JmpAbsoluteAddress]
>> -    mov    qword [rcx - 8], rax
>> -
>>       ret
>>
>>   ;-------------------------------------------------------------------------------------
>>
> 
> With this patch applied, the differences with the "original" remain:
> 
> $ git diff 2db0ccc2d7fe^..HEAD -- \
>        UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ExceptionHandlerAsm.nasm
> 
>> diff --git a/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ExceptionHandlerAsm.nasm b/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ExceptionHandlerAsm.nasm
>> index ba8993d84b0b..3814f9de3703 100644
>> --- a/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ExceptionHandlerAsm.nasm
>> +++ b/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ExceptionHandlerAsm.nasm
>> @@ -1,12 +1,6 @@
>>   ;------------------------------------------------------------------------------ ;
>> -; Copyright (c) 2012 - 2014, Intel Corporation. All rights reserved.<BR>
>> -; This program and the accompanying materials
>> -; are licensed and made available under the terms and conditions of the BSD License
>> -; which accompanies this distribution.  The full text of the license may be found at
>> -; https://nam11.safelinks.protection.outlook.com/?url=http%3A%2F%2Fopensource.org%2Flicenses%2Fbsd-license.php&amp;data=02%7C01%7Cthomas.lendacky%40amd.com%7Cd2ec699d2c644a55724008d7f141d96f%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637243137431443098&amp;sdata=SZAc83Y%2BwZauGcj47EDgc10fnxSucy2ljeI9PcaJSvE%3D&amp;reserved=0.
>> -;
>> -; THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS,
>> -; WITHOUT WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
>> +; Copyright (c) 2012 - 2018, Intel Corporation. All rights reserved.<BR>
>> +; SPDX-License-Identifier: BSD-2-Clause-Patent
>>   ;
>>   ; Module Name:
>>   ;
> 
> This is expected.
> 
>> @@ -189,17 +183,19 @@ HasErrorCode:
>>       push    rax
>>       push    rax
>>       sidt    [rsp]
>> -    xchg    rax, [rsp + 2]
>> -    xchg    rax, [rsp]
>> -    xchg    rax, [rsp + 8]
>> +    mov     bx, word [rsp]
>> +    mov     rax, qword [rsp + 2]
>> +    mov     qword [rsp], rax
>> +    mov     word [rsp + 8], bx
>>
>>       xor     rax, rax
>>       push    rax
>>       push    rax
>>       sgdt    [rsp]
>> -    xchg    rax, [rsp + 2]
>> -    xchg    rax, [rsp]
>> -    xchg    rax, [rsp + 8]
>> +    mov     bx, word [rsp]
>> +    mov     rax, qword [rsp + 2]
>> +    mov     qword [rsp], rax
>> +    mov     word [rsp + 8], bx
>>
>>   ;; UINT64  Ldtr, Tr;
>>       xor     rax, rax
>>
> 
> Also expected, from commit f4c898f2b2db
> ("UefiCpuPkg/CpuExceptionHandlerLib: Fix split lock", 2019-09-20).
> 
> Therefore, for this patch:
> 
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> 
> *However*, this revert must be restricted to the original
> "SecPeiCpuExceptionHandlerLib.inf" instance, i.e. where binary patching
> is not acceptable. (Otherwise, in combination with my request (1) under
> patch#1, we'd needlessly break the PEI / DXE / SMM lib instances under
> XCODE5.)
> 
> (1) Therefore, please insert a new patch between patches #1 and #2, such
> that the new patch flip
> 
> - PeiCpuExceptionHandlerLib.inf
> - DxeCpuExceptionHandlerLib.inf
> - SmmCpuExceptionHandlerLib.inf
> 
> to using "Xcode5ExceptionHandlerAsm.nasm".
> 
> (If you wish, you can squash these modifications into the updated
> patch#1, rather than inserting them as a separate patch between #1 and
> #2.)
> 
> 
> In summary, I suggest the following end-state:
> 
> - we should have a self-patching NASM file, and one without
> self-patching,
> 
> - the self-patching variant should be called
> "Xcode5ExceptionHandlerAsm.nasm" (because the *only* reason for the
> self-patching is xcode5),
> 
> - we should have 5 INF files in total,
> 
> - "PeiCpuExceptionHandlerLib.inf", "DxeCpuExceptionHandlerLib.inf",
> "SmmCpuExceptionHandlerLib.inf" should use
> "Xcode5ExceptionHandlerAsm.nasm" (the self-patching is harmless),
> 
> - "SecPeiCpuExceptionHandlerLib.inf" should use
> "ExceptionHandlerAsm.nasm" (self-patching is invalid, so don't do it),
> 
> - "Xcode5SecPeiCpuExceptionHandlerLib.inf" should use
> "Xcode5ExceptionHandlerAsm.nasm" (the self-patching is invalid, but we
> can't avoid it when building with XCODE5),
> 
> - platforms should resolve the CpuExceptionHandlerLib class to
> "Xcode5SecPeiCpuExceptionHandlerLib.inf" only for the XCODE5 toolchain
> *and* for the SEC phase.

Ok, I have v2 ready to go, but when I ran it through the integration tests 
using a pull request I received some errors (see 
https://dev.azure.com/tianocore/edk2-ci/_build/results?buildId=6516&view=results). 
The error is the same in all cases and the error message is:

CRITICAL - 
UefiCpuPkg/Library/CpuExceptionHandlerLib/Xcode5SecPeiCpuExceptionHandlerLib.inf 
not in UefiCpuPkg/UefiCpuPkg.dsc

Any idea about the reason for this message? Is it due to the [Components] 
section of the UefiCpuPkg/UefiCpuPkg.dsc file? Should that section not use 
the !if check and just list both .inf files 
(SecPeiCpuExceptionHandlerLib.inf and Xcode5SecPeiCpuExceptionHandlerLib.inf)?

Thanks,
Tom

> 
> Thanks,
> Laszlo
> 
> 
> 
>