From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-1.mimecast.com (us-smtp-1.mimecast.com [205.139.110.120]) by mx.groups.io with SMTP id smtpd.web12.8068.1588716939593371691 for ; Tue, 05 May 2020 15:15:39 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=O2HIDuYo; spf=pass (domain: redhat.com, ip: 205.139.110.120, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1588716938; 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=ZVW9rejqfvLa05Y1b5XCe+7ednLqWkYC+LeGDcGhnWc=; b=O2HIDuYoelt18DShijTuQ9dn48zhl+h+tpZgrEjiMU9Dd1b64jl2vkvPAsE69XH/iRMB1I h3VoLd26Ce+HHnMo+c2CD/sz1VneaUGjLLqYRZ7kM7/Ewgp97aQtux7XeqQFXqByl58KqC 9OoqgS8ymcSSXezzGjFVMRx7o4qSBXA= 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-294-4KRxcQvwPcW19RL5ijnI3Q-1; Tue, 05 May 2020 18:15:32 -0400 X-MC-Unique: 4KRxcQvwPcW19RL5ijnI3Q-1 Received: from smtp.corp.redhat.com (int-mx06.intmail.prod.int.phx2.redhat.com [10.5.11.16]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id D7E2380B71A; Tue, 5 May 2020 22:15:30 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-114-134.ams2.redhat.com [10.36.114.134]) by smtp.corp.redhat.com (Postfix) with ESMTP id B33F85C1B2; Tue, 5 May 2020 22:15:27 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH 4/4] UefiCpuPkg/CpuExceptionHandler: Revert binary patching in standard CpuExceptionHandlerLib To: devel@edk2.groups.io, thomas.lendacky@amd.com 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: From: "Laszlo Ersek" Message-ID: Date: Wed, 6 May 2020 00:15:26 +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: X-Scanned-By: MIMEDefang 2.79 on 10.5.11.16 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit On 05/01/20 22:17, Lendacky, Thomas wrote: > BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=2340 > > 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 > --- > .../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.
> -; 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 > -; http://opensource.org/licenses/bsd-license.php. > -; > -; 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 > ; > ; 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 *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. Thanks, Laszlo