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