Re: [PATCH v2 1/3] UefiCpuPkg/PiSmmCpuDxeSmm: handle dynamic PcdCpuMaxLogicalProcessorNumber

[Laszlo Ersek <lersek@redhat.com>]

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
>