[edk2-devel] [PATCH] UefiCpuPkg/Library/MpInitLib: Remove BSP index == 0 Assumption.
Dong, Eric
eric.dong at intel.com
Thu Jan 16 03:15:42 UTC 2020
Hi Laszlo,
> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek at redhat.com]
> Sent: Wednesday, January 15, 2020 6:05 PM
> To: Dong, Eric <eric.dong at intel.com>; devel at edk2.groups.io
> Cc: Ni, Ray <ray.ni at 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 at intel.com>
> > Cc: Laszlo Ersek <lersek at redhat.com>
> > Signed-off-by: Eric Dong <eric.dong at 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/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list