Re: [PATCH] UefiCpuPkg/Library/MpInitLib: Remove BSP index == 0 Assumption.

["Laszlo Ersek" <lersek@redhat.com>]

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