Reviewed-by: Ray Ni <ray...@intel.com> > -----Original Message----- > From: Dong, Eric <eric.d...@intel.com> > Sent: Friday, December 27, 2019 3:49 PM > To: devel@edk2.groups.io > Cc: Ni, Ray <ray...@intel.com>; Laszlo Ersek <ler...@redhat.com> > Subject: [PATCH v2] UefiCpuPkg/PiSmmCpuDxeSmm: Pre-allocate > PROCEDURE_TOKEN buffer > > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=2388 > > Token is new introduced by MM MP Protocol. Current logic allocate Token > every time when need to use it. The logic caused SMI latency raised to > very high. Update logic to allocate Token buffer at driver's entry point. > Later use the token from the allocated token buffer. Only when all the > buffer have been used, then need to allocate new buffer. > > Former change (9caaa79dd7e078ebb4012dde3b3d3a5d451df609) missed > PROCEDURE_TOKEN part, this change covers it. > > Cc: Ray Ni <ray...@intel.com> > Cc: Laszlo Ersek <ler...@redhat.com> > Signed-off-by: Eric Dong <eric.d...@intel.com> > --- > > v2 changes: > > Remove the not used variable. > > > UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c | 190 ++++++++++++------- > -- > UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h | 6 +- > 2 files changed, 109 insertions(+), 87 deletions(-) > > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c > b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c > index 4808045f71..870250b0c5 100644 > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c > @@ -429,38 +429,29 @@ ReleaseToken ( > > > **/ > > VOID > > -FreeTokens ( > > +ResetTokens ( > > VOID > > ) > > { > > LIST_ENTRY *Link; > > PROCEDURE_TOKEN *ProcToken; > > - TOKEN_BUFFER *TokenBuf; > > > > - // > > - // Only free the token buffer recorded in the OldTOkenBufList > > - // upon exiting SMI. Current token buffer stays allocated so > > - // next SMI doesn't need to re-allocate. > > - // > > - gSmmCpuPrivate->UsedTokenNum = 0; > > - > > - Link = GetFirstNode (&gSmmCpuPrivate->OldTokenBufList); > > - while (!IsNull (&gSmmCpuPrivate->OldTokenBufList, Link)) { > > - TokenBuf = TOKEN_BUFFER_FROM_LINK (Link); > > - > > - Link = RemoveEntryList (&TokenBuf->Link); > > - > > - FreePool (TokenBuf->Buffer); > > - FreePool (TokenBuf); > > - } > > - > > - while (!IsListEmpty (&gSmmCpuPrivate->TokenList)) { > > - Link = GetFirstNode (&gSmmCpuPrivate->TokenList); > > + Link = GetFirstNode (&gSmmCpuPrivate->TokenList); > > + while (!IsNull (&gSmmCpuPrivate->TokenList, Link)) { > > ProcToken = PROCEDURE_TOKEN_FROM_LINK (Link); > > > > - RemoveEntryList (&ProcToken->Link); > > + ProcToken->RunningApCount = 0; > > + ProcToken->Used = FALSE; > > + > > + // > > + // Check the spinlock status and release it if not released yet. > > + // > > + if (!AcquireSpinLockOrFail(ProcToken->SpinLock)) { > > + DEBUG((DEBUG_ERROR, "Risk::SpinLock still not released!")); > > + } > > + ReleaseSpinLock (ProcToken->SpinLock); > > > > - FreePool (ProcToken); > > + Link = GetNextNode (&gSmmCpuPrivate->TokenList, Link); > > } > > } > > > > @@ -685,9 +676,9 @@ BSPHandler ( > WaitForAllAPs (ApCount); > > > > // > > - // Clean the tokens buffer. > > + // Reset the tokens buffer. > > // > > - FreeTokens (); > > + ResetTokens (); > > > > // > > // Reset BspIndex to -1, meaning BSP has not been elected. > > @@ -1056,7 +1047,7 @@ IsTokenInUse ( > while (!IsNull (&gSmmCpuPrivate->TokenList, Link)) { > > ProcToken = PROCEDURE_TOKEN_FROM_LINK (Link); > > > > - if (ProcToken->SpinLock == Token) { > > + if (ProcToken->Used && ProcToken->SpinLock == Token) { > > return TRUE; > > } > > > > @@ -1067,61 +1058,112 @@ IsTokenInUse ( > } > > > > /** > > - create token and save it to the maintain list. > > - > > - @param RunningApCount Input running AP count. > > - > > - @retval return the spin lock used as token. > > + Allocate buffer for the SPIN_LOCK and PROCEDURE_TOKEN. > > > > **/ > > -PROCEDURE_TOKEN * > > -CreateToken ( > > - IN UINT32 RunningApCount > > +VOID > > +AllocateTokenBuffer ( > > + VOID > > ) > > { > > - PROCEDURE_TOKEN *ProcToken; > > - SPIN_LOCK *SpinLock; > > UINTN SpinLockSize; > > - TOKEN_BUFFER *TokenBuf; > > UINT32 TokenCountPerChunk; > > + UINTN ProcTokenSize; > > + UINTN Index; > > + PROCEDURE_TOKEN *ProcToken; > > + SPIN_LOCK *SpinLock; > > + UINT8 *SpinLockBuffer; > > + UINT8 *ProcTokenBuffer; > > > > SpinLockSize = GetSpinLockProperties (); > > + ProcTokenSize = sizeof (PROCEDURE_TOKEN); > > + > > TokenCountPerChunk = FixedPcdGet32 > (PcdCpuSmmMpTokenCountPerChunk); > > + ASSERT (TokenCountPerChunk != 0); > > + if (TokenCountPerChunk == 0) { > > + DEBUG ((DEBUG_ERROR, "PcdCpuSmmMpTokenCountPerChunk should > not be Zero!\n")); > > + CpuDeadLoop (); > > + } > > + DEBUG ((DEBUG_INFO, "CpuSmm: SpinLock Size = 0x%x, > PcdCpuSmmMpTokenCountPerChunk = 0x%x\n", SpinLockSize, > TokenCountPerChunk)); > > > > - if (gSmmCpuPrivate->UsedTokenNum == TokenCountPerChunk) { > > - DEBUG ((DEBUG_VERBOSE, "CpuSmm: No free token buffer, allocate new > buffer!\n")); > > + // > > + // Separate the Spin_lock and Proc_token because the alignment requires > by Spin_Lock. > > + // > > + SpinLockBuffer = AllocatePool (SpinLockSize * TokenCountPerChunk); > > + ASSERT (SpinLockBuffer != NULL); > > > > - // > > - // Record current token buffer for later free action usage. > > - // Current used token buffer not in this list. > > - // > > - TokenBuf = AllocatePool (sizeof (TOKEN_BUFFER)); > > - ASSERT (TokenBuf != NULL); > > - TokenBuf->Signature = TOKEN_BUFFER_SIGNATURE; > > - TokenBuf->Buffer = gSmmCpuPrivate->CurrentTokenBuf; > > + ProcTokenBuffer = AllocatePool (ProcTokenSize * TokenCountPerChunk); > > + ASSERT (ProcTokenBuffer != NULL); > > + > > + for (Index = 0; Index < TokenCountPerChunk; Index++) { > > + SpinLock = (SPIN_LOCK *)(SpinLockBuffer + SpinLockSize * Index); > > + InitializeSpinLock (SpinLock); > > + > > + ProcToken = (PROCEDURE_TOKEN *)(ProcTokenBuffer + ProcTokenSize * > Index); > > + ProcToken->Signature = PROCEDURE_TOKEN_SIGNATURE; > > + ProcToken->SpinLock = SpinLock; > > + ProcToken->Used = FALSE; > > + ProcToken->RunningApCount = 0; > > + > > + InsertTailList (&gSmmCpuPrivate->TokenList, &ProcToken->Link); > > + } > > +} > > > > - InsertTailList (&gSmmCpuPrivate->OldTokenBufList, &TokenBuf->Link); > > +/** > > + Find first free token in the allocated token list. > > + > > + @retval return the first free PROCEDURE_TOKEN. > > + > > +**/ > > +PROCEDURE_TOKEN * > > +FindFirstFreeToken ( > > + VOID > > + ) > > +{ > > + LIST_ENTRY *Link; > > + PROCEDURE_TOKEN *ProcToken; > > > > - gSmmCpuPrivate->CurrentTokenBuf = AllocatePool (SpinLockSize * > TokenCountPerChunk); > > - ASSERT (gSmmCpuPrivate->CurrentTokenBuf != NULL); > > - gSmmCpuPrivate->UsedTokenNum = 0; > > + Link = GetFirstNode (&gSmmCpuPrivate->TokenList); > > + while (!IsNull (&gSmmCpuPrivate->TokenList, Link)) { > > + ProcToken = PROCEDURE_TOKEN_FROM_LINK (Link); > > + > > + if (!ProcToken->Used) { > > + return ProcToken; > > + } > > + > > + Link = GetNextNode (&gSmmCpuPrivate->TokenList, Link); > > } > > > > - SpinLock = (SPIN_LOCK *)(gSmmCpuPrivate->CurrentTokenBuf + > SpinLockSize * gSmmCpuPrivate->UsedTokenNum); > > - gSmmCpuPrivate->UsedTokenNum++; > > + return NULL; > > +} > > + > > +/** > > + Get the free token. > > + > > + If no free token, allocate new tokens then return the free one. > > + > > + @retval return the first free PROCEDURE_TOKEN. > > > > - InitializeSpinLock (SpinLock); > > - AcquireSpinLock (SpinLock); > > +**/ > > +PROCEDURE_TOKEN * > > +GetFreeToken ( > > + IN UINT32 RunningApsCount > > + ) > > +{ > > + PROCEDURE_TOKEN *NewToken; > > > > - ProcToken = AllocatePool (sizeof (PROCEDURE_TOKEN)); > > - ASSERT (ProcToken != NULL); > > - ProcToken->Signature = PROCEDURE_TOKEN_SIGNATURE; > > - ProcToken->SpinLock = SpinLock; > > - ProcToken->RunningApCount = RunningApCount; > > + NewToken = FindFirstFreeToken (); > > + if (NewToken == NULL) { > > + AllocateTokenBuffer (); > > + NewToken = FindFirstFreeToken (); > > + } > > + ASSERT (NewToken != NULL); > > > > - InsertTailList (&gSmmCpuPrivate->TokenList, &ProcToken->Link); > > + NewToken->Used = TRUE; > > + NewToken->RunningApCount = RunningApsCount; > > + AcquireSpinLock (NewToken->SpinLock); > > > > - return ProcToken; > > + return NewToken; > > } > > > > /** > > @@ -1231,7 +1273,7 @@ InternalSmmStartupThisAp ( > mSmmMpSyncData->CpuData[CpuIndex].Procedure = Procedure; > > mSmmMpSyncData->CpuData[CpuIndex].Parameter = ProcArguments; > > if (Token != NULL) { > > - ProcToken= CreateToken (1); > > + ProcToken= GetFreeToken (1); > > mSmmMpSyncData->CpuData[CpuIndex].Token = ProcToken; > > *Token = (MM_COMPLETION)ProcToken->SpinLock; > > } > > @@ -1320,7 +1362,7 @@ InternalSmmStartupAllAPs ( > } > > > > if (Token != NULL) { > > - ProcToken = CreateToken ((UINT32)mMaxNumberOfCpus); > > + ProcToken = GetFreeToken ((UINT32)mMaxNumberOfCpus); > > *Token = (MM_COMPLETION)ProcToken->SpinLock; > > } else { > > ProcToken = NULL; > > @@ -1732,28 +1774,12 @@ InitializeDataForMmMp ( > VOID > > ) > > { > > - UINTN SpinLockSize; > > - UINT32 TokenCountPerChunk; > > - > > - SpinLockSize = GetSpinLockProperties (); > > - TokenCountPerChunk = FixedPcdGet32 > (PcdCpuSmmMpTokenCountPerChunk); > > - ASSERT (TokenCountPerChunk != 0); > > - if (TokenCountPerChunk == 0) { > > - DEBUG ((DEBUG_ERROR, "PcdCpuSmmMpTokenCountPerChunk should > not be Zero!\n")); > > - CpuDeadLoop (); > > - } > > - DEBUG ((DEBUG_INFO, "CpuSmm: SpinLock Size = 0x%x, > PcdCpuSmmMpTokenCountPerChunk = 0x%x\n", SpinLockSize, > TokenCountPerChunk)); > > - > > - gSmmCpuPrivate->CurrentTokenBuf = AllocatePool (SpinLockSize * > TokenCountPerChunk); > > - ASSERT (gSmmCpuPrivate->CurrentTokenBuf != NULL); > > - > > - gSmmCpuPrivate->UsedTokenNum = 0; > > - > > gSmmCpuPrivate->ApWrapperFunc = AllocatePool (sizeof > (PROCEDURE_WRAPPER) * gSmmCpuPrivate- > >SmmCoreEntryContext.NumberOfCpus); > > ASSERT (gSmmCpuPrivate->ApWrapperFunc != NULL); > > > > InitializeListHead (&gSmmCpuPrivate->TokenList); > > - InitializeListHead (&gSmmCpuPrivate->OldTokenBufList); > > + > > + AllocateTokenBuffer (); > > } > > > > /** > > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h > b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h > index 5c98494e2c..33b3dd140e 100644 > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h > @@ -214,6 +214,7 @@ typedef struct { > > > SPIN_LOCK *SpinLock; > > volatile UINT32 RunningApCount; > > + BOOLEAN Used; > > } PROCEDURE_TOKEN; > > > > #define PROCEDURE_TOKEN_FROM_LINK(a) CR (a, PROCEDURE_TOKEN, > Link, PROCEDURE_TOKEN_SIGNATURE) > > @@ -254,11 +255,6 @@ typedef struct { > > > PROCEDURE_WRAPPER *ApWrapperFunc; > > LIST_ENTRY TokenList; > > - > > - LIST_ENTRY OldTokenBufList; > > - > > - UINT8 *CurrentTokenBuf; > > - UINT32 UsedTokenNum; // Only record tokens > used in > CurrentTokenBuf. > > } SMM_CPU_PRIVATE_DATA; > > > > extern SMM_CPU_PRIVATE_DATA *gSmmCpuPrivate; > > -- > 2.23.0.windows.1
-=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#52644): https://edk2.groups.io/g/devel/message/52644 Mute This Topic: https://groups.io/mt/69283273/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
