From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=66.187.233.73; helo=mx1.redhat.com; envelope-from=lersek@redhat.com; receiver=edk2-devel@lists.01.org Received: from mx1.redhat.com (mx3-rdu2.redhat.com [66.187.233.73]) (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 6B088210C0CE0 for ; Mon, 2 Jul 2018 10:11:35 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.rdu2.redhat.com [10.11.54.4]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 9326B4022909; Mon, 2 Jul 2018 17:11:34 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-121-236.rdu2.redhat.com [10.10.121.236]) by smtp.corp.redhat.com (Postfix) with ESMTP id 194912026D5B; Mon, 2 Jul 2018 17:11:32 +0000 (UTC) To: Ruiyu Ni , edk2-devel@lists.01.org Cc: Jiewen Yao , Eric Dong , Fish Andrew References: <20180702060135.264676-1-ruiyu.ni@intel.com> From: Laszlo Ersek Message-ID: <71829607-e640-2c26-23bf-4a69d5af6de9@redhat.com> Date: Mon, 2 Jul 2018 19:11:31 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.7.0 MIME-Version: 1.0 In-Reply-To: <20180702060135.264676-1-ruiyu.ni@intel.com> X-Scanned-By: MIMEDefang 2.78 on 10.11.54.4 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.6]); Mon, 02 Jul 2018 17:11:34 +0000 (UTC) X-Greylist: inspected by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.6]); Mon, 02 Jul 2018 17:11:34 +0000 (UTC) for IP:'10.11.54.4' DOMAIN:'int-mx04.intmail.prod.int.rdu2.redhat.com' HELO:'smtp.corp.redhat.com' FROM:'lersek@redhat.com' RCPT:'' 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: Mon, 02 Jul 2018 17:11:36 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit 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 > 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(-) > > 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 Tested-by: Laszlo Ersek (I performed my usual regression tests, with/without SMM, and S3.) Thanks! Laszlo