From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) (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 8239D81EE4 for ; Thu, 24 Nov 2016 01:20:47 -0800 (PST) Received: from int-mx09.intmail.prod.int.phx2.redhat.com (int-mx09.intmail.prod.int.phx2.redhat.com [10.5.11.22]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 0549F19CF3E; Thu, 24 Nov 2016 09:20:47 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-81.phx2.redhat.com [10.3.116.81]) by int-mx09.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id uAO9KjUd009018; Thu, 24 Nov 2016 04:20:45 -0500 To: "Fan, Jeff" , "edk2-devel@ml01.01.org" 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> Cc: "Tian, Feng" , "Kinney, Michael D" From: Laszlo Ersek Message-ID: Date: Thu, 24 Nov 2016 10:20:44 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.5.0 MIME-Version: 1.0 In-Reply-To: <542CF652F8836A4AB8DBFAAD40ED192A4A2E686B@shsmsx102.ccr.corp.intel.com> X-Scanned-By: MIMEDefang 2.68 on 10.5.11.22 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.29]); Thu, 24 Nov 2016 09:20:47 +0000 (UTC) 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 09:20:47 -0000 Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit 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 < 4GB > I have some more questions about the preexistent code: > > (8) The MONITOR statement seems to set up an address *range* for > monitoring with MWAIT. EAX provides the base address of the range, > and we point it to our new stack. However, what determines the *size* > of the address range? Obviously, it must fit in our new stack. > [Jeff] Yes. Monitor has size requirement to avoid fault wakeup. But > this case, fault wakeup has no any real impact. It has rare case to > write the memory. Even though fault wakeup happens, safe mwait-loop > could make sure AP enter into C-state again. I'm sorry, I think my question was not clear enough. I'm actually interested in three things here. * The first question is just for my general education, because I'm not familiar with the MONITOR configuration. The question is independent of the code, 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? * My second note is that whatever size we configure for the monitor, it should fit into our new stack. I see that you agree (although spurious wakeups don't matter), so this part is done then. * And third, I thought of something else last night. Please consider the Ia32 case: > diff --git a/UefiCpuPkg/Library/MpInitLib/Ia32/MpFuncs.nasm 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 has 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 decremented first, and then the value being pushed is written to the location pointed-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 = 0, "TopOfApStack" is set exactly to "mReservedTopOfApStack". 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 means that for ProcessorNumber = 0, we will be monitoring an area that is strictly outside of the allocated range, regardless of the size of the monitored range. And, for ProcessorNumber (N+1), the monitored area will actually 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 (== decrement ESP manually) before invoking MONITOR. ... Hm, okay, here's what the Intel SDM tells us about MONITOR: "the address range that the monitoring hardware checks for store operations can be determined 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, or 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 didn't check it. Thank you, Laszlo