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 691E581E3A for ; Mon, 14 Nov 2016 17:40:11 -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 9A2157F6B2; Tue, 15 Nov 2016 01:40:15 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-50.phx2.redhat.com [10.3.116.50]) by int-mx14.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id uAF1eDGY029259; Mon, 14 Nov 2016 20:40:13 -0500 To: Jeff Fan , edk2-devel@ml01.01.org References: <20161111085644.11512-1-jeff.fan@intel.com> Cc: Michael D Kinney , Paolo Bonzini , Jiewen Yao From: Laszlo Ersek Message-ID: <54156d2c-cbf5-ea32-8a29-7b1e5ae9bdd7@redhat.com> Date: Tue, 15 Nov 2016 02:40:12 +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: <20161111085644.11512-1-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.25]); Tue, 15 Nov 2016 01:40:15 +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: Tue, 15 Nov 2016 01:40:11 -0000 Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Jeff, independently from the other discussion, I found a small mistake in this patch (after I reviewed, tested, and committed it for you...): 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 > + // 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)), The "sizeof" operator should be unnecessary here. (It is no problem in practice because the function size is really small, so it gets rounded up to 1 page anyway.) Sorry for not noticing this earlier. Thanks Laszlo > + &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, >