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

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

On 05/01/20 22:17, Lendacky, Thomas wrote:
> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=2340
>
> 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
> -; http://opensource.org/licenses/bsd-license.php.
> -;
> -; 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.

Thanks,
Laszlo