From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=2607:f8b0:4001:c06::229; helo=mail-io0-x229.google.com; envelope-from=ard.biesheuvel@linaro.org; receiver=edk2-devel@lists.01.org Received: from mail-io0-x229.google.com (mail-io0-x229.google.com [IPv6:2607:f8b0:4001:c06::229]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 51094223CCEF2 for ; Fri, 2 Feb 2018 02:00:31 -0800 (PST) Received: by mail-io0-x229.google.com with SMTP id f4so22415014ioh.8 for ; Fri, 02 Feb 2018 02:06:09 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=oUqBTybEMtnf+ynzsYWjcxsbolf0sU+Fui7tKpnGtC8=; b=GpynEAGudmtzPJOkVOhUFh3i1OrrTum6Y+4xwfC9MMsSkPPGDwfHEFAK2ZDg+PjKht hojzpUfoGzeJuzG5mi9/H6f5+W0gXsa4PDW02IXROfngWxheiBa+oVaKBusTvTMsYnc2 ybJ1rAnsKLD6TEhS50PrFnFf1vYTJYZqMUw6s= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=oUqBTybEMtnf+ynzsYWjcxsbolf0sU+Fui7tKpnGtC8=; b=s1pMxrvxC6eiBYWYwINDivGdne7uoCrPG1v88+exRYQNBwbFvmvGiFHrdwWJLQAovz oonWE0aKyywrMwO/uergAlQiQJ55Fe+At9/Ivu0UdQve4jLs+V1EH687anfV9tLQukiC /Y9x3dyvUB53ajUNVlQ3F8QE8DZ+DuD+Ja6WoKoaIpWJ+CHjE6E9kVq3uhOJzi341fK0 nzh1AD/9pLYdUmd9pdJcFwiVqiiaPx5ttGUW7aC9YTDIH4p0nYbUZUJW4rhisgpXrFkM AM4Yp2Kj/w6Giw3JrE+hGs1IZzeqPcwdjbGkaar4cFz1O8MVX6rEOAB7bg9gWuebiezN vEtw== X-Gm-Message-State: AKwxytdzNOPIhjOw+AINYQuhX/ezxYQUUb3/n+SwNC0w8adkybNBOjZf tvK+0v8eG/5FG4Aguk59OJ4VRVZCNJSJT6/+wNPHFA== X-Google-Smtp-Source: AH8x225+YjdhljIj8qIy7E3do880jKM8YZwvd7q0AH7xQngu8LcV+5uunfb33W1BTi4f0M69y/jmVkMZR/IgLowZt74= X-Received: by 10.107.20.194 with SMTP id 185mr40710684iou.127.1517565968539; Fri, 02 Feb 2018 02:06:08 -0800 (PST) MIME-Version: 1.0 Received: by 10.107.112.13 with HTTP; Fri, 2 Feb 2018 02:06:07 -0800 (PST) In-Reply-To: <352efa04-a5c3-af45-2da7-8e9e0043aee9@redhat.com> 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: Ard Biesheuvel Date: Fri, 2 Feb 2018 10:06:07 +0000 Message-ID: To: Laszlo Ersek Cc: "Kinney, Michael D" , edk2-devel-01 , "Ni, Ruiyu" , Paolo Bonzini , "Yao, Jiewen" , "Dong, Eric" , "Leif Lindholm (Linaro address)" 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 10:00:31 -0000 Content-Type: text/plain; charset="UTF-8" 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. > - 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 >> >