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

[Laszlo Ersek <lersek@redhat.com>]

On 07/02/18 08:01, Ruiyu Ni wrote:
> Today's MpInitLib PEI implementation directly calls
> PeiServices->GetHobList() from AP which may cause racing issue.
> 
> This patch fixes this issue by duplicating IDT for APs.
> Because CpuMpData structure is stored just after IDT, the CpuMPData
> address equals to IDTR.BASE + IDTR.LIMIT + 1.
> 
> v2:
>   1. Add ALIGN_VALUE() on BufferSize.
>   2. Add ASSERT() to make sure no memory usage outside of the allocated buffer.
>   3. Add more comments in InitConfig path when restoring CpuData[0].VolatileRegisters.
> 
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Ruiyu Ni <ruiyu.ni@intel.com>
> Cc: Jeff Fan <vanjeff_919@hotmail.com>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Jiewen Yao <jiewen.yao@intel.com>
> Cc: Fish Andrew <afish@apple.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> ---
>  UefiCpuPkg/Library/MpInitLib/MpLib.c    | 59 +++++++++++++++++++++++++++------
>  UefiCpuPkg/Library/MpInitLib/PeiMpLib.c | 18 +++++++---
>  2 files changed, 63 insertions(+), 14 deletions(-)
> 
> diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> index eb2765910c..108eea0a6f 100644
> --- a/UefiCpuPkg/Library/MpInitLib/MpLib.c
> +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c
> @@ -615,7 +615,9 @@ ApWakeupFunction (
>        //
>        ApInitializeSync (CpuMpData);
>        //
> -      // Sync BSP's Control registers to APs
> +      // CpuMpData->CpuData[0].VolatileRegisters is initialized based on BSP environment,
> +      //   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);
>        InitializeApData (CpuMpData, ProcessorNumber, BistData, ApTopOfStack);
> @@ -1506,6 +1508,7 @@ MpInitLibInitialize (
>    UINT32                   MaxLogicalProcessorNumber;
>    UINT32                   ApStackSize;
>    MP_ASSEMBLY_ADDRESS_MAP  AddressMap;
> +  CPU_VOLATILE_REGISTERS   VolatileRegisters;
>    UINTN                    BufferSize;
>    UINT32                   MonitorFilterSize;
>    VOID                     *MpBuffer;
> @@ -1516,6 +1519,7 @@ MpInitLibInitialize (
>    UINTN                    Index;
>    UINTN                    ApResetVectorSize;
>    UINTN                    BackupBufferAddr;
> +  UINTN                    ApIdtBase;
>  
>    OldCpuMpData = GetCpuMpDataFromGuidedHob ();
>    if (OldCpuMpData == NULL) {
> @@ -1530,19 +1534,48 @@ MpInitLibInitialize (
>    ApStackSize = PcdGet32(PcdCpuApStackSize);
>    ApLoopMode  = GetApLoopMode (&MonitorFilterSize);
>  
> +  //
> +  // Save BSP's Control registers for APs
> +  //
> +  SaveVolatileRegisters (&VolatileRegisters);
> +
>    BufferSize  = ApStackSize * MaxLogicalProcessorNumber;
>    BufferSize += MonitorFilterSize * MaxLogicalProcessorNumber;
> -  BufferSize += sizeof (CPU_MP_DATA);
>    BufferSize += ApResetVectorSize;
> +  BufferSize  = ALIGN_VALUE (BufferSize, 8);
> +  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)
> +  //    +--------------------+ <-- BackupBufferAddr (CpuMpData->BackupBuffer)
> +  //         Backup Buffer
> +  //    +--------------------+
> +  //           Padding
> +  //    +--------------------+ <-- 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->CpuInfoInHob
> +  //      CPU_INFO_IN_HOB (N)
> +  //    +--------------------+
> +  //
>    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 +1590,20 @@ MpInitLibInitialize (
>    CpuMpData->MicrocodePatchAddress    = PcdGet64 (PcdCpuMicrocodePatchAddress);
>    CpuMpData->MicrocodePatchRegionSize = PcdGet64 (PcdCpuMicrocodePatchRegionSize);
>    InitializeSpinLock(&CpuMpData->MpLock);
> +
> +  //
> +  // Make sure no memory usage outside of the allocated buffer.
>    //
> -  // Save BSP's Control registers to APs
> +  ASSERT ((CpuMpData->CpuInfoInHob + sizeof (CPU_INFO_IN_HOB) * MaxLogicalProcessorNumber) ==
> +          Buffer + BufferSize);
> +
> +  //
> +  // 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
>    //
> @@ -1618,11 +1661,7 @@ MpInitLibInitialize (
>        }
>        CpuMpData->CpuData[Index].CpuHealthy = (CpuInfoInHob[Index].Health == 0)? TRUE:FALSE;
>        CpuMpData->CpuData[Index].ApFunction = 0;
> -      CopyMem (
> -        &CpuMpData->CpuData[Index].VolatileRegisters,
> -        &CpuMpData->CpuData[0].VolatileRegisters,
> -        sizeof (CPU_VOLATILE_REGISTERS)
> -        );
> +      CopyMem (&CpuMpData->CpuData[Index].VolatileRegisters, &VolatileRegisters, sizeof (CPU_VOLATILE_REGISTERS));
>      }
>      if (MaxLogicalProcessorNumber > 1) {
>        //
> diff --git a/UefiCpuPkg/Library/MpInitLib/PeiMpLib.c b/UefiCpuPkg/Library/MpInitLib/PeiMpLib.c
> index 791ae9db6e..92f28681e4 100644
> --- a/UefiCpuPkg/Library/MpInitLib/PeiMpLib.c
> +++ b/UefiCpuPkg/Library/MpInitLib/PeiMpLib.c
> @@ -27,6 +27,8 @@ EnableDebugAgent (
>  
>  /**
>    Get pointer to CPU MP Data structure.
> +  For BSP, the pointer is retrieved from HOB.
> +  For AP, the structure is just after IDT.
>  
>    @return  The pointer to CPU MP Data structure.
>  **/
> @@ -35,10 +37,18 @@ GetCpuMpData (
>    VOID
>    )
>  {
> -  CPU_MP_DATA      *CpuMpData;
> -
> -  CpuMpData = GetCpuMpDataFromGuidedHob ();
> -  ASSERT (CpuMpData != NULL);
> +  CPU_MP_DATA                  *CpuMpData;
> +  MSR_IA32_APIC_BASE_REGISTER  ApicBaseMsr;
> +  IA32_DESCRIPTOR              Idtr;
> +
> +  ApicBaseMsr.Uint64 = AsmReadMsr64 (MSR_IA32_APIC_BASE);
> +  if (ApicBaseMsr.Bits.BSP == 1) {
> +    CpuMpData = GetCpuMpDataFromGuidedHob ();
> +    ASSERT (CpuMpData != NULL);
> +  } else {
> +    AsmReadIdtr (&Idtr);
> +    CpuMpData = (CPU_MP_DATA *) (Idtr.Base + Idtr.Limit + 1);
> +  }
>    return CpuMpData;
>  }
>  
> 

Acked-by: Laszlo Ersek <lersek@redhat.com>
Tested-by: Laszlo Ersek <lersek@redhat.com>

(I performed my usual regression tests, with/without SMM, and S3.)

Thanks!
Laszlo