From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga14.intel.com (mga14.intel.com [192.55.52.115]) (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 A277A81E1B for ; Sun, 27 Nov 2016 18:15:07 -0800 (PST) Received: from fmsmga005.fm.intel.com ([10.253.24.32]) by fmsmga103.fm.intel.com with ESMTP; 27 Nov 2016 18:15:07 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.31,561,1473145200"; d="scan'208";a="36219362" Received: from fmsmsx104.amr.corp.intel.com ([10.18.124.202]) by fmsmga005.fm.intel.com with ESMTP; 27 Nov 2016 18:15:07 -0800 Received: from fmsmsx153.amr.corp.intel.com (10.18.125.6) by fmsmsx104.amr.corp.intel.com (10.18.124.202) with Microsoft SMTP Server (TLS) id 14.3.248.2; Sun, 27 Nov 2016 18:15:07 -0800 Received: from shsmsx101.ccr.corp.intel.com (10.239.4.153) by FMSMSX153.amr.corp.intel.com (10.18.125.6) with Microsoft SMTP Server (TLS) id 14.3.248.2; Sun, 27 Nov 2016 18:15:06 -0800 Received: from shsmsx102.ccr.corp.intel.com ([169.254.2.239]) by SHSMSX101.ccr.corp.intel.com ([169.254.1.239]) with mapi id 14.03.0248.002; Mon, 28 Nov 2016 10:15:04 +0800 From: "Fan, Jeff" To: Laszlo Ersek , edk2-devel-01 CC: Igor Mammedov , "Justen, Jordan L" , "Kinney, Michael D" Thread-Topic: [PATCH v2 1/3] UefiCpuPkg/PiSmmCpuDxeSmm: handle dynamic PcdCpuMaxLogicalProcessorNumber Thread-Index: AQHSRwpAFEM2Jx+Kkk2Zg0dyxjRDIKDtnIyw Date: Mon, 28 Nov 2016 02:15:04 +0000 Message-ID: <542CF652F8836A4AB8DBFAAD40ED192A4A2E97A3@shsmsx102.ccr.corp.intel.com> References: <20161124205652.12113-1-lersek@redhat.com> <20161124205652.12113-2-lersek@redhat.com> In-Reply-To: <20161124205652.12113-2-lersek@redhat.com> Accept-Language: zh-CN, en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiNTkyNjMxMGQtN2NiZS00NzQ1LWEwM2QtODQ4YmNkZDRlMWU0IiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX0lDIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE1LjkuNi42IiwiVHJ1c3RlZExhYmVsSGFzaCI6InFnbklLSDE3dlBadUQzYU1PR244V0lxZVRpbE9VZzhvNEVQWWtBd0JkRkk9In0= x-ctpclassification: CTP_IC x-originating-ip: [10.239.127.40] MIME-Version: 1.0 Subject: Re: [PATCH v2 1/3] UefiCpuPkg/PiSmmCpuDxeSmm: handle dynamic PcdCpuMaxLogicalProcessorNumber X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 28 Nov 2016 02:15:07 -0000 Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Laszlo, Even getting PcdCpuMaxLogicalProcessorNumber is legal in InitSmmProfileInte= rnal() that is only invoked on boot time, I agree to do this updating in th= is patch. We need to try to use mMaxNumberOfCpus as possible for size saving and perf= ormance improvement. Reviewed-by: Jeff Fan Jeff -----Original Message----- From: Laszlo Ersek [mailto:lersek@redhat.com]=20 Sent: Friday, November 25, 2016 4:57 AM To: edk2-devel-01 Cc: Igor Mammedov; Fan, Jeff; Justen, Jordan L; Kinney, Michael D Subject: [PATCH v2 1/3] UefiCpuPkg/PiSmmCpuDxeSmm: handle dynamic PcdCpuMax= LogicalProcessorNumber "UefiCpuPkg/UefiCpuPkg.dec" already allows platforms to make PcdCpuMaxLogic= alProcessorNumber dynamic, however PiSmmCpuDxeSmm does not take this into a= ccount everywhere. As soon as a platform turns the PCD into a dynamic one, = at least S3 fails. When the PCD is dynamic, all PcdGet() calls translate into PCD DXE protocol calls, which are only permit= ted at boot time, not at runtime or during S3 resume. We already have a variable called mMaxNumberOfCpus; it is initialized in th= e entry point function like this: > // > // If support CPU hot plug, we need to allocate resources for possibly=20 > // hot-added processors // if (FeaturePcdGet (PcdCpuHotPlugSupport)) { > mMaxNumberOfCpus =3D PcdGet32 (PcdCpuMaxLogicalProcessorNumber); > } else { > mMaxNumberOfCpus =3D mNumberOfCpus; > } There's another use of the PCD a bit higher up, also in the entry point function: > // > // Use MP Services Protocol to retrieve the number of processors and=20 > // number of enabled processors // Status =3D=20 > MpServices->GetNumberOfProcessors (MpServices, &mNumberOfCpus, > &NumberOfEnabledProcessors); ASSERT_EFI_ERROR=20 > (Status); ASSERT (mNumberOfCpus <=3D PcdGet32=20 > (PcdCpuMaxLogicalProcessorNumber)); Preserve these calls in the entry point function, and replace all other use= s of PcdCpuMaxLogicalProcessorNumber -- there are only reads -- with mMaxNu= mberOfCpus. For PcdCpuHotPlugSupport=3D=3DTRUE, this is an unobservable change. For PcdCpuHotPlugSupport=3D=3DFALSE, we even save SMRAM, because we no long= er allocate resources needlessly for CPUs that can never appear in the syst= em. PcdCpuMaxLogicalProcessorNumber is also retrieved in "UefiCpuPkg/Library/Sm= mCpuFeaturesLib/SmmCpuFeaturesLib.c", but only in the library instance con= structor, which runs even before the entry point function is called. Cc: Igor Mammedov Cc: Jeff Fan Cc: Jordan Justen Cc: Michael Kinney Bugzilla: https://bugzilla.tianocore.org/show_bug.cgi?id=3D116 Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Laszlo Ersek --- UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c | 2 +- UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.c | 18 +++++++++--------- 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c b/UefiCpuPkg/PiSmmC= puDxeSmm/PiSmmCpuDxeSmm.c index 4baba1e9f8dc..f957de1f4764 100644 --- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c @@ -344,7 +344,7 @@ SmmInitHandler ( AsmWriteIdtr (&gcSmiIdtr); ApicId =3D GetApicId (); =20 - ASSERT (mNumberOfCpus <=3D PcdGet32 (PcdCpuMaxLogicalProcessorNumber)); + ASSERT (mNumberOfCpus <=3D mMaxNumberOfCpus); =20 for (Index =3D 0; Index < mNumberOfCpus; Index++) { if (ApicId =3D=3D (UINT32)gSmmCpuPrivate->ProcessorInfo[Index].Process= orId) { diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.c b/UefiCpuPkg/Pi= SmmCpuDxeSmm/SmmProfile.c index c1a48d100e0f..f53819ee24c2 100644 --- a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.c +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.c @@ -139,7 +139,7 @@ GetCpuIndex ( =20 ApicId =3D GetApicId (); =20 - for (Index =3D 0; Index < PcdGet32 (PcdCpuMaxLogicalProcessorNumber); In= dex++) { + for (Index =3D 0; Index < mMaxNumberOfCpus; Index++) { if (gSmmCpuPrivate->ProcessorInfo[Index].ProcessorId =3D=3D ApicId) { return Index; } @@ -825,13 +825,13 @@ InitSmmProfileInternal ( UINTN MsrDsAreaSizePerCpu; UINTN TotalSize; =20 - mPFEntryCount =3D (UINTN *)AllocateZeroPool (sizeof (UINTN) * PcdGet32 (= PcdCpuMaxLogicalProcessorNumber)); + mPFEntryCount =3D (UINTN *)AllocateZeroPool (sizeof (UINTN) *=20 + mMaxNumberOfCpus); ASSERT (mPFEntryCount !=3D NULL); mLastPFEntryValue =3D (UINT64 (*)[MAX_PF_ENTRY_COUNT])AllocateZeroPool = ( - sizeof (mLastPFEn= tryValue[0]) * PcdGet32 (PcdCpuMaxLogicalProcessorNumber)); + sizeof=20 + (mLastPFEntryValue[0]) * mMaxNumberOfCpus); ASSERT (mLastPFEntryValue !=3D NULL); mLastPFEntryPointer =3D (UINT64 *(*)[MAX_PF_ENTRY_COUNT])AllocateZeroPoo= l ( - sizeof (mLastPF= EntryPointer[0]) * PcdGet32 (PcdCpuMaxLogicalProcessorNumber)); + sizeof=20 + (mLastPFEntryPointer[0]) * mMaxNumberOfCpus); ASSERT (mLastPFEntryPointer !=3D NULL); =20 // @@ -872,17 +872,17 @@ InitSmmProfileInternal ( mSmmProfileBase->NumCpus =3D gSmmCpuPrivate->SmmCoreEntryContext.= NumberOfCpus; =20 if (mBtsSupported) { - mMsrDsArea =3D (MSR_DS_AREA_STRUCT **)AllocateZeroPool (sizeof (MSR_DS= _AREA_STRUCT *) * PcdGet32 (PcdCpuMaxLogicalProcessorNumber)); + mMsrDsArea =3D (MSR_DS_AREA_STRUCT **)AllocateZeroPool (sizeof=20 + (MSR_DS_AREA_STRUCT *) * mMaxNumberOfCpus); ASSERT (mMsrDsArea !=3D NULL); - mMsrBTSRecord =3D (BRANCH_TRACE_RECORD **)AllocateZeroPool (sizeof (BR= ANCH_TRACE_RECORD *) * PcdGet32 (PcdCpuMaxLogicalProcessorNumber)); + mMsrBTSRecord =3D (BRANCH_TRACE_RECORD **)AllocateZeroPool (sizeof=20 + (BRANCH_TRACE_RECORD *) * mMaxNumberOfCpus); ASSERT (mMsrBTSRecord !=3D NULL); - mMsrPEBSRecord =3D (PEBS_RECORD **)AllocateZeroPool (sizeof (PEBS_RECO= RD *) * PcdGet32 (PcdCpuMaxLogicalProcessorNumber)); + mMsrPEBSRecord =3D (PEBS_RECORD **)AllocateZeroPool (sizeof=20 + (PEBS_RECORD *) * mMaxNumberOfCpus); ASSERT (mMsrPEBSRecord !=3D NULL); =20 mMsrDsAreaBase =3D (MSR_DS_AREA_STRUCT *)((UINTN)Base + mSmmProfileSi= ze); - MsrDsAreaSizePerCpu =3D mMsrDsAreaSize / PcdGet32 (PcdCpuMaxLogicalPro= cessorNumber); + MsrDsAreaSizePerCpu =3D mMsrDsAreaSize / mMaxNumberOfCpus; mBTSRecordNumber =3D (MsrDsAreaSizePerCpu - sizeof(PEBS_RECORD) * P= EBS_RECORD_NUMBER - sizeof(MSR_DS_AREA_STRUCT)) / sizeof(BRANCH_TRACE_RECOR= D); - for (Index =3D 0; Index < PcdGet32 (PcdCpuMaxLogicalProcessorNumber); = Index++) { + for (Index =3D 0; Index < mMaxNumberOfCpus; Index++) { mMsrDsArea[Index] =3D (MSR_DS_AREA_STRUCT *)((UINTN)mMsrDsAreaBa= se + MsrDsAreaSizePerCpu * Index); mMsrBTSRecord[Index] =3D (BRANCH_TRACE_RECORD *)((UINTN)mMsrDsArea[= Index] + sizeof(MSR_DS_AREA_STRUCT)); mMsrPEBSRecord[Index] =3D (PEBS_RECORD *)((UINTN)mMsrDsArea[Index] += MsrDsAreaSizePerCpu - sizeof(PEBS_RECORD) * PEBS_RECORD_NUMBER); -- 2.9.2