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 D97BC22364883 for ; Wed, 31 Jan 2018 02:34:36 -0800 (PST) Received: from smtp.corp.redhat.com (int-mx02.intmail.prod.int.phx2.redhat.com [10.5.11.12]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 51AA93AA1D; Wed, 31 Jan 2018 10:40:12 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-185.rdu2.redhat.com [10.10.120.185]) by smtp.corp.redhat.com (Postfix) with ESMTP id D890660BE7; Wed, 31 Jan 2018 10:40:07 +0000 (UTC) To: "Kinney, Michael D" , edk2-devel-01 Cc: "Ni, Ruiyu" , Paolo Bonzini , "Yao, Jiewen" , "Dong, Eric" , Ard Biesheuvel , "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> From: Laszlo Ersek Message-ID: <352efa04-a5c3-af45-2da7-8e9e0043aee9@redhat.com> Date: Wed, 31 Jan 2018 11:40:06 +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.12 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.38]); Wed, 31 Jan 2018 10:40:12 +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: Wed, 31 Jan 2018 10:34:37 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit 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). - 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 >