From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=66.187.233.73; helo=mx1.redhat.com; envelope-from=lersek@redhat.com; receiver=edk2-devel@lists.01.org Received: from mx1.redhat.com (mx3-rdu2.redhat.com [66.187.233.73]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 121C12112A2B7 for ; Wed, 12 Sep 2018 06:23:38 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.rdu2.redhat.com [10.11.54.5]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id E3A5740216EB; Wed, 12 Sep 2018 13:23:37 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-31.rdu2.redhat.com [10.10.120.31]) by smtp.corp.redhat.com (Postfix) with ESMTP id 54D50A30C4; Wed, 12 Sep 2018 13:23:36 +0000 (UTC) To: Liming Gao Cc: edk2-devel@lists.01.org, Jiewen Yao , Eric Dong , Michael Kinney , "Leif Lindholm (Linaro address)" , Ard Biesheuvel , Andrew Fish , "Cetola, Stephano" References: <1536729218-8884-1-git-send-email-liming.gao@intel.com> From: Laszlo Ersek Message-ID: Date: Wed, 12 Sep 2018 15:23:35 +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: <1536729218-8884-1-git-send-email-liming.gao@intel.com> X-Scanned-By: MIMEDefang 2.79 on 10.11.54.5 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.5]); Wed, 12 Sep 2018 13:23:38 +0000 (UTC) X-Greylist: inspected by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.5]); Wed, 12 Sep 2018 13:23:38 +0000 (UTC) for IP:'10.11.54.5' DOMAIN:'int-mx05.intmail.prod.int.rdu2.redhat.com' HELO:'smtp.corp.redhat.com' FROM:'lersek@redhat.com' RCPT:'' Subject: Re: [Patch] UefiCpuPkg PiSmmCpuDxeSmm: Update SmiEntry function run the same position X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 12 Sep 2018 13:23:39 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 09/12/18 07:13, Liming Gao wrote: > 1. Remove jmp _SmiHandler, and run the code at the same position. > 2. Fix up the function call address as the absolute address. > Verify OVMF SMM boot to shell with VS2017, GCC5 and XCODE5 tool chain. > > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Liming Gao > Cc: Laszlo Ersek > Cc: Eric Dong > Cc: Jiewen Yao > --- > UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.nasm | 34 +++++++++++++++++++++-------- > 1 file changed, 25 insertions(+), 9 deletions(-) > > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.nasm b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.nasm > index 315d0f8..d8259de 100644 > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.nasm > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.nasm > @@ -173,9 +173,6 @@ SmiHandlerIdtrAbsAddr: > mov gs, eax > mov ax, [rbx + DSC_SS] > mov ss, eax > - mov rax, strict qword 0 ; mov rax, _SmiHandler > -_SmiHandlerAbsAddr: > - jmp rax > > _SmiHandler: > mov rbx, [rsp + 0x8] ; rcx <- CpuIndex > @@ -189,13 +186,19 @@ _SmiHandler: > add rsp, -0x20 > > mov rcx, rbx > - call ASM_PFX(CpuSmmDebugEntry) > + mov rax, strict qword 0 ; call ASM_PFX(CpuSmmDebugEntry) > +CpuSmmDebugEntryAbsAddr: > + call rax > > mov rcx, rbx > - call ASM_PFX(SmiRendezvous) > + mov rax, strict qword 0 ; call ASM_PFX(SmiRendezvous) > +SmiRendezvousAbsAddr: > + call rax > > mov rcx, rbx > - call ASM_PFX(CpuSmmDebugExit) > + mov rax, strict qword 0 ; call ASM_PFX(CpuSmmDebugExit) > +CpuSmmDebugExitAbsAddr: > + call rax > > add rsp, 0x20 > > @@ -206,7 +209,8 @@ _SmiHandler: > > add rsp, 0x200 > > - lea rax, [ASM_PFX(mXdSupported)] > + mov rax, strict qword 0 ; lea rax, [ASM_PFX(mXdSupported)] > +mXdSupportedAbsAddr: > mov al, [rax] > cmp al, 0 > jz .1 > @@ -230,7 +234,19 @@ ASM_PFX(PiSmmCpuSmiEntryFixupAddress): > lea rcx, [SmiHandlerIdtrAbsAddr] > mov qword [rcx - 8], rax > > - lea rax, [_SmiHandler] > - lea rcx, [_SmiHandlerAbsAddr] > + lea rax, [ASM_PFX(CpuSmmDebugEntry)] > + lea rcx, [CpuSmmDebugEntryAbsAddr] > + mov qword [rcx - 8], rax > + > + lea rax, [ASM_PFX(SmiRendezvous)] > + lea rcx, [SmiRendezvousAbsAddr] > + mov qword [rcx - 8], rax > + > + lea rax, [ASM_PFX(CpuSmmDebugExit)] > + lea rcx, [CpuSmmDebugExitAbsAddr] > + mov qword [rcx - 8], rax > + > + lea rax, [ASM_PFX(mXdSupported)] > + lea rcx, [mXdSupportedAbsAddr] > mov qword [rcx - 8], rax > ret > (a) The patch seems to do two things (listed as (1) and (2) in the commit message). They should be implemented in separate patches, in a series. (b) Regarding part (1): why did "jmp _SmiHandler" exist in the first place? That jump instruction was originally added in commit 9a36d4dc3f5e: "UefiCpuPkg PiSmmCpuDxeSmm: Convert X64/SmiEntry.asm to NASM", 2016-06-28 with the comment "instruction is not needed". If it was not needed, then why had we added it? For that, let's look at "UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.asm" instead (that is, the pre-NASM file from which the NASM version was created). In that file, the same comment was introduced as part of commit 427e3573426f: "UefiCpuPkg: Add PiSmmCpuDxeSmm module X64 files", 2015-10-19 That means that we've had this unnecessary jump ever since the inception of PiSmmCpuDxeSmm (in the open source edk2 tree). And now we are removing the jump, for code simplification. That's a good thing. But it should be a separate patch, with the argument clearly spelled out: Remove the _SmiHandler label and all the instructions that facilitate the jump to it, because the jump to _SmiHandler has always been unnecessary, as stated by the comment in original commit 427e3573426f as well. Note: the _SmiHandler label itself should likely be removed too, not just _SmiHandlerAbsAddr. It is never referenced, after the change. (c) I must say that the description of part (2) is extremely lacking. I've had to spend more than two hours to reconstruct the argument. Normally the commit message should describe - what the expected behavior is, for what use case, - what the current (problematic) behavior is, on the master branch, - how the change addresses the issue, - if there is a BZ for the issue, it should be referenced. So here's my understanding of part (2) -- which should be a separate patch, again --: * Before commit e21e355e2ca7f, we had mov rax, ASM_PFX(CpuSmmDebugEntry) call rax This was not good, because XCODE can only handle RIP-relative addressing in assembly code. So in bug 849 / commit e21e355e2ca7f, we changed it to: call ASM_PFX(CpuSmmDebugEntry) * While this was good for XCODE (I guess?), it wasn't good for runtime behavior. Because: this assembly code is used as a template, it is copied to a different place, and jumps / calls / data references made from that other place will not work with the same relative distance. For runtime, we need the jumps / calls / data references to be absolute, despite XCODE not supporting such. * Therefore we split the solution in two. In the PiSmmCpuSmiEntryFixupAddress routine, we use the LEA instruction (with RIP-relative addressing) to calculate the absolute addresses that are needed. XCODE copes with these LEA instructions just fine. Then we patch the absolute addresses into the template instructions, so that the code, when copied to another place, will continue to work correctly -- the absolute addresses are not affected by the copying. If this is correct, then please document all of it in the commit message. (Or, please open a BZ, and document all of it there, and reference the BZ in the commit message.) I really shouldn't have to spend hours to reconstruct the thought process, from bits and pieces in the commit log and in various mailing list threads. (d) Now here's my final question. If the current (RIP-relative) calls are incorrect, and this patch is needed to fix them, then: - Why am I not experiencing any issues on my side? - Why weren't the issues in question found when you were working on bug 849 / commit e21e355e2ca7f in the first place? What has changed or exposed the remaining problem? (e) The current standard of commit documentation is unacceptable for a mature open source & open development project. Just writing good code is entirely insufficient. Patches are not written and posted for specific employers, or for specific colleagues at the same office, but for the global TianoCore/edk2 community. We've discussed this several times now at the stewards' meeting, and it's high time we started enforcing it. Nacked-by: Laszlo Ersek I think next time I might nack a patch without looking at the code even, if the commit message (and the BZ referenced by it, if any) are unsatisfactory. I do highly appreciate that you tested the change with OVMF. Thanks, Laszlo