On 01/02/20 04:30, Ni, Ray wrote: > Reviewed-by: Ray Ni <ray...@intel.com>
Eric, please go ahead with Ray's R-b. Thanks Laszlo > >> -----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 (#52655): https://edk2.groups.io/g/devel/message/52655 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] -=-=-=-=-=-=-=-=-=-=-=-
