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 276EB81E8B for ; Wed, 23 Nov 2016 10:52:08 -0800 (PST) Received: from int-mx11.intmail.prod.int.phx2.redhat.com (int-mx11.intmail.prod.int.phx2.redhat.com [10.5.11.24]) (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 99BC661E5B; Wed, 23 Nov 2016 18:52:07 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-97.phx2.redhat.com [10.3.116.97]) by int-mx11.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id uANIq5qF014034; Wed, 23 Nov 2016 13:52:06 -0500 To: Jeff Fan , edk2-devel@ml01.01.org References: <20161123140138.15976-1-jeff.fan@intel.com> <20161123140138.15976-4-jeff.fan@intel.com> Cc: Feng Tian , Michael D Kinney From: Laszlo Ersek Message-ID: <6250a0f2-9431-1e5b-3ef0-0485826acc94@redhat.com> Date: Wed, 23 Nov 2016 19:52:05 +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: <20161123140138.15976-4-jeff.fan@intel.com> X-Scanned-By: MIMEDefang 2.68 on 10.5.11.24 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.39]); Wed, 23 Nov 2016 18:52:07 +0000 (UTC) Subject: Re: [PATCH 3/3] UefiCpuPkg/DxeMpLib: Make sure APs in safe loop code 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: Wed, 23 Nov 2016 18:52:08 -0000 Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit On 11/23/16 15:01, Jeff Fan wrote: > Add one semaphore to make sure BSP to wait till all APs run in AP safe loop > code. > > Cc: Laszlo Ersek > Cc: Feng Tian > Cc: Michael D Kinney > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Jeff Fan > --- > UefiCpuPkg/Library/MpInitLib/DxeMpLib.c | 8 +++++++- > UefiCpuPkg/Library/MpInitLib/Ia32/MpFuncs.nasm | 2 ++ > UefiCpuPkg/Library/MpInitLib/MpLib.h | 3 ++- > UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm | 2 ++ > 4 files changed, 13 insertions(+), 2 deletions(-) > > diff --git a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c > index d0f9f7e..64c0c52 100644 > --- a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c > +++ b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c > @@ -27,6 +27,7 @@ EFI_EVENT mLegacyBootEvent = NULL; > volatile BOOLEAN mStopCheckAllApsStatus = TRUE; > VOID *mReservedApLoopFunc = NULL; > UINTN mReservedTopOfApStack; > +volatile UINT32 mNumberToFinish = 0; > > /** > Get the pointer to CPU MP Data structure. > @@ -253,7 +254,8 @@ RelocateApLoop ( > MwaitSupport, > CpuMpData->ApTargetCState, > CpuMpData->PmCodeSegment, > - mReservedTopOfApStack - ProcessorNumber * AP_SAFT_STACK_SIZE > + mReservedTopOfApStack - ProcessorNumber * AP_SAFT_STACK_SIZE, > + (UINTN) &mNumberToFinish > ); > // > // It should never reach here > @@ -282,8 +284,12 @@ MpInitChangeApLoopCallback ( > CpuMpData->SaveRestoreFlag = TRUE; > CpuMpData->PmCodeSegment = GetProtectedModeCS (); > CpuMpData->ApLoopMode = PcdGet8 (PcdCpuApLoopMode); > + mNumberToFinish = CpuMpData->CpuCount - 1; > WakeUpAP (CpuMpData, TRUE, 0, RelocateApLoop, NULL); > DEBUG ((DEBUG_INFO, "%a() done!\n", __FUNCTION__)); > + while (mNumberToFinish > 0) { > + CpuPause (); > + } > } (1) Can you hoist the counting above the "done" message? > > /** > diff --git a/UefiCpuPkg/Library/MpInitLib/Ia32/MpFuncs.nasm b/UefiCpuPkg/Library/MpInitLib/Ia32/MpFuncs.nasm > index 4e55760..6235748 100644 > --- a/UefiCpuPkg/Library/MpInitLib/Ia32/MpFuncs.nasm > +++ b/UefiCpuPkg/Library/MpInitLib/Ia32/MpFuncs.nasm > @@ -222,6 +222,8 @@ AsmRelocateApLoopStart: > mov ecx, [ebp + 8] ; MwaitSupport > mov ebx, [ebp + 12] ; ApTargetCState > mov esp, [ebp + 20] ; TopOfApStack > + mov eax, [ebp + 24] ; CountTofinish > + lock dec dword [eax] ; CountTofinish-- (2) The comments should say ; NumberToFinish ; (*NumberToFinish)-- > cmp cl, 1 ; Check mwait-monitor support > jnz HltLoop > MwaitLoop: > diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.h b/UefiCpuPkg/Library/MpInitLib/MpLib.h > index e6dea18..49305ad 100644 > --- a/UefiCpuPkg/Library/MpInitLib/MpLib.h > +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.h > @@ -251,7 +251,8 @@ VOID > IN BOOLEAN MwaitSupport, > IN UINTN ApTargetCState, > IN UINTN PmCodeSegment, > - IN UINTN TopOfApStack > + IN UINTN TopOfApStack, > + IN UINTN NumberToFinish > ); > > /** > diff --git a/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm b/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm > index 7c8fa45..deaee9e 100644 > --- a/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm > +++ b/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm > @@ -227,6 +227,8 @@ AsmRelocateApLoopStart: > push rbp > mov rbp, rsp > mov rsp, r9 > + mov rax, [rbp + 48] ; CountTofinish > + lock dec dword [rax] ; CountTofinish-- > push rcx > push rdx > > Hmmm, in the X64 case, this decrement seems to be pretty far from the actual loop; a window still remains. On the other hand, if we wanted to do the decrement from protected mode, then the semaphore would have to be moved into the specially allocated area too. Anyway, the point is that we are on the new stack, and will make no further accesses to BootServicesData type memory. (3) The comments should say ; NumberToFinish ; (*NumberToFinish)-- I think I'll postpone testing until the next version (if you decide to post v3). Thanks! Laszlo