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

[Laszlo Ersek <lersek@redhat.com>]

On 09/12/18 07:13, Liming Gao wrote:
> 1. Remove jmp _SmiHandler, and run the code at the same position.
> 2. Fix up the function call address as the absolute address.
> Verify OVMF SMM boot to shell with VS2017, GCC5 and XCODE5 tool chain.
> 
> 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 | 34 +++++++++++++++++++++--------
>  1 file changed, 25 insertions(+), 9 deletions(-)
> 
> diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.nasm b/UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.nasm
> index 315d0f8..d8259de 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
> @@ -230,7 +234,19 @@ ASM_PFX(PiSmmCpuSmiEntryFixupAddress):
>      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
> 

(a) The patch seems to do two things (listed as (1) and (2) in the
commit message). They should be implemented in separate patches, in a
series.


(b) Regarding part (1): why did "jmp _SmiHandler" exist in the first place?

That jump instruction was originally added in commit 9a36d4dc3f5e:

    "UefiCpuPkg PiSmmCpuDxeSmm: Convert X64/SmiEntry.asm to NASM",
     2016-06-28

with the comment

    "instruction is not needed".

If it was not needed, then why had we added it?

For that, let's look at "UefiCpuPkg/PiSmmCpuDxeSmm/X64/SmiEntry.asm"
instead (that is, the pre-NASM file from which the NASM version was
created). In that file, the same comment was introduced as part of
commit 427e3573426f:

    "UefiCpuPkg: Add PiSmmCpuDxeSmm module X64 files", 2015-10-19

That means that we've had this unnecessary jump ever since the inception
of PiSmmCpuDxeSmm (in the open source edk2 tree). And now we are
removing the jump, for code simplification.

That's a good thing. But it should be a separate patch, with the
argument clearly spelled out:

    Remove the _SmiHandler label and all the instructions that
    facilitate the jump to it, because the jump to _SmiHandler has
    always been unnecessary, as stated by the comment in original commit
    427e3573426f as well.

Note: the _SmiHandler label itself should likely be removed too, not
just _SmiHandlerAbsAddr. It is never referenced, after the change.


(c) I must say that the description of part (2) is extremely lacking.
I've had to spend more than two hours to reconstruct the argument.
Normally the commit message should describe

- what the expected behavior is, for what use case,
- what the current (problematic) behavior is, on the master branch,
- how the change addresses the issue,
- if there is a BZ for the issue, it should be referenced.

So here's my understanding of part (2) -- which should be a separate
patch, again --:

* Before commit e21e355e2ca7f, we had

    mov     rax, ASM_PFX(CpuSmmDebugEntry)
    call    rax

This was not good, because XCODE can only handle RIP-relative addressing
in assembly code. So in bug 849 / commit e21e355e2ca7f, we changed it to:

    call    ASM_PFX(CpuSmmDebugEntry)

* While this was good for XCODE (I guess?), it wasn't good for runtime
behavior. Because: this assembly code is used as a template, it is
copied to a different place, and jumps / calls / data references made
from that other place will not work with the same relative distance. For
runtime, we need the jumps / calls / data references to be absolute,
despite XCODE not supporting such.

* Therefore we split the solution in two. In the
PiSmmCpuSmiEntryFixupAddress routine, we use the LEA instruction (with
RIP-relative addressing) to calculate the absolute addresses that are
needed. XCODE copes with these LEA instructions just fine. Then we patch
the absolute addresses into the template instructions, so that the code,
when copied to another place, will continue to work correctly -- the
absolute addresses are not affected by the copying.


If this is correct, then please document all of it in the commit
message. (Or, please open a BZ, and document all of it there, and
reference the BZ in the commit message.) I really shouldn't have to
spend hours to reconstruct the thought process, from bits and pieces in
the commit log and in various mailing list threads.


(d) Now here's my final question. If the current (RIP-relative) calls
are incorrect, and this patch is needed to fix them, then:

- Why am I not experiencing any issues on my side?

- Why weren't the issues in question found when you were working on
  bug 849 / commit e21e355e2ca7f in the first place? What has changed or
  exposed the remaining problem?


(e) The current standard of commit documentation is unacceptable for a
mature open source & open development project. Just writing good code is
entirely insufficient. Patches are not written and posted for specific
employers, or for specific colleagues at the same office, but for the
global TianoCore/edk2 community.

We've discussed this several times now at the stewards' meeting, and
it's high time we started enforcing it.

Nacked-by: Laszlo Ersek <lersek@redhat.com>

I think next time I might nack a patch without looking at the code even,
if the commit message (and the BZ referenced by it, if any) are
unsatisfactory.

I do highly appreciate that you tested the change with OVMF.

Thanks,
Laszlo