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.d...@intel.com>
> Cc: Ray Ni <ray...@intel.com>
> Cc: Laszlo Ersek <ler...@redhat.com>
> Cc: Liming Gao <liming....@intel.com>
> Signed-off-by: Tom Lendacky <thomas.lenda...@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 <ler...@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
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#58666): https://edk2.groups.io/g/devel/message/58666
Mute This Topic: https://groups.io/mt/73406891/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
