From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-1.mimecast.com (us-smtp-1.mimecast.com [207.211.31.120]) by mx.groups.io with SMTP id smtpd.web10.4198.1588782833938298171 for ; Wed, 06 May 2020 09:33:54 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=fbG2H6BP; spf=pass (domain: redhat.com, ip: 207.211.31.120, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1588782833; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=xOVyS+qq6F5uxQnQ7W/0B0lJrkWsPTQ7dPbgaM+PtqU=; b=fbG2H6BPQ8QFRG4h329aR55M1vfkiGVr2oBuYHHb40RRIqe1nDKDdb2Gu6Pg6XWbpKd9Cn 4/lmTcp35EJjnl2IV28zbLklznsR7Zvw0X/pYqex0gOgcEsnzO/fessOX3PJZHv3og4ez3 UaEH3lzWR3VyFVRkAyUMpwPny+axtxU= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-241-WdoXbRlkOB2TGRXLzrni9A-1; Wed, 06 May 2020 12:33:44 -0400 X-MC-Unique: WdoXbRlkOB2TGRXLzrni9A-1 Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.phx2.redhat.com [10.5.11.13]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id C985A107ACCA; Wed, 6 May 2020 16:33:41 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-112-227.ams2.redhat.com [10.36.112.227]) by smtp.corp.redhat.com (Postfix) with ESMTP id B586E6918A; Wed, 6 May 2020 16:33:38 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH 4/4] UefiCpuPkg/CpuExceptionHandler: Revert binary patching in standard CpuExceptionHandlerLib To: Tom Lendacky , devel@edk2.groups.io Cc: Jordan Justen , Ard Biesheuvel , Liming Gao , Eric Dong , Ray Ni , Brijesh Singh , Anthony Perard , Benjamin You , Guo Dong , Julien Grall , Maurice Ma , Andrew Fish References: <26e51d24-fe19-4fa1-6bac-6936041af39d@amd.com> From: "Laszlo Ersek" Message-ID: <39964d2d-e9fa-31bb-78b4-f65ba54a9f36@redhat.com> Date: Wed, 6 May 2020 18:33:37 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <26e51d24-fe19-4fa1-6bac-6936041af39d@amd.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.13 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=WINDOWS-1252 Content-Transfer-Encoding: quoted-printable 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=3Dhttps%3A%2F%2Fbug= zilla.tianocore.org%2Fshow_bug.cgi%3Fid%3D2340&data=3D02%7C01%7Cthomas.= lendacky%40amd.com%7Cd2ec699d2c644a55724008d7f141d96f%7C3dd8961fe4884e608e1= 1a82d994e183d%7C0%7C0%7C637243137431443098&sdata=3DoTTju7144KZc8VCmQqu7= 4UilIOzQyji9jlO%2BMJeZYyU%3D&reserved=3D0 >>> >>> >>> 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 >>> Cc: Ray Ni >>> Cc: Laszlo Ersek >>> Cc: Liming Gao >>> Signed-off-by: Tom Lendacky >>> --- >>> =A0 .../X64/ExceptionHandlerAsm.nasm=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0= =A0=A0 | 25 +++++-------------- >>> =A0 1 file changed, 6 insertions(+), 19 deletions(-) >>> >>> diff --git >>> a/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ExceptionHandlerAsm.nas= m >>> b/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ExceptionHandlerAsm.nas= m >>> index 19198f273137..3814f9de3703 100644 >>> --- >>> a/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ExceptionHandlerAsm.nas= m >>> +++ >>> b/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ExceptionHandlerAsm.nas= m >>> @@ -34,7 +34,7 @@ AsmIdtVectorBegin: >>> =A0=A0=A0=A0=A0 db=A0=A0=A0=A0=A0 0x6a=A0=A0=A0=A0=A0=A0=A0 ; push=A0 #= VectorNum >>> =A0=A0=A0=A0=A0 db=A0=A0=A0=A0=A0 ($ - AsmIdtVectorBegin) / ((AsmIdtVec= torEnd - >>> AsmIdtVectorBegin) / 32) ; VectorNum >>> =A0=A0=A0=A0=A0 push=A0=A0=A0 rax >>> -=A0=A0=A0 mov=A0=A0=A0=A0 rax, strict qword 0 ;=A0=A0=A0 mov=A0=A0=A0= =A0 rax, >>> ASM_PFX(CommonInterruptEntry) >>> +=A0=A0=A0 mov=A0=A0=A0=A0 rax, ASM_PFX(CommonInterruptEntry) >>> =A0=A0=A0=A0=A0 jmp=A0=A0=A0=A0 rax >>> =A0 %endrep >>> =A0 AsmIdtVectorEnd: >>> @@ -44,8 +44,7 @@ HookAfterStubHeaderBegin: >>> =A0 @VectorNum: >>> =A0=A0=A0=A0=A0 db=A0=A0=A0=A0=A0 0=A0=A0=A0=A0=A0=A0=A0=A0=A0 ; 0 will= be fixed >>> =A0=A0=A0=A0=A0 push=A0=A0=A0 rax >>> -=A0=A0=A0 mov=A0=A0=A0=A0 rax, strict qword 0 ;=A0=A0=A0=A0 mov=A0=A0= =A0=A0 rax, >>> HookAfterStubHeaderEnd >>> -JmpAbsoluteAddress: >>> +=A0=A0=A0 mov=A0=A0=A0=A0 rax, HookAfterStubHeaderEnd >>> =A0=A0=A0=A0=A0 jmp=A0=A0=A0=A0 rax >>> =A0 HookAfterStubHeaderEnd: >>> =A0=A0=A0=A0=A0 mov=A0=A0=A0=A0 rax, rsp >>> @@ -257,7 +256,8 @@ HasErrorCode: >>> =A0=A0=A0=A0=A0 ; and make sure RSP is 16-byte aligned >>> =A0=A0=A0=A0=A0 ; >>> =A0=A0=A0=A0=A0 sub=A0=A0=A0=A0 rsp, 4 * 8 + 8 >>> -=A0=A0=A0 call=A0=A0=A0 ASM_PFX(CommonExceptionHandler) >>> +=A0=A0=A0 mov=A0=A0=A0=A0 rax, ASM_PFX(CommonExceptionHandler) >>> +=A0=A0=A0 call=A0=A0=A0 rax >>> =A0=A0=A0=A0=A0 add=A0=A0=A0=A0 rsp, 4 * 8 + 8 >>> >>> =A0=A0=A0=A0=A0 cli >>> @@ -365,24 +365,11 @@ DoIret: >>> =A0 ; comments here for definition of address map >>> =A0 global ASM_PFX(AsmGetTemplateAddressMap) >>> =A0 ASM_PFX(AsmGetTemplateAddressMap): >>> -=A0=A0=A0 lea=A0=A0=A0=A0 rax, [AsmIdtVectorBegin] >>> +=A0=A0=A0 mov=A0=A0=A0=A0 rax, AsmIdtVectorBegin >>> =A0=A0=A0=A0=A0 mov=A0=A0=A0=A0 qword [rcx], rax >>> =A0=A0=A0=A0=A0 mov=A0=A0=A0=A0 qword [rcx + 0x8],=A0 (AsmIdtVectorEnd = - >>> AsmIdtVectorBegin) / 32 >>> -=A0=A0=A0 lea=A0=A0=A0=A0 rax, [HookAfterStubHeaderBegin] >>> +=A0=A0=A0 mov=A0=A0=A0=A0 rax, HookAfterStubHeaderBegin >>> =A0=A0=A0=A0=A0 mov=A0=A0=A0=A0 qword [rcx + 0x10], rax >>> - >>> -; Fix up CommonInterruptEntry address >>> -=A0=A0=A0 lea=A0=A0=A0 rax, [ASM_PFX(CommonInterruptEntry)] >>> -=A0=A0=A0 lea=A0=A0=A0 rcx, [AsmIdtVectorBegin] >>> -%rep=A0 32 >>> -=A0=A0=A0 mov=A0=A0=A0 qword [rcx + (JmpAbsoluteAddress - 8 - >>> HookAfterStubHeaderBegin)], rax >>> -=A0=A0=A0 add=A0=A0=A0 rcx, (AsmIdtVectorEnd - AsmIdtVectorBegin) / 32 >>> -%endrep >>> -; Fix up HookAfterStubHeaderEnd >>> -=A0=A0=A0 lea=A0=A0=A0 rax, [HookAfterStubHeaderEnd] >>> -=A0=A0=A0 lea=A0=A0=A0 rcx, [JmpAbsoluteAddress] >>> -=A0=A0=A0 mov=A0=A0=A0 qword [rcx - 8], rax >>> - >>> =A0=A0=A0=A0=A0 ret >>> >>> =A0 >>> ;----------------------------------------------------------------------= --------------- >>> >>> >> >> With this patch applied, the differences with the "original" remain: >> >> $ git diff 2db0ccc2d7fe^..HEAD -- \ >> =A0=A0=A0=A0=A0=A0 >> UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ExceptionHandlerAsm.nasm >> >>> diff --git >>> a/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ExceptionHandlerAsm.nas= m >>> b/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ExceptionHandlerAsm.nas= m >>> index ba8993d84b0b..3814f9de3703 100644 >>> --- >>> a/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ExceptionHandlerAsm.nas= m >>> +++ >>> b/UefiCpuPkg/Library/CpuExceptionHandlerLib/X64/ExceptionHandlerAsm.nas= m >>> @@ -1,12 +1,6 @@ >>> =A0 >>> ;----------------------------------------------------------------------= -------- >>> ; >>> -; Copyright (c) 2012 - 2014, Intel Corporation. All rights >>> reserved.
>>> -; This program and the accompanying materials >>> -; are licensed and made available under the terms and conditions of >>> the BSD License >>> -; which accompanies this distribution.=A0 The full text of the license >>> may be found at >>> -; >>> https://nam11.safelinks.protection.outlook.com/?url=3Dhttp%3A%2F%2Fopen= source.org%2Flicenses%2Fbsd-license.php&data=3D02%7C01%7Cthomas.lendack= y%40amd.com%7Cd2ec699d2c644a55724008d7f141d96f%7C3dd8961fe4884e608e11a82d99= 4e183d%7C0%7C0%7C637243137431443098&sdata=3DSZAc83Y%2BwZauGcj47EDgc10fn= xSucy2ljeI9PcaJSvE%3D&reserved=3D0. >>> >>> -; >>> -; 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.
>>> +; SPDX-License-Identifier: BSD-2-Clause-Patent >>> =A0 ; >>> =A0 ; Module Name: >>> =A0 ; >> >> This is expected. >> >>> @@ -189,17 +183,19 @@ HasErrorCode: >>> =A0=A0=A0=A0=A0 push=A0=A0=A0 rax >>> =A0=A0=A0=A0=A0 push=A0=A0=A0 rax >>> =A0=A0=A0=A0=A0 sidt=A0=A0=A0 [rsp] >>> -=A0=A0=A0 xchg=A0=A0=A0 rax, [rsp + 2] >>> -=A0=A0=A0 xchg=A0=A0=A0 rax, [rsp] >>> -=A0=A0=A0 xchg=A0=A0=A0 rax, [rsp + 8] >>> +=A0=A0=A0 mov=A0=A0=A0=A0 bx, word [rsp] >>> +=A0=A0=A0 mov=A0=A0=A0=A0 rax, qword [rsp + 2] >>> +=A0=A0=A0 mov=A0=A0=A0=A0 qword [rsp], rax >>> +=A0=A0=A0 mov=A0=A0=A0=A0 word [rsp + 8], bx >>> >>> =A0=A0=A0=A0=A0 xor=A0=A0=A0=A0 rax, rax >>> =A0=A0=A0=A0=A0 push=A0=A0=A0 rax >>> =A0=A0=A0=A0=A0 push=A0=A0=A0 rax >>> =A0=A0=A0=A0=A0 sgdt=A0=A0=A0 [rsp] >>> -=A0=A0=A0 xchg=A0=A0=A0 rax, [rsp + 2] >>> -=A0=A0=A0 xchg=A0=A0=A0 rax, [rsp] >>> -=A0=A0=A0 xchg=A0=A0=A0 rax, [rsp + 8] >>> +=A0=A0=A0 mov=A0=A0=A0=A0 bx, word [rsp] >>> +=A0=A0=A0 mov=A0=A0=A0=A0 rax, qword [rsp + 2] >>> +=A0=A0=A0 mov=A0=A0=A0=A0 qword [rsp], rax >>> +=A0=A0=A0 mov=A0=A0=A0=A0 word [rsp + 8], bx >>> >>> =A0 ;; UINT64=A0 Ldtr, Tr; >>> =A0=A0=A0=A0=A0 xor=A0=A0=A0=A0 rax, rax >>> >> >> Also expected, from commit f4c898f2b2db >> ("UefiCpuPkg/CpuExceptionHandlerLib: Fix split lock", 2019-09-20). >> >> Therefore, for this patch: >> >> Reviewed-by: Laszlo Ersek >> >> *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. >=20 > 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=3D6516&vie= w=3Dresults). > The error is the same in all cases and the error message is: >=20 > CRITICAL - > UefiCpuPkg/Library/CpuExceptionHandlerLib/Xcode5SecPeiCpuExceptionHandler= Lib.inf > not in UefiCpuPkg/UefiCpuPkg.dsc >=20 > 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 (=3Dreverted) "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