From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=209.132.183.28; helo=mx1.redhat.com; envelope-from=lersek@redhat.com; receiver=edk2-devel@lists.01.org Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) (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 EC924223CCEF2 for ; Fri, 2 Feb 2018 05:31:09 -0800 (PST) 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 mx1.redhat.com (Postfix) with ESMTPS id A9EB867C4E; Fri, 2 Feb 2018 13:36:47 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-121-50.rdu2.redhat.com [10.10.121.50]) by smtp.corp.redhat.com (Postfix) with ESMTP id 115A75C25B; Fri, 2 Feb 2018 13:36:44 +0000 (UTC) To: Leif Lindholm , Ard Biesheuvel Cc: "Kinney, Michael D" , edk2-devel-01 , "Ni, Ruiyu" , Paolo Bonzini , "Yao, Jiewen" , "Dong, Eric" References: <20180130153348.31992-1-lersek@redhat.com> <20180130153348.31992-2-lersek@redhat.com> <31138ce7-0637-a755-ec57-e36ab812f259@redhat.com> <17c44add-ca8e-c346-8cc8-7e94b694a7e1@redhat.com> <352efa04-a5c3-af45-2da7-8e9e0043aee9@redhat.com> <20180202132832.h37jcf3ksi2sdnxl@bivouac.eciton.net> From: Laszlo Ersek Message-ID: <954715b5-dda0-f819-32ad-0767f538f171@redhat.com> Date: Fri, 2 Feb 2018 14:36:43 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.5.2 MIME-Version: 1.0 In-Reply-To: <20180202132832.h37jcf3ksi2sdnxl@bivouac.eciton.net> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.16 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.25]); Fri, 02 Feb 2018 13:36:47 +0000 (UTC) Subject: Re: [PATCH 1/3] UefiCpuPkg/PiSmmCpuDxeSmm: update comments in IA32 SmmStartup() X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 02 Feb 2018 13:31:10 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 02/02/18 14:28, Leif Lindholm wrote: > On Fri, Feb 02, 2018 at 10:06:07AM +0000, Ard Biesheuvel wrote: >> On 31 January 2018 at 10:40, Laszlo Ersek wrote: >>> On 01/30/18 23:25, Kinney, Michael D wrote: >>>> Laszlo, >>>> >>>> I agree that the function is better than a macro. >>>> >>>> I thought of the alignment issues as well. CopyMem() >>>> is a good solution. We could also consider >>>> WriteUnalignedxx() functions in BaseLib. >>> >>> IMO, the WriteUnalignedxx functions are a bit pointless in the exact >>> form they are declared (this was discussed earlier esp. with regard to >>> aarch64). The functions take pointers to objects that already have the >>> target type, such as >>> >>> UINT32 >>> EFIAPI >>> WriteUnaligned32 ( >>> OUT UINT32 *Buffer, >>> IN UINT32 Value >>> ) >>> >>> Here the type of Buffer should be (VOID *), not (UINT32 *). Otherwise, >>> the undefined behavior (due to mis-alignment) surfaces as soon as the >>> function is called with an unaligned pointer (i.e. before the target >>> area is actually written). >>> >>>> I was originally thinking this functionality would go >>>> into BaseLib. But with the use of CopyMem(), we can't >>>> do that. >>> >>> Can we put it in BaseMemoryLib instead (which is where CopyMem() is >>> from)? That library class is still low-level enough. And, while I count >>> 9 library instances, PatchAssembly() is not a large function, we could >>> tolerate adding it to all 9 instances, identically. >>> >>> Let me also ask the opposite question: should we perhaps make the >>> PatchAssembly() API *less* abstract? (Also suggested by your naming of >>> the macro, PATCH_X86_ASM.) If the instruction encoding on e.g. AARCH64 >>> doesn't lend itself to such patching (= expressed through the address >>> right after the instruction), then even BaseMemoryLib may be too generic >>> for the API. >>> >>>> Maybe we should use WriteUnalignedxx() and >>>> add some ASSERT() checks. >>>> >>>> VOID >>>> PatchAssembly ( >>>> VOID *BufferEnd, >>>> UINT64 PatchValue, >>>> UINTN ValueSize >>>> ) >>>> { >>>> ASSERT ((UINTN)BufferEnd > ValueSize); >>>> switch (ValueSize) { >>>> case 1: >>>> ASSERT (PatchValue <= MAX_UINT8); >>>> *((UINT8 *)BufferEnd - 1) = (UINT8)PatchValue; >>>> case 2: >>>> ASSERT (PatchValue <= MAX_UINT16); >>>> WriteUnaligned16 ((UINT16 *)(BufferEnd) - 1, (UINT16)PatchValue)); >>>> break; >>>> case 4: >>>> ASSERT (PatchValue <= MAX_UINT32); >>>> WriteUnaligned32 ((UINT32 *)(BufferEnd) - 1, (UINT32)PatchValue)); >>>> break; >>>> case 8: >>>> WriteUnaligned64 ((UINT64 *)(BufferEnd) - 1, PatchValue)); >>>> break; >>>> default: >>>> ASSERT (FALSE); >>>> } >>>> } >>> >>> In my opinion: >>> >>> - If Ard and Leif say that PatchAssembly() API makes sense for AARCH64, >>> then I think we can go with the above generic implementation (for >>> BaseLib). >>> >> >> Code patching on ARM/AARCH64 has some hoops to jump through, i.e., >> clean the D-cache to the point of unification, invalidate the I-cache, >> probably some barriers in case the patching function happened to end >> up in the same cache line as the patchee > > Not just the same cache line. Prefetching can happen whenever, for > whatever reason. > >> (which may not be a concern >> for this specific use case, but it does need to be taken into account >> if this is turned into a patch-any-assembly-anywhere function) >> >> So if the PatchAssembly() prototype does end up in a generic library >> class, we'd have to provide ARM and AARCH64 specific implementations >> anyway, and given that I don't see any use for this on ARM/AARCH64 in >> the first place, I think this should belong in an IA32/X64 specific >> package. > > I also don't see a specific use for this on ARM* at the moment. But if > this is going to become more widespread, it would be useful to > introduce a higher-level layer with more portable semantics (I don't > know RISC-V, but could imagine they require similar). > However, at that point, we would probably want something > buffer-oriented rather than instruction-oriented, since we'd like to > keep the overhead down if writing more than one register's worth. I'll CC you and Ard on the BaseLib patches; hopefully PatchInstructionX86() will be possible to reimplement in terms of the more generic, buffer-oriented API, once we introduce that. Thanks! Laszlo