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 7396722364882 for ; Tue, 30 Jan 2018 13:39:40 -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 40464C0587DC; Tue, 30 Jan 2018 21:45:15 +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 B2610600D3; Tue, 30 Jan 2018 21:45:13 +0000 (UTC) To: "Kinney, Michael D" , edk2-devel-01 Cc: "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> From: Laszlo Ersek Message-ID: <17c44add-ca8e-c346-8cc8-7e94b694a7e1@redhat.com> Date: Tue, 30 Jan 2018 22:45:12 +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.32]); Tue, 30 Jan 2018 21:45:15 +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: Tue, 30 Jan 2018 21:39:41 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit 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