| Message ID | 20161124205652.12113-2-lersek@redhat.com |
|---|---|
| State | Accepted |
| Commit | bb767506b265b1cdbf0dbec58c9a3f4c6be6ab2b |
| Headers | show |
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 <jeff.fan@intel.com> 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 <imammedo@redhat.com> > Cc: Jeff Fan <jeff.fan@intel.com> > Cc: Jordan Justen <jordan.l.justen@intel.com> > Cc: Michael Kinney <michael.d.kinney@intel.com> > Bugzilla: https://bugzilla.tianocore.org/show_bug.cgi?id=116 > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Laszlo Ersek <lersek@redhat.com> > --- > 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 > _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
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);