From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=192.55.52.88; helo=mga01.intel.com; envelope-from=jiewen.yao@intel.com; receiver=edk2-devel@lists.01.org Received: from mga01.intel.com (mga01.intel.com [192.55.52.88]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 142B521129455 for ; Tue, 11 Sep 2018 18:35:51 -0700 (PDT) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga006.jf.intel.com ([10.7.209.51]) by fmsmga101.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 11 Sep 2018 18:35:51 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.53,362,1531810800"; d="scan'208";a="73519710" Received: from fmsmsx104.amr.corp.intel.com ([10.18.124.202]) by orsmga006.jf.intel.com with ESMTP; 11 Sep 2018 18:33:38 -0700 Received: from fmsmsx111.amr.corp.intel.com (10.18.116.5) by fmsmsx104.amr.corp.intel.com (10.18.124.202) with Microsoft SMTP Server (TLS) id 14.3.319.2; Tue, 11 Sep 2018 18:33:38 -0700 Received: from shsmsx103.ccr.corp.intel.com (10.239.4.69) by fmsmsx111.amr.corp.intel.com (10.18.116.5) with Microsoft SMTP Server (TLS) id 14.3.319.2; Tue, 11 Sep 2018 18:33:37 -0700 Received: from shsmsx102.ccr.corp.intel.com ([169.254.2.226]) by SHSMSX103.ccr.corp.intel.com ([169.254.4.240]) with mapi id 14.03.0319.002; Wed, 12 Sep 2018 09:33:35 +0800 From: "Yao, Jiewen" To: "Gao, Liming" , Laszlo Ersek , "edk2-devel@lists.01.org" CC: "Dong, Eric" Thread-Topic: [edk2] [Patch] UefiCpuPkg PiSmmCpuDxeSmm: Remove unnecessary jmp _SmiHandler Thread-Index: AQHUSN8xD8V1gRuKEEm+8A26kUclC6Tqqh2AgAD6PwD//6trgIAAhu7w//+BpACAAIZKgA== Date: Wed, 12 Sep 2018 01:33:34 +0000 Message-ID: <74D8A39837DF1E4DA445A8C0B3885C503AD52F07@shsmsx102.ccr.corp.intel.com> References: <1536567625-9540-1-git-send-email-liming.gao@intel.com> <74D8A39837DF1E4DA445A8C0B3885C503AD51D73@shsmsx102.ccr.corp.intel.com> <4A89E2EF3DFEDB4C8BFDE51014F606A14E2F5D28@SHSMSX104.ccr.corp.intel.com> <74D8A39837DF1E4DA445A8C0B3885C503AD52D8F@shsmsx102.ccr.corp.intel.com> <4A89E2EF3DFEDB4C8BFDE51014F606A14E2F5F38@SHSMSX104.ccr.corp.intel.com> In-Reply-To: <4A89E2EF3DFEDB4C8BFDE51014F606A14E2F5F38@SHSMSX104.ccr.corp.intel.com> Accept-Language: zh-CN, en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiMTgzZTNiMjktMjFjZS00ZGZjLWI5NDItYzMzNDJiYTdmZmI3IiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX05UIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE3LjEwLjE4MDQuNDkiLCJUcnVzdGVkTGFiZWxIYXNoIjoib1VpSko1QmsxWHZTOHpcL0FvaGc1RWdcL3hld3hOTEkzU0dYK1MwTGRCcVFJd0t0bmdyUFl6ZjBsK2tCQVZ6elRjIn0= x-ctpclassification: CTP_NT dlp-product: dlpe-windows dlp-version: 11.0.400.15 dlp-reaction: no-action x-originating-ip: [10.239.127.40] MIME-Version: 1.0 Subject: Re: [Patch] UefiCpuPkg PiSmmCpuDxeSmm: Remove unnecessary jmp _SmiHandler X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 12 Sep 2018 01:35:52 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable We have some internal feature, required to run the code at the each SMI ent= rypoint, 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 ; Laszlo Ersek ; > edk2-devel@lists.01.org > Cc: Dong, Eric ; Gao, Liming > Subject: RE: [edk2] [Patch] UefiCpuPkg PiSmmCpuDxeSmm: Remove > unnecessary jmp _SmiHandler >=20 > 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. >=20 > Could you let me know why can't use current way? >=20 > Thanks > Liming > >-----Original Message----- > >From: Yao, Jiewen > >Sent: Wednesday, September 12, 2018 9:04 AM > >To: Gao, Liming ; Laszlo Ersek > ; > >edk2-devel@lists.01.org > >Cc: Dong, Eric > >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 ; Laszlo Ersek > >; > >> edk2-devel@lists.01.org > >> Cc: Dong, Eric > >> Subject: RE: [edk2] [Patch] UefiCpuPkg PiSmmCpuDxeSmm: Remove > >> unnecessary jmp _SmiHandler > >> > >> Jiewen: > >> After do more verification, I recall this change. Current code is re= ally > >> 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 anoth= er > >> 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 jm= p > 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 ; Gao, Liming > >> ; > >> >edk2-devel@lists.01.org > >> >Cc: Dong, Eric > >> >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 ; edk2-devel@lists.01.org > >> >> Cc: Yao, Jiewen ; Dong, Eric > >> > >> >> 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 > >> >> > Cc: Jiewen Yao > >> >> > --- > >> >> > 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 co= de. > >> >> > >> >> Reviewed-by: Laszlo Ersek > >> >> > >> >> Can you perhaps add another sentence to the commit message, > before > >> you > >> >> push the patch, such as "the original code already uses RIP-relativ= e > >> >> addressing"? > >> >> > >> >> Thanks > >> >> Laszlo > >> >> _______________________________________________ > >> >> edk2-devel mailing list > >> >> edk2-devel@lists.01.org > >> >> https://lists.01.org/mailman/listinfo/edk2-devel