* [PATCH] UefiCpuPkg/Library/MpInitLib: Remove BSP index == 0 Assumption. @ 2020-01-15 6:06 Dong, Eric 2020-01-15 7:43 ` [edk2-devel] " Ni, Ray 2020-01-15 10:04 ` Laszlo Ersek 0 siblings, 2 replies; 7+ messages in thread From: Dong, Eric @ 2020-01-15 6:06 UTC (permalink / raw) To: devel; +Cc: Ray Ni, Laszlo Ersek 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [edk2-devel] [PATCH] UefiCpuPkg/Library/MpInitLib: Remove BSP index == 0 Assumption. 2020-01-15 6:06 [PATCH] UefiCpuPkg/Library/MpInitLib: Remove BSP index == 0 Assumption Dong, Eric @ 2020-01-15 7:43 ` Ni, Ray 2020-01-15 7:52 ` Dong, Eric 2020-01-15 10:04 ` Laszlo Ersek 1 sibling, 1 reply; 7+ messages in thread From: Ni, Ray @ 2020-01-15 7:43 UTC (permalink / raw) To: devel@edk2.groups.io, Dong, Eric; +Cc: Laszlo Ersek 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] > -=-=-=-=-=-= ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [edk2-devel] [PATCH] UefiCpuPkg/Library/MpInitLib: Remove BSP index == 0 Assumption. 2020-01-15 7:43 ` [edk2-devel] " Ni, Ray @ 2020-01-15 7:52 ` Dong, Eric 2020-01-16 12:23 ` Ni, Ray 0 siblings, 1 reply; 7+ messages in thread From: Dong, Eric @ 2020-01-15 7:52 UTC (permalink / raw) To: Ni, Ray, devel@edk2.groups.io; +Cc: Laszlo Ersek [-- Attachment #1: Type: text/plain, Size: 4412 bytes --] 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] > -=-=-=-=-=-= [-- Attachment #2: Type: text/html, Size: 18649 bytes --] ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [edk2-devel] [PATCH] UefiCpuPkg/Library/MpInitLib: Remove BSP index == 0 Assumption. 2020-01-15 7:52 ` Dong, Eric @ 2020-01-16 12:23 ` Ni, Ray 0 siblings, 0 replies; 7+ messages in thread From: Ni, Ray @ 2020-01-16 12:23 UTC (permalink / raw) To: Dong, Eric, devel@edk2.groups.io; +Cc: Laszlo Ersek [-- Attachment #1: Type: text/plain, Size: 4838 bytes --] 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] > -=-=-=-=-=-= [-- Attachment #2: Type: text/html, Size: 19733 bytes --] ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] UefiCpuPkg/Library/MpInitLib: Remove BSP index == 0 Assumption. 2020-01-15 6:06 [PATCH] UefiCpuPkg/Library/MpInitLib: Remove BSP index == 0 Assumption Dong, Eric 2020-01-15 7:43 ` [edk2-devel] " Ni, Ray @ 2020-01-15 10:04 ` Laszlo Ersek 2020-01-16 3:15 ` Dong, Eric 1 sibling, 1 reply; 7+ messages in thread From: Laszlo Ersek @ 2020-01-15 10:04 UTC (permalink / raw) To: Eric Dong, devel; +Cc: Ray Ni 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] UefiCpuPkg/Library/MpInitLib: Remove BSP index == 0 Assumption. 2020-01-15 10:04 ` Laszlo Ersek @ 2020-01-16 3:15 ` Dong, Eric 2020-01-16 8:35 ` Laszlo Ersek 0 siblings, 1 reply; 7+ messages in thread From: Dong, Eric @ 2020-01-16 3:15 UTC (permalink / raw) To: Laszlo Ersek, devel@edk2.groups.io; +Cc: Ni, Ray 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] UefiCpuPkg/Library/MpInitLib: Remove BSP index == 0 Assumption. 2020-01-16 3:15 ` Dong, Eric @ 2020-01-16 8:35 ` Laszlo Ersek 0 siblings, 0 replies; 7+ messages in thread From: Laszlo Ersek @ 2020-01-16 8:35 UTC (permalink / raw) To: Dong, Eric, devel@edk2.groups.io; +Cc: Ni, Ray 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2020-01-16 12:23 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2020-01-15 6:06 [PATCH] UefiCpuPkg/Library/MpInitLib: Remove BSP index == 0 Assumption Dong, Eric 2020-01-15 7:43 ` [edk2-devel] " Ni, Ray 2020-01-15 7:52 ` Dong, Eric 2020-01-16 12:23 ` Ni, Ray 2020-01-15 10:04 ` Laszlo Ersek 2020-01-16 3:15 ` Dong, Eric 2020-01-16 8:35 ` Laszlo Ersek
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox