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 A657981F10 for ; Fri, 25 Nov 2016 02:55:13 -0800 (PST) Received: from int-mx14.intmail.prod.int.phx2.redhat.com (int-mx14.intmail.prod.int.phx2.redhat.com [10.5.11.27]) (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 25C0990904; Fri, 25 Nov 2016 10:55:13 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-35.phx2.redhat.com [10.3.116.35]) by int-mx14.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id uAPAtBAF017097; Fri, 25 Nov 2016 05:55:11 -0500 To: Jeff Fan , edk2-devel@ml01.01.org References: <20161125060312.27932-1-jeff.fan@intel.com> <20161125060312.27932-3-jeff.fan@intel.com> Cc: Feng Tian , Michael D Kinney From: Laszlo Ersek Message-ID: Date: Fri, 25 Nov 2016 11:55:10 +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: <20161125060312.27932-3-jeff.fan@intel.com> X-Scanned-By: MIMEDefang 2.68 on 10.5.11.27 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.29]); Fri, 25 Nov 2016 10:55:13 +0000 (UTC) Subject: Re: [PATCH v2 2/5] 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: Fri, 25 Nov 2016 10:55:13 -0000 Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit On 11/25/16 07:03, Jeff Fan wrote: > For long mode DXE, we will disable paging on AP to protected mode to execute AP > safe loop code in reserved memory range under 4GB. But we forget to allocate > stack for AP under 4GB and AP still are using original AP stack. If original AP > stack is larger than 4GB, it cannot be used after AP is transferred to protected > mode. Besides MwaitSupport == TRUE, AP stack is still required during phase of > disabling paging in long mode DXE. > > MMoreover, even though AP stack is always under 4GB (a) in Ia32 DXE and (b) with Small typo in "MMoreover", can be fixed up before pushing. Reviewed-by: Laszlo Ersek Thanks Laszlo > this patch, after transferring to protected mode from X64 DXE, AP stack > (in BootServiceData) maybe crashed by OS after Exit Boot Service event. > > This fix is to allocate reserved memory range under 4GB together with AP safe > loop code. APs will switch to new stack in 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 | 19 +++++++++++++++++-- > UefiCpuPkg/Library/MpInitLib/Ia32/MpFuncs.nasm | 13 ++++++++++--- > UefiCpuPkg/Library/MpInitLib/MpLib.h | 3 ++- > UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm | 3 ++- > 4 files changed, 31 insertions(+), 7 deletions(-) > > diff --git a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c > index a0d5eeb..5a3b02c 100644 > --- a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c > +++ b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c > @@ -18,6 +18,7 @@ > #include > > #define AP_CHECK_INTERVAL (EFI_TIMER_PERIOD_MILLISECONDS (100)) > +#define AP_SAFE_STACK_SIZE 128 > > CPU_MP_DATA *mCpuMpData = NULL; > EFI_EVENT mCheckAllApsEvent = NULL; > @@ -25,6 +26,7 @@ EFI_EVENT mMpInitExitBootServicesEvent = NULL; > EFI_EVENT mLegacyBootEvent = NULL; > volatile BOOLEAN mStopCheckAllApsStatus = TRUE; > VOID *mReservedApLoopFunc = NULL; > +UINTN mReservedTopOfApStack; > > /** > Get the pointer to CPU MP Data structure. > @@ -241,11 +243,18 @@ RelocateApLoop ( > CPU_MP_DATA *CpuMpData; > BOOLEAN MwaitSupport; > ASM_RELOCATE_AP_LOOP AsmRelocateApLoopFunc; > + UINTN ProcessorNumber; > > + MpInitLibWhoAmI (&ProcessorNumber); > CpuMpData = GetCpuMpData (); > MwaitSupport = IsMwaitSupport (); > AsmRelocateApLoopFunc = (ASM_RELOCATE_AP_LOOP) (UINTN) mReservedApLoopFunc; > - AsmRelocateApLoopFunc (MwaitSupport, CpuMpData->ApTargetCState, CpuMpData->PmCodeSegment); > + AsmRelocateApLoopFunc ( > + MwaitSupport, > + CpuMpData->ApTargetCState, > + CpuMpData->PmCodeSegment, > + mReservedTopOfApStack - ProcessorNumber * AP_SAFE_STACK_SIZE > + ); > // > // It should never reach here > // > @@ -289,6 +298,7 @@ InitMpGlobalData ( > { > EFI_STATUS Status; > EFI_PHYSICAL_ADDRESS Address; > + UINTN ApSafeBufferSize; > > SaveCpuMpData (CpuMpData); > > @@ -307,16 +317,21 @@ InitMpGlobalData ( > // Allocating it in advance since memory services are not available in > // Exit Boot Services callback function. > // > + ApSafeBufferSize = CpuMpData->AddressMap.RelocateApLoopFuncSize; > + ApSafeBufferSize += CpuMpData->CpuCount * AP_SAFE_STACK_SIZE; > + > Address = BASE_4GB - 1; > Status = gBS->AllocatePages ( > AllocateMaxAddress, > EfiReservedMemoryType, > - EFI_SIZE_TO_PAGES (sizeof (CpuMpData->AddressMap.RelocateApLoopFuncSize)), > + EFI_SIZE_TO_PAGES (ApSafeBufferSize), > &Address > ); > ASSERT_EFI_ERROR (Status); > mReservedApLoopFunc = (VOID *) (UINTN) Address; > ASSERT (mReservedApLoopFunc != NULL); > + mReservedTopOfApStack = (UINTN) Address + EFI_PAGES_TO_SIZE (EFI_SIZE_TO_PAGES (ApSafeBufferSize)); > + ASSERT ((mReservedTopOfApStack & (UINTN)(CPU_STACK_ALIGNMENT - 1)) == 0); > CopyMem ( > mReservedApLoopFunc, > CpuMpData->AddressMap.RelocateApLoopFuncAddress, > diff --git a/UefiCpuPkg/Library/MpInitLib/Ia32/MpFuncs.nasm b/UefiCpuPkg/Library/MpInitLib/Ia32/MpFuncs.nasm > index 9067f78..7ab136b 100644 > --- a/UefiCpuPkg/Library/MpInitLib/Ia32/MpFuncs.nasm > +++ b/UefiCpuPkg/Library/MpInitLib/Ia32/MpFuncs.nasm > @@ -215,19 +215,26 @@ CProcedureInvoke: > RendezvousFunnelProcEnd: > > ;------------------------------------------------------------------------------------- > -; AsmRelocateApLoop (MwaitSupport, ApTargetCState, PmCodeSegment); > +; AsmRelocateApLoop (MwaitSupport, ApTargetCState, PmCodeSegment, TopOfApStack); > ;------------------------------------------------------------------------------------- > global ASM_PFX(AsmRelocateApLoop) > ASM_PFX(AsmRelocateApLoop): > AsmRelocateApLoopStart: > - cmp byte [esp + 4], 1 > + mov eax, esp > + mov esp, [eax + 16] ; TopOfApStack > + push dword [eax] ; push return address for stack trace > + push ebp > + mov ebp, esp > + mov ebx, [eax + 8] ; ApTargetCState > + mov ecx, [eax + 4] ; MwaitSupport > + cmp cl, 1 ; Check mwait-monitor support > jnz HltLoop > MwaitLoop: > mov eax, esp > xor ecx, ecx > xor edx, edx > monitor > - 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 > diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.h b/UefiCpuPkg/Library/MpInitLib/MpLib.h > index f73a469..e6dea18 100644 > --- a/UefiCpuPkg/Library/MpInitLib/MpLib.h > +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.h > @@ -250,7 +250,8 @@ VOID > (EFIAPI * ASM_RELOCATE_AP_LOOP) ( > IN BOOLEAN MwaitSupport, > IN UINTN ApTargetCState, > - IN UINTN PmCodeSegment > + IN UINTN PmCodeSegment, > + IN UINTN TopOfApStack > ); > > /** > diff --git a/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm b/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm > index e7e7d80..7869970 100644 > --- a/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm > +++ b/UefiCpuPkg/Library/MpInitLib/X64/MpFuncs.nasm > @@ -222,11 +222,12 @@ CProcedureInvoke: > RendezvousFunnelProcEnd: > > ;------------------------------------------------------------------------------------- > -; AsmRelocateApLoop (MwaitSupport, ApTargetCState, PmCodeSegment); > +; AsmRelocateApLoop (MwaitSupport, ApTargetCState, PmCodeSegment, TopOfApStack); > ;------------------------------------------------------------------------------------- > global ASM_PFX(AsmRelocateApLoop) > ASM_PFX(AsmRelocateApLoop): > AsmRelocateApLoopStart: > + mov rsp, r9 > push rcx > push rdx > >