Thomas: > -----Original Message----- > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Lendacky, > Thomas > Sent: Wednesday, May 6, 2020 10:35 PM > To: devel@edk2.groups.io; ler...@redhat.com > Cc: Justen, Jordan L <jordan.l.jus...@intel.com>; Ard Biesheuvel > <ard.biesheu...@linaro.org>; Gao, Liming <liming....@intel.com>; > Dong, Eric <eric.d...@intel.com>; Ni, Ray <ray...@intel.com>; Brijesh Singh > <brijesh.si...@amd.com>; Anthony Perard > <anthony.per...@citrix.com>; You, Benjamin <benjamin....@intel.com>; Dong, > Guo <guo.d...@intel.com>; Julien Grall > <jul...@xen.org>; Ma, Maurice <maurice...@intel.com>; Andrew Fish > <af...@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.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 > >> -; > >> 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 <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. > > 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/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
