From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=192.55.52.88; helo=mga01.intel.com; envelope-from=ruiyu.ni@intel.com; receiver=edk2-devel@lists.01.org Received: from mga01.intel.com (mga01.intel.com [192.55.52.88]) (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 889E12222C23C for ; Tue, 30 Jan 2018 21:48:31 -0800 (PST) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga007.jf.intel.com ([10.7.209.58]) by fmsmga101.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 30 Jan 2018 21:54:06 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.46,438,1511856000"; d="scan'208";a="13889456" Received: from ray-dev.ccr.corp.intel.com (HELO [10.239.9.19]) ([10.239.9.19]) by orsmga007.jf.intel.com with ESMTP; 30 Jan 2018 21:54:05 -0800 From: "Ni, Ruiyu" To: "Kinney, Michael D" , Laszlo Ersek , edk2-devel-01 Cc: 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> <17c44add-ca8e-c346-8cc8-7e94b694a7e1@redhat.com> <3932fc60-2a38-261f-183c-fd686643d684@Intel.com> Message-ID: Date: Wed, 31 Jan 2018 13:54:04 +0800 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.5.2 MIME-Version: 1.0 In-Reply-To: <3932fc60-2a38-261f-183c-fd686643d684@Intel.com> 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 05:48:32 -0000 Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 8bit On 1/31/2018 1:44 PM, 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? > > > 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: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 >> > > Laszlo, Mike, Considering this patch doesn't make the code worse, actually improved a tiny bit, can we firstly check in the three patches? Reviewed-by: Ruiyu Ni -- Thanks, Ray