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 9981281E8F for ; Fri, 11 Nov 2016 04:08:09 -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 1F8D561BB2; Fri, 11 Nov 2016 12:08:13 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-102.phx2.redhat.com [10.3.116.102]) by int-mx11.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id uABC8BUi006068; Fri, 11 Nov 2016 07:08:11 -0500 To: Jeff Fan , edk2-devel@ml01.01.org References: <20161111085644.11512-1-jeff.fan@intel.com> <94d628af-b57c-9d69-3629-3e57cf9192e6@redhat.com> Cc: Michael D Kinney , Paolo Bonzini , Jiewen Yao From: Laszlo Ersek Message-ID: <1a64a6db-39a6-6e67-218d-58e2feb9cd5d@redhat.com> Date: Fri, 11 Nov 2016 13:08:10 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.4.0 MIME-Version: 1.0 In-Reply-To: <94d628af-b57c-9d69-3629-3e57cf9192e6@redhat.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]); Fri, 11 Nov 2016 12:08:13 +0000 (UTC) Subject: Re: [PATCH v2] UefiCpuPkg/DxeMpLib: Allocate below 4GB mem for AsmRelocateApLoopFunc 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, 11 Nov 2016 12:08:09 -0000 Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit On 11/11/16 11:51, Laszlo Ersek wrote: > On 11/11/16 09:56, Jeff Fan wrote: >> Current implementation just allocates reserve memory for AsmRelocateApLoopFunc. >> It not be safe because APs will be placed into 32bit protected mode on long mode >> DXE. This reserve memory must be located below 4GB memory. >> >> This fix is to allocate < 4GB memory for AsmRelocateApLoopFunc. >> >> Cc: Laszlo Ersek >> Cc: Paolo Bonzini >> Cc: Jiewen Yao >> Cc: Michael D Kinney >> Contributed-under: TianoCore Contribution Agreement 1.0 >> Signed-off-by: Jeff Fan >> --- >> UefiCpuPkg/Library/MpInitLib/DxeMpLib.c | 27 ++++++++++++++++++++------- >> 1 file changed, 20 insertions(+), 7 deletions(-) >> >> diff --git a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c >> index eb36d6f..4b929ff 100644 >> --- a/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c >> +++ b/UefiCpuPkg/Library/MpInitLib/DxeMpLib.c >> @@ -286,7 +286,8 @@ InitMpGlobalData ( >> IN CPU_MP_DATA *CpuMpData >> ) >> { >> - EFI_STATUS Status; >> + EFI_STATUS Status; >> + EFI_PHYSICAL_ADDRESS Address; >> >> SaveCpuMpData (CpuMpData); >> >> @@ -298,16 +299,28 @@ InitMpGlobalData ( >> } >> >> // >> - // Avoid APs access invalid buff data which allocated by BootServices, >> - // so we will allocate reserved data for AP loop code. >> + // Avoid APs access invalid buffer data which allocated by BootServices, >> + // so we will allocate reserved data for AP loop code. We also need to There was a superfluous space character at the end of this line (reported by git-am), but I removed it. >> + // allocate this buffer below 4GB due to APs may be transferred to 32bit >> + // protected mode on long mode DXE. >> // Allocating it in advance since memory services are not available in >> // Exit Boot Services callback function. >> // >> - mReservedApLoopFunc = AllocateReservedCopyPool ( >> - CpuMpData->AddressMap.RelocateApLoopFuncSize, >> - CpuMpData->AddressMap.RelocateApLoopFuncAddress >> - ); >> + Address = BASE_4GB - 1; >> + Status = gBS->AllocatePages ( >> + AllocateMaxAddress, >> + EfiReservedMemoryType, >> + EFI_SIZE_TO_PAGES (sizeof (CpuMpData->AddressMap.RelocateApLoopFuncSize)), >> + &Address >> + ); >> + ASSERT_EFI_ERROR (Status); >> + mReservedApLoopFunc = (VOID *) (UINTN) Address; >> ASSERT (mReservedApLoopFunc != NULL); >> + CopyMem ( >> + mReservedApLoopFunc, >> + CpuMpData->AddressMap.RelocateApLoopFuncAddress, >> + CpuMpData->AddressMap.RelocateApLoopFuncSize >> + ); >> >> Status = gBS->CreateEvent ( >> EVT_TIMER | EVT_NOTIFY_SIGNAL, >> > > Reviewed-by: Laszlo Ersek > Tested-by: Laszlo Ersek [lersek@redhat.com: strip whitespace at EOL] Signed-off-by: Laszlo Ersek Commit ffd6b0b1b65e. (I pushed the patch because it's simple -- Jeff is the sole maintainer, according to Maintainers.txt, of UefiCpuPkg, and I thought he'd push this simple patch with just my review anyway. I'm trying to flush my review / test queue, simplifying the possible orderings between the pending patch sets.) Thanks Laszlo