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...@intel.com> > Cc: Laszlo Ersek <ler...@redhat.com> > Signed-off-by: Eric Dong <eric.d...@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/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
