[edk2-devel] [PATCH 4/4] UefiCpuPkg/CpuExceptionHandler: Revert binary patching in standard CpuExceptionHandlerLib
Liming Gao
liming.gao at intel.com
Wed May 6 14:53:11 UTC 2020
Thomas:
> -----Original Message-----
> From: devel at edk2.groups.io <devel at edk2.groups.io> On Behalf Of Lendacky, Thomas
> Sent: Wednesday, May 6, 2020 10:35 PM
> To: devel at edk2.groups.io; lersek at redhat.com
> Cc: Justen, Jordan L <jordan.l.justen at intel.com>; Ard Biesheuvel <ard.biesheuvel at linaro.org>; Gao, Liming <liming.gao at intel.com>;
> Dong, Eric <eric.dong at intel.com>; Ni, Ray <ray.ni at intel.com>; Brijesh Singh <brijesh.singh at amd.com>; Anthony Perard
> <anthony.perard at citrix.com>; You, Benjamin <benjamin.you at intel.com>; Dong, Guo <guo.dong at intel.com>; Julien Grall
> <julien at xen.org>; Ma, Maurice <maurice.ma at intel.com>; Andrew Fish <afish at apple.com>
> Subject: Re: [edk2-devel] [PATCH 4/4] UefiCpuPkg/CpuExceptionHandler: Revert binary patching in standard CpuExceptionHandlerLib
>
> 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&d
> ata=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 at intel.com>
> >> Cc: Ray Ni <ray.ni at intel.com>
> >> Cc: Laszlo Ersek <lersek at redhat.com>
> >> Cc: Liming Gao <liming.gao at intel.com>
> >> Signed-off-by: Tom Lendacky <thomas.lendacky at 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&data=02%7C01%7Cthomas.lendacky%40amd.com%7Cd2ec699d2c644a55724008d7f141d96f%7C3dd8961fe4884e608e
> 11a82d994e183d%7C0%7C0%7C637243137431443098&sdata=SZAc83Y%2BwZauGcj47EDgc10fnxSucy2ljeI9PcaJSvE%3D&reser
> ved=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 at 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)?
>
Yes. Package DSC [Components] section should list all module INF files, then verify their build.
Thanks
Liming
> Thanks,
> Tom
>
> >
> > Thanks,
> > Laszlo
> >
> >
> >
> >
>
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#58714): https://edk2.groups.io/g/devel/message/58714
Mute This Topic: https://groups.io/mt/73406891/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list