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

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

[Liming Gao]

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
-- 
2.10.0.windows.1

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

[Yao, Jiewen]

Thank you!

Reviewed-by: Jiewen.yao@intel.com


> -----Original Message-----
> From: Gao, Liming
> Sent: Wednesday, September 12, 2018 1:14 PM
> To: edk2-devel@lists.01.org
> Cc: Laszlo Ersek <lersek@redhat.com>; Dong, Eric <eric.dong@intel.com>;
> Yao, Jiewen <jiewen.yao@intel.com>
> Subject: [Patch] UefiCpuPkg PiSmmCpuDxeSmm: Update SmiEntry function
> run the same position
> 
> 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
> --
> 2.10.0.windows.1

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

[Laszlo Ersek]

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

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

[Gao, Liming]

Laszlo:
  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. Like you say, 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. I choose one tricky way to fix it. Although SmiEntry logic is copied to another place and run, but here I enable jmp _SmiHandler to jmp the original code location, then call ASM_PFX(CpuSmmDebugEntry) with the relative address can work.
    mov     rax, strict qword 0         ;   mov     rax, _SmiHandler
_SmiHandlerAbsAddr:
    jmp     rax
...
    call    ASM_PFX(CpuSmmDebugEntry)

  Today, Jiewen raises the issue that SmiHandler should run in the copied address, can't run in the common address. So, I update its logic and remove jmp _SmiHandler, then keep code continue run in copied address. But, I still need to fix up CpuSmmDebugEntry address to the absolute address. They are both for this issue. They can't be separated. 

...
    mov     rax, strict qword 0         ;   call    ASM_PFX(CpuSmmDebugEntry)
CpuSmmDebugEntryAbsAddr:
    call    rax

Thanks
Liming
> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Wednesday, September 12, 2018 9:24 PM
> To: Gao, Liming <liming.gao@intel.com>
> Cc: edk2-devel@lists.01.org; Yao, Jiewen <jiewen.yao@intel.com>; Dong, Eric <eric.dong@intel.com>; Kinney, Michael D
> <michael.d.kinney@intel.com>; Leif Lindholm (Linaro address) <leif.lindholm@linaro.org>; Ard Biesheuvel <ard.biesheuvel@linaro.org>;
> Andrew Fish <afish@apple.com>; Cetola, Stephano <stephano.cetola@intel.com>
> Subject: Re: [edk2] [Patch] UefiCpuPkg PiSmmCpuDxeSmm: Update SmiEntry function run the same position
> 
> 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

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

[Laszlo Ersek]

On 09/12/18 17:42, Gao, Liming wrote:
> Laszlo:
>   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. Like you say, 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. I choose one tricky way to fix it. Although SmiEntry logic is copied to another place and run, but here I enable jmp _SmiHandler to jmp the original code location, then call ASM_PFX(CpuSmmDebugEntry) with the relative address can work.
>     mov     rax, strict qword 0         ;   mov     rax, _SmiHandler
> _SmiHandlerAbsAddr:
>     jmp     rax
> ...
>     call    ASM_PFX(CpuSmmDebugEntry)
> 
>   Today, Jiewen raises the issue that SmiHandler should run in the copied address, can't run in the common address. So, I update its logic and remove jmp _SmiHandler, then keep code continue run in copied address. But, I still need to fix up CpuSmmDebugEntry address to the absolute address. They are both for this issue. They can't be separated. 
> 
> ...
>     mov     rax, strict qword 0         ;   call    ASM_PFX(CpuSmmDebugEntry)
> CpuSmmDebugEntryAbsAddr:
>     call    rax

Thank you very much for the explanation. I understand now.

I also understand why I got confused so much earlier. The reason is that
the code was not commented sufficiently. It should have been pointed out
what part of the instruction stream was meant to be executed from the
copied address space (that is, not from the PiSmmCpuDxeSmm image
itself), and what part of the instruction stream was meant to execute
from within the the PiSmmCpuDxeSmm image. Without such comments, it's
too hard to understand the transitions.

You or Jiewen should please file a TianoCore BZ describing the current
issue. Namely that, due to circumstances you are not allowed to share,
no part of the _SmiHandler routine should execute from within the
PiSmmCpuDxeSmm image; instead, all of it should execute from the
copied-to address space. This requires eliminating the jump back into
the PiSmmCpuDxeSmm image, and also patching up the relative addresses
(to absolute addresses), for those instructions that used to run from
within the PiSmmCpuDxeSmm image, but now no longer will. The necessary
changes should not affect the behavior of platforms that already consume
PiSmmCpuDxeSmm from edk2.

The historical overview you provide above is also valuable, please
include it in the commit message and/or the BZ as well.

It would still be nice to comment what parts of the source file run from
within the PiSmmCpuDxeSmm image. For example,
PiSmmCpuSmiEntryFixupAddress() does, right?

--*--

Please understand that my issue here is more serious than just missing
the explanation / motivation, for this specific patch. My issue is that
the posting of this work to edk2-devel should have *started* with the
above explanation. You and Jiewen understand the issue. Noone else on
the list (that doesn't work at your office) does. I've wasted half a day
because you didn't write up the background up-front.

I don't need to know your specific internal proprietary feature that
requires this change. I do need to know that an internal feature with
such a requirement exists, and that it is your motivation. Every ten
minutes you save for yourselves on documentation is amplified to several
hours of struggle, for each reviewer. And we specifically discussed this
scenario with Mike at one of the earlier stewards' meetings.

(The general topic back then was my raising that you [plural] liberally
commit whatever you want to MdePkg / MdeModulePkg, without really naming
the use cases, let alone adding client code to edk2. Whereas non-Intel
contributors with a demonstrated need are heavily gated from adding code
to MdePkg / MdeModulePkg.

Compare: the addition of the "PciSegmentLibSegmentInfo" instances
(commit 5c9bb86f171c), without any consumer in edk2 whatsoever, versus
BZ#957, where the new libraries -- originally proposed for MdePkg -- had
demonstrated consumers, just not from Intel. Do you see the double
standard?)

I wish I could just ignore UefiCpuPkg. I can't do that; it is very
brittle / sensitive, and has caused numerous regressions for OVMF. I
must keep a close eye on it. But patches from you [plural] certainly
don't make that easy for me.

It kills me that after years of us begging on the list, you [plural]
still don't care about non-Intel participants as first class citizens
up-front, until they raise a stink.

Here's another recent example:

https://bugzilla.tianocore.org/show_bug.cgi?id=1116#c3
https://bugzilla.tianocore.org/show_bug.cgi?id=1116#c4

Please start actually believing in, and practicing, Open Development.
You no longer write the code and the docs for Intel only. Convincing
your colleagues is easy -- that's no different from the proprietary /
closed source days of firmware development. It was Intel's decision to
turn edk2 into an open source project; please live that decision. Now
your primary audience is the contributors that depend on collaborating
with you *without* working for your employer.

Well I've probably overstated my case. It's just that I've hoped for
better commit messages since that stewards' meeting.

Laszlo

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

[Gao, Liming]

Laszlo:
  I understand your point. I agree your suggestion. BZ https://bugzilla.tianocore.org/show_bug.cgi?id=1191 has been submitted. Yes. PiSmmCpuSmiEntryFixupAddress() is called in the driver entry point to fix up the address first. 

  I will send V2 patch with the detail commit message and code comments.

> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Thursday, September 13, 2018 3:49 AM
> To: Gao, Liming <liming.gao@intel.com>
> Cc: edk2-devel@lists.01.org; Yao, Jiewen <jiewen.yao@intel.com>; Dong, Eric <eric.dong@intel.com>; Kinney, Michael D
> <michael.d.kinney@intel.com>; Leif Lindholm (Linaro address) <leif.lindholm@linaro.org>; Ard Biesheuvel <ard.biesheuvel@linaro.org>;
> Andrew Fish <afish@apple.com>; Cetola, Stephano <stephano.cetola@intel.com>
> Subject: Re: [edk2] [Patch] UefiCpuPkg PiSmmCpuDxeSmm: Update SmiEntry function run the same position
> 
> On 09/12/18 17:42, Gao, Liming wrote:
> > Laszlo:
> >   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. Like you say, 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. I choose one tricky way to fix it. Although SmiEntry
> logic is copied to another place and run, but here I enable jmp _SmiHandler to jmp the original code location, then call
> ASM_PFX(CpuSmmDebugEntry) with the relative address can work.
> >     mov     rax, strict qword 0         ;   mov     rax, _SmiHandler
> > _SmiHandlerAbsAddr:
> >     jmp     rax
> > ...
> >     call    ASM_PFX(CpuSmmDebugEntry)
> >
> >   Today, Jiewen raises the issue that SmiHandler should run in the copied address, can't run in the common address. So, I update its
> logic and remove jmp _SmiHandler, then keep code continue run in copied address. But, I still need to fix up CpuSmmDebugEntry
> address to the absolute address. They are both for this issue. They can't be separated.
> >
> > ...
> >     mov     rax, strict qword 0         ;   call    ASM_PFX(CpuSmmDebugEntry)
> > CpuSmmDebugEntryAbsAddr:
> >     call    rax
> 
> Thank you very much for the explanation. I understand now.
> 
> I also understand why I got confused so much earlier. The reason is that
> the code was not commented sufficiently. It should have been pointed out
> what part of the instruction stream was meant to be executed from the
> copied address space (that is, not from the PiSmmCpuDxeSmm image
> itself), and what part of the instruction stream was meant to execute
> from within the the PiSmmCpuDxeSmm image. Without such comments, it's
> too hard to understand the transitions.
> 
> You or Jiewen should please file a TianoCore BZ describing the current
> issue. Namely that, due to circumstances you are not allowed to share,
> no part of the _SmiHandler routine should execute from within the
> PiSmmCpuDxeSmm image; instead, all of it should execute from the
> copied-to address space. This requires eliminating the jump back into
> the PiSmmCpuDxeSmm image, and also patching up the relative addresses
> (to absolute addresses), for those instructions that used to run from
> within the PiSmmCpuDxeSmm image, but now no longer will. The necessary
> changes should not affect the behavior of platforms that already consume
> PiSmmCpuDxeSmm from edk2.
> 
> The historical overview you provide above is also valuable, please
> include it in the commit message and/or the BZ as well.
> 
> It would still be nice to comment what parts of the source file run from
> within the PiSmmCpuDxeSmm image. For example,
> PiSmmCpuSmiEntryFixupAddress() does, right?
> 
> --*--
> 
> Please understand that my issue here is more serious than just missing
> the explanation / motivation, for this specific patch. My issue is that
> the posting of this work to edk2-devel should have *started* with the
> above explanation. You and Jiewen understand the issue. Noone else on
> the list (that doesn't work at your office) does. I've wasted half a day
> because you didn't write up the background up-front.
> 
> I don't need to know your specific internal proprietary feature that
> requires this change. I do need to know that an internal feature with
> such a requirement exists, and that it is your motivation. Every ten
> minutes you save for yourselves on documentation is amplified to several
> hours of struggle, for each reviewer. And we specifically discussed this
> scenario with Mike at one of the earlier stewards' meetings.
> 
> (The general topic back then was my raising that you [plural] liberally
> commit whatever you want to MdePkg / MdeModulePkg, without really naming
> the use cases, let alone adding client code to edk2. Whereas non-Intel
> contributors with a demonstrated need are heavily gated from adding code
> to MdePkg / MdeModulePkg.
> 
> Compare: the addition of the "PciSegmentLibSegmentInfo" instances
> (commit 5c9bb86f171c), without any consumer in edk2 whatsoever, versus
> BZ#957, where the new libraries -- originally proposed for MdePkg -- had
> demonstrated consumers, just not from Intel. Do you see the double
> standard?)
> 
> I wish I could just ignore UefiCpuPkg. I can't do that; it is very
> brittle / sensitive, and has caused numerous regressions for OVMF. I
> must keep a close eye on it. But patches from you [plural] certainly
> don't make that easy for me.
> 
> It kills me that after years of us begging on the list, you [plural]
> still don't care about non-Intel participants as first class citizens
> up-front, until they raise a stink.
> 
> Here's another recent example:
> 
> https://bugzilla.tianocore.org/show_bug.cgi?id=1116#c3
> https://bugzilla.tianocore.org/show_bug.cgi?id=1116#c4
> 
> Please start actually believing in, and practicing, Open Development.
> You no longer write the code and the docs for Intel only. Convincing
> your colleagues is easy -- that's no different from the proprietary /
> closed source days of firmware development. It was Intel's decision to
> turn edk2 into an open source project; please live that decision. Now
> your primary audience is the contributors that depend on collaborating
> with you *without* working for your employer.
> 
> Well I've probably overstated my case. It's just that I've hoped for
> better commit messages since that stewards' meeting.
> 
> Laszlo

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

[Yao, Jiewen]

Reviewed-by: Jiewen.yao@intel.com

> -----Original Message-----
> From: Gao, Liming
> Sent: Friday, September 21, 2018 8:56 AM
> To: edk2-devel@lists.01.org
> Cc: Laszlo Ersek <lersek@redhat.com>; Dong, Eric <eric.dong@intel.com>;
> Yao, Jiewen <jiewen.yao@intel.com>
> Subject: [PATCH v2] UefiCpuPkg PiSmmCpuDxeSmm: Update SmiEntry
> function run the same position
> 
> 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 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
> --
> 2.10.0.windows.1

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

[Laszlo Ersek]

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
> 

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

[Gao, Liming]

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
>>