On 01/31/18 06:44, Ni, Ruiyu wrote: > Mike, Laszlo, > Does the patch only apply to the operand? > If so, PatchAssembly looks too general. > How about the name like PatchAssemblyOperand?
Originally I meant to call the function "PatchInstruction", but then I thought it could be used for patching any other artifacts too, in object code that comes from NASM. I'm also fine calling it PatchAssemblyOperand, or even PatchInstructionTrailingOperand :) Thanks Laszlo > On 1/31/2018 6:25 AM, 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. >> >> I was originally thinking this functionality would go >> into BaseLib. But with the use of CopyMem(), we can't >> do that. 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); >> } >> } >> >> Mike >> >>> -----Original Message----- >>> From: Laszlo Ersek [mailto:ler...@redhat.com] >>> Sent: Tuesday, January 30, 2018 1:45 PM >>> 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 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