REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2392
Current code implementation assumes BSP index is 0 at the begin.
This code change removes this assumption. It get BSP index from
the saved data structure if it existed.
Cc: Ray Ni <ray.ni@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Signed-off-by: Eric Dong <eric.dong@intel.com>
---
UefiCpuPkg/Library/MpInitLib/MpLib.c | 10 ++++++----
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c b/UefiCpuPkg/Library/MpInitLib/MpLib.c
index 6ec9b172b8..922c87b766 100644
--- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
+++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
@@ -636,7 +636,7 @@ ApWakeupFunction (
// to initialize AP in InitConfig path.
// NOTE: IDTR.BASE stored in CpuMpData->CpuData[0].VolatileRegisters points to a different IDT shared by all APs.
//
- RestoreVolatileRegisters (&CpuMpData->CpuData[0].VolatileRegisters, FALSE);
+ RestoreVolatileRegisters (&CpuMpData->CpuData[CpuMpData->BspNumber].VolatileRegisters, FALSE);
InitializeApData (CpuMpData, ProcessorNumber, BistData, ApTopOfStack);
ApStartupSignalBuffer = CpuMpData->CpuData[ProcessorNumber].StartupApSignal;
@@ -1615,6 +1615,7 @@ MpInitLibInitialize (
UINTN ApResetVectorSize;
UINTN BackupBufferAddr;
UINTN ApIdtBase;
+ UINT64 BspTopOfStack;
OldCpuMpData = GetCpuMpDataFromGuidedHob ();
if (OldCpuMpData == NULL) {
@@ -1677,7 +1678,7 @@ MpInitLibInitialize (
CpuMpData->BackupBufferSize = ApResetVectorSize;
CpuMpData->WakeupBuffer = (UINTN) -1;
CpuMpData->CpuCount = 1;
- CpuMpData->BspNumber = 0;
+ CpuMpData->BspNumber = OldCpuMpData != NULL ? OldCpuMpData->BspNumber : 0;
CpuMpData->WaitEvent = NULL;
CpuMpData->SwitchBspFlag = FALSE;
CpuMpData->CpuData = (CPU_AP_DATA *) (CpuMpData + 1);
@@ -1704,11 +1705,12 @@ MpInitLibInitialize (
// Don't pass BSP's TR to APs to avoid AP init failure.
//
VolatileRegisters.Tr = 0;
- CopyMem (&CpuMpData->CpuData[0].VolatileRegisters, &VolatileRegisters, sizeof (VolatileRegisters));
+ CopyMem (&CpuMpData->CpuData[CpuMpData->BspNumber].VolatileRegisters, &VolatileRegisters, sizeof (VolatileRegisters));
//
// Set BSP basic information
//
- InitializeApData (CpuMpData, 0, 0, CpuMpData->Buffer + ApStackSize);
+ BspTopOfStack = CpuMpData->Buffer + (CpuMpData->BspNumber + 1) * CpuMpData->CpuApStackSize;
+ InitializeApData (CpuMpData, CpuMpData->BspNumber, 0, BspTopOfStack);
//
// Save assembly code information
//
--
2.23.0.windows.1
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#53263): https://edk2.groups.io/g/devel/message/53263
Mute This Topic: https://groups.io/mt/69712223/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Eric,
What unit test was done for this patch?
> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Dong, Eric
> Sent: Wednesday, January 15, 2020 2:07 PM
> To: devel@edk2.groups.io
> Cc: Ni, Ray <ray.ni@intel.com>; Laszlo Ersek <lersek@redhat.com>
> Subject: [edk2-devel] [PATCH] UefiCpuPkg/Library/MpInitLib: Remove BSP index == 0 Assumption.
>
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2392
>
>
> Current code implementation assumes BSP index is 0 at the begin.
> This code change removes this assumption. It get BSP index from
> the saved data structure if it existed.
>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Signed-off-by: Eric Dong <eric.dong@intel.com>
> ---
> UefiCpuPkg/Library/MpInitLib/MpLib.c | 10 ++++++----
> 1 file changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> index 6ec9b172b8..922c87b766 100644
> --- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
> +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> @@ -636,7 +636,7 @@ ApWakeupFunction (
> // to initialize AP in InitConfig path.
>
> // NOTE: IDTR.BASE stored in CpuMpData->CpuData[0].VolatileRegisters points to a different IDT shared by all APs.
>
> //
>
> - RestoreVolatileRegisters (&CpuMpData->CpuData[0].VolatileRegisters, FALSE);
>
> + RestoreVolatileRegisters (&CpuMpData->CpuData[CpuMpData->BspNumber].VolatileRegisters, FALSE);
>
> InitializeApData (CpuMpData, ProcessorNumber, BistData, ApTopOfStack);
>
> ApStartupSignalBuffer = CpuMpData->CpuData[ProcessorNumber].StartupApSignal;
>
>
>
> @@ -1615,6 +1615,7 @@ MpInitLibInitialize (
> UINTN ApResetVectorSize;
>
> UINTN BackupBufferAddr;
>
> UINTN ApIdtBase;
>
> + UINT64 BspTopOfStack;
>
>
>
> OldCpuMpData = GetCpuMpDataFromGuidedHob ();
>
> if (OldCpuMpData == NULL) {
>
> @@ -1677,7 +1678,7 @@ MpInitLibInitialize (
> CpuMpData->BackupBufferSize = ApResetVectorSize;
>
> CpuMpData->WakeupBuffer = (UINTN) -1;
>
> CpuMpData->CpuCount = 1;
>
> - CpuMpData->BspNumber = 0;
>
> + CpuMpData->BspNumber = OldCpuMpData != NULL ? OldCpuMpData->BspNumber : 0;
>
> CpuMpData->WaitEvent = NULL;
>
> CpuMpData->SwitchBspFlag = FALSE;
>
> CpuMpData->CpuData = (CPU_AP_DATA *) (CpuMpData + 1);
>
> @@ -1704,11 +1705,12 @@ MpInitLibInitialize (
> // Don't pass BSP's TR to APs to avoid AP init failure.
>
> //
>
> VolatileRegisters.Tr = 0;
>
> - CopyMem (&CpuMpData->CpuData[0].VolatileRegisters, &VolatileRegisters, sizeof (VolatileRegisters));
>
> + CopyMem (&CpuMpData->CpuData[CpuMpData->BspNumber].VolatileRegisters, &VolatileRegisters, sizeof
> (VolatileRegisters));
>
> //
>
> // Set BSP basic information
>
> //
>
> - InitializeApData (CpuMpData, 0, 0, CpuMpData->Buffer + ApStackSize);
>
> + BspTopOfStack = CpuMpData->Buffer + (CpuMpData->BspNumber + 1) * CpuMpData->CpuApStackSize;
>
> + InitializeApData (CpuMpData, CpuMpData->BspNumber, 0, BspTopOfStack);
>
> //
>
> // Save assembly code information
>
> //
>
> --
> 2.23.0.windows.1
>
>
> -=-=-=-=-=-=
> Groups.io Links: You receive all messages sent to this group.
>
> View/Reply Online (#53263): https://edk2.groups.io/g/devel/message/53263
> Mute This Topic: https://groups.io/mt/69712223/1712937
> Group Owner: devel+owner@edk2.groups.io
> Unsubscribe: https://edk2.groups.io/g/devel/unsub [ray.ni@intel.com]
> -=-=-=-=-=-=
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#53264): https://edk2.groups.io/g/devel/message/53264
Mute This Topic: https://groups.io/mt/69712223/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Ray,
I applied this change to an internal desktop machine and did below tests:
1. boot it to shell
2. reboot the system to shell.
Thanks,
Eric
From: Ni, Ray
Sent: Wednesday, January 15, 2020 3:43 PM
To: devel@edk2.groups.io; Dong, Eric <eric.dong@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Subject: RE: [edk2-devel] [PATCH] UefiCpuPkg/Library/MpInitLib: Remove BSP index == 0 Assumption.
Eric,
What unit test was done for this patch?
> -----Original Message-----
> From: devel@edk2.groups.io<mailto:devel@edk2.groups.io> <devel@edk2.groups.io<mailto:devel@edk2.groups.io>> On Behalf Of Dong, Eric
> Sent: Wednesday, January 15, 2020 2:07 PM
> To: devel@edk2.groups.io<mailto:devel@edk2.groups.io>
> Cc: Ni, Ray <ray.ni@intel.com<mailto:ray.ni@intel.com>>; Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>>
> Subject: [edk2-devel] [PATCH] UefiCpuPkg/Library/MpInitLib: Remove BSP index == 0 Assumption.
>
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2392
>
>
> Current code implementation assumes BSP index is 0 at the begin.
> This code change removes this assumption. It get BSP index from
> the saved data structure if it existed.
>
> Cc: Ray Ni <ray.ni@intel.com<mailto:ray.ni@intel.com>>
> Cc: Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>>
> Signed-off-by: Eric Dong <eric.dong@intel.com<mailto:eric.dong@intel.com>>
> ---
> UefiCpuPkg/Library/MpInitLib/MpLib.c | 10 ++++++----
> 1 file changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> index 6ec9b172b8..922c87b766 100644
> --- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
> +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> @@ -636,7 +636,7 @@ ApWakeupFunction (
> // to initialize AP in InitConfig path.
>
> // NOTE: IDTR.BASE stored in CpuMpData->CpuData[0].VolatileRegisters points to a different IDT shared by all APs.
>
> //
>
> - RestoreVolatileRegisters (&CpuMpData->CpuData[0].VolatileRegisters, FALSE);
>
> + RestoreVolatileRegisters (&CpuMpData->CpuData[CpuMpData->BspNumber].VolatileRegisters, FALSE);
>
> InitializeApData (CpuMpData, ProcessorNumber, BistData, ApTopOfStack);
>
> ApStartupSignalBuffer = CpuMpData->CpuData[ProcessorNumber].StartupApSignal;
>
>
>
> @@ -1615,6 +1615,7 @@ MpInitLibInitialize (
> UINTN ApResetVectorSize;
>
> UINTN BackupBufferAddr;
>
> UINTN ApIdtBase;
>
> + UINT64 BspTopOfStack;
>
>
>
> OldCpuMpData = GetCpuMpDataFromGuidedHob ();
>
> if (OldCpuMpData == NULL) {
>
> @@ -1677,7 +1678,7 @@ MpInitLibInitialize (
> CpuMpData->BackupBufferSize = ApResetVectorSize;
>
> CpuMpData->WakeupBuffer = (UINTN) -1;
>
> CpuMpData->CpuCount = 1;
>
> - CpuMpData->BspNumber = 0;
>
> + CpuMpData->BspNumber = OldCpuMpData != NULL ? OldCpuMpData->BspNumber : 0;
>
> CpuMpData->WaitEvent = NULL;
>
> CpuMpData->SwitchBspFlag = FALSE;
>
> CpuMpData->CpuData = (CPU_AP_DATA *) (CpuMpData + 1);
>
> @@ -1704,11 +1705,12 @@ MpInitLibInitialize (
> // Don't pass BSP's TR to APs to avoid AP init failure.
>
> //
>
> VolatileRegisters.Tr = 0;
>
> - CopyMem (&CpuMpData->CpuData[0].VolatileRegisters, &VolatileRegisters, sizeof (VolatileRegisters));
>
> + CopyMem (&CpuMpData->CpuData[CpuMpData->BspNumber].VolatileRegisters, &VolatileRegisters, sizeof
> (VolatileRegisters));
>
> //
>
> // Set BSP basic information
>
> //
>
> - InitializeApData (CpuMpData, 0, 0, CpuMpData->Buffer + ApStackSize);
>
> + BspTopOfStack = CpuMpData->Buffer + (CpuMpData->BspNumber + 1) * CpuMpData->CpuApStackSize;
>
> + InitializeApData (CpuMpData, CpuMpData->BspNumber, 0, BspTopOfStack);
>
> //
>
> // Save assembly code information
>
> //
>
> --
> 2.23.0.windows.1
>
>
> -=-=-=-=-=-=
> Groups.io Links: You receive all messages sent to this group.
>
> View/Reply Online (#53263): https://edk2.groups.io/g/devel/message/53263
> Mute This Topic: https://groups.io/mt/69712223/1712937
> Group Owner: devel+owner@edk2.groups.io<mailto:devel+owner@edk2.groups.io>
> Unsubscribe: https://edk2.groups.io/g/devel/unsub [ray.ni@intel.com]
> -=-=-=-=-=-=
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#53265): https://edk2.groups.io/g/devel/message/53265
Mute This Topic: https://groups.io/mt/69712223/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
I recommend to switch BSP in PEI and see whether system is good in DXE.
From: Dong, Eric <eric.dong@intel.com>
Sent: Wednesday, January 15, 2020 3:53 PM
To: Ni, Ray <ray.ni@intel.com>; devel@edk2.groups.io
Cc: Laszlo Ersek <lersek@redhat.com>
Subject: RE: [edk2-devel] [PATCH] UefiCpuPkg/Library/MpInitLib: Remove BSP index == 0 Assumption.
Ray,
I applied this change to an internal desktop machine and did below tests:
1. boot it to shell
2. reboot the system to shell.
Thanks,
Eric
From: Ni, Ray
Sent: Wednesday, January 15, 2020 3:43 PM
To: devel@edk2.groups.io<mailto:devel@edk2.groups.io>; Dong, Eric <eric.dong@intel.com<mailto:eric.dong@intel.com>>
Cc: Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>>
Subject: RE: [edk2-devel] [PATCH] UefiCpuPkg/Library/MpInitLib: Remove BSP index == 0 Assumption.
Eric,
What unit test was done for this patch?
> -----Original Message-----
> From: devel@edk2.groups.io<mailto:devel@edk2.groups.io> <devel@edk2.groups.io<mailto:devel@edk2.groups.io>> On Behalf Of Dong, Eric
> Sent: Wednesday, January 15, 2020 2:07 PM
> To: devel@edk2.groups.io<mailto:devel@edk2.groups.io>
> Cc: Ni, Ray <ray.ni@intel.com<mailto:ray.ni@intel.com>>; Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>>
> Subject: [edk2-devel] [PATCH] UefiCpuPkg/Library/MpInitLib: Remove BSP index == 0 Assumption.
>
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2392
>
>
> Current code implementation assumes BSP index is 0 at the begin.
> This code change removes this assumption. It get BSP index from
> the saved data structure if it existed.
>
> Cc: Ray Ni <ray.ni@intel.com<mailto:ray.ni@intel.com>>
> Cc: Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>>
> Signed-off-by: Eric Dong <eric.dong@intel.com<mailto:eric.dong@intel.com>>
> ---
> UefiCpuPkg/Library/MpInitLib/MpLib.c | 10 ++++++----
> 1 file changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> index 6ec9b172b8..922c87b766 100644
> --- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
> +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> @@ -636,7 +636,7 @@ ApWakeupFunction (
> // to initialize AP in InitConfig path.
>
> // NOTE: IDTR.BASE stored in CpuMpData->CpuData[0].VolatileRegisters points to a different IDT shared by all APs.
>
> //
>
> - RestoreVolatileRegisters (&CpuMpData->CpuData[0].VolatileRegisters, FALSE);
>
> + RestoreVolatileRegisters (&CpuMpData->CpuData[CpuMpData->BspNumber].VolatileRegisters, FALSE);
>
> InitializeApData (CpuMpData, ProcessorNumber, BistData, ApTopOfStack);
>
> ApStartupSignalBuffer = CpuMpData->CpuData[ProcessorNumber].StartupApSignal;
>
>
>
> @@ -1615,6 +1615,7 @@ MpInitLibInitialize (
> UINTN ApResetVectorSize;
>
> UINTN BackupBufferAddr;
>
> UINTN ApIdtBase;
>
> + UINT64 BspTopOfStack;
>
>
>
> OldCpuMpData = GetCpuMpDataFromGuidedHob ();
>
> if (OldCpuMpData == NULL) {
>
> @@ -1677,7 +1678,7 @@ MpInitLibInitialize (
> CpuMpData->BackupBufferSize = ApResetVectorSize;
>
> CpuMpData->WakeupBuffer = (UINTN) -1;
>
> CpuMpData->CpuCount = 1;
>
> - CpuMpData->BspNumber = 0;
>
> + CpuMpData->BspNumber = OldCpuMpData != NULL ? OldCpuMpData->BspNumber : 0;
>
> CpuMpData->WaitEvent = NULL;
>
> CpuMpData->SwitchBspFlag = FALSE;
>
> CpuMpData->CpuData = (CPU_AP_DATA *) (CpuMpData + 1);
>
> @@ -1704,11 +1705,12 @@ MpInitLibInitialize (
> // Don't pass BSP's TR to APs to avoid AP init failure.
>
> //
>
> VolatileRegisters.Tr = 0;
>
> - CopyMem (&CpuMpData->CpuData[0].VolatileRegisters, &VolatileRegisters, sizeof (VolatileRegisters));
>
> + CopyMem (&CpuMpData->CpuData[CpuMpData->BspNumber].VolatileRegisters, &VolatileRegisters, sizeof
> (VolatileRegisters));
>
> //
>
> // Set BSP basic information
>
> //
>
> - InitializeApData (CpuMpData, 0, 0, CpuMpData->Buffer + ApStackSize);
>
> + BspTopOfStack = CpuMpData->Buffer + (CpuMpData->BspNumber + 1) * CpuMpData->CpuApStackSize;
>
> + InitializeApData (CpuMpData, CpuMpData->BspNumber, 0, BspTopOfStack);
>
> //
>
> // Save assembly code information
>
> //
>
> --
> 2.23.0.windows.1
>
>
> -=-=-=-=-=-=
> Groups.io Links: You receive all messages sent to this group.
>
> View/Reply Online (#53263): https://edk2.groups.io/g/devel/message/53263
> Mute This Topic: https://groups.io/mt/69712223/1712937
> Group Owner: devel+owner@edk2.groups.io<mailto:devel+owner@edk2.groups.io>
> Unsubscribe: https://edk2.groups.io/g/devel/unsub [ray.ni@intel.com]
> -=-=-=-=-=-=
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#53306): https://edk2.groups.io/g/devel/message/53306
Mute This Topic: https://groups.io/mt/69712223/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
On 01/15/20 07:06, Eric Dong wrote:
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2392
>
> Current code implementation assumes BSP index is 0 at the begin.
> This code change removes this assumption. It get BSP index from
> the saved data structure if it existed.
>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Signed-off-by: Eric Dong <eric.dong@intel.com>
> ---
> UefiCpuPkg/Library/MpInitLib/MpLib.c | 10 ++++++----
> 1 file changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> index 6ec9b172b8..922c87b766 100644
> --- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
> +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> @@ -636,7 +636,7 @@ ApWakeupFunction (
> // to initialize AP in InitConfig path.
> // NOTE: IDTR.BASE stored in CpuMpData->CpuData[0].VolatileRegisters points to a different IDT shared by all APs.
> //
> - RestoreVolatileRegisters (&CpuMpData->CpuData[0].VolatileRegisters, FALSE);
> + RestoreVolatileRegisters (&CpuMpData->CpuData[CpuMpData->BspNumber].VolatileRegisters, FALSE);
> InitializeApData (CpuMpData, ProcessorNumber, BistData, ApTopOfStack);
> ApStartupSignalBuffer = CpuMpData->CpuData[ProcessorNumber].StartupApSignal;
>
> @@ -1615,6 +1615,7 @@ MpInitLibInitialize (
> UINTN ApResetVectorSize;
> UINTN BackupBufferAddr;
> UINTN ApIdtBase;
> + UINT64 BspTopOfStack;
>
> OldCpuMpData = GetCpuMpDataFromGuidedHob ();
> if (OldCpuMpData == NULL) {
> @@ -1677,7 +1678,7 @@ MpInitLibInitialize (
> CpuMpData->BackupBufferSize = ApResetVectorSize;
> CpuMpData->WakeupBuffer = (UINTN) -1;
> CpuMpData->CpuCount = 1;
> - CpuMpData->BspNumber = 0;
> + CpuMpData->BspNumber = OldCpuMpData != NULL ? OldCpuMpData->BspNumber : 0;
> CpuMpData->WaitEvent = NULL;
> CpuMpData->SwitchBspFlag = FALSE;
> CpuMpData->CpuData = (CPU_AP_DATA *) (CpuMpData + 1);
> @@ -1704,11 +1705,12 @@ MpInitLibInitialize (
> // Don't pass BSP's TR to APs to avoid AP init failure.
> //
> VolatileRegisters.Tr = 0;
> - CopyMem (&CpuMpData->CpuData[0].VolatileRegisters, &VolatileRegisters, sizeof (VolatileRegisters));
> + CopyMem (&CpuMpData->CpuData[CpuMpData->BspNumber].VolatileRegisters, &VolatileRegisters, sizeof (VolatileRegisters));
> //
> // Set BSP basic information
> //
> - InitializeApData (CpuMpData, 0, 0, CpuMpData->Buffer + ApStackSize);
> + BspTopOfStack = CpuMpData->Buffer + (CpuMpData->BspNumber + 1) * CpuMpData->CpuApStackSize;
> + InitializeApData (CpuMpData, CpuMpData->BspNumber, 0, BspTopOfStack);
> //
> // Save assembly code information
> //
>
The patch seems reasonable to me (although I have not tried verifying
that all necessary spots are updated).
However, there is one thing I certainly don't understand, and the commit
message doesn't explain it. In the "BspTopOfStack" calculation, why do
we change the *second* factor, when we change the multiplication from:
(0 + 1) * ApStackSize
(where the (0 + 1) is implied in the old code), to:
(CpuMpData->BspNumber + 1) * CpuMpData->CpuApStackSize
?
I understand why the *first* factor is changed -- we basically replace
"0" with "CpuMpData->BspNumber" --; what I don't understand is why we
replace "ApStackSize" with "CpuMpData->CpuApStackSize", in the second
factor.
... Higher up in the code, we have:
CpuMpData->CpuApStackSize = ApStackSize;
so this part of the patch might actually have no effect. But, even then,
I think it makes the patch harder to understand. So in that case, I'd
suggest sticking with "ApStackSize", just for keeping the patch simpler.
Thanks
Laszlo
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#53267): https://edk2.groups.io/g/devel/message/53267
Mute This Topic: https://groups.io/mt/69712223/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
Hi Laszlo,
> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Wednesday, January 15, 2020 6:05 PM
> To: Dong, Eric <eric.dong@intel.com>; devel@edk2.groups.io
> Cc: Ni, Ray <ray.ni@intel.com>
> Subject: Re: [PATCH] UefiCpuPkg/Library/MpInitLib: Remove BSP index == 0
> Assumption.
>
> On 01/15/20 07:06, Eric Dong wrote:
> > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2392
> >
> > Current code implementation assumes BSP index is 0 at the begin.
> > This code change removes this assumption. It get BSP index from the
> > saved data structure if it existed.
> >
> > Cc: Ray Ni <ray.ni@intel.com>
> > Cc: Laszlo Ersek <lersek@redhat.com>
> > Signed-off-by: Eric Dong <eric.dong@intel.com>
> > ---
> > UefiCpuPkg/Library/MpInitLib/MpLib.c | 10 ++++++----
> > 1 file changed, 6 insertions(+), 4 deletions(-)
> >
> > diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c
> > b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> > index 6ec9b172b8..922c87b766 100644
> > --- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
> > +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> > @@ -636,7 +636,7 @@ ApWakeupFunction (
> > // to initialize AP in InitConfig path.
> > // NOTE: IDTR.BASE stored in CpuMpData-
> >CpuData[0].VolatileRegisters points to a different IDT shared by all APs.
> > //
> > - RestoreVolatileRegisters (&CpuMpData->CpuData[0].VolatileRegisters,
> FALSE);
> > + RestoreVolatileRegisters
> > + (&CpuMpData->CpuData[CpuMpData->BspNumber].VolatileRegisters,
> > + FALSE);
> > InitializeApData (CpuMpData, ProcessorNumber, BistData,
> ApTopOfStack);
> > ApStartupSignalBuffer =
> > CpuMpData->CpuData[ProcessorNumber].StartupApSignal;
> >
> > @@ -1615,6 +1615,7 @@ MpInitLibInitialize (
> > UINTN ApResetVectorSize;
> > UINTN BackupBufferAddr;
> > UINTN ApIdtBase;
> > + UINT64 BspTopOfStack;
> >
> > OldCpuMpData = GetCpuMpDataFromGuidedHob ();
> > if (OldCpuMpData == NULL) {
> > @@ -1677,7 +1678,7 @@ MpInitLibInitialize (
> > CpuMpData->BackupBufferSize = ApResetVectorSize;
> > CpuMpData->WakeupBuffer = (UINTN) -1;
> > CpuMpData->CpuCount = 1;
> > - CpuMpData->BspNumber = 0;
> > + CpuMpData->BspNumber = OldCpuMpData != NULL ?
> OldCpuMpData->BspNumber : 0;
> > CpuMpData->WaitEvent = NULL;
> > CpuMpData->SwitchBspFlag = FALSE;
> > CpuMpData->CpuData = (CPU_AP_DATA *) (CpuMpData + 1);
> > @@ -1704,11 +1705,12 @@ MpInitLibInitialize (
> > // Don't pass BSP's TR to APs to avoid AP init failure.
> > //
> > VolatileRegisters.Tr = 0;
> > - CopyMem (&CpuMpData->CpuData[0].VolatileRegisters,
> > &VolatileRegisters, sizeof (VolatileRegisters));
> > + CopyMem
> > + (&CpuMpData->CpuData[CpuMpData->BspNumber].VolatileRegisters,
> > + &VolatileRegisters, sizeof (VolatileRegisters));
> > //
> > // Set BSP basic information
> > //
> > - InitializeApData (CpuMpData, 0, 0, CpuMpData->Buffer +
> > ApStackSize);
> > + BspTopOfStack = CpuMpData->Buffer + (CpuMpData->BspNumber + 1)
> *
> > + CpuMpData->CpuApStackSize; InitializeApData (CpuMpData,
> > + CpuMpData->BspNumber, 0, BspTopOfStack);
> > //
> > // Save assembly code information
> > //
> >
>
> The patch seems reasonable to me (although I have not tried verifying that
> all necessary spots are updated).
>
> However, there is one thing I certainly don't understand, and the commit
> message doesn't explain it. In the "BspTopOfStack" calculation, why do we
> change the *second* factor, when we change the multiplication from:
>
> (0 + 1) * ApStackSize
>
> (where the (0 + 1) is implied in the old code), to:
>
> (CpuMpData->BspNumber + 1) * CpuMpData->CpuApStackSize
>
> ?
>
> I understand why the *first* factor is changed -- we basically replace "0" with
> "CpuMpData->BspNumber" --; what I don't understand is why we replace
> "ApStackSize" with "CpuMpData->CpuApStackSize", in the second factor.
>
> ... Higher up in the code, we have:
>
> CpuMpData->CpuApStackSize = ApStackSize;
>
> so this part of the patch might actually have no effect. But, even then, I think
> it makes the patch harder to understand. So in that case, I'd suggest sticking
> with "ApStackSize", just for keeping the patch simpler.
>
[[Eric]] driver has two places to call InitializeApData (). Here is one and the other in ApWakeupFunction().
InitializeApData (CpuMpData, ProcessorNumber, BistData, ApTopOfStack);
At that function, it calculates the ApTopOfStack like below:
ApTopOfStack = CpuMpData->Buffer + (ProcessorNumber + 1) * CpuMpData->CpuApStackSize;
I update new code to follow this coding style. I think after this change, the exit two code pieces are follow
the same coding style. So I think we can keep my original change.
Thanks,
Eric
> Thanks
> Laszlo
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#53291): https://edk2.groups.io/g/devel/message/53291
Mute This Topic: https://groups.io/mt/69712223/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
On 01/16/20 04:15, Dong, Eric wrote:
> Hi Laszlo,
>
>> -----Original Message-----
>> From: Laszlo Ersek [mailto:lersek@redhat.com]
>> Sent: Wednesday, January 15, 2020 6:05 PM
>> To: Dong, Eric <eric.dong@intel.com>; devel@edk2.groups.io
>> Cc: Ni, Ray <ray.ni@intel.com>
>> Subject: Re: [PATCH] UefiCpuPkg/Library/MpInitLib: Remove BSP index == 0
>> Assumption.
>>
>> On 01/15/20 07:06, Eric Dong wrote:
>>> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2392
>>>
>>> Current code implementation assumes BSP index is 0 at the begin.
>>> This code change removes this assumption. It get BSP index from the
>>> saved data structure if it existed.
>>>
>>> Cc: Ray Ni <ray.ni@intel.com>
>>> Cc: Laszlo Ersek <lersek@redhat.com>
>>> Signed-off-by: Eric Dong <eric.dong@intel.com>
>>> ---
>>> UefiCpuPkg/Library/MpInitLib/MpLib.c | 10 ++++++----
>>> 1 file changed, 6 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c
>>> b/UefiCpuPkg/Library/MpInitLib/MpLib.c
>>> index 6ec9b172b8..922c87b766 100644
>>> --- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
>>> +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
>>> @@ -636,7 +636,7 @@ ApWakeupFunction (
>>> // to initialize AP in InitConfig path.
>>> // NOTE: IDTR.BASE stored in CpuMpData-
>>> CpuData[0].VolatileRegisters points to a different IDT shared by all APs.
>>> //
>>> - RestoreVolatileRegisters (&CpuMpData->CpuData[0].VolatileRegisters,
>> FALSE);
>>> + RestoreVolatileRegisters
>>> + (&CpuMpData->CpuData[CpuMpData->BspNumber].VolatileRegisters,
>>> + FALSE);
>>> InitializeApData (CpuMpData, ProcessorNumber, BistData,
>> ApTopOfStack);
>>> ApStartupSignalBuffer =
>>> CpuMpData->CpuData[ProcessorNumber].StartupApSignal;
>>>
>>> @@ -1615,6 +1615,7 @@ MpInitLibInitialize (
>>> UINTN ApResetVectorSize;
>>> UINTN BackupBufferAddr;
>>> UINTN ApIdtBase;
>>> + UINT64 BspTopOfStack;
>>>
>>> OldCpuMpData = GetCpuMpDataFromGuidedHob ();
>>> if (OldCpuMpData == NULL) {
>>> @@ -1677,7 +1678,7 @@ MpInitLibInitialize (
>>> CpuMpData->BackupBufferSize = ApResetVectorSize;
>>> CpuMpData->WakeupBuffer = (UINTN) -1;
>>> CpuMpData->CpuCount = 1;
>>> - CpuMpData->BspNumber = 0;
>>> + CpuMpData->BspNumber = OldCpuMpData != NULL ?
>> OldCpuMpData->BspNumber : 0;
>>> CpuMpData->WaitEvent = NULL;
>>> CpuMpData->SwitchBspFlag = FALSE;
>>> CpuMpData->CpuData = (CPU_AP_DATA *) (CpuMpData + 1);
>>> @@ -1704,11 +1705,12 @@ MpInitLibInitialize (
>>> // Don't pass BSP's TR to APs to avoid AP init failure.
>>> //
>>> VolatileRegisters.Tr = 0;
>>> - CopyMem (&CpuMpData->CpuData[0].VolatileRegisters,
>>> &VolatileRegisters, sizeof (VolatileRegisters));
>>> + CopyMem
>>> + (&CpuMpData->CpuData[CpuMpData->BspNumber].VolatileRegisters,
>>> + &VolatileRegisters, sizeof (VolatileRegisters));
>>> //
>>> // Set BSP basic information
>>> //
>>> - InitializeApData (CpuMpData, 0, 0, CpuMpData->Buffer +
>>> ApStackSize);
>>> + BspTopOfStack = CpuMpData->Buffer + (CpuMpData->BspNumber + 1)
>> *
>>> + CpuMpData->CpuApStackSize; InitializeApData (CpuMpData,
>>> + CpuMpData->BspNumber, 0, BspTopOfStack);
>>> //
>>> // Save assembly code information
>>> //
>>>
>>
>> The patch seems reasonable to me (although I have not tried verifying that
>> all necessary spots are updated).
>>
>> However, there is one thing I certainly don't understand, and the commit
>> message doesn't explain it. In the "BspTopOfStack" calculation, why do we
>> change the *second* factor, when we change the multiplication from:
>>
>> (0 + 1) * ApStackSize
>>
>> (where the (0 + 1) is implied in the old code), to:
>>
>> (CpuMpData->BspNumber + 1) * CpuMpData->CpuApStackSize
>>
>> ?
>>
>> I understand why the *first* factor is changed -- we basically replace "0" with
>> "CpuMpData->BspNumber" --; what I don't understand is why we replace
>> "ApStackSize" with "CpuMpData->CpuApStackSize", in the second factor.
>>
>> ... Higher up in the code, we have:
>>
>> CpuMpData->CpuApStackSize = ApStackSize;
>>
>> so this part of the patch might actually have no effect. But, even then, I think
>> it makes the patch harder to understand. So in that case, I'd suggest sticking
>> with "ApStackSize", just for keeping the patch simpler.
>>
> [[Eric]] driver has two places to call InitializeApData (). Here is one and the other in ApWakeupFunction().
> InitializeApData (CpuMpData, ProcessorNumber, BistData, ApTopOfStack);
> At that function, it calculates the ApTopOfStack like below:
> ApTopOfStack = CpuMpData->Buffer + (ProcessorNumber + 1) * CpuMpData->CpuApStackSize;
>
> I update new code to follow this coding style. I think after this change, the exit two code pieces are follow
> the same coding style. So I think we can keep my original change.
That's fine, but then please include this specific argument in the
commit message.
Thanks,
Laszlo
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#53303): https://edk2.groups.io/g/devel/message/53303
Mute This Topic: https://groups.io/mt/69712223/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-
© 2016 - 2024 Red Hat, Inc.