* [Patch] UefiCpuPkg PiSmmCpuDxeSmm: Remove unnecessary jmp _SmiHandler
@ 2018-09-10 8:20 Liming Gao
2018-09-11 15:06 ` Laszlo Ersek
0 siblings, 1 reply; 7+ messages in thread
From: Liming Gao @ 2018-09-10 8:20 UTC (permalink / raw)
To: edk2-devel; +Cc: Jiewen Yao
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
--
2.10.0.windows.1
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [Patch] UefiCpuPkg PiSmmCpuDxeSmm: Remove unnecessary jmp _SmiHandler
2018-09-10 8:20 [Patch] UefiCpuPkg PiSmmCpuDxeSmm: Remove unnecessary jmp _SmiHandler Liming Gao
@ 2018-09-11 15:06 ` Laszlo Ersek
2018-09-11 22:02 ` Yao, Jiewen
0 siblings, 1 reply; 7+ messages in thread
From: Laszlo Ersek @ 2018-09-11 15:06 UTC (permalink / raw)
To: Liming Gao, edk2-devel; +Cc: Jiewen Yao, Eric Dong
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Patch] UefiCpuPkg PiSmmCpuDxeSmm: Remove unnecessary jmp _SmiHandler
2018-09-11 15:06 ` Laszlo Ersek
@ 2018-09-11 22:02 ` Yao, Jiewen
2018-09-12 0:59 ` Gao, Liming
0 siblings, 1 reply; 7+ messages in thread
From: Yao, Jiewen @ 2018-09-11 22:02 UTC (permalink / raw)
To: Laszlo Ersek, Gao, Liming, edk2-devel@lists.01.org; +Cc: Dong, Eric
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Patch] UefiCpuPkg PiSmmCpuDxeSmm: Remove unnecessary jmp _SmiHandler
2018-09-11 22:02 ` Yao, Jiewen
@ 2018-09-12 0:59 ` Gao, Liming
2018-09-12 1:03 ` Yao, Jiewen
0 siblings, 1 reply; 7+ messages in thread
From: Gao, Liming @ 2018-09-12 0:59 UTC (permalink / raw)
To: Yao, Jiewen, Laszlo Ersek, edk2-devel@lists.01.org; +Cc: Dong, Eric
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Patch] UefiCpuPkg PiSmmCpuDxeSmm: Remove unnecessary jmp _SmiHandler
2018-09-12 0:59 ` Gao, Liming
@ 2018-09-12 1:03 ` Yao, Jiewen
2018-09-12 1:29 ` Gao, Liming
0 siblings, 1 reply; 7+ messages in thread
From: Yao, Jiewen @ 2018-09-12 1:03 UTC (permalink / raw)
To: Gao, Liming, Laszlo Ersek, edk2-devel@lists.01.org; +Cc: Dong, Eric
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Patch] UefiCpuPkg PiSmmCpuDxeSmm: Remove unnecessary jmp _SmiHandler
2018-09-12 1:03 ` Yao, Jiewen
@ 2018-09-12 1:29 ` Gao, Liming
2018-09-12 1:33 ` Yao, Jiewen
0 siblings, 1 reply; 7+ messages in thread
From: Gao, Liming @ 2018-09-12 1:29 UTC (permalink / raw)
To: Yao, Jiewen, Laszlo Ersek, edk2-devel@lists.01.org
Cc: Dong, Eric, Gao, Liming
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Patch] UefiCpuPkg PiSmmCpuDxeSmm: Remove unnecessary jmp _SmiHandler
2018-09-12 1:29 ` Gao, Liming
@ 2018-09-12 1:33 ` Yao, Jiewen
0 siblings, 0 replies; 7+ messages in thread
From: Yao, Jiewen @ 2018-09-12 1:33 UTC (permalink / raw)
To: Gao, Liming, Laszlo Ersek, edk2-devel@lists.01.org; +Cc: Dong, Eric
We have some internal feature, required to run the code at the each SMI entrypoint, instead of common code.
That is why we write code in previous way.
I suggest we keep using the previous way and provide a good example on how to handle absolute address in new way.
Can we provide an update for below:
> > mov rax, ASM_PFX(CpuSmmDebugEntry)
> > call rax
Thank you
Yao Jiewen
> -----Original Message-----
> From: Gao, Liming
> Sent: Wednesday, September 12, 2018 9:30 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>; Gao, Liming <liming.gao@intel.com>
> Subject: RE: [edk2] [Patch] UefiCpuPkg PiSmmCpuDxeSmm: Remove
> unnecessary jmp _SmiHandler
>
> 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
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2018-09-12 1:35 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-09-10 8:20 [Patch] UefiCpuPkg PiSmmCpuDxeSmm: Remove unnecessary jmp _SmiHandler Liming Gao
2018-09-11 15:06 ` Laszlo Ersek
2018-09-11 22:02 ` Yao, Jiewen
2018-09-12 0:59 ` Gao, Liming
2018-09-12 1:03 ` Yao, Jiewen
2018-09-12 1:29 ` Gao, Liming
2018-09-12 1:33 ` Yao, Jiewen
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox