Reviewed-by: Ray Ni <ray...@intel.com> > -----Original Message----- > From: Wu, Jiaxin <jiaxin...@intel.com> > Sent: Wednesday, November 30, 2022 1:14 PM > To: devel@edk2.groups.io > Cc: Dong, Eric <eric.d...@intel.com>; Ni, Ray <ray...@intel.com>; Zeng, Star > <star.z...@intel.com> > Subject: [PATCH v3] UefiCpuPkg: Check SMM Delayed/Blocked AP Count > > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4173 > > Due to more core count increasement, it's hard to reflect all APs > state via AP bitvector support in the register. Actually, SMM CPU > driver doesn't need to check each AP state to know all CPUs in SMI > or not, one alternative method is to check the SMM Delayed & Blocked > AP Count number: > > APs in SMI + Blocked Count + Disabled Count >= All supported Aps > (code comments explained why can be > All supported Aps) > > With above change, the returned value of "SmmRegSmmEnable" & > "SmmRegSmmDelayed" & "SmmRegSmmBlocked" from SmmCpuFeaturesLib > should be the AP count number within the existing CPU package. > > For register that return the bitvector state, require > SmmCpuFeaturesGetSmmRegister() returns count number of all bit per > logical processor within the same package. > > For register that return the AP count, require > SmmCpuFeaturesGetSmmRegister() returns the register value directly. > > v3: > - Refine the coding style > > v2: > - Rename "mPackageBspInfo" to "mPackageFirstThreadIndex" > - Clarify the expected value of "SmmRegSmmEnable" & > "SmmRegSmmDelayed" & > "SmmRegSmmBlocked" returned from SmmCpuFeaturesLib. > - Thread: https://edk2.groups.io/g/devel/message/96722 > > v1: > - Thread: https://edk2.groups.io/g/devel/message/96671 > > Cc: Eric Dong <eric.d...@intel.com> > Cc: Ray Ni <ray...@intel.com> > Cc: Zeng Star <star.z...@intel.com> > Signed-off-by: Jiaxin Wu <jiaxin...@intel.com> > --- > UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c | 195 > +++++++++++++++++++++++++---- > UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c | 5 + > UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h | 16 ++- > 3 files changed, 184 insertions(+), 32 deletions(-) > > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c > b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c > index c79da418e3..a0967eb69c 100644 > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c > @@ -22,10 +22,15 @@ UINTN mSemaphoreSize; > SPIN_LOCK *mPFLock = NULL; > SMM_CPU_SYNC_MODE mCpuSmmSyncMode; > BOOLEAN mMachineCheckSupported = FALSE; > MM_COMPLETION mSmmStartupThisApToken; > > +// > +// Processor specified by mPackageFirstThreadIndex[PackageIndex] will do > the package-scope register check. > +// > +UINT32 *mPackageFirstThreadIndex = NULL; > + > extern UINTN mSmmShadowStackSize; > > /** > Performs an atomic compare exchange operation to get semaphore. > The compare exchange operation must be performed using > @@ -155,54 +160,129 @@ ReleaseAllAPs ( > } > } > } > > /** > - Checks if all CPUs (with certain exceptions) have checked in for this SMI > run > + Check whether the index of CPU perform the package level register > + programming during System Management Mode initialization. > > - @param Exceptions CPU Arrival exception flags. > + The index of Processor specified by > mPackageFirstThreadIndex[PackageIndex] > + will do the package-scope register programming. > > - @retval TRUE if all CPUs the have checked in. > - @retval FALSE if at least one Normal AP hasn't checked in. > + @param[in] CpuIndex Processor Index. > + > + @retval TRUE Perform the package level register programming. > + @retval FALSE Don't perform the package level register programming. > > **/ > BOOLEAN > -AllCpusInSmmWithExceptions ( > - SMM_CPU_ARRIVAL_EXCEPTIONS Exceptions > +IsPackageFirstThread ( > + IN UINTN CpuIndex > ) > { > - UINTN Index; > - SMM_CPU_DATA_BLOCK *CpuData; > - EFI_PROCESSOR_INFORMATION *ProcessorInfo; > + UINT32 PackageIndex; > > - ASSERT (*mSmmMpSyncData->Counter <= mNumberOfCpus); > + PackageIndex = gSmmCpuPrivate- > >ProcessorInfo[CpuIndex].Location.Package; > > - if (*mSmmMpSyncData->Counter == mNumberOfCpus) { > - return TRUE; > + ASSERT (mPackageFirstThreadIndex != NULL); > + > + // > + // Set the value of mPackageFirstThreadIndex[PackageIndex]. > + // The package-scope register are checked by the first processor > (CpuIndex) in Package. > + // > + // If mPackageFirstThreadIndex[PackageIndex] equals to (UINT32)-1, then > update > + // to current CpuIndex. If it doesn't equal to (UINT32)-1, don't change it. > + // > + if (mPackageFirstThreadIndex[PackageIndex] == (UINT32)-1) { > + mPackageFirstThreadIndex[PackageIndex] = (UINT32)CpuIndex; > } > > - CpuData = mSmmMpSyncData->CpuData; > - ProcessorInfo = gSmmCpuPrivate->ProcessorInfo; > - for (Index = 0; Index < mMaxNumberOfCpus; Index++) { > - if (!(*(CpuData[Index].Present)) && (ProcessorInfo[Index].ProcessorId != > INVALID_APIC_ID)) { > - if (((Exceptions & ARRIVAL_EXCEPTION_DELAYED) != 0) && > (SmmCpuFeaturesGetSmmRegister (Index, SmmRegSmmDelayed) != 0)) { > - continue; > - } > + return (BOOLEAN)(mPackageFirstThreadIndex[PackageIndex] == > CpuIndex); > +} > + > +/** > + Returns the Number of SMM Delayed & Blocked & Disabled Thread Count. > + > + @param[in,out] DelayedCount The Number of SMM Delayed Thread > Count. > + @param[in,out] BlockedCount The Number of SMM Blocked Thread > Count. > + @param[in,out] DisabledCount The Number of SMM Disabled Thread > Count. > + > +**/ > +VOID > +GetSmmDelayedBlockedDisabledCount ( > + IN OUT UINT32 *DelayedCount, > + IN OUT UINT32 *BlockedCount, > + IN OUT UINT32 *DisabledCount > + ) > +{ > + UINTN Index; > > - if (((Exceptions & ARRIVAL_EXCEPTION_BLOCKED) != 0) && > (SmmCpuFeaturesGetSmmRegister (Index, SmmRegSmmBlocked) != 0)) { > - continue; > + for (Index = 0; Index < mNumberOfCpus; Index++) { > + if (IsPackageFirstThread (Index)) { > + if (DelayedCount != NULL) { > + *DelayedCount += (UINT32)SmmCpuFeaturesGetSmmRegister (Index, > SmmRegSmmDelayed); > } > > - if (((Exceptions & ARRIVAL_EXCEPTION_SMI_DISABLED) != 0) && > (SmmCpuFeaturesGetSmmRegister (Index, SmmRegSmmEnable) != 0)) { > - continue; > + if (BlockedCount != NULL) { > + *BlockedCount += (UINT32)SmmCpuFeaturesGetSmmRegister (Index, > SmmRegSmmBlocked); > } > > - return FALSE; > + if (DisabledCount != NULL) { > + *DisabledCount += (UINT32)SmmCpuFeaturesGetSmmRegister (Index, > SmmRegSmmEnable); > + } > } > } > +} > > - return TRUE; > +/** > + Checks if all CPUs (except Blocked & Disabled) have checked in for this SMI > run > + > + @retval TRUE if all CPUs the have checked in. > + @retval FALSE if at least one Normal AP hasn't checked in. > + > +**/ > +BOOLEAN > +AllCpusInSmmExceptBlockedDisabled ( > + VOID > + ) > +{ > + UINT32 BlockedCount; > + UINT32 DisabledCount; > + > + BlockedCount = 0; > + DisabledCount = 0; > + > + // > + // Check to make sure mSmmMpSyncData->Counter is valid and not > locked. > + // > + ASSERT (*mSmmMpSyncData->Counter <= mNumberOfCpus); > + > + // > + // Check whether all CPUs in SMM. > + // > + if (*mSmmMpSyncData->Counter == mNumberOfCpus) { > + return TRUE; > + } > + > + // > + // Check for the Blocked & Disabled Exceptions Case. > + // > + GetSmmDelayedBlockedDisabledCount (NULL, &BlockedCount, > &DisabledCount); > + > + // > + // *mSmmMpSyncData->Counter might be updated by all APs > concurrently. The value > + // can be dynamic changed. If some Aps enter the SMI after the > BlockedCount & > + // DisabledCount check, then the *mSmmMpSyncData->Counter will be > increased, thus > + // leading the *mSmmMpSyncData->Counter + BlockedCount + > DisabledCount > mNumberOfCpus. > + // since the BlockedCount & DisabledCount are local variable, it's ok here > only for > + // the checking of all CPUs In Smm. > + // > + if (*mSmmMpSyncData->Counter + BlockedCount + DisabledCount >= > mNumberOfCpus) { > + return TRUE; > + } > + > + return FALSE; > } > > /** > Has OS enabled Lmce in the MSR_IA32_MCG_EXT_CTL > > @@ -266,10 +346,15 @@ SmmWaitForApArrival ( > { > UINT64 Timer; > UINTN Index; > BOOLEAN LmceEn; > BOOLEAN LmceSignal; > + UINT32 DelayedCount; > + UINT32 BlockedCount; > + > + DelayedCount = 0; > + BlockedCount = 0; > > ASSERT (*mSmmMpSyncData->Counter <= mNumberOfCpus); > > LmceEn = FALSE; > LmceSignal = FALSE; > @@ -294,11 +379,11 @@ SmmWaitForApArrival ( > // > for (Timer = StartSyncTimer (); > !IsSyncTimerTimeout (Timer) && !(LmceEn && LmceSignal); > ) > { > - mSmmMpSyncData->AllApArrivedWithException = > AllCpusInSmmWithExceptions (ARRIVAL_EXCEPTION_BLOCKED | > ARRIVAL_EXCEPTION_SMI_DISABLED); > + mSmmMpSyncData->AllApArrivedWithException = > AllCpusInSmmExceptBlockedDisabled (); > if (mSmmMpSyncData->AllApArrivedWithException) { > break; > } > > CpuPause (); > @@ -335,19 +420,27 @@ SmmWaitForApArrival ( > // > for (Timer = StartSyncTimer (); > !IsSyncTimerTimeout (Timer); > ) > { > - mSmmMpSyncData->AllApArrivedWithException = > AllCpusInSmmWithExceptions (ARRIVAL_EXCEPTION_BLOCKED | > ARRIVAL_EXCEPTION_SMI_DISABLED); > + mSmmMpSyncData->AllApArrivedWithException = > AllCpusInSmmExceptBlockedDisabled (); > if (mSmmMpSyncData->AllApArrivedWithException) { > break; > } > > CpuPause (); > } > } > > + if (!mSmmMpSyncData->AllApArrivedWithException) { > + // > + // Check for the Blocked & Delayed Case. > + // > + GetSmmDelayedBlockedDisabledCount (&DelayedCount, &BlockedCount, > NULL); > + DEBUG ((DEBUG_INFO, "SmmWaitForApArrival: Delayed AP Count = %d, > Blocked AP Count = %d\n", DelayedCount, BlockedCount)); > + } > + > return; > } > > /** > Replace OS MTRR's with SMI MTRR's. > @@ -737,10 +830,11 @@ APHandler ( > // BSP timeout in the first round > // > if (mSmmMpSyncData->BspIndex != -1) { > // > // BSP Index is known > + // Existing AP is in SMI now but BSP not in, so, try bring BSP in SMM. > // > BspIndex = mSmmMpSyncData->BspIndex; > ASSERT (CpuIndex != BspIndex); > > // > @@ -761,16 +855,19 @@ APHandler ( > > if (!(*mSmmMpSyncData->InsideSmm)) { > // > // Give up since BSP is unable to enter SMM > // and signal the completion of this AP > + // Reduce the mSmmMpSyncData->Counter! > + // > WaitForSemaphore (mSmmMpSyncData->Counter); > return; > } > } else { > // > // Don't know BSP index. Give up without sending IPI to BSP. > + // Reduce the mSmmMpSyncData->Counter! > // > WaitForSemaphore (mSmmMpSyncData->Counter); > return; > } > } > @@ -1666,14 +1763,17 @@ SmiRendezvous ( > // > goto Exit; > } else { > // > // Signal presence of this processor > + // mSmmMpSyncData->Counter is increased here! > + // "ReleaseSemaphore (mSmmMpSyncData->Counter) == 0" means BSP > has already ended the synchronization. > // > if (ReleaseSemaphore (mSmmMpSyncData->Counter) == 0) { > // > // BSP has already ended the synchronization, so QUIT!!! > + // Existing AP is too late now to enter SMI since BSP has already ended > the synchronization!!! > // > > // > // Wait for BSP's signal to finish SMI > // > @@ -1781,10 +1881,51 @@ Exit: > // Restore Cr2 > // > RestoreCr2 (Cr2); > } > > +/** > + Initialize PackageBsp Info. Processor specified by > mPackageFirstThreadIndex[PackageIndex] > + will do the package-scope register programming. Set default CpuIndex to > (UINT32)-1, which > + means not specified yet. > + > +**/ > +VOID > +InitPackageFirstThreadIndexInfo ( > + VOID > + ) > +{ > + UINT32 Index; > + UINT32 PackageId; > + UINT32 PackageCount; > + > + PackageId = 0; > + PackageCount = 0; > + > + // > + // Count the number of package, set to max PackageId + 1 > + // > + for (Index = 0; Index < mNumberOfCpus; Index++) { > + if (PackageId < gSmmCpuPrivate->ProcessorInfo[Index].Location.Package) > { > + PackageId = gSmmCpuPrivate->ProcessorInfo[Index].Location.Package; > + } > + } > + > + PackageCount = PackageId + 1; > + > + mPackageFirstThreadIndex = (UINT32 *)AllocatePool (sizeof (UINT32) * > PackageCount); > + ASSERT (mPackageFirstThreadIndex != NULL); > + if (mPackageFirstThreadIndex == NULL) { > + return; > + } > + > + // > + // Set default CpuIndex to (UINT32)-1, which means not specified yet. > + // > + SetMem32 (mPackageFirstThreadIndex, sizeof (UINT32) * PackageCount, > (UINT32)-1); > +} > + > /** > Allocate buffer for SpinLock and Wrapper function buffer. > > **/ > VOID > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c > b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c > index 40aabeda72..37e3cfc449 100644 > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.c > @@ -1043,10 +1043,15 @@ PiCpuSmmEntry ( > // > // Initialize global buffer for MM MP. > // > InitializeDataForMmMp (); > > + // > + // Initialize Package First Thread Index Info. > + // > + InitPackageFirstThreadIndexInfo (); > + > // > // Install the SMM Mp Protocol into SMM protocol database > // > Status = gSmst->SmmInstallProtocolInterface ( > &mSmmCpuHandle, > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h > b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h > index ef8bf5947d..0bfba7e359 100644 > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h > @@ -192,15 +192,10 @@ typedef struct { > > #define EXCEPTION_VECTOR_NUMBER 0x20 > > #define INVALID_APIC_ID 0xFFFFFFFFFFFFFFFFULL > > -typedef UINT32 SMM_CPU_ARRIVAL_EXCEPTIONS; > -#define ARRIVAL_EXCEPTION_BLOCKED 0x1 > -#define ARRIVAL_EXCEPTION_DELAYED 0x2 > -#define ARRIVAL_EXCEPTION_SMI_DISABLED 0x4 > - > // > // Wrapper used to convert EFI_AP_PROCEDURE2 and EFI_AP_PROCEDURE. > // > typedef struct { > EFI_AP_PROCEDURE Procedure; > @@ -1460,10 +1455,21 @@ EFI_STATUS > RegisterStartupProcedure ( > IN EFI_AP_PROCEDURE Procedure, > IN OUT VOID *ProcedureArguments OPTIONAL > ); > > +/** > + Initialize PackageBsp Info. Processor specified by > mPackageFirstThreadIndex[PackageIndex] > + will do the package-scope register programming. Set default CpuIndex to > (UINT32)-1, which > + means not specified yet. > + > +**/ > +VOID > +InitPackageFirstThreadIndexInfo ( > + VOID > + ); > + > /** > Allocate buffer for SpinLock and Wrapper function buffer. > > **/ > VOID > -- > 2.16.2.windows.1
-=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#96957): https://edk2.groups.io/g/devel/message/96957 Mute This Topic: https://groups.io/mt/95352666/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
