From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mga14.intel.com (mga14.intel.com [192.55.52.115]) by mx.groups.io with SMTP id smtpd.web11.7671.1577935845223571820 for ; Wed, 01 Jan 2020 19:30:45 -0800 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: intel.com, ip: 192.55.52.115, mailfrom: ray.ni@intel.com) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga005.fm.intel.com ([10.253.24.32]) by fmsmga103.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 01 Jan 2020 19:30:44 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.69,385,1571727600"; d="scan'208";a="419618365" Received: from fmsmsx104.amr.corp.intel.com ([10.18.124.202]) by fmsmga005.fm.intel.com with ESMTP; 01 Jan 2020 19:30:44 -0800 Received: from fmsmsx113.amr.corp.intel.com (10.18.116.7) by fmsmsx104.amr.corp.intel.com (10.18.124.202) with Microsoft SMTP Server (TLS) id 14.3.439.0; Wed, 1 Jan 2020 19:30:44 -0800 Received: from shsmsx153.ccr.corp.intel.com (10.239.6.53) by FMSMSX113.amr.corp.intel.com (10.18.116.7) with Microsoft SMTP Server (TLS) id 14.3.439.0; Wed, 1 Jan 2020 19:30:44 -0800 Received: from shsmsx104.ccr.corp.intel.com ([169.254.5.197]) by SHSMSX153.ccr.corp.intel.com ([169.254.12.89]) with mapi id 14.03.0439.000; Thu, 2 Jan 2020 11:30:42 +0800 From: "Ni, Ray" To: "Dong, Eric" , "devel@edk2.groups.io" CC: Laszlo Ersek Subject: Re: [PATCH v2] UefiCpuPkg/PiSmmCpuDxeSmm: Pre-allocate PROCEDURE_TOKEN buffer Thread-Topic: [PATCH v2] UefiCpuPkg/PiSmmCpuDxeSmm: Pre-allocate PROCEDURE_TOKEN buffer Thread-Index: AQHVvIoYT2FdPgqf3EKfZiIVcZI/KqfWwGaw Date: Thu, 2 Jan 2020 03:30:41 +0000 Deferred-Delivery: Thu, 2 Jan 2020 03:29:00 +0000 Message-ID: <734D49CCEBEEF84792F5B80ED585239D5C3D17A0@SHSMSX104.ccr.corp.intel.com> References: <20191227074852.1332-1-eric.dong@intel.com> In-Reply-To: <20191227074852.1332-1-eric.dong@intel.com> Accept-Language: en-US, zh-CN X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.239.127.40] MIME-Version: 1.0 Return-Path: ray.ni@intel.com Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Reviewed-by: Ray Ni > -----Original Message----- > From: Dong, Eric > Sent: Friday, December 27, 2019 3:49 PM > To: devel@edk2.groups.io > Cc: Ni, Ray ; Laszlo Ersek > Subject: [PATCH v2] UefiCpuPkg/PiSmmCpuDxeSmm: Pre-allocate > PROCEDURE_TOKEN buffer >=20 > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3D2388 >=20 > 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. >=20 > Former change (9caaa79dd7e078ebb4012dde3b3d3a5d451df609) missed > PROCEDURE_TOKEN part, this change covers it. >=20 > Cc: Ray Ni > Cc: Laszlo Ersek > Signed-off-by: Eric Dong > --- >=20 > v2 changes: >=20 > Remove the not used variable. >=20 >=20 > UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c | 190 ++++++++++++------- > -- > UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h | 6 +- > 2 files changed, 109 insertions(+), 87 deletions(-) >=20 > 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 ( >=20 >=20 > **/ >=20 > VOID >=20 > -FreeTokens ( >=20 > +ResetTokens ( >=20 > VOID >=20 > ) >=20 > { >=20 > LIST_ENTRY *Link; >=20 > PROCEDURE_TOKEN *ProcToken; >=20 > - TOKEN_BUFFER *TokenBuf; >=20 >=20 >=20 > - // >=20 > - // Only free the token buffer recorded in the OldTOkenBufList >=20 > - // upon exiting SMI. Current token buffer stays allocated so >=20 > - // next SMI doesn't need to re-allocate. >=20 > - // >=20 > - gSmmCpuPrivate->UsedTokenNum =3D 0; >=20 > - >=20 > - Link =3D GetFirstNode (&gSmmCpuPrivate->OldTokenBufList); >=20 > - while (!IsNull (&gSmmCpuPrivate->OldTokenBufList, Link)) { >=20 > - TokenBuf =3D TOKEN_BUFFER_FROM_LINK (Link); >=20 > - >=20 > - Link =3D RemoveEntryList (&TokenBuf->Link); >=20 > - >=20 > - FreePool (TokenBuf->Buffer); >=20 > - FreePool (TokenBuf); >=20 > - } >=20 > - >=20 > - while (!IsListEmpty (&gSmmCpuPrivate->TokenList)) { >=20 > - Link =3D GetFirstNode (&gSmmCpuPrivate->TokenList); >=20 > + Link =3D GetFirstNode (&gSmmCpuPrivate->TokenList); >=20 > + while (!IsNull (&gSmmCpuPrivate->TokenList, Link)) { >=20 > ProcToken =3D PROCEDURE_TOKEN_FROM_LINK (Link); >=20 >=20 >=20 > - RemoveEntryList (&ProcToken->Link); >=20 > + ProcToken->RunningApCount =3D 0; >=20 > + ProcToken->Used =3D FALSE; >=20 > + >=20 > + // >=20 > + // Check the spinlock status and release it if not released yet. >=20 > + // >=20 > + if (!AcquireSpinLockOrFail(ProcToken->SpinLock)) { >=20 > + DEBUG((DEBUG_ERROR, "Risk::SpinLock still not released!")); >=20 > + } >=20 > + ReleaseSpinLock (ProcToken->SpinLock); >=20 >=20 >=20 > - FreePool (ProcToken); >=20 > + Link =3D GetNextNode (&gSmmCpuPrivate->TokenList, Link); >=20 > } >=20 > } >=20 >=20 >=20 > @@ -685,9 +676,9 @@ BSPHandler ( > WaitForAllAPs (ApCount); >=20 >=20 >=20 > // >=20 > - // Clean the tokens buffer. >=20 > + // Reset the tokens buffer. >=20 > // >=20 > - FreeTokens (); >=20 > + ResetTokens (); >=20 >=20 >=20 > // >=20 > // Reset BspIndex to -1, meaning BSP has not been elected. >=20 > @@ -1056,7 +1047,7 @@ IsTokenInUse ( > while (!IsNull (&gSmmCpuPrivate->TokenList, Link)) { >=20 > ProcToken =3D PROCEDURE_TOKEN_FROM_LINK (Link); >=20 >=20 >=20 > - if (ProcToken->SpinLock =3D=3D Token) { >=20 > + if (ProcToken->Used && ProcToken->SpinLock =3D=3D Token) { >=20 > return TRUE; >=20 > } >=20 >=20 >=20 > @@ -1067,61 +1058,112 @@ IsTokenInUse ( > } >=20 >=20 >=20 > /** >=20 > - create token and save it to the maintain list. >=20 > - >=20 > - @param RunningApCount Input running AP count. >=20 > - >=20 > - @retval return the spin lock used as token. >=20 > + Allocate buffer for the SPIN_LOCK and PROCEDURE_TOKEN. >=20 >=20 >=20 > **/ >=20 > -PROCEDURE_TOKEN * >=20 > -CreateToken ( >=20 > - IN UINT32 RunningApCount >=20 > +VOID >=20 > +AllocateTokenBuffer ( >=20 > + VOID >=20 > ) >=20 > { >=20 > - PROCEDURE_TOKEN *ProcToken; >=20 > - SPIN_LOCK *SpinLock; >=20 > UINTN SpinLockSize; >=20 > - TOKEN_BUFFER *TokenBuf; >=20 > UINT32 TokenCountPerChunk; >=20 > + UINTN ProcTokenSize; >=20 > + UINTN Index; >=20 > + PROCEDURE_TOKEN *ProcToken; >=20 > + SPIN_LOCK *SpinLock; >=20 > + UINT8 *SpinLockBuffer; >=20 > + UINT8 *ProcTokenBuffer; >=20 >=20 >=20 > SpinLockSize =3D GetSpinLockProperties (); >=20 > + ProcTokenSize =3D sizeof (PROCEDURE_TOKEN); >=20 > + >=20 > TokenCountPerChunk =3D FixedPcdGet32 > (PcdCpuSmmMpTokenCountPerChunk); >=20 > + ASSERT (TokenCountPerChunk !=3D 0); >=20 > + if (TokenCountPerChunk =3D=3D 0) { >=20 > + DEBUG ((DEBUG_ERROR, "PcdCpuSmmMpTokenCountPerChunk should > not be Zero!\n")); >=20 > + CpuDeadLoop (); >=20 > + } >=20 > + DEBUG ((DEBUG_INFO, "CpuSmm: SpinLock Size =3D 0x%x, > PcdCpuSmmMpTokenCountPerChunk =3D 0x%x\n", SpinLockSize, > TokenCountPerChunk)); >=20 >=20 >=20 > - if (gSmmCpuPrivate->UsedTokenNum =3D=3D TokenCountPerChunk) { >=20 > - DEBUG ((DEBUG_VERBOSE, "CpuSmm: No free token buffer, allocate new > buffer!\n")); >=20 > + // >=20 > + // Separate the Spin_lock and Proc_token because the alignment require= s > by Spin_Lock. >=20 > + // >=20 > + SpinLockBuffer =3D AllocatePool (SpinLockSize * TokenCountPerChunk); >=20 > + ASSERT (SpinLockBuffer !=3D NULL); >=20 >=20 >=20 > - // >=20 > - // Record current token buffer for later free action usage. >=20 > - // Current used token buffer not in this list. >=20 > - // >=20 > - TokenBuf =3D AllocatePool (sizeof (TOKEN_BUFFER)); >=20 > - ASSERT (TokenBuf !=3D NULL); >=20 > - TokenBuf->Signature =3D TOKEN_BUFFER_SIGNATURE; >=20 > - TokenBuf->Buffer =3D gSmmCpuPrivate->CurrentTokenBuf; >=20 > + ProcTokenBuffer =3D AllocatePool (ProcTokenSize * TokenCountPerChunk); >=20 > + ASSERT (ProcTokenBuffer !=3D NULL); >=20 > + >=20 > + for (Index =3D 0; Index < TokenCountPerChunk; Index++) { >=20 > + SpinLock =3D (SPIN_LOCK *)(SpinLockBuffer + SpinLockSize * Index); >=20 > + InitializeSpinLock (SpinLock); >=20 > + >=20 > + ProcToken =3D (PROCEDURE_TOKEN *)(ProcTokenBuffer + ProcTokenSize * > Index); >=20 > + ProcToken->Signature =3D PROCEDURE_TOKEN_SIGNATURE; >=20 > + ProcToken->SpinLock =3D SpinLock; >=20 > + ProcToken->Used =3D FALSE; >=20 > + ProcToken->RunningApCount =3D 0; >=20 > + >=20 > + InsertTailList (&gSmmCpuPrivate->TokenList, &ProcToken->Link); >=20 > + } >=20 > +} >=20 >=20 >=20 > - InsertTailList (&gSmmCpuPrivate->OldTokenBufList, &TokenBuf->Link); >=20 > +/** >=20 > + Find first free token in the allocated token list. >=20 > + >=20 > + @retval return the first free PROCEDURE_TOKEN. >=20 > + >=20 > +**/ >=20 > +PROCEDURE_TOKEN * >=20 > +FindFirstFreeToken ( >=20 > + VOID >=20 > + ) >=20 > +{ >=20 > + LIST_ENTRY *Link; >=20 > + PROCEDURE_TOKEN *ProcToken; >=20 >=20 >=20 > - gSmmCpuPrivate->CurrentTokenBuf =3D AllocatePool (SpinLockSize * > TokenCountPerChunk); >=20 > - ASSERT (gSmmCpuPrivate->CurrentTokenBuf !=3D NULL); >=20 > - gSmmCpuPrivate->UsedTokenNum =3D 0; >=20 > + Link =3D GetFirstNode (&gSmmCpuPrivate->TokenList); >=20 > + while (!IsNull (&gSmmCpuPrivate->TokenList, Link)) { >=20 > + ProcToken =3D PROCEDURE_TOKEN_FROM_LINK (Link); >=20 > + >=20 > + if (!ProcToken->Used) { >=20 > + return ProcToken; >=20 > + } >=20 > + >=20 > + Link =3D GetNextNode (&gSmmCpuPrivate->TokenList, Link); >=20 > } >=20 >=20 >=20 > - SpinLock =3D (SPIN_LOCK *)(gSmmCpuPrivate->CurrentTokenBuf + > SpinLockSize * gSmmCpuPrivate->UsedTokenNum); >=20 > - gSmmCpuPrivate->UsedTokenNum++; >=20 > + return NULL; >=20 > +} >=20 > + >=20 > +/** >=20 > + Get the free token. >=20 > + >=20 > + If no free token, allocate new tokens then return the free one. >=20 > + >=20 > + @retval return the first free PROCEDURE_TOKEN. >=20 >=20 >=20 > - InitializeSpinLock (SpinLock); >=20 > - AcquireSpinLock (SpinLock); >=20 > +**/ >=20 > +PROCEDURE_TOKEN * >=20 > +GetFreeToken ( >=20 > + IN UINT32 RunningApsCount >=20 > + ) >=20 > +{ >=20 > + PROCEDURE_TOKEN *NewToken; >=20 >=20 >=20 > - ProcToken =3D AllocatePool (sizeof (PROCEDURE_TOKEN)); >=20 > - ASSERT (ProcToken !=3D NULL); >=20 > - ProcToken->Signature =3D PROCEDURE_TOKEN_SIGNATURE; >=20 > - ProcToken->SpinLock =3D SpinLock; >=20 > - ProcToken->RunningApCount =3D RunningApCount; >=20 > + NewToken =3D FindFirstFreeToken (); >=20 > + if (NewToken =3D=3D NULL) { >=20 > + AllocateTokenBuffer (); >=20 > + NewToken =3D FindFirstFreeToken (); >=20 > + } >=20 > + ASSERT (NewToken !=3D NULL); >=20 >=20 >=20 > - InsertTailList (&gSmmCpuPrivate->TokenList, &ProcToken->Link); >=20 > + NewToken->Used =3D TRUE; >=20 > + NewToken->RunningApCount =3D RunningApsCount; >=20 > + AcquireSpinLock (NewToken->SpinLock); >=20 >=20 >=20 > - return ProcToken; >=20 > + return NewToken; >=20 > } >=20 >=20 >=20 > /** >=20 > @@ -1231,7 +1273,7 @@ InternalSmmStartupThisAp ( > mSmmMpSyncData->CpuData[CpuIndex].Procedure =3D Procedure; >=20 > mSmmMpSyncData->CpuData[CpuIndex].Parameter =3D ProcArguments; >=20 > if (Token !=3D NULL) { >=20 > - ProcToken=3D CreateToken (1); >=20 > + ProcToken=3D GetFreeToken (1); >=20 > mSmmMpSyncData->CpuData[CpuIndex].Token =3D ProcToken; >=20 > *Token =3D (MM_COMPLETION)ProcToken->SpinLock; >=20 > } >=20 > @@ -1320,7 +1362,7 @@ InternalSmmStartupAllAPs ( > } >=20 >=20 >=20 > if (Token !=3D NULL) { >=20 > - ProcToken =3D CreateToken ((UINT32)mMaxNumberOfCpus); >=20 > + ProcToken =3D GetFreeToken ((UINT32)mMaxNumberOfCpus); >=20 > *Token =3D (MM_COMPLETION)ProcToken->SpinLock; >=20 > } else { >=20 > ProcToken =3D NULL; >=20 > @@ -1732,28 +1774,12 @@ InitializeDataForMmMp ( > VOID >=20 > ) >=20 > { >=20 > - UINTN SpinLockSize; >=20 > - UINT32 TokenCountPerChunk; >=20 > - >=20 > - SpinLockSize =3D GetSpinLockProperties (); >=20 > - TokenCountPerChunk =3D FixedPcdGet32 > (PcdCpuSmmMpTokenCountPerChunk); >=20 > - ASSERT (TokenCountPerChunk !=3D 0); >=20 > - if (TokenCountPerChunk =3D=3D 0) { >=20 > - DEBUG ((DEBUG_ERROR, "PcdCpuSmmMpTokenCountPerChunk should > not be Zero!\n")); >=20 > - CpuDeadLoop (); >=20 > - } >=20 > - DEBUG ((DEBUG_INFO, "CpuSmm: SpinLock Size =3D 0x%x, > PcdCpuSmmMpTokenCountPerChunk =3D 0x%x\n", SpinLockSize, > TokenCountPerChunk)); >=20 > - >=20 > - gSmmCpuPrivate->CurrentTokenBuf =3D AllocatePool (SpinLockSize * > TokenCountPerChunk); >=20 > - ASSERT (gSmmCpuPrivate->CurrentTokenBuf !=3D NULL); >=20 > - >=20 > - gSmmCpuPrivate->UsedTokenNum =3D 0; >=20 > - >=20 > gSmmCpuPrivate->ApWrapperFunc =3D AllocatePool (sizeof > (PROCEDURE_WRAPPER) * gSmmCpuPrivate- > >SmmCoreEntryContext.NumberOfCpus); >=20 > ASSERT (gSmmCpuPrivate->ApWrapperFunc !=3D NULL); >=20 >=20 >=20 > InitializeListHead (&gSmmCpuPrivate->TokenList); >=20 > - InitializeListHead (&gSmmCpuPrivate->OldTokenBufList); >=20 > + >=20 > + AllocateTokenBuffer (); >=20 > } >=20 >=20 >=20 > /** >=20 > 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 { >=20 >=20 > SPIN_LOCK *SpinLock; >=20 > volatile UINT32 RunningApCount; >=20 > + BOOLEAN Used; >=20 > } PROCEDURE_TOKEN; >=20 >=20 >=20 > #define PROCEDURE_TOKEN_FROM_LINK(a) CR (a, PROCEDURE_TOKEN, > Link, PROCEDURE_TOKEN_SIGNATURE) >=20 > @@ -254,11 +255,6 @@ typedef struct { >=20 >=20 > PROCEDURE_WRAPPER *ApWrapperFunc; >=20 > LIST_ENTRY TokenList; >=20 > - >=20 > - LIST_ENTRY OldTokenBufList; >=20 > - >=20 > - UINT8 *CurrentTokenBuf; >=20 > - UINT32 UsedTokenNum; // Only record token= s used in > CurrentTokenBuf. >=20 > } SMM_CPU_PRIVATE_DATA; >=20 >=20 >=20 > extern SMM_CPU_PRIVATE_DATA *gSmmCpuPrivate; >=20 > -- > 2.23.0.windows.1