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 23873223CCF17 for ; Fri, 2 Feb 2018 05:20:54 -0800 (PST) Received: from smtp.corp.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 0F6003E502; Fri, 2 Feb 2018 13:26:32 +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 D1A6987B1; Fri, 2 Feb 2018 13:26:29 +0000 (UTC) To: Ard Biesheuvel Cc: "Kinney, Michael D" , edk2-devel-01 , "Ni, Ruiyu" , Paolo Bonzini , "Yao, Jiewen" , "Dong, Eric" , "Leif Lindholm (Linaro address)" 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> From: Laszlo Ersek Message-ID: Date: Fri, 2 Feb 2018 14:26:28 +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.11 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.26]); Fri, 02 Feb 2018 13:26:32 +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:20:55 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 02/02/18 11:06, 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 (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. Thank you for the response! For now I'm going to post the series with the function introduced as PatchInstructionX86() to BaseLib, visibly only to IA32 and X64 edk2 platforms. If a better place than BaseLib looks necessary, I can move it according to review comments. Thanks! Laszlo > >> - If Ard and Leif say the API is only useful on x86, then I suggest that >> we implement the API separately for all arches (still in BaseLib): >> >> - On x86, we should simply open-code the unaligned accesses (like you >> originall suggested). The pointer arithmetic will look a bit wild, >> but it's safely hidden behind a BaseLib API, so client code will >> look nice. >> >> - On all other arches, we should implement the function with >> ASSERT(FALSE). >> >> Thanks! >> Laszlo >> >>> >>> Mike >>> >>>> -----Original Message----- >>>> From: Laszlo Ersek [mailto:lersek@redhat.com] >>>> Sent: Tuesday, January 30, 2018 1:45 PM >>>> To: Kinney, Michael D ; edk2- >>>> devel-01 >>>> Cc: Ni, Ruiyu ; Paolo Bonzini >>>> ; Yao, Jiewen >>>> ; Dong, Eric >>>> Subject: Re: [edk2] [PATCH 1/3] >>>> UefiCpuPkg/PiSmmCpuDxeSmm: update comments in IA32 >>>> SmmStartup() >>>> >>>> On 01/30/18 21:31, Kinney, Michael D wrote: >>>>> Laszlo, >>>>> >>>>> We have already used this technique in other NASM files >>>>> to remove DBs. >>>> >>>> OK. >>>> >>>>> Let us know if you have suggestions on how to make the >>>>> C code that performs the patches easier to read and >>>>> maintain. >>>> >>>> How about this: >>>> >>>> VOID >>>> PatchAssembly ( >>>> VOID *BufferEnd, >>>> UINT64 PatchValue, >>>> UINTN ValueSize >>>> ) >>>> { >>>> CopyMem ( >>>> (VOID *)((UINTN)BufferEnd - ValueSize), >>>> &PatchValue, >>>> ValueSize >>>> ); >>>> } >>>> >>>> extern UINT8 gAsmSmmCr0; >>>> extern UINT8 gAsmSmmCr3; >>>> extern UINT8 gAsmSmmCr4; >>>> >>>> ... >>>> { >>>> PatchAssembly (&gAsmSmmCr0, AsmReadCr0 (), 4); >>>> PatchAssembly (&gAsmSmmCr3, AsmReadCr3 (), 4); >>>> PatchAssembly (&gAsmSmmCr4, AsmReadCr4 (), 4); >>>> ... >>>> } >>>> >>>> (I think it's fine to open-code the last argument as "4", >>>> rather than >>>> "sizeof (UINT32)", because for patching, we must have >>>> intimate knowledge >>>> of the instruction anyway.) >>>> >>>> To me, this is easier to read, because: >>>> >>>> - there are no complex casts in the "business logic" >>>> - the size is spelled out once per patching >>>> - the function name and the variable names make it clear >>>> we are patching >>>> separately compiled assembly code that was linked into >>>> the same >>>> module. >>>> >>>> What do you think? >>>> >>>> Thanks! >>>> Laszlo >>>> >>>>>> -----Original Message----- >>>>>> From: edk2-devel [mailto:edk2-devel- >>>> bounces@lists.01.org] >>>>>> On Behalf Of Laszlo Ersek >>>>>> Sent: Tuesday, January 30, 2018 10:17 AM >>>>>> To: Kinney, Michael D ; >>>> edk2- >>>>>> devel-01 >>>>>> Cc: Ni, Ruiyu ; Paolo Bonzini >>>>>> ; Yao, Jiewen >>>>>> ; Dong, Eric >>>> >>>>>> Subject: Re: [edk2] [PATCH 1/3] >>>>>> UefiCpuPkg/PiSmmCpuDxeSmm: update comments in IA32 >>>>>> SmmStartup() >>>>>> >>>>>> On 01/30/18 18:22, Kinney, Michael D wrote: >>>>>>> Laszlo, >>>>>>> >>>>>>> The DBs can be removed if the label is moved after >>>>>>> the instruction and the patch is done to the label >>>>>>> minus the size of the patch value. >>>>>> >>>>>> Indeed I haven't thought of this. >>>>>> >>>>>> If I understand correctly, it means >>>>>> >>>>>> extern UINT8 gSmmCr0; >>>>>> >>>>>> *(UINT32*)(&gSmmCr0 - sizeof (UINT32)) = >>>>>> (UINT32)AsmReadCr0 (); >>>>>> >>>>>> TBH, the DB feels less ugly to me than this :) >>>>>> >>>>>> Still, if you think it would be an acceptable price to >>>>>> pay for removing >>>>>> the remaining DBs, I can respin. >>>>>> >>>>>> Thanks >>>>>> Laszlo >>>>>> >>>>>>>> -----Original Message----- >>>>>>>> From: edk2-devel [mailto:edk2-devel- >>>>>> bounces@lists.01.org] >>>>>>>> On Behalf Of Laszlo Ersek >>>>>>>> Sent: Tuesday, January 30, 2018 7:34 AM >>>>>>>> To: edk2-devel-01 >>>>>>>> Cc: Ni, Ruiyu ; Yao, Jiewen >>>>>>>> ; Dong, Eric >>>>>> ; >>>>>>>> Paolo Bonzini >>>>>>>> Subject: [edk2] [PATCH 1/3] >>>> UefiCpuPkg/PiSmmCpuDxeSmm: >>>>>>>> update comments in IA32 SmmStartup() >>>>>>>> >>>>>>>> The gSmmCr3, gSmmCr4, gSmmCr0 and gSmmJmpAddr global >>>>>>>> variables are used >>>>>>>> for patching assembly instructions, thus we can >>>> never >>>>>>>> remove the DB >>>>>>>> encodings for those instructions. At least we should >>>>>> add >>>>>>>> the intended >>>>>>>> meanings in comments. >>>>>>>> >>>>>>>> This patch only changes comments. >>>>>>>> >>>>>>>> Cc: Eric Dong >>>>>>>> Cc: Jian J Wang >>>>>>>> Cc: Jiewen Yao >>>>>>>> Cc: Paolo Bonzini >>>>>>>> Cc: Ruiyu Ni >>>>>>>> Contributed-under: TianoCore Contribution Agreement >>>>>> 1.1 >>>>>>>> Signed-off-by: Laszlo Ersek >>>>>>>> --- >>>>>>>> UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm | 8 >>>> ++++- >>>>>> --- >>>>>>>> 1 file changed, 4 insertions(+), 4 deletions(-) >>>>>>>> >>>>>>>> diff --git >>>>>> a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm >>>>>>>> b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm >>>>>>>> index e96dd8d2392a..08534dba64b7 100644 >>>>>>>> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm >>>>>>>> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/Ia32/SmmInit.nasm >>>>>>>> @@ -44,34 +44,34 @@ global ASM_PFX(SmmStartup) >>>>>>>> ASM_PFX(SmmStartup): >>>>>>>> DB 0x66 >>>>>>>> mov eax, 0x80000001 ; read >>>>>>>> capability >>>>>>>> cpuid >>>>>>>> DB 0x66 >>>>>>>> mov ebx, edx ; rdmsr >>>> will >>>>>>>> change edx. keep it in ebx. >>>>>>>> - DB 0x66, 0xb8 >>>>>>>> + DB 0x66, 0xb8 ; mov eax, >>>>>> imm32 >>>>>>>> ASM_PFX(gSmmCr3): DD 0 >>>>>>>> mov cr3, eax >>>>>>>> DB 0x67, 0x66 >>>>>>>> lgdt [cs:ebp + (ASM_PFX(gcSmiInitGdtr) - >>>>>>>> ASM_PFX(SmmStartup))] >>>>>>>> - DB 0x66, 0xb8 >>>>>>>> + DB 0x66, 0xb8 ; mov eax, >>>>>> imm32 >>>>>>>> ASM_PFX(gSmmCr4): DD 0 >>>>>>>> mov cr4, eax >>>>>>>> DB 0x66 >>>>>>>> mov ecx, 0xc0000080 ; IA32_EFER >>>>>> MSR >>>>>>>> rdmsr >>>>>>>> DB 0x66 >>>>>>>> test ebx, BIT20 ; check NXE >>>>>>>> capability >>>>>>>> jz .1 >>>>>>>> or ah, BIT3 ; set NXE >>>> bit >>>>>>>> wrmsr >>>>>>>> .1: >>>>>>>> - DB 0x66, 0xb8 >>>>>>>> + DB 0x66, 0xb8 ; mov eax, >>>>>> imm32 >>>>>>>> ASM_PFX(gSmmCr0): DD 0 >>>>>>>> DB 0xbf, PROTECT_MODE_DS, 0 ; mov di, >>>>>>>> PROTECT_MODE_DS >>>>>>>> mov cr0, eax >>>>>>>> - DB 0x66, 0xea ; jmp far >>>>>>>> [ptr48] >>>>>>>> + DB 0x66, 0xea ; jmp far >>>>>>>> [ptr48] >>>>>>>> ASM_PFX(gSmmJmpAddr): >>>>>>>> DD @32bit >>>>>>>> DW PROTECT_MODE_CS >>>>>>>> @32bit: >>>>>>>> mov ds, edi >>>>>>>> mov es, edi >>>>>>>> -- >>>>>>>> 2.14.1.3.gb7cf6e02401b >>>>>>>> >>>>>>>> >>>>>>>> _______________________________________________ >>>>>>>> edk2-devel mailing list >>>>>>>> edk2-devel@lists.01.org >>>>>>>> https://lists.01.org/mailman/listinfo/edk2-devel >>>>>> >>>>>> _______________________________________________ >>>>>> edk2-devel mailing list >>>>>> edk2-devel@lists.01.org >>>>>> https://lists.01.org/mailman/listinfo/edk2-devel >>> >>> _______________________________________________ >>> edk2-devel mailing list >>> edk2-devel@lists.01.org >>> https://lists.01.org/mailman/listinfo/edk2-devel >>> >>