Re: [PATCH v3] UefiCpuPkg/MpInitLib: Avoid calling PEI services from AP

["Ni, Ruiyu" <ruiyu.ni@Intel.com>]

On 6/30/2018 3:25 AM, Laszlo Ersek wrote:
> (1) I think that, right after the above line, you are missing:
> 
>    BufferSize  = ALIGN_VALUE (BufferSize, 8);
Yes. That's a bug in the patch. I will fix it in V4.

> 
> It is not noticed in practice because
> 
>    AllocatePages (EFI_SIZE_TO_PAGES (BufferSize))
> 
> rounds up the allocation anyway. However, the above ALIGN_VALUE() would
> be necessary to remain consistent with the ALIGN_VALUE() that is used
> below, in the assignment to "ApIdtBase".
> 
>> +  BufferSize += VolatileRegisters.Idtr.Limit + 1;
>> +  BufferSize += sizeof (CPU_MP_DATA);
>>     BufferSize += (sizeof (CPU_AP_DATA) + sizeof (CPU_INFO_IN_HOB))* MaxLogicalProcessorNumber;
>>     MpBuffer    = AllocatePages (EFI_SIZE_TO_PAGES (BufferSize));
>>     ASSERT (MpBuffer != NULL);
>>     ZeroMem (MpBuffer, BufferSize);
>>     Buffer = (UINTN) MpBuffer;
>>
>> +  /*
>> +    The layout of the Buffer is as below:
>> +
>> +      +--------------------+ <-- Buffer
>> +          AP Stacks (N)
>> +      +--------------------+ <-- MonitorBuffer
>> +      AP Monitor Filters (N)
>> +      +--------------------+ <-- BackupBuffer
>> +           Backup Buffer
>> +      +--------------------+ <-- ApIdtBase (8-byte boundary)
>> +             AP IDT          All APs share one separate IDT. So AP can get address of CPU_MP_DATA from IDT Base.
>> +      +--------------------+ <-- CpuMpData
>> +           CPU_MP_DATA
>> +      +--------------------+ <-- CpuMpData->CpuData
>> +          CPU_AP_DATA (N)
>> +      +--------------------+ <-- CpuMpData->CpuInfoHob
>> +        CPU_INFO_IN_HOB (N)
>> +      +--------------------+
>> +
>> +  */
> Some remarks for this comment block:
> 
> (2) We should likely use the "//" comment style.
OK.
> 
> (3) The "BackupBuffer" comment should be "CpuMpData->BackupBuffer". We
> don't have a "BackupBuffer" local variable, only "BackupBufferAddr".
OK.

> 
> (4) "CpuMpData->CpuInfoHob" is a typo; it should be
> "CpuMpData->CpuInfoInHob".
OK.
> 
>>     MonitorBuffer    = (UINT8 *) (Buffer + ApStackSize * MaxLogicalProcessorNumber);
>>     BackupBufferAddr = (UINTN) MonitorBuffer + MonitorFilterSize * MaxLogicalProcessorNumber;
>> -  CpuMpData = (CPU_MP_DATA *) (BackupBufferAddr + ApResetVectorSize);
>> +  ApIdtBase        = ALIGN_VALUE (BackupBufferAddr + ApResetVectorSize, 8);
>> +  CpuMpData        = (CPU_MP_DATA *) (ApIdtBase + VolatileRegisters.Idtr.Limit + 1);
>>     CpuMpData->Buffer           = Buffer;
>>     CpuMpData->CpuApStackSize   = ApStackSize;
>>     CpuMpData->BackupBuffer     = BackupBufferAddr;
>> @@ -1557,10 +1586,14 @@ MpInitLibInitialize (
>>     CpuMpData->MicrocodePatchAddress    = PcdGet64 (PcdCpuMicrocodePatchAddress);
>>     CpuMpData->MicrocodePatchRegionSize = PcdGet64 (PcdCpuMicrocodePatchRegionSize);
>>     InitializeSpinLock(&CpuMpData->MpLock);
>> +
> (5) At this point, I suggest a self-check, something like:
> 
>    ASSERT ((CpuMpData->CpuInfoInHob +
>             sizeof (CPU_INFO_IN_HOB) * MaxLogicalProcessorNumber) ==
>            Buffer + BufferSize);
> 
OK.
>>     //
>> -  // Save BSP's Control registers to APs
>> +  // Duplicate BSP's IDT to APs.
>> +  // All APs share one separate IDT. So AP can get the address of CpuMpData by using IDTR.BASE + IDTR.LIMIT + 1
>>     //
>> -  SaveVolatileRegisters (&CpuMpData->CpuData[0].VolatileRegisters);
>> +  CopyMem ((VOID *)ApIdtBase, (VOID *)VolatileRegisters.Idtr.Base, VolatileRegisters.Idtr.Limit + 1);
>> +  VolatileRegisters.Idtr.Base = ApIdtBase;
>> +  CopyMem (&CpuMpData->CpuData[0].VolatileRegisters, &VolatileRegisters, sizeof (VolatileRegisters));
>>     //
>>     // Set BSP basic information
>>     //
> This is my main concern.
> 
> The first CopyMem() correctly populates the IDT that is supposed to be
> shared by all the APs, from the BSP's IDT.
> 
> Then, we modify "VolatileRegisters" (which is never used in the function
> beyond the second CopyMem()), so that the IDTR base points to the APs'
> shared IDT. OK.
> 
> Then, with the second CopyMem(), we put the*modified*  VolatileRegisters
> into CpuData[0]. This replaces the original SaveVolatileRegisters()
> call, and as a result, the CpuData[0] IDTR base will now point to the
> APs' shared IDT, and not the BSPs own IDT.
> 
> (6) Is this not a problem for the BSP itself?
> 
> I see three explicit uses of CpuData[0] in the code, namely:
> 
> (6a) The one added above by the patch.
> 
> (6b) Lower down in the same MpInitLibInitialize() function:
> 
>        CopyMem (
>          &CpuMpData->CpuData[Index].VolatileRegisters,
>          &CpuMpData->CpuData[0].VolatileRegisters,
>          sizeof (CPU_VOLATILE_REGISTERS)
>          );
> 
> I think this is fine, for all (Index > 0) values. We could as well use
> the local "VolatileRegisters" here, as source argument.
OK.
> 
> (6c) And in ApWakeupFunction():
> 
>        //
>        // Sync BSP's Control registers to APs
>        //
>        RestoreVolatileRegisters (&CpuMpData->CpuData[0].VolatileRegisters, FALSE);
> 
> Minimally, we should update the comment to:
> 
>        //
>        // Sync BSP's Control registers to APs. Note that IDTR.BASE is different.
>        //
> 
> Which means that, from now on, we generally consider CpuData[0] to stand
> for the BSP,*except*  IDTR.BASE. (I guess we could redefine CpuData[0],
> not as "BSP", but as "initializer for APs
> 
> Thus, are we sure that the BSP never uses CpuData[0].IDTR.BASE for its
> own purposes?
> 
> Should we perhaps:
> - keep CpuData[0] unchanged,
> - introduce a new, dedicated field, "CpuMpData->ApIdtBase", and use that
>    *explicitly*  wherever necessary?
Then we will make the RestoreVolatileRegisters() a bit complex. It needs 
to explicitly restores the IDT.
I will add more comments for CpuData[0].VolatileRegisters.
> 
> 
> (7) My next question is a corollary of the above -- what about
> SwitchBSP()?
> 
> - Is SwitchBSP() compatible with this design?
Yes.
> - Will the new BSP start using the old BSP's IDT?
Yes.
> - More importantly, will the old BSP start using the IDT that is shared
>    by the APs?
Yes.
1). AsmExchangeRole () exchanges the IDT of BSP and AP.
2). After returning from AsmExchangeRole(), old BSP runs in
     ApWakeupFunction(). It saves IDTR value using below call:
SaveVolatileRegisters(&CpuMpData->CpuData[ProcessorNumber].VolatileRegisters);
     It guarantees next time when it's waken up, IDTR is restored using
     below call:
RestoreVolatileRegisters(&CpuMpData->CpuData[ProcessorNumber].VolatileRegisters, 
TRUE);
     ProcessorNumber is 0 after first SwitchBSP() call.
     Please don't worry about your (6c) restore operation. That only
     happens in InitConfig path.

> 
> Without these, I think GetCpuMpData() would break. The new BSP would go
> to the HOB, which is OK. However, the old BSP (= new AP) would go to the
> *original*  IDTR, and that's not usable for getting CpuMpData.
> 
> (I don't know how CpuData[0] relates to the*current*  -- possibly
> switched -- BSP.)
> 
> Thank you!
> Laszlo
> 


-- 
Thanks,
Ray