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

[Laszlo Ersek <lersek@redhat.com>]

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
>