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 365EC21F0DA56 for ; Mon, 5 Feb 2018 11:17:32 -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 079D8356D2; Mon, 5 Feb 2018 19:23:14 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-166.rdu2.redhat.com [10.10.120.166]) by smtp.corp.redhat.com (Postfix) with ESMTP id BF6A45C544; Mon, 5 Feb 2018 19:23:11 +0000 (UTC) To: "Kinney, Michael D" , edk2-devel-01 Cc: Ard Biesheuvel , "Dong, Eric" , "Yao, Jiewen" , Leif Lindholm , "Gao, Liming" , "Ni, Ruiyu" References: <20180202143954.7357-1-lersek@redhat.com> <7d4749bf-f25f-f40e-e991-5af05232000f@redhat.com> From: Laszlo Ersek Message-ID: Date: Mon, 5 Feb 2018 20:23:10 +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: 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.30]); Mon, 05 Feb 2018 19:23:14 +0000 (UTC) Subject: Re: [PATCH 00/14] rid PiSmmCpuDxeSmm of DB-encoded instructions 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: Mon, 05 Feb 2018 19:17:33 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 02/05/18 19:22, Kinney, Michael D wrote: > Laszlo, > > Let's see if we can close on the timeline for > the .S/.asm RFC this week. > > I am concerned about making them UINT8 from C code > because future maintainer may think that the patch > value type is UINT8. > > Labels in assembly that are defined to be a function > that is callable from C code does not have a storage > type. Why can't we make these labels the same way? To my understanding, the labels in the NASM source code for functions and variables look the same; the actual declaration only comes from the C code. (Assuming we declare a NASM label as a function in the C source, nothing in the toolchain enforces an actual match between caller and callee; it is possible to call the function (from C) through a declaration that doesn't match the actual assembly implementation. IOW it's up to us to avoid such bugs.) If I understand correctly, you are suggesting that we take a label from the NASM source that stands right after an instruction to patch, and we declare it as a function in the C source. (With what prototype though? The label does not actually introduce a function definition in the assembly code; it would make no sense to call it.) Then, for the patching, I presume your suggestion is to convert the address of the function to UINTN, perform the subtraction, etc. Something like: typedef VOID (X86_ASSEMBLY_LABEL) (VOID); (This is not a pointer-to-function type, but a function type.) A declaration using the typedef would be extern X86_ASSEMBLY_LABEL gPatchCr3; (This declares an extern function, not a pointer to a function.) The patching function would take a pointer to a function: VOID EFIAPI PatchInstructionX86 ( OUT X86_ASSEMBLY_LABEL *InstructionEnd, IN UINT64 PatchValue, IN UINTN ValueSize ); and the implementation would have to do e.g. WriteUnaligned32 ( (UINT32 *)(UINTN)InstructionEnd - 1, (UINT32)PatchValue ); It would be called like PatchInstructionX86 (&gPatchCr3, Value, 4); But, what does this buy us in comparison to just: typedef UINT8 X86_ASSEMBLY_LABEL; ? If you worry that a future maintainer misunderstands the UINT8, then we can as well hide the UINT8 behind a typedef; X86_ASSEMBLY_LABEL doesn't have to be a function type for the hiding. (Conversely, when using a function type as underlying type, I worry that a future maintainer might be tempted to call them :) ) Thanks, Laszlo >> -----Original Message----- >> From: Laszlo Ersek [mailto:lersek@redhat.com] >> Sent: Monday, February 5, 2018 2:28 AM >> To: Kinney, Michael D ; edk2- >> devel-01 >> Cc: Ard Biesheuvel ; Dong, >> Eric ; Yao, Jiewen >> ; Leif Lindholm >> ; Gao, Liming >> ; Ni, Ruiyu >> Subject: Re: [PATCH 00/14] rid PiSmmCpuDxeSmm of DB- >> encoded instructions >> >> On 02/03/18 01:45, Kinney, Michael D wrote: >>> Laszlo, >>> >>> Thanks for all the work on this series and the very >>> detailed commit messages. >>> >>> Liming's email on removing the .S and .asm files is an >>> RFC. We need to see this RFC approved before we can >>> commit changes to remove .S and .asm files. This >> should >>> be a separate activity. >> >> Sure, I can drop that patch, but then the PiSmmCpuDxeSmm >> changes in the >> other patches will divert the NASM files from the .S and >> .asm files. Is >> that (temporary) non-uniformity better than removing the >> .S and .asm files? >> >>> One odd thing I see in this series is that the >> instruction >>> patch label in the .nasm file is just a label and does >> not >>> have any storage associated with it. >> >> No, this is not correct; the storage that is associated >> with each of >> these "patch labels" is the one byte (UINT8) directly >> following the >> label -- whatever that byte might be. It is generally >> part of a totally >> unrelated instruction. >> >> In case we had to patch an immediate operand that >> happened to comprise >> the very last byte(s) of a NASM source file, *then* we'd >> have to add one >> dummy DB at the end, just so there was something that the >> label directly >> refered to. >> >> This is why UINT8 is a good type here, because it >> requires us to add the >> least amount of padding. >> >>> But in the C code >>> the type UINT8 is used with the label which implies >> some >>> storage. Can we make the globals in C code be a >> pointer >>> (maybe VOID *) instead of UINT8? >> >> I don't think so. For building the addresses, we rely on >> the linker, and >> the linker needs definitions (allocations) of objects. >> Your above >> observation is correct (i.e. that storage is required), >> my addition to >> that is that storage is *already* allocated (one UINT8 >> per patch label / >> symbol). >> >> Thanks! >> Laszlo