<Quote> > Should that > section not use the !if check and just list both .inf files > (SecPeiCpuExceptionHandlerLib.inf and > Xcode5SecPeiCpuExceptionHandlerLib.inf)?
Hmmm, this is a very good point; after all, the updated (=reverted) "SecPeiCpuExceptionHandlerLib.inf" instance will not build with XCODE5. Therefore we should list both lib instance INF files under [Components], but make "SecPeiCpuExceptionHandlerLib.inf" conditional on non-XCODE5. </Quote> Xcode5SecPeiCpuExceptionHandlerLib.inf could be added to the ignore list in the CI yaml file. PR gating CI currently only uses GCC and VS2017/19 and shouldn’t have a problem with the reverted lib. This makes Xcode5SecPeiCpuExceptionHandlerLib the exception which can be documented in the ignore list (why it’s being ignored). Thoughts? - Bret From: Laszlo Ersek via groups.io<mailto:lersek=redhat....@groups.io> Sent: Wednesday, May 6, 2020 9:33 AM To: Tom Lendacky<mailto:thomas.lenda...@amd.com>; devel@edk2.groups.io<mailto:devel@edk2.groups.io> Cc: Jordan Justen<mailto:jordan.l.jus...@intel.com>; Ard Biesheuvel<mailto:ard.biesheu...@linaro.org>; Liming Gao<mailto:liming....@intel.com>; Eric Dong<mailto:eric.d...@intel.com>; Ray Ni<mailto:ray...@intel.com>; Brijesh Singh<mailto:brijesh.si...@amd.com>; Anthony Perard<mailto:anthony.per...@citrix.com>; Benjamin You<mailto:benjamin....@intel.com>; Guo Dong<mailto:guo.d...@intel.com>; Julien Grall<mailto:jul...@xen.org>; Maurice Ma<mailto:maurice...@intel.com>; Andrew Fish<mailto:af...@apple.com> Subject: [EXTERNAL] Re: [edk2-devel] [PATCH 4/4] UefiCpuPkg/CpuExceptionHandler: Revert binary patching in standard CpuExceptionHandlerLib On 05/06/20 16:35, Tom Lendacky wrote: > On 5/5/20 5:15 PM, Laszlo Ersek via groups.io wrote: >> On 05/01/20 22:17, Lendacky, Thomas wrote: >>> BZ: >>> https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D2340&data=02%7C01%7CBret.Barkelew%40microsoft.com%7Ca9365fbefc53477c9e3308d7f1db4560%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637243796372782584&sdata=R%2B5IgncDzx7viv3yIwX3MloX1QlzuUJ4bZlKnc%2B0xoI%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://nam06.safelinks.protection.outlook.com/?url=http%3A%2F%2Fopensource.org%2Flicenses%2Fbsd-license.php&data=02%7C01%7CBret.Barkelew%40microsoft.com%7Ca9365fbefc53477c9e3308d7f1db4560%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637243796372792580&sdata=CZovXRJ6HURgo6sz%2FDi55SUy9IsrJ2pFzcHX%2Bp%2Fv8qA%3D&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 <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://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fdev.azure.com%2Ftianocore%2Fedk2-ci%2F_build%2Fresults%3FbuildId%3D6516%26view%3Dresults&data=02%7C01%7CBret.Barkelew%40microsoft.com%7Ca9365fbefc53477c9e3308d7f1db4560%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637243796372792580&sdata=N2aYxzfr%2BNw7hkhBTEg0C%2Fm4Hv00xXBZhSz3%2FFgiW1s%3D&reserved=0). > 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? The reason is that the CI code found a library instance (INF) in UefiCpuPkg that cannot be built *stand-alone* (i.e. without being consumed by a different UEFI module / INF file). In core packages, library instances should be buildable stand-alone with the "-m" build flag, and for that, the lib instances need to be listed in the [Components] section. > Is it due to the > [Components] section of the UefiCpuPkg/UefiCpuPkg.dsc file? Yes. > Should that > section not use the !if check and just list both .inf files > (SecPeiCpuExceptionHandlerLib.inf and > Xcode5SecPeiCpuExceptionHandlerLib.inf)? Hmmm, this is a very good point; after all, the updated (=reverted) "SecPeiCpuExceptionHandlerLib.inf" instance will not build with XCODE5. Therefore we should list both lib instance INF files under [Components], but make "SecPeiCpuExceptionHandlerLib.inf" conditional on non-XCODE5. I wonder how the CI logic will cope with this! Thanks, Laszlo -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#58735): https://edk2.groups.io/g/devel/message/58735 Mute This Topic: https://groups.io/mt/74034484/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
