From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) (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 2289381D5C for ; Mon, 28 Nov 2016 03:23:42 -0800 (PST) Received: from int-mx13.intmail.prod.int.phx2.redhat.com (int-mx13.intmail.prod.int.phx2.redhat.com [10.5.11.26]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 533F8883A4; Mon, 28 Nov 2016 11:23:41 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-107.phx2.redhat.com [10.3.116.107]) by int-mx13.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id uASBNd8M021010; Mon, 28 Nov 2016 06:23:40 -0500 To: "Fan, Jeff" , edk2-devel-01 References: <20161124205652.12113-1-lersek@redhat.com> <20161124205652.12113-2-lersek@redhat.com> <542CF652F8836A4AB8DBFAAD40ED192A4A2E97A3@shsmsx102.ccr.corp.intel.com> Cc: "Kinney, Michael D" , "Justen, Jordan L" From: Laszlo Ersek Message-ID: Date: Mon, 28 Nov 2016 12:23:39 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.5.0 MIME-Version: 1.0 In-Reply-To: <542CF652F8836A4AB8DBFAAD40ED192A4A2E97A3@shsmsx102.ccr.corp.intel.com> X-Scanned-By: MIMEDefang 2.68 on 10.5.11.26 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.26]); Mon, 28 Nov 2016 11:23:41 +0000 (UTC) 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 11:23:42 -0000 Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit On 11/28/16 03:15, Fan, Jeff wrote: > Laszlo, > > Even getting PcdCpuMaxLogicalProcessorNumber is legal in InitSmmProfileInternal() that is only invoked on boot time, I agree to do this updating in this patch. > We need to try to use mMaxNumberOfCpus as possible for size saving and performance improvement. > > Reviewed-by: Jeff Fan Thanks, I committed this patch (for PiSmmCpuDxeSmm) as bb767506b265. I'll submit a new version for the rest. Cheers Laszlo > -----Original Message----- > From: Laszlo Ersek [mailto:lersek@redhat.com] > 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 PcdCpuMaxLogicalProcessorNumber > > "UefiCpuPkg/UefiCpuPkg.dec" already allows platforms to make PcdCpuMaxLogicalProcessorNumber dynamic, however PiSmmCpuDxeSmm does not take this into account 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 permitted at boot time, not at runtime or during S3 resume. > > We already have a variable called mMaxNumberOfCpus; it is initialized in the entry point function like this: > >> // >> // If support CPU hot plug, we need to allocate resources for possibly >> // hot-added processors // if (FeaturePcdGet (PcdCpuHotPlugSupport)) { >> mMaxNumberOfCpus = PcdGet32 (PcdCpuMaxLogicalProcessorNumber); >> } else { >> mMaxNumberOfCpus = 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 >> // number of enabled processors // Status = >> MpServices->GetNumberOfProcessors (MpServices, &mNumberOfCpus, >> &NumberOfEnabledProcessors); ASSERT_EFI_ERROR >> (Status); ASSERT (mNumberOfCpus <= PcdGet32 >> (PcdCpuMaxLogicalProcessorNumber)); > > Preserve these calls in the entry point function, and replace all other uses of PcdCpuMaxLogicalProcessorNumber -- there are only reads -- with mMaxNumberOfCpus. > > For PcdCpuHotPlugSupport==TRUE, this is an unobservable change. > > For PcdCpuHotPlugSupport==FALSE, we even save SMRAM, because we no longer allocate resources needlessly for CPUs that can never appear in the system. > > PcdCpuMaxLogicalProcessorNumber is also retrieved in "UefiCpuPkg/Library/SmmCpuFeaturesLib/SmmCpuFeaturesLib.c", but only in the library instance constructor, 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=116 > 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/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c > index 4baba1e9f8dc..f957de1f4764 100644 > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c > @@ -344,7 +344,7 @@ SmmInitHandler ( > AsmWriteIdtr (&gcSmiIdtr); > ApicId = GetApicId (); > > - ASSERT (mNumberOfCpus <= PcdGet32 (PcdCpuMaxLogicalProcessorNumber)); > + ASSERT (mNumberOfCpus <= mMaxNumberOfCpus); > > for (Index = 0; Index < mNumberOfCpus; Index++) { > if (ApicId == (UINT32)gSmmCpuPrivate->ProcessorInfo[Index].ProcessorId) { diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.c b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.c > index c1a48d100e0f..f53819ee24c2 100644 > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.c > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/SmmProfile.c > @@ -139,7 +139,7 @@ GetCpuIndex ( > > ApicId = GetApicId (); > > - for (Index = 0; Index < PcdGet32 (PcdCpuMaxLogicalProcessorNumber); Index++) { > + for (Index = 0; Index < mMaxNumberOfCpus; Index++) { > if (gSmmCpuPrivate->ProcessorInfo[Index].ProcessorId == ApicId) { > return Index; > } > @@ -825,13 +825,13 @@ InitSmmProfileInternal ( > UINTN MsrDsAreaSizePerCpu; > UINTN TotalSize; > > - mPFEntryCount = (UINTN *)AllocateZeroPool (sizeof (UINTN) * PcdGet32 (PcdCpuMaxLogicalProcessorNumber)); > + mPFEntryCount = (UINTN *)AllocateZeroPool (sizeof (UINTN) * > + mMaxNumberOfCpus); > ASSERT (mPFEntryCount != NULL); > mLastPFEntryValue = (UINT64 (*)[MAX_PF_ENTRY_COUNT])AllocateZeroPool ( > - sizeof (mLastPFEntryValue[0]) * PcdGet32 (PcdCpuMaxLogicalProcessorNumber)); > + sizeof > + (mLastPFEntryValue[0]) * mMaxNumberOfCpus); > ASSERT (mLastPFEntryValue != NULL); > mLastPFEntryPointer = (UINT64 *(*)[MAX_PF_ENTRY_COUNT])AllocateZeroPool ( > - sizeof (mLastPFEntryPointer[0]) * PcdGet32 (PcdCpuMaxLogicalProcessorNumber)); > + sizeof > + (mLastPFEntryPointer[0]) * mMaxNumberOfCpus); > ASSERT (mLastPFEntryPointer != NULL); > > // > @@ -872,17 +872,17 @@ InitSmmProfileInternal ( > mSmmProfileBase->NumCpus = gSmmCpuPrivate->SmmCoreEntryContext.NumberOfCpus; > > if (mBtsSupported) { > - mMsrDsArea = (MSR_DS_AREA_STRUCT **)AllocateZeroPool (sizeof (MSR_DS_AREA_STRUCT *) * PcdGet32 (PcdCpuMaxLogicalProcessorNumber)); > + mMsrDsArea = (MSR_DS_AREA_STRUCT **)AllocateZeroPool (sizeof > + (MSR_DS_AREA_STRUCT *) * mMaxNumberOfCpus); > ASSERT (mMsrDsArea != NULL); > - mMsrBTSRecord = (BRANCH_TRACE_RECORD **)AllocateZeroPool (sizeof (BRANCH_TRACE_RECORD *) * PcdGet32 (PcdCpuMaxLogicalProcessorNumber)); > + mMsrBTSRecord = (BRANCH_TRACE_RECORD **)AllocateZeroPool (sizeof > + (BRANCH_TRACE_RECORD *) * mMaxNumberOfCpus); > ASSERT (mMsrBTSRecord != NULL); > - mMsrPEBSRecord = (PEBS_RECORD **)AllocateZeroPool (sizeof (PEBS_RECORD *) * PcdGet32 (PcdCpuMaxLogicalProcessorNumber)); > + mMsrPEBSRecord = (PEBS_RECORD **)AllocateZeroPool (sizeof > + (PEBS_RECORD *) * mMaxNumberOfCpus); > ASSERT (mMsrPEBSRecord != NULL); > > mMsrDsAreaBase = (MSR_DS_AREA_STRUCT *)((UINTN)Base + mSmmProfileSize); > - MsrDsAreaSizePerCpu = mMsrDsAreaSize / PcdGet32 (PcdCpuMaxLogicalProcessorNumber); > + MsrDsAreaSizePerCpu = mMsrDsAreaSize / mMaxNumberOfCpus; > mBTSRecordNumber = (MsrDsAreaSizePerCpu - sizeof(PEBS_RECORD) * PEBS_RECORD_NUMBER - sizeof(MSR_DS_AREA_STRUCT)) / sizeof(BRANCH_TRACE_RECORD); > - for (Index = 0; Index < PcdGet32 (PcdCpuMaxLogicalProcessorNumber); Index++) { > + for (Index = 0; Index < mMaxNumberOfCpus; Index++) { > mMsrDsArea[Index] = (MSR_DS_AREA_STRUCT *)((UINTN)mMsrDsAreaBase + MsrDsAreaSizePerCpu * Index); > mMsrBTSRecord[Index] = (BRANCH_TRACE_RECORD *)((UINTN)mMsrDsArea[Index] + sizeof(MSR_DS_AREA_STRUCT)); > mMsrPEBSRecord[Index] = (PEBS_RECORD *)((UINTN)mMsrDsArea[Index] + MsrDsAreaSizePerCpu - sizeof(PEBS_RECORD) * PEBS_RECORD_NUMBER); > -- > 2.9.2 > > > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.01.org > https://lists.01.org/mailman/listinfo/edk2-devel >