Re: [PATCH v2] UefiCpuPkg PiSmmCpuDxeSmm: Update SmiEntry function run the same position

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

Laszlo:
  Thanks for your comments. I have corrected the patch and pushed it into edk2. 

Thanks
Liming
>-----Original Message-----
>From: Laszlo Ersek [mailto:lersek@redhat.com]
>Sent: Friday, September 21, 2018 6:49 PM
>To: Gao, Liming <liming.gao@intel.com>; edk2-devel@lists.01.org
>Cc: Dong, Eric <eric.dong@intel.com>; Yao, Jiewen <jiewen.yao@intel.com>
>Subject: Re: [PATCH v2] UefiCpuPkg PiSmmCpuDxeSmm: Update SmiEntry
>function run the same position
>
>Hi Liming,
>
>On 09/21/18 02:56, Liming Gao wrote:
>> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=1191
>>
>> Before commit e21e355e2ca7fefb15b4df7078f995d3fb9c2b89, jmp
>_SmiHandler
>> is commented. And below code, ASM_PFX(CpuSmmDebugEntry) is moved
>into rax,
>> then call it. But, this code doesn't work in XCODE5 tool chain. Because
>XCODE5
>> doesn't generated the absolute address in the EFI image. So, rax stores the
>> relative address. Once this logic is moved to another place, it will not work.
>> ;   jmp     _SmiHandler                 ; instruction is not needed
>> ...
>>     mov     rax, ASM_PFX(CpuSmmDebugEntry)
>>     call    rax
>>
>> Commit e21e355e2ca7fefb15b4df7078f995d3fb9c2b89 is to support XCODE5.
>> One tricky way is selected to fix it. Although SmiEntry logic is copied to
>> another place and run, but here jmp _SmiHandler is enabled to jmp the
>original
>> code place, then call ASM_PFX(CpuSmmDebugEntry) with the relative
>address.
>>     mov     rax, strict qword 0         ;   mov     rax, _SmiHandler
>> _SmiHandlerAbsAddr:
>>     jmp     rax
>> ...
>>     call    ASM_PFX(CpuSmmDebugEntry)
>>
>> Now, BZ 1191 raises the issue that SmiHandler should run in the copied
>address,
>> can't run in the common address. So, jmp _SmiHandler is required to be
>removed,
>> the code is kept to run in copied address. And, the relative address is
>> requried to be fixed up to the absolute address. The necessary changes
>should
>> not affect the behavior of platforms that already consume
>PiSmmCpuDxeSmm.
>> OVMF SMM boot to shell with VS2017, GCC5 and XCODE5 tool chain has
>been verified.
>> ...
>>     mov     rax, strict qword 0         ;   call    ASM_PFX(CpuSmmDebugEntry)
>> CpuSmmDebugEntryAbsAddr:
>>     call    rax
>>
>> Contributed-under: TianoCore Contribution Agreement 1.1
>> Signed-off-by: Liming Gao <liming.gao@intel.com>
>> Cc: Laszlo Ersek <lersek@redhat.com>
>> Cc: Eric Dong <eric.dong@intel.com>
>> Cc: Jiewen Yao <jiewen.yao@intel.com>
>> ---
>>  UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.nasm | 42
>++++++++++++++++++++++-------
>>  1 file changed, 33 insertions(+), 9 deletions(-)
>>
>> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.nasm
>b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.nasm
>> index 315d0f8..815f95b 100644
>> --- a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.nasm
>> +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.nasm
>> @@ -173,9 +173,6 @@ SmiHandlerIdtrAbsAddr:
>>      mov     gs, eax
>>      mov     ax, [rbx + DSC_SS]
>>      mov     ss, eax
>> -    mov     rax, strict qword 0         ;   mov     rax, _SmiHandler
>> -_SmiHandlerAbsAddr:
>> -    jmp     rax
>>
>>  _SmiHandler:
>>      mov     rbx, [rsp + 0x8]             ; rcx <- CpuIndex
>> @@ -189,13 +186,19 @@ _SmiHandler:
>>      add     rsp, -0x20
>>
>>      mov     rcx, rbx
>> -    call    ASM_PFX(CpuSmmDebugEntry)
>> +    mov     rax, strict qword 0         ;   call    ASM_PFX(CpuSmmDebugEntry)
>> +CpuSmmDebugEntryAbsAddr:
>> +    call    rax
>>
>>      mov     rcx, rbx
>> -    call    ASM_PFX(SmiRendezvous)
>> +    mov     rax, strict qword 0         ;   call    ASM_PFX(SmiRendezvous)
>> +SmiRendezvousAbsAddr:
>> +    call    rax
>>
>>      mov     rcx, rbx
>> -    call    ASM_PFX(CpuSmmDebugExit)
>> +    mov     rax, strict qword 0         ;   call    ASM_PFX(CpuSmmDebugExit)
>> +CpuSmmDebugExitAbsAddr:
>> +    call    rax
>>
>>      add     rsp, 0x20
>>
>> @@ -206,7 +209,8 @@ _SmiHandler:
>>
>>      add     rsp, 0x200
>>
>> -    lea     rax, [ASM_PFX(mXdSupported)]
>> +    mov     rax, strict qword 0         ;       lea     rax, [ASM_PFX(mXdSupported)]
>> +mXdSupportedAbsAddr:
>>      mov     al, [rax]
>>      cmp     al, 0
>>      jz      .1
>> @@ -224,13 +228,33 @@ _SmiHandler:
>>
>>  ASM_PFX(gcSmiHandlerSize)    DW      $ - _SmiEntryPoint
>>
>> +;
>> +; Retrieve the address and fill it into mov opcode.
>> +;
>> +; It is called in the driver entry point first.
>> +; It is used to fix up the real address in mov opcode.
>> +; Then, after the code logic is copied to the different location,
>
>The "git am" command complained that the line above added a whitespace
>error. Can you please strip the trailing space character when you push
>the patch?
>
>Reviewed-by: Laszlo Ersek <lersek@redhat.com>
>Tested-by: Laszlo Ersek <lersek@redhat.com>
>
>Thanks,
>Laszlo
>
>
>> +; the code can also run.
>> +;
>>  global ASM_PFX(PiSmmCpuSmiEntryFixupAddress)
>>  ASM_PFX(PiSmmCpuSmiEntryFixupAddress):
>>      lea    rax, [ASM_PFX(gSmiHandlerIdtr)]
>>      lea    rcx, [SmiHandlerIdtrAbsAddr]
>>      mov    qword [rcx - 8], rax
>>
>> -    lea    rax, [_SmiHandler]
>> -    lea    rcx, [_SmiHandlerAbsAddr]
>> +    lea    rax, [ASM_PFX(CpuSmmDebugEntry)]
>> +    lea    rcx, [CpuSmmDebugEntryAbsAddr]
>> +    mov    qword [rcx - 8], rax
>> +
>> +    lea    rax, [ASM_PFX(SmiRendezvous)]
>> +    lea    rcx, [SmiRendezvousAbsAddr]
>> +    mov    qword [rcx - 8], rax
>> +
>> +    lea    rax, [ASM_PFX(CpuSmmDebugExit)]
>> +    lea    rcx, [CpuSmmDebugExitAbsAddr]
>> +    mov    qword [rcx - 8], rax
>> +
>> +    lea    rax, [ASM_PFX(mXdSupported)]
>> +    lea    rcx, [mXdSupportedAbsAddr]
>>      mov    qword [rcx - 8], rax
>>      ret
>>