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-boun...@lists.01.org] >> On Behalf Of Laszlo Ersek >> Sent: Tuesday, January 30, 2018 10:17 AM >> To: Kinney, Michael D <michael.d.kin...@intel.com>; edk2- >> devel-01 <edk2-devel@lists.01.org> >> Cc: Ni, Ruiyu <ruiyu...@intel.com>; Paolo Bonzini >> <pbonz...@redhat.com>; Yao, Jiewen >> <jiewen....@intel.com>; Dong, Eric <eric.d...@intel.com> >> 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- >> boun...@lists.01.org] >>>> On Behalf Of Laszlo Ersek >>>> Sent: Tuesday, January 30, 2018 7:34 AM >>>> To: edk2-devel-01 <edk2-devel@lists.01.org> >>>> Cc: Ni, Ruiyu <ruiyu...@intel.com>; Yao, Jiewen >>>> <jiewen....@intel.com>; Dong, Eric >> <eric.d...@intel.com>; >>>> Paolo Bonzini <pbonz...@redhat.com> >>>> 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 <eric.d...@intel.com> >>>> Cc: Jian J Wang <jian.j.w...@intel.com> >>>> Cc: Jiewen Yao <jiewen....@intel.com> >>>> Cc: Paolo Bonzini <pbonz...@redhat.com> >>>> Cc: Ruiyu Ni <ruiyu...@intel.com> >>>> Contributed-under: TianoCore Contribution Agreement >> 1.1 >>>> Signed-off-by: Laszlo Ersek <ler...@redhat.com> >>>> --- >>>> 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