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 C363021B02822 for ; Wed, 12 Sep 2018 12:49:12 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx06.intmail.prod.int.rdu2.redhat.com [10.11.54.6]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 4456787A72; Wed, 12 Sep 2018 19:49:11 +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 859102166BA2; Wed, 12 Sep 2018 19:49:09 +0000 (UTC) To: "Gao, Liming" Cc: "edk2-devel@lists.01.org" , "Yao, Jiewen" , "Dong, Eric" , "Kinney, Michael D" , "Leif Lindholm (Linaro address)" , Ard Biesheuvel , Andrew Fish , "Cetola, Stephano" References: <1536729218-8884-1-git-send-email-liming.gao@intel.com> <4A89E2EF3DFEDB4C8BFDE51014F606A14E2F66CA@SHSMSX104.ccr.corp.intel.com> From: Laszlo Ersek Message-ID: <93d6b5eb-3e11-ed12-e860-b6802aaaec0d@redhat.com> Date: Wed, 12 Sep 2018 21:49:08 +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: <4A89E2EF3DFEDB4C8BFDE51014F606A14E2F66CA@SHSMSX104.ccr.corp.intel.com> X-Scanned-By: MIMEDefang 2.78 on 10.11.54.6 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.1]); Wed, 12 Sep 2018 19:49:11 +0000 (UTC) X-Greylist: inspected by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.1]); Wed, 12 Sep 2018 19:49:11 +0000 (UTC) for IP:'10.11.54.6' DOMAIN:'int-mx06.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 19:49:13 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 09/12/18 17:42, Gao, Liming wrote: > Laszlo: > Before commit e21e355e2ca7fefb15b4df7078f995d3fb9c2b89, jmp _SmiHandler is commented. And below code, ASM_PFX(CpuSmmDebugEntry) is moved into rax, then call it. But, this code doesn't work in XCODE5 tool chain. Like you say, XCODE5 doesn't generated the absolute address in the EFI image. So, rax stores the relative address. Once this logic is moved to another place, it will not work. > ; jmp _SmiHandler ; instruction is not needed > ... > mov rax, ASM_PFX(CpuSmmDebugEntry) > call rax > > Commit e21e355e2ca7fefb15b4df7078f995d3fb9c2b89 is to support XCODE5. I choose one tricky way to fix it. Although SmiEntry logic is copied to another place and run, but here I enable jmp _SmiHandler to jmp the original code location, then call ASM_PFX(CpuSmmDebugEntry) with the relative address can work. > mov rax, strict qword 0 ; mov rax, _SmiHandler > _SmiHandlerAbsAddr: > jmp rax > ... > call ASM_PFX(CpuSmmDebugEntry) > > Today, Jiewen raises the issue that SmiHandler should run in the copied address, can't run in the common address. So, I update its logic and remove jmp _SmiHandler, then keep code continue run in copied address. But, I still need to fix up CpuSmmDebugEntry address to the absolute address. They are both for this issue. They can't be separated. > > ... > mov rax, strict qword 0 ; call ASM_PFX(CpuSmmDebugEntry) > CpuSmmDebugEntryAbsAddr: > call rax Thank you very much for the explanation. I understand now. I also understand why I got confused so much earlier. The reason is that the code was not commented sufficiently. It should have been pointed out what part of the instruction stream was meant to be executed from the copied address space (that is, not from the PiSmmCpuDxeSmm image itself), and what part of the instruction stream was meant to execute from within the the PiSmmCpuDxeSmm image. Without such comments, it's too hard to understand the transitions. You or Jiewen should please file a TianoCore BZ describing the current issue. Namely that, due to circumstances you are not allowed to share, no part of the _SmiHandler routine should execute from within the PiSmmCpuDxeSmm image; instead, all of it should execute from the copied-to address space. This requires eliminating the jump back into the PiSmmCpuDxeSmm image, and also patching up the relative addresses (to absolute addresses), for those instructions that used to run from within the PiSmmCpuDxeSmm image, but now no longer will. The necessary changes should not affect the behavior of platforms that already consume PiSmmCpuDxeSmm from edk2. The historical overview you provide above is also valuable, please include it in the commit message and/or the BZ as well. It would still be nice to comment what parts of the source file run from within the PiSmmCpuDxeSmm image. For example, PiSmmCpuSmiEntryFixupAddress() does, right? --*-- Please understand that my issue here is more serious than just missing the explanation / motivation, for this specific patch. My issue is that the posting of this work to edk2-devel should have *started* with the above explanation. You and Jiewen understand the issue. Noone else on the list (that doesn't work at your office) does. I've wasted half a day because you didn't write up the background up-front. I don't need to know your specific internal proprietary feature that requires this change. I do need to know that an internal feature with such a requirement exists, and that it is your motivation. Every ten minutes you save for yourselves on documentation is amplified to several hours of struggle, for each reviewer. And we specifically discussed this scenario with Mike at one of the earlier stewards' meetings. (The general topic back then was my raising that you [plural] liberally commit whatever you want to MdePkg / MdeModulePkg, without really naming the use cases, let alone adding client code to edk2. Whereas non-Intel contributors with a demonstrated need are heavily gated from adding code to MdePkg / MdeModulePkg. Compare: the addition of the "PciSegmentLibSegmentInfo" instances (commit 5c9bb86f171c), without any consumer in edk2 whatsoever, versus BZ#957, where the new libraries -- originally proposed for MdePkg -- had demonstrated consumers, just not from Intel. Do you see the double standard?) I wish I could just ignore UefiCpuPkg. I can't do that; it is very brittle / sensitive, and has caused numerous regressions for OVMF. I must keep a close eye on it. But patches from you [plural] certainly don't make that easy for me. It kills me that after years of us begging on the list, you [plural] still don't care about non-Intel participants as first class citizens up-front, until they raise a stink. Here's another recent example: https://bugzilla.tianocore.org/show_bug.cgi?id=1116#c3 https://bugzilla.tianocore.org/show_bug.cgi?id=1116#c4 Please start actually believing in, and practicing, Open Development. You no longer write the code and the docs for Intel only. Convincing your colleagues is easy -- that's no different from the proprietary / closed source days of firmware development. It was Intel's decision to turn edk2 into an open source project; please live that decision. Now your primary audience is the contributors that depend on collaborating with you *without* working for your employer. Well I've probably overstated my case. It's just that I've hoped for better commit messages since that stewards' meeting. Laszlo