* [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