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://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugzilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D2340&data=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%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637243137431443098&sdata=SZAc83Y%2BwZauGcj47EDgc10fnxSucy2ljeI9PcaJSvE%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://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?
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 (#58723): https://edk2.groups.io/g/devel/message/58723 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] -=-=-=-=-=-=-=-=-=-=-=-
