* [PATCH v2] UefiCpuPkg/DxeMpLib: Allocate below 4GB mem for AsmRelocateApLoopFunc @ 2016-11-11 8:56 Jeff Fan 2016-11-11 10:51 ` Laszlo Ersek 2016-11-15 1:40 ` Laszlo Ersek 0 siblings, 2 replies; 8+ messages in thread From: Jeff Fan @ 2016-11-11 8:56 UTC (permalink / raw) To: edk2-devel; +Cc: Laszlo Ersek, Paolo Bonzini, Jiewen Yao, Michael D Kinney 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 <lersek@redhat.com> Cc: Paolo Bonzini <pbonzini@redhat.com> Cc: Jiewen Yao <jiewen.yao@intel.com> Cc: Michael D Kinney <michael.d.kinney@intel.com> Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Jeff Fan <jeff.fan@intel.com> --- 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)), + &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, -- 2.9.3.windows.2 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v2] UefiCpuPkg/DxeMpLib: Allocate below 4GB mem for AsmRelocateApLoopFunc 2016-11-11 8:56 [PATCH v2] UefiCpuPkg/DxeMpLib: Allocate below 4GB mem for AsmRelocateApLoopFunc Jeff Fan @ 2016-11-11 10:51 ` Laszlo Ersek 2016-11-11 12:08 ` Laszlo Ersek 2016-11-15 1:40 ` Laszlo Ersek 1 sibling, 1 reply; 8+ messages in thread From: Laszlo Ersek @ 2016-11-11 10:51 UTC (permalink / raw) To: Jeff Fan, edk2-devel; +Cc: Michael D Kinney, Paolo Bonzini, Jiewen Yao 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 <lersek@redhat.com> > Cc: Paolo Bonzini <pbonzini@redhat.com> > Cc: Jiewen Yao <jiewen.yao@intel.com> > Cc: Michael D Kinney <michael.d.kinney@intel.com> > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Jeff Fan <jeff.fan@intel.com> > --- > 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)), > + &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 <lersek@redhat.com> ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2] UefiCpuPkg/DxeMpLib: Allocate below 4GB mem for AsmRelocateApLoopFunc 2016-11-11 10:51 ` Laszlo Ersek @ 2016-11-11 12:08 ` Laszlo Ersek 2016-11-11 12:15 ` Fan, Jeff 0 siblings, 1 reply; 8+ messages in thread From: Laszlo Ersek @ 2016-11-11 12:08 UTC (permalink / raw) To: Jeff Fan, edk2-devel; +Cc: Michael D Kinney, Paolo Bonzini, Jiewen Yao 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 <lersek@redhat.com> >> Cc: Paolo Bonzini <pbonzini@redhat.com> >> Cc: Jiewen Yao <jiewen.yao@intel.com> >> Cc: Michael D Kinney <michael.d.kinney@intel.com> >> Contributed-under: TianoCore Contribution Agreement 1.0 >> Signed-off-by: Jeff Fan <jeff.fan@intel.com> >> --- >> 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 <lersek@redhat.com> > Tested-by: Laszlo Ersek <lersek@redhat.com> [lersek@redhat.com: strip whitespace at EOL] Signed-off-by: Laszlo Ersek <lersek@redhat.com> 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2] UefiCpuPkg/DxeMpLib: Allocate below 4GB mem for AsmRelocateApLoopFunc 2016-11-11 12:08 ` Laszlo Ersek @ 2016-11-11 12:15 ` Fan, Jeff 0 siblings, 0 replies; 8+ messages in thread From: Fan, Jeff @ 2016-11-11 12:15 UTC (permalink / raw) To: Laszlo Ersek, edk2-devel@ml01.01.org Cc: Kinney, Michael D, Paolo Bonzini, Yao, Jiewen Laszlo, Thanks your updating and pushing. Jeff -----Original Message----- From: Laszlo Ersek [mailto:lersek@redhat.com] Sent: Friday, November 11, 2016 8:08 PM To: Fan, Jeff; edk2-devel@ml01.01.org Cc: Kinney, Michael D; Paolo Bonzini; Yao, Jiewen Subject: Re: [edk2] [PATCH v2] UefiCpuPkg/DxeMpLib: Allocate below 4GB mem for AsmRelocateApLoopFunc 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 <lersek@redhat.com> >> Cc: Paolo Bonzini <pbonzini@redhat.com> >> Cc: Jiewen Yao <jiewen.yao@intel.com> >> Cc: Michael D Kinney <michael.d.kinney@intel.com> >> Contributed-under: TianoCore Contribution Agreement 1.0 >> Signed-off-by: Jeff Fan <jeff.fan@intel.com> >> --- >> 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 <lersek@redhat.com> > Tested-by: Laszlo Ersek <lersek@redhat.com> [lersek@redhat.com: strip whitespace at EOL] Signed-off-by: Laszlo Ersek <lersek@redhat.com> 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2] UefiCpuPkg/DxeMpLib: Allocate below 4GB mem for AsmRelocateApLoopFunc 2016-11-11 8:56 [PATCH v2] UefiCpuPkg/DxeMpLib: Allocate below 4GB mem for AsmRelocateApLoopFunc Jeff Fan 2016-11-11 10:51 ` Laszlo Ersek @ 2016-11-15 1:40 ` Laszlo Ersek 2016-11-15 1:42 ` Fan, Jeff 2016-11-15 1:46 ` Fan, Jeff 1 sibling, 2 replies; 8+ messages in thread From: Laszlo Ersek @ 2016-11-15 1:40 UTC (permalink / raw) To: Jeff Fan, edk2-devel; +Cc: Michael D Kinney, Paolo Bonzini, Jiewen Yao 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 <lersek@redhat.com> > Cc: Paolo Bonzini <pbonzini@redhat.com> > Cc: Jiewen Yao <jiewen.yao@intel.com> > Cc: Michael D Kinney <michael.d.kinney@intel.com> > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Jeff Fan <jeff.fan@intel.com> > --- > 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, > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2] UefiCpuPkg/DxeMpLib: Allocate below 4GB mem for AsmRelocateApLoopFunc 2016-11-15 1:40 ` Laszlo Ersek @ 2016-11-15 1:42 ` Fan, Jeff 2016-11-15 1:46 ` Fan, Jeff 1 sibling, 0 replies; 8+ messages in thread From: Fan, Jeff @ 2016-11-15 1:42 UTC (permalink / raw) To: Laszlo Ersek, edk2-devel@ml01.01.org Cc: Kinney, Michael D, Paolo Bonzini, Yao, Jiewen Laszlo, Good catch before I push this serial of patch. I will remove sizeof() when I push the patch. Thanks! Jeff -----Original Message----- From: Laszlo Ersek [mailto:lersek@redhat.com] Sent: Tuesday, November 15, 2016 9:40 AM To: Fan, Jeff; edk2-devel@ml01.01.org Cc: Kinney, Michael D; Paolo Bonzini; Yao, Jiewen Subject: Re: [edk2] [PATCH v2] UefiCpuPkg/DxeMpLib: Allocate below 4GB mem for AsmRelocateApLoopFunc 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 <lersek@redhat.com> > Cc: Paolo Bonzini <pbonzini@redhat.com> > Cc: Jiewen Yao <jiewen.yao@intel.com> > Cc: Michael D Kinney <michael.d.kinney@intel.com> > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Jeff Fan <jeff.fan@intel.com> > --- > 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, > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2] UefiCpuPkg/DxeMpLib: Allocate below 4GB mem for AsmRelocateApLoopFunc 2016-11-15 1:40 ` Laszlo Ersek 2016-11-15 1:42 ` Fan, Jeff @ 2016-11-15 1:46 ` Fan, Jeff 2016-11-15 1:50 ` Laszlo Ersek 1 sibling, 1 reply; 8+ messages in thread From: Fan, Jeff @ 2016-11-15 1:46 UTC (permalink / raw) To: Laszlo Ersek, edk2-devel@ml01.01.org Cc: Kinney, Michael D, Paolo Bonzini, Yao, Jiewen Laszlo, Oh. It's another committed patch, I will send another patch to fix it. Jeff -----Original Message----- From: Fan, Jeff Sent: Tuesday, November 15, 2016 9:43 AM To: Laszlo Ersek; edk2-devel@ml01.01.org Cc: Kinney, Michael D; Paolo Bonzini; Yao, Jiewen Subject: RE: [edk2] [PATCH v2] UefiCpuPkg/DxeMpLib: Allocate below 4GB mem for AsmRelocateApLoopFunc Laszlo, Good catch before I push this serial of patch. I will remove sizeof() when I push the patch. Thanks! Jeff -----Original Message----- From: Laszlo Ersek [mailto:lersek@redhat.com] Sent: Tuesday, November 15, 2016 9:40 AM To: Fan, Jeff; edk2-devel@ml01.01.org Cc: Kinney, Michael D; Paolo Bonzini; Yao, Jiewen Subject: Re: [edk2] [PATCH v2] UefiCpuPkg/DxeMpLib: Allocate below 4GB mem for AsmRelocateApLoopFunc 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 <lersek@redhat.com> > Cc: Paolo Bonzini <pbonzini@redhat.com> > Cc: Jiewen Yao <jiewen.yao@intel.com> > Cc: Michael D Kinney <michael.d.kinney@intel.com> > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Jeff Fan <jeff.fan@intel.com> > --- > 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, > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2] UefiCpuPkg/DxeMpLib: Allocate below 4GB mem for AsmRelocateApLoopFunc 2016-11-15 1:46 ` Fan, Jeff @ 2016-11-15 1:50 ` Laszlo Ersek 0 siblings, 0 replies; 8+ messages in thread From: Laszlo Ersek @ 2016-11-15 1:50 UTC (permalink / raw) To: Fan, Jeff, edk2-devel@ml01.01.org Cc: Kinney, Michael D, Paolo Bonzini, Yao, Jiewen On 11/15/16 02:46, Fan, Jeff wrote: > Laszlo, > > Oh. It's another committed patch, I will send another patch to fix it. Thanks! It's embarrassing that I didn't notice it :/ Laszlo ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2016-11-15 1:50 UTC | newest] Thread overview: 8+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2016-11-11 8:56 [PATCH v2] UefiCpuPkg/DxeMpLib: Allocate below 4GB mem for AsmRelocateApLoopFunc Jeff Fan 2016-11-11 10:51 ` Laszlo Ersek 2016-11-11 12:08 ` Laszlo Ersek 2016-11-11 12:15 ` Fan, Jeff 2016-11-15 1:40 ` Laszlo Ersek 2016-11-15 1:42 ` Fan, Jeff 2016-11-15 1:46 ` Fan, Jeff 2016-11-15 1:50 ` Laszlo Ersek
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox