From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga11.intel.com (mga11.intel.com [192.55.52.93]) (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 25E1B81EDC for ; Thu, 24 Nov 2016 05:48:32 -0800 (PST) Received: from fmsmga005.fm.intel.com ([10.253.24.32]) by fmsmga102.fm.intel.com with ESMTP; 24 Nov 2016 05:48:31 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.31,543,1473145200"; d="scan'208";a="35200152" Received: from fmsmsx104.amr.corp.intel.com ([10.18.124.202]) by fmsmga005.fm.intel.com with ESMTP; 24 Nov 2016 05:48:31 -0800 Received: from fmsmsx114.amr.corp.intel.com (10.18.116.8) by fmsmsx104.amr.corp.intel.com (10.18.124.202) with Microsoft SMTP Server (TLS) id 14.3.248.2; Thu, 24 Nov 2016 05:48:31 -0800 Received: from shsmsx151.ccr.corp.intel.com (10.239.6.50) by FMSMSX114.amr.corp.intel.com (10.18.116.8) with Microsoft SMTP Server (TLS) id 14.3.248.2; Thu, 24 Nov 2016 05:48:31 -0800 Received: from shsmsx102.ccr.corp.intel.com ([169.254.2.239]) by SHSMSX151.ccr.corp.intel.com ([169.254.3.96]) with mapi id 14.03.0248.002; Thu, 24 Nov 2016 21:48:27 +0800 From: "Fan, Jeff" To: Laszlo Ersek , "edk2-devel@ml01.01.org" CC: "Tian, Feng" , "Kinney, Michael D" Thread-Topic: [PATCH 2/3] UefiCpuPkg/DxeMpLib: Allocate new safe stack < 4GB Thread-Index: AQHSRbfVt1zH0gBkRk6yLiXUjQeOMKDnSwiQgAALfACAAMpVkA== Date: Thu, 24 Nov 2016 13:48:26 +0000 Message-ID: <542CF652F8836A4AB8DBFAAD40ED192A4A2E7262@shsmsx102.ccr.corp.intel.com> References: <20161123140138.15976-1-jeff.fan@intel.com> <20161123140138.15976-3-jeff.fan@intel.com> <25db000e-ad85-2fd6-6936-e0147e7c20b4@redhat.com> <542CF652F8836A4AB8DBFAAD40ED192A4A2E686B@shsmsx102.ccr.corp.intel.com> In-Reply-To: Accept-Language: zh-CN, en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiOWM3OTI4NGYtNTNjMC00OTg1LTg0ZWItOTRlNzJlNTkyMGFkIiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX0lDIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE1LjkuNi42IiwiVHJ1c3RlZExhYmVsSGFzaCI6IkIzdkdxTXYza1lCc0tSb2VzVGw5OUR3M0dQMXpDVHo0eCtrMkFEVld1c289In0= x-ctpclassification: CTP_IC x-originating-ip: [10.239.127.40] MIME-Version: 1.0 Subject: Re: [PATCH 2/3] UefiCpuPkg/DxeMpLib: Allocate new safe stack < 4GB X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 24 Nov 2016 13:48:32 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Laszlo, I added my feedback for your three questions. -----Original Message----- From: Laszlo Ersek [mailto:lersek@redhat.com]=20 Sent: Thursday, November 24, 2016 5:21 PM To: Fan, Jeff; edk2-devel@ml01.01.org Cc: Tian, Feng; Kinney, Michael D Subject: Re: [PATCH 2/3] UefiCpuPkg/DxeMpLib: Allocate new safe stack < 4GB On 11/24/16 03:23, Fan, Jeff wrote: > -----Original Message----- > From: Laszlo Ersek [mailto:lersek@redhat.com] > Sent: Thursday, November 24, 2016 2:31 AM > To: Fan, Jeff; edk2-devel@ml01.01.org > Cc: Tian, Feng; Kinney, Michael D > Subject: Re: [PATCH 2/3] UefiCpuPkg/DxeMpLib: Allocate new safe stack=20 > < 4GB > I have some more questions about the preexistent code: >=20 > (8) The MONITOR statement seems to set up an address *range* for=20 > monitoring with MWAIT. EAX provides the base address of the range, and=20 > we point it to our new stack. However, what determines the *size* of=20 > the address range? Obviously, it must fit in our new stack. > [Jeff] Yes. Monitor has size requirement to avoid fault wakeup. But=20 > this case, fault wakeup has no any real impact. It has rare case to=20 > write the memory. Even though fault wakeup happens, safe mwait-loop=20 > could make sure AP enter into C-state again. I'm sorry, I think my question was not clear enough. I'm actually intereste= d in three things here. * The first question is just for my general education, because I'm not fami= liar with the MONITOR configuration. The question is independent of the cod= e, it is just for me to learn about MONITOR. So, how does the programmer configure the size of the monitored area? The base address is taken from EAX. Where does the size come from? [Jeff] Monitor size is from CPUID. I observed it equal to the cache line si= ze. But we needn't to set size of Monitor and only set base address of Moni= tor buffer when executing MONITOR instruction. * My second note is that whatever size we configure for the monitor, it sho= uld fit into our new stack. I see that you agree (although spurious wakeups= don't matter), so this part is done then. [Jeff] Yes. It should be in new stack. * And third, I thought of something else last night. Please consider the Ia32 case: > diff --git a/UefiCpuPkg/Library/MpInitLib/Ia32/MpFuncs.nasm=20 > b/UefiCpuPkg/Library/MpInitLib/Ia32/MpFuncs.nasm > index 64e51d8..4e55760 100644 > --- a/UefiCpuPkg/Library/MpInitLib/Ia32/MpFuncs.nasm > +++ b/UefiCpuPkg/Library/MpInitLib/Ia32/MpFuncs.nasm > @@ -217,14 +217,19 @@ RendezvousFunnelProcEnd: > global ASM_PFX(AsmRelocateApLoop) > ASM_PFX(AsmRelocateApLoop): > AsmRelocateApLoopStart: > - cmp byte [esp + 4], 1 > + push ebp > + mov ebp, esp > + mov ecx, [ebp + 8] ; MwaitSupport > + mov ebx, [ebp + 12] ; ApTargetCState > + mov esp, [ebp + 20] ; TopOfApStack We set ESP precisely to TopOfApStack here. > + cmp cl, 1 ; Check mwait-monitor support > jnz HltLoop > MwaitLoop: > mov eax, esp Here we move the same value to EAX -- so EAX equals TopOfApStack. > xor ecx, ecx > xor edx, edx > monitor And here we point the monitor to TopOfApStack exactly. Note that nothing ha= s been pushed. > - mov eax, [esp + 8] ; Mwait Cx, Target C-State per eax[7:4] > + mov eax, ebx ; Mwait Cx, Target C-State per eax[7:= 4] > shl eax, 4 > mwait > jmp MwaitLoop The stack grows down. Whenever we push something, the stack pointer is decr= emented first, and then the value being pushed is written to the location p= ointed-to by the new (decremented) stack pointer. (I verified this order of= operations in the Intel SDM). "mReservedTopOfApStack" points to the byte *right past* the allocated area.= In turn, for ProcessorNumber =3D 0, "TopOfApStack" is set exactly to "mRes= ervedTopOfApStack". This is valid if we push something, because then the pointer is decremented= first, and the value will be written into the allocated area. However, with MONITOR, the area to watch extends *upward* from EAX. This me= ans that for ProcessorNumber =3D 0, we will be monitoring an area that is s= trictly outside of the allocated range, regardless of the size of the monit= ored range. And, for ProcessorNumber (N+1), the monitored area will actuall= y be located in the stack slice of ProcessorNumber (N). So, my point is, we should determine the size of the monitored area exactly= , and push that much empty space (=3D=3D decrement ESP manually) before inv= oking MONITOR. ... Hm, okay, here's what the Intel SDM tells us about MONITOR: "the addres= s range that the monitoring hardware checks for store operations can be det= ermined by using CPUID". We should consider this address range in both the assembly code, and in the= original allocation (so replace AP_SAFE_STACK_SIZE with a dynamic value, o= r at least make sure that AP_SAFT_STACK_SIZE is larger than any imaginable = monitor area size). The same issue could apply to the X64 code, but that's more complex so I di= dn't check it. [Jeff] On normal case, we DO should check monitor size and make sure we wri= te the correct monitor buffer to wakeup Aps. But for this case, we do not w= ant to wake up Aps at all. So, we just have one mwait deadloop here. Once w= e found AP is wake up by any reason (SMI, Interrupt, or monitor buffer writ= ing), we will go on executing mwait and place Aps into C-State again. As long as this mwait loop does not touch any unsafe code/stack, it should = be OK. This could simply the implementation. Thank you, Laszlo