From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga02.intel.com (mga02.intel.com [134.134.136.20]) (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 522E581EDA for ; Wed, 23 Nov 2016 18:38:01 -0800 (PST) Received: from fmsmga005.fm.intel.com ([10.253.24.32]) by orsmga101.jf.intel.com with ESMTP; 23 Nov 2016 18:38:00 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.31,689,1473145200"; d="scan'208";a="35036877" Received: from fmsmsx108.amr.corp.intel.com ([10.18.124.206]) by fmsmga005.fm.intel.com with ESMTP; 23 Nov 2016 18:38:00 -0800 Received: from fmsmsx114.amr.corp.intel.com (10.18.116.8) by FMSMSX108.amr.corp.intel.com (10.18.124.206) with Microsoft SMTP Server (TLS) id 14.3.248.2; Wed, 23 Nov 2016 18:38:00 -0800 Received: from shsmsx103.ccr.corp.intel.com (10.239.4.69) by FMSMSX114.amr.corp.intel.com (10.18.116.8) with Microsoft SMTP Server (TLS) id 14.3.248.2; Wed, 23 Nov 2016 18:38:00 -0800 Received: from shsmsx102.ccr.corp.intel.com ([169.254.2.239]) by SHSMSX103.ccr.corp.intel.com ([169.254.4.96]) with mapi id 14.03.0248.002; Thu, 24 Nov 2016 10:37:58 +0800 From: "Fan, Jeff" To: Laszlo Ersek , "edk2-devel@ml01.01.org" CC: "Tian, Feng" , "Kinney, Michael D" Thread-Topic: [PATCH 3/3] UefiCpuPkg/DxeMpLib: Make sure APs in safe loop code Thread-Index: AQHSRbq091BU/PSmOUW5DVUCaCVqFKDnaBOg Date: Thu, 24 Nov 2016 02:37:57 +0000 Message-ID: <542CF652F8836A4AB8DBFAAD40ED192A4A2E68B7@shsmsx102.ccr.corp.intel.com> References: <20161123140138.15976-1-jeff.fan@intel.com> <20161123140138.15976-4-jeff.fan@intel.com> <6250a0f2-9431-1e5b-3ef0-0485826acc94@redhat.com> In-Reply-To: <6250a0f2-9431-1e5b-3ef0-0485826acc94@redhat.com> Accept-Language: zh-CN, en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiOTExYjM4MjctOTYxNC00OGZiLWEyZGUtYTQ5OTczODMyMTMwIiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX0lDIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE1LjkuNi42IiwiVHJ1c3RlZExhYmVsSGFzaCI6IlFJam42NEpvZzllYU1FKzBGTVFIZlNDd242TVIyV1QzTzBJQ0VESURwekE9In0= x-ctpclassification: CTP_IC x-originating-ip: [10.239.127.40] MIME-Version: 1.0 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: Thu, 24 Nov 2016 02:38:01 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Laszlo, I added my feedback in [Jeff]. -----Original Message----- From: Laszlo Ersek [mailto:lersek@redhat.com]=20 Sent: Thursday, November 24, 2016 2:52 AM To: Fan, Jeff; edk2-devel@ml01.01.org Cc: Tian, Feng; Kinney, Michael D Subject: Re: [PATCH 3/3] UefiCpuPkg/DxeMpLib: Make sure APs in safe loop co= de 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=20 > loop code. >=20 > 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(-) >=20 > diff --git a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c=20 > 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 =3D NULL; > volatile BOOLEAN mStopCheckAllApsStatus =3D TRUE; > VOID *mReservedApLoopFunc =3D NULL; > UINTN mReservedTopOfApStack; > +volatile UINT32 mNumberToFinish =3D 0; > =20 > /** > 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 =3D TRUE; > CpuMpData->PmCodeSegment =3D GetProtectedModeCS (); > CpuMpData->ApLoopMode =3D PcdGet8 (PcdCpuApLoopMode); > + mNumberToFinish =3D 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? [Jeff] Agree. > =20 > /** > diff --git a/UefiCpuPkg/Library/MpInitLib/Ia32/MpFuncs.nasm=20 > 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)-- [Jeff] Agree. > cmp cl, 1 ; Check mwait-monitor support > jnz HltLoop > MwaitLoop: > diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.h=20 > 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 > ); > =20 > /** > diff --git a/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm=20 > 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 > =20 >=20 Hmmm, in the X64 case, this decrement seems to be pretty far from the actua= l loop; a window still remains. On the other hand, if we wanted to do the d= ecrement from protected mode, then the semaphore would have to be moved int= o the specially allocated area too. [Jeff] There is no window on this case. After "move rsp, r9" all AP code wi= ll be updated in safe code / safe stack. And it is in long mode. 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)-- [Jeff] Agree. I think I'll postpone testing until the next version (if you decide to post= v3). Thanks! Laszlo