Re: [Patch] UefiCpuPkg PiSmmCpuDxeSmm: Remove unnecessary jmp _SmiHandler

["Gao, Liming" <liming.gao@intel.com>]

Jiewen:
  Because nasm doesn't generate the absolute address, we can change the below code, but we need to fix up its value at boot time like current _SmiHandler way. 

  Could you let me know why can't use current way? 

Thanks
Liming
>-----Original Message-----
>From: Yao, Jiewen
>Sent: Wednesday, September 12, 2018 9:04 AM
>To: Gao, Liming <liming.gao@intel.com>; Laszlo Ersek <lersek@redhat.com>;
>edk2-devel@lists.01.org
>Cc: Dong, Eric <eric.dong@intel.com>
>Subject: RE: [edk2] [Patch] UefiCpuPkg PiSmmCpuDxeSmm: Remove
>unnecessary jmp _SmiHandler
>
>The original code is below. Can we rollback to the indirect call?
>
>    mov   rax, ASM_PFX(CpuSmmDebugEntry)
>    call    rax
>
>Thank you
>Yao Jiewen
>
>> -----Original Message-----
>> From: Gao, Liming
>> Sent: Wednesday, September 12, 2018 8:59 AM
>> To: Yao, Jiewen <jiewen.yao@intel.com>; Laszlo Ersek
><lersek@redhat.com>;
>> edk2-devel@lists.01.org
>> Cc: Dong, Eric <eric.dong@intel.com>
>> Subject: RE: [edk2] [Patch] UefiCpuPkg PiSmmCpuDxeSmm: Remove
>> unnecessary jmp _SmiHandler
>>
>> Jiewen:
>>   After do more verification, I recall this change. Current code is really
>> required. Without it, OVMF SMM can't boot. So, below code can't be
>> removed.
>>   The reason is that nasm _SmiEntryPoint() function is copied to another
>> memory location and run. But, _ SmiEntryPoint() calls the external C
>function
>> CpuSmmDebugEntry(). nasm compiler generates the function call with the
>> relative address. After _SmiEntryPoint() function is copied and run in the
>new
>> address, its external function call will not work. To fix it, I add jmp instruction
>> to the original address, then process function all and works.
>>
>>     mov     rax, strict qword 0         ;   mov     rax, _SmiHandler
>> _SmiHandlerAbsAddr:
>>     jmp     rax
>>
>> ...
>>     mov     rcx, rbx
>>     call    ASM_PFX(CpuSmmDebugEntry)
>>
>> Thanks
>> Liming
>> >-----Original Message-----
>> >From: Yao, Jiewen
>> >Sent: Wednesday, September 12, 2018 6:03 AM
>> >To: Laszlo Ersek <lersek@redhat.com>; Gao, Liming
>> <liming.gao@intel.com>;
>> >edk2-devel@lists.01.org
>> >Cc: Dong, Eric <eric.dong@intel.com>
>> >Subject: RE: [edk2] [Patch] UefiCpuPkg PiSmmCpuDxeSmm: Remove
>> >unnecessary jmp _SmiHandler
>> >
>> >HI
>> >Would you please add info on what unit test has been done?
>> >
>> >Thank you
>> >Yao Jiewen
>> >
>> >
>> >> -----Original Message-----
>> >> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf
>Of
>> >> Laszlo Ersek
>> >> Sent: Tuesday, September 11, 2018 11:06 PM
>> >> To: Gao, Liming <liming.gao@intel.com>; edk2-devel@lists.01.org
>> >> Cc: Yao, Jiewen <jiewen.yao@intel.com>; Dong, Eric
>> <eric.dong@intel.com>
>> >> Subject: Re: [edk2] [Patch] UefiCpuPkg PiSmmCpuDxeSmm: Remove
>> >> unnecessary jmp _SmiHandler
>> >>
>> >> On 09/10/18 10:20, Liming Gao wrote:
>> >> > This change is wrong introduced by
>> >> e21e355e2ca7fefb15b4df7078f995d3fb9c2b89
>> >> > It is not required. So, revert it.
>> >> >
>> >> > Contributed-under: TianoCore Contribution Agreement 1.1
>> >> > Signed-off-by: Liming Gao <liming.gao@intel.com>
>> >> > Cc: Jiewen Yao <jiewen.yao@intel.com>
>> >> > ---
>> >> >  UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.nasm | 9 ++-------
>> >> >  1 file changed, 2 insertions(+), 7 deletions(-)
>> >> >
>> >> > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.nasm
>> >> b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.nasm
>> >> > index 315d0f8..7b1b3ca 100644
>> >> > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.nasm
>> >> > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.nasm
>> >> > @@ -173,9 +173,8 @@ SmiHandlerIdtrAbsAddr:
>> >> >      mov     gs, eax
>> >> >      mov     ax, [rbx + DSC_SS]
>> >> >      mov     ss, eax
>> >> > -    mov     rax, strict qword 0         ;   mov     rax,
>> >> _SmiHandler
>> >> > -_SmiHandlerAbsAddr:
>> >> > -    jmp     rax
>> >> > +
>> >> > +;   jmp     _SmiHandler                 ; instruction is not
>> needed
>> >> >
>> >> >  _SmiHandler:
>> >> >      mov     rbx, [rsp + 0x8]             ; rcx <- CpuIndex
>> >> > @@ -229,8 +228,4 @@ ASM_PFX(PiSmmCpuSmiEntryFixupAddress):
>> >> >      lea    rax, [ASM_PFX(gSmiHandlerIdtr)]
>> >> >      lea    rcx, [SmiHandlerIdtrAbsAddr]
>> >> >      mov    qword [rcx - 8], rax
>> >> > -
>> >> > -    lea    rax, [_SmiHandler]
>> >> > -    lea    rcx, [_SmiHandlerAbsAddr]
>> >> > -    mov    qword [rcx - 8], rax
>> >> >      ret
>> >> >
>> >>
>> >> Please remember to CC package maintainers / reviewers directly.
>> >>
>> >> The patch makes sense to me, and indeed it restores the original code.
>> >>
>> >> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
>> >>
>> >> Can you perhaps add another sentence to the commit message, before
>> you
>> >> push the patch, such as "the original code already uses RIP-relative
>> >> addressing"?
>> >>
>> >> Thanks
>> >> Laszlo
>> >> _______________________________________________
>> >> edk2-devel mailing list
>> >> edk2-devel@lists.01.org
>> >> https://lists.01.org/mailman/listinfo/edk2-devel