From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=134.134.136.20; helo=mga02.intel.com; envelope-from=eric.dong@intel.com; receiver=edk2-devel@lists.01.org Received: from mga02.intel.com (mga02.intel.com [134.134.136.20]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id BEBE520968904 for ; Tue, 3 Jul 2018 19:33:51 -0700 (PDT) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga002.jf.intel.com ([10.7.209.21]) by orsmga101.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 03 Jul 2018 19:33:51 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.51,306,1526367600"; d="scan'208";a="72067856" Received: from fmsmsx104.amr.corp.intel.com ([10.18.124.202]) by orsmga002.jf.intel.com with ESMTP; 03 Jul 2018 19:33:27 -0700 Received: from fmsmsx115.amr.corp.intel.com (10.18.116.19) by fmsmsx104.amr.corp.intel.com (10.18.124.202) with Microsoft SMTP Server (TLS) id 14.3.319.2; Tue, 3 Jul 2018 19:33:15 -0700 Received: from shsmsx103.ccr.corp.intel.com (10.239.4.69) by fmsmsx115.amr.corp.intel.com (10.18.116.19) with Microsoft SMTP Server (TLS) id 14.3.319.2; Tue, 3 Jul 2018 19:33:15 -0700 Received: from shsmsx101.ccr.corp.intel.com ([169.254.1.57]) by SHSMSX103.ccr.corp.intel.com ([169.254.4.100]) with mapi id 14.03.0319.002; Wed, 4 Jul 2018 10:33:13 +0800 From: "Dong, Eric" To: "Ni, Ruiyu" , "edk2-devel@lists.01.org" CC: Jeff Fan , "Yao, Jiewen" , Fish Andrew , Laszlo Ersek Thread-Topic: [PATCH] UefiCpuPkg/MpInitLib: Avoid calling PEI services from AP Thread-Index: AQHUEcofWcC+qXSI0ECy+s0sW82o/aR+Ww7Q Date: Wed, 4 Jul 2018 02:33:13 +0000 Message-ID: References: <20180702060135.264676-1-ruiyu.ni@intel.com> In-Reply-To: <20180702060135.264676-1-ruiyu.ni@intel.com> Accept-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.239.127.40] MIME-Version: 1.0 Subject: Re: [PATCH] UefiCpuPkg/MpInitLib: Avoid calling PEI services from AP X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.26 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 04 Jul 2018 02:33:51 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Reviewed-by: Eric Dong > -----Original Message----- > From: Ni, Ruiyu > Sent: Monday, July 2, 2018 2:02 PM > To: edk2-devel@lists.01.org > Cc: Jeff Fan ; Dong, Eric ; > Yao, Jiewen ; Fish Andrew ; Laszlo > Ersek > Subject: [PATCH] UefiCpuPkg/MpInitLib: Avoid calling PEI services from AP >=20 > Today's MpInitLib PEI implementation directly calls > PeiServices->GetHobList() from AP which may cause racing issue. >=20 > 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. >=20 > 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. >=20 > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Ruiyu Ni > Cc: Jeff Fan > Cc: Eric Dong > Cc: Jiewen Yao > Cc: Fish Andrew > Cc: Laszlo Ersek > --- > UefiCpuPkg/Library/MpInitLib/MpLib.c | 59 > +++++++++++++++++++++++++++------ > UefiCpuPkg/Library/MpInitLib/PeiMpLib.c | 18 +++++++--- > 2 files changed, 63 insertions(+), 14 deletions(-) >=20 > 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].VolatileRegiste= rs > points to a different IDT shared by all APs. > // > RestoreVolatileRegisters (&CpuMpData->CpuData[0].VolatileRegisters= , > FALSE); > InitializeApData (CpuMpData, ProcessorNumber, BistData, ApTopOfSta= ck); > @@ -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; >=20 > OldCpuMpData =3D GetCpuMpDataFromGuidedHob (); > if (OldCpuMpData =3D=3D NULL) { > @@ -1530,19 +1534,48 @@ MpInitLibInitialize ( > ApStackSize =3D PcdGet32(PcdCpuApStackSize); > ApLoopMode =3D GetApLoopMode (&MonitorFilterSize); >=20 > + // > + // Save BSP's Control registers for APs > + // > + SaveVolatileRegisters (&VolatileRegisters); > + > BufferSize =3D ApStackSize * MaxLogicalProcessorNumber; > BufferSize +=3D MonitorFilterSize * MaxLogicalProcessorNumber; > - BufferSize +=3D sizeof (CPU_MP_DATA); > BufferSize +=3D ApResetVectorSize; > + BufferSize =3D ALIGN_VALUE (BufferSize, 8); > + BufferSize +=3D VolatileRegisters.Idtr.Limit + 1; > + BufferSize +=3D sizeof (CPU_MP_DATA); > BufferSize +=3D (sizeof (CPU_AP_DATA) + sizeof (CPU_INFO_IN_HOB))* > MaxLogicalProcessorNumber; > MpBuffer =3D AllocatePages (EFI_SIZE_TO_PAGES (BufferSize)); > ASSERT (MpBuffer !=3D NULL); > ZeroMem (MpBuffer, BufferSize); > Buffer =3D (UINTN) MpBuffer; >=20 > + // > + // The layout of the Buffer is as below: > + // > + // +--------------------+ <-- Buffer > + // AP Stacks (N) > + // +--------------------+ <-- MonitorBuffer > + // AP Monitor Filters (N) > + // +--------------------+ <-- BackupBufferAddr (CpuMpData->BackupBu= ffer) > + // 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 =3D (UINT8 *) (Buffer + ApStackSize * > MaxLogicalProcessorNumber); > BackupBufferAddr =3D (UINTN) MonitorBuffer + MonitorFilterSize * > MaxLogicalProcessorNumber; > - CpuMpData =3D (CPU_MP_DATA *) (BackupBufferAddr + ApResetVectorSize); > + ApIdtBase =3D ALIGN_VALUE (BackupBufferAddr + ApResetVectorSize= , 8); > + CpuMpData =3D (CPU_MP_DATA *) (ApIdtBase + > VolatileRegisters.Idtr.Limit + 1); > CpuMpData->Buffer =3D Buffer; > CpuMpData->CpuApStackSize =3D ApStackSize; > CpuMpData->BackupBuffer =3D BackupBufferAddr; > @@ -1557,10 +1590,20 @@ MpInitLibInitialize ( > CpuMpData->MicrocodePatchAddress =3D PcdGet64 > (PcdCpuMicrocodePatchAddress); > CpuMpData->MicrocodePatchRegionSize =3D 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) =3D=3D > + Buffer + BufferSize); > + > + // > + // Duplicate BSP's IDT to APs. > + // All APs share one separate IDT. So AP can get the address of CpuMpD= ata > 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 =3D ApIdtBase; > + CopyMem (&CpuMpData->CpuData[0].VolatileRegisters, > &VolatileRegisters, sizeof (VolatileRegisters)); > // > // Set BSP basic information > // > @@ -1618,11 +1661,7 @@ MpInitLibInitialize ( > } > CpuMpData->CpuData[Index].CpuHealthy =3D (CpuInfoInHob[Index].Heal= th > =3D=3D 0)? TRUE:FALSE; > CpuMpData->CpuData[Index].ApFunction =3D 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 ( >=20 > /** > Get pointer to CPU MP Data structure. > + For BSP, the pointer is retrieved from HOB. > + For AP, the structure is just after IDT. >=20 > @return The pointer to CPU MP Data structure. > **/ > @@ -35,10 +37,18 @@ GetCpuMpData ( > VOID > ) > { > - CPU_MP_DATA *CpuMpData; > - > - CpuMpData =3D GetCpuMpDataFromGuidedHob (); > - ASSERT (CpuMpData !=3D NULL); > + CPU_MP_DATA *CpuMpData; > + MSR_IA32_APIC_BASE_REGISTER ApicBaseMsr; > + IA32_DESCRIPTOR Idtr; > + > + ApicBaseMsr.Uint64 =3D AsmReadMsr64 (MSR_IA32_APIC_BASE); > + if (ApicBaseMsr.Bits.BSP =3D=3D 1) { > + CpuMpData =3D GetCpuMpDataFromGuidedHob (); > + ASSERT (CpuMpData !=3D NULL); > + } else { > + AsmReadIdtr (&Idtr); > + CpuMpData =3D (CPU_MP_DATA *) (Idtr.Base + Idtr.Limit + 1); > + } > return CpuMpData; > } >=20 > -- > 2.16.1.windows.1