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...@intel.com>
> Cc: Jeff Fan <vanjeff_...@hotmail.com>
> Cc: Eric Dong <eric.d...@intel.com>
> Cc: Jiewen Yao <jiewen....@intel.com>
> Cc: Fish Andrew <af...@apple.com>
> Cc: Laszlo Ersek <ler...@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 <ler...@redhat.com>
Tested-by: Laszlo Ersek <ler...@redhat.com>
(I performed my usual regression tests, with/without SMM, and S3.)
Thanks!
Laszlo
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
