From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mga06.intel.com (mga06.intel.com [134.134.136.31]) by mx.groups.io with SMTP id smtpd.web12.865.1577155444289033349 for ; Mon, 23 Dec 2019 18:44:04 -0800 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: intel.com, ip: 134.134.136.31, mailfrom: ray.ni@intel.com) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga004.fm.intel.com ([10.253.24.48]) by orsmga104.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 23 Dec 2019 18:44:03 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.69,350,1571727600"; d="scan'208";a="242390119" Received: from fmsmsx108.amr.corp.intel.com ([10.18.124.206]) by fmsmga004.fm.intel.com with ESMTP; 23 Dec 2019 18:44:03 -0800 Received: from fmsmsx152.amr.corp.intel.com (10.18.125.5) by FMSMSX108.amr.corp.intel.com (10.18.124.206) with Microsoft SMTP Server (TLS) id 14.3.439.0; Mon, 23 Dec 2019 18:44:03 -0800 Received: from shsmsx103.ccr.corp.intel.com (10.239.4.69) by FMSMSX152.amr.corp.intel.com (10.18.125.5) with Microsoft SMTP Server (TLS) id 14.3.439.0; Mon, 23 Dec 2019 18:44:02 -0800 Received: from shsmsx104.ccr.corp.intel.com ([169.254.5.90]) by SHSMSX103.ccr.corp.intel.com ([169.254.4.29]) with mapi id 14.03.0439.000; Tue, 24 Dec 2019 10:44:01 +0800 From: "Ni, Ray" To: "devel@edk2.groups.io" , "Dong, Eric" CC: Laszlo Ersek Subject: Re: [edk2-devel] [PATCH v3 1/2] UefiCpuPkg/PiSmmCpuDxeSmm: Remove dependence between APs Thread-Topic: [edk2-devel] [PATCH v3 1/2] UefiCpuPkg/PiSmmCpuDxeSmm: Remove dependence between APs Thread-Index: AQHVuWh51xC3CwY3lkqJUjoQtt+nfqfIlI1g Date: Tue, 24 Dec 2019 02:44:00 +0000 Deferred-Delivery: Tue, 24 Dec 2019 02:42:00 +0000 Message-ID: <734D49CCEBEEF84792F5B80ED585239D5C3A9DBF@SHSMSX104.ccr.corp.intel.com> References: <20191223081037.1565-1-eric.dong@intel.com> <20191223081037.1565-2-eric.dong@intel.com> In-Reply-To: <20191223081037.1565-2-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: devel@edk2.groups.io On Behalf Of Dong, > Eric > Sent: Monday, December 23, 2019 4:11 PM > To: devel@edk2.groups.io > Cc: Ni, Ray ; Laszlo Ersek > Subject: [edk2-devel] [PATCH v3 1/2] UefiCpuPkg/PiSmmCpuDxeSmm: > Remove dependence between APs >=20 > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3D2268 >=20 > In current implementation, when check whether APs called by StartUpAllAPs > or StartUpThisAp, it checks the Tokens value used by other APs. Also the = AP > will update the Token value for itself if its task finished. In this > case, the potential race condition issues happens for the tokens. > Because of this, system may trig ASSERT during cycling test. >=20 > This change enhance the code logic, add new attributes for the token to > remove the reference for the tokens belongs to other APs. >=20 > Cc: Ray Ni > Cc: Laszlo Ersek > Signed-off-by: Eric Dong > --- > UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c | 125 +++++++-------------- > UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h | 5 +- > 2 files changed, 45 insertions(+), 85 deletions(-) >=20 > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c > b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c > index 757f1056f7..35951cc43e 100644 > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c > @@ -402,38 +402,6 @@ IsPresentAp ( > *(mSmmMpSyncData->CpuData[CpuIndex].Present)); >=20 > } >=20 >=20 >=20 > -/** >=20 > - Check whether execute in single AP or all APs. >=20 > - >=20 > - Compare two Tokens used by different APs to know whether in StartAllAp= s > call. >=20 > - >=20 > - Whether is an valid AP base on AP's Present flag. >=20 > - >=20 > - @retval TRUE IN StartAllAps call. >=20 > - @retval FALSE Not in StartAllAps call. >=20 > - >=20 > -**/ >=20 > -BOOLEAN >=20 > -InStartAllApsCall ( >=20 > - VOID >=20 > - ) >=20 > -{ >=20 > - UINTN ApIndex; >=20 > - UINTN ApIndex2; >=20 > - >=20 > - for (ApIndex =3D mMaxNumberOfCpus; ApIndex-- > 0;) { >=20 > - if (IsPresentAp (ApIndex) && (mSmmMpSyncData- > >CpuData[ApIndex].Token !=3D NULL)) { >=20 > - for (ApIndex2 =3D ApIndex; ApIndex2-- > 0;) { >=20 > - if (IsPresentAp (ApIndex2) && (mSmmMpSyncData- > >CpuData[ApIndex2].Token !=3D NULL)) { >=20 > - return mSmmMpSyncData->CpuData[ApIndex2].Token =3D=3D > mSmmMpSyncData->CpuData[ApIndex].Token; >=20 > - } >=20 > - } >=20 > - } >=20 > - } >=20 > - >=20 > - return FALSE; >=20 > -} >=20 > - >=20 > /** >=20 > Clean up the status flags used during executing the procedure. >=20 >=20 >=20 > @@ -445,40 +413,15 @@ ReleaseToken ( > IN UINTN CpuIndex >=20 > ) >=20 > { >=20 > - UINTN Index; >=20 > - BOOLEAN Released; >=20 > + PROCEDURE_TOKEN *Token; >=20 >=20 >=20 > - if (InStartAllApsCall ()) { >=20 > - // >=20 > - // In Start All APs mode, make sure all APs have finished task. >=20 > - // >=20 > - if (WaitForAllAPsNotBusy (FALSE)) { >=20 > - // >=20 > - // Clean the flags update in the function call. >=20 > - // >=20 > - Released =3D FALSE; >=20 > - for (Index =3D mMaxNumberOfCpus; Index-- > 0;) { >=20 > - // >=20 > - // Only In SMM APs need to be clean up. >=20 > - // >=20 > - if (mSmmMpSyncData->CpuData[Index].Present && > mSmmMpSyncData->CpuData[Index].Token !=3D NULL) { >=20 > - if (!Released) { >=20 > - ReleaseSpinLock (mSmmMpSyncData->CpuData[Index].Token); >=20 > - Released =3D TRUE; >=20 > - } >=20 > - mSmmMpSyncData->CpuData[Index].Token =3D NULL; >=20 > - } >=20 > - } >=20 > - } >=20 > - } else { >=20 > - // >=20 > - // In single AP mode. >=20 > - // >=20 > - if (mSmmMpSyncData->CpuData[CpuIndex].Token !=3D NULL) { >=20 > - ReleaseSpinLock (mSmmMpSyncData->CpuData[CpuIndex].Token); >=20 > - mSmmMpSyncData->CpuData[CpuIndex].Token =3D NULL; >=20 > - } >=20 > + Token =3D mSmmMpSyncData->CpuData[CpuIndex].Token; >=20 > + >=20 > + if (InterlockedDecrement (&Token->RunningApCount) =3D=3D 0) { >=20 > + ReleaseSpinLock (Token->SpinLock); >=20 > } >=20 > + >=20 > + mSmmMpSyncData->CpuData[CpuIndex].Token =3D NULL; >=20 > } >=20 >=20 >=20 > /** >=20 > @@ -912,12 +855,14 @@ APHandler ( > *mSmmMpSyncData->CpuData[CpuIndex].Status =3D ProcedureStatus; >=20 > } >=20 >=20 >=20 > + if (mSmmMpSyncData->CpuData[CpuIndex].Token !=3D NULL) { >=20 > + ReleaseToken (CpuIndex); >=20 > + } >=20 > + >=20 > // >=20 > // Release BUSY >=20 > // >=20 > ReleaseSpinLock (mSmmMpSyncData->CpuData[CpuIndex].Busy); >=20 > - >=20 > - ReleaseToken (CpuIndex); >=20 > } >=20 >=20 >=20 > if (SmmCpuFeaturesNeedConfigureMtrrs()) { >=20 > @@ -1111,7 +1056,7 @@ IsTokenInUse ( > while (!IsNull (&gSmmCpuPrivate->TokenList, Link)) { >=20 > ProcToken =3D PROCEDURE_TOKEN_FROM_LINK (Link); >=20 >=20 >=20 > - if (ProcToken->ProcedureToken =3D=3D Token) { >=20 > + if (ProcToken->SpinLock =3D=3D Token) { >=20 > return TRUE; >=20 > } >=20 >=20 >=20 > @@ -1124,16 +1069,18 @@ IsTokenInUse ( > /** >=20 > create token and save it to the maintain list. >=20 >=20 >=20 > + @param RunningApCount Input running AP count. >=20 > + >=20 > @retval return the spin lock used as token. >=20 >=20 >=20 > **/ >=20 > -SPIN_LOCK * >=20 > +PROCEDURE_TOKEN * >=20 > CreateToken ( >=20 > - VOID >=20 > + IN UINT32 RunningApCount >=20 > ) >=20 > { >=20 > PROCEDURE_TOKEN *ProcToken; >=20 > - SPIN_LOCK *CpuToken; >=20 > + SPIN_LOCK *SpinLock; >=20 > UINTN SpinLockSize; >=20 > TOKEN_BUFFER *TokenBuf; >=20 > UINT32 TokenCountPerChunk; >=20 > @@ -1160,20 +1107,21 @@ CreateToken ( > gSmmCpuPrivate->UsedTokenNum =3D 0; >=20 > } >=20 >=20 >=20 > - CpuToken =3D (SPIN_LOCK *)(gSmmCpuPrivate->CurrentTokenBuf + > SpinLockSize * gSmmCpuPrivate->UsedTokenNum); >=20 > + SpinLock =3D (SPIN_LOCK *)(gSmmCpuPrivate->CurrentTokenBuf + > SpinLockSize * gSmmCpuPrivate->UsedTokenNum); >=20 > gSmmCpuPrivate->UsedTokenNum++; >=20 >=20 >=20 > - InitializeSpinLock (CpuToken); >=20 > - AcquireSpinLock (CpuToken); >=20 > + InitializeSpinLock (SpinLock); >=20 > + AcquireSpinLock (SpinLock); >=20 >=20 >=20 > ProcToken =3D AllocatePool (sizeof (PROCEDURE_TOKEN)); >=20 > ASSERT (ProcToken !=3D NULL); >=20 > ProcToken->Signature =3D PROCEDURE_TOKEN_SIGNATURE; >=20 > - ProcToken->ProcedureToken =3D CpuToken; >=20 > + ProcToken->SpinLock =3D SpinLock; >=20 > + ProcToken->RunningApCount =3D RunningApCount; >=20 >=20 >=20 > InsertTailList (&gSmmCpuPrivate->TokenList, &ProcToken->Link); >=20 >=20 >=20 > - return CpuToken; >=20 > + return ProcToken; >=20 > } >=20 >=20 >=20 > /** >=20 > @@ -1246,6 +1194,8 @@ InternalSmmStartupThisAp ( > IN OUT EFI_STATUS *CpuStatus >=20 > ) >=20 > { >=20 > + PROCEDURE_TOKEN *ProcToken; >=20 > + >=20 > if (CpuIndex >=3D gSmmCpuPrivate->SmmCoreEntryContext.NumberOfCpus) > { >=20 > DEBUG((DEBUG_ERROR, "CpuIndex(%d) >=3D gSmmCpuPrivate- > >SmmCoreEntryContext.NumberOfCpus(%d)\n", CpuIndex, > gSmmCpuPrivate->SmmCoreEntryContext.NumberOfCpus)); >=20 > return EFI_INVALID_PARAMETER; >=20 > @@ -1278,14 +1228,12 @@ InternalSmmStartupThisAp ( >=20 >=20 > AcquireSpinLock (mSmmMpSyncData->CpuData[CpuIndex].Busy); >=20 >=20 >=20 > - if (Token !=3D NULL) { >=20 > - *Token =3D (MM_COMPLETION) CreateToken (); >=20 > - } >=20 > - >=20 > mSmmMpSyncData->CpuData[CpuIndex].Procedure =3D Procedure; >=20 > mSmmMpSyncData->CpuData[CpuIndex].Parameter =3D ProcArguments; >=20 > if (Token !=3D NULL) { >=20 > - mSmmMpSyncData->CpuData[CpuIndex].Token =3D (SPIN_LOCK > *)(*Token); >=20 > + ProcToken=3D CreateToken (1); >=20 > + mSmmMpSyncData->CpuData[CpuIndex].Token =3D ProcToken; >=20 > + *Token =3D (MM_COMPLETION)ProcToken->SpinLock; >=20 > } >=20 > mSmmMpSyncData->CpuData[CpuIndex].Status =3D CpuStatus; >=20 > if (mSmmMpSyncData->CpuData[CpuIndex].Status !=3D NULL) { >=20 > @@ -1343,6 +1291,7 @@ InternalSmmStartupAllAPs ( > { >=20 > UINTN Index; >=20 > UINTN CpuCount; >=20 > + PROCEDURE_TOKEN *ProcToken; >=20 >=20 >=20 > if ((TimeoutInMicroseconds !=3D 0) && ((mSmmMp.Attributes & > EFI_MM_MP_TIMEOUT_SUPPORTED) =3D=3D 0)) { >=20 > return EFI_INVALID_PARAMETER; >=20 > @@ -1371,7 +1320,10 @@ InternalSmmStartupAllAPs ( > } >=20 >=20 >=20 > if (Token !=3D NULL) { >=20 > - *Token =3D (MM_COMPLETION) CreateToken (); >=20 > + ProcToken =3D CreateToken ((UINT32)mMaxNumberOfCpus); >=20 > + *Token =3D (MM_COMPLETION)ProcToken->SpinLock; >=20 > + } else { >=20 > + ProcToken =3D NULL; >=20 > } >=20 >=20 >=20 > // >=20 > @@ -1391,8 +1343,8 @@ InternalSmmStartupAllAPs ( > if (IsPresentAp (Index)) { >=20 > mSmmMpSyncData->CpuData[Index].Procedure =3D > (EFI_AP_PROCEDURE2) Procedure; >=20 > mSmmMpSyncData->CpuData[Index].Parameter =3D ProcedureArguments; >=20 > - if (Token !=3D NULL) { >=20 > - mSmmMpSyncData->CpuData[Index].Token =3D (SPIN_LOCK *)(*Token)= ; >=20 > + if (ProcToken !=3D NULL) { >=20 > + mSmmMpSyncData->CpuData[Index].Token =3D ProcToken; >=20 > } >=20 > if (CPUStatus !=3D NULL) { >=20 > mSmmMpSyncData->CpuData[Index].Status =3D &CPUStatus[Index]; >=20 > @@ -1408,6 +1360,13 @@ InternalSmmStartupAllAPs ( > if (CPUStatus !=3D NULL) { >=20 > CPUStatus[Index] =3D EFI_NOT_STARTED; >=20 > } >=20 > + >=20 > + // >=20 > + // Decrease the count to mark this processor(AP or BSP) as finishe= d. >=20 > + // >=20 > + if (ProcToken !=3D NULL) { >=20 > + WaitForSemaphore (&ProcToken->RunningApCount); >=20 > + } >=20 > } >=20 > } >=20 >=20 >=20 > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h > b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h > index 5c1a01e42b..5c98494e2c 100644 > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h > @@ -212,7 +212,8 @@ typedef struct { > UINTN Signature; >=20 > LIST_ENTRY Link; >=20 >=20 >=20 > - SPIN_LOCK *ProcedureToken; >=20 > + SPIN_LOCK *SpinLock; >=20 > + volatile UINT32 RunningApCount; >=20 > } PROCEDURE_TOKEN; >=20 >=20 >=20 > #define PROCEDURE_TOKEN_FROM_LINK(a) CR (a, PROCEDURE_TOKEN, > Link, PROCEDURE_TOKEN_SIGNATURE) >=20 > @@ -407,7 +408,7 @@ typedef struct { > volatile VOID *Parameter; >=20 > volatile UINT32 *Run; >=20 > volatile BOOLEAN *Present; >=20 > - SPIN_LOCK *Token; >=20 > + PROCEDURE_TOKEN *Token; >=20 > EFI_STATUS *Status; >=20 > } SMM_CPU_DATA_BLOCK; >=20 >=20 >=20 > -- > 2.23.0.windows.1 >=20 >=20 > -=3D-=3D-=3D-=3D-=3D-=3D > Groups.io Links: You receive all messages sent to this group. >=20 > View/Reply Online (#52503): https://edk2.groups.io/g/devel/message/52503 > Mute This Topic: https://groups.io/mt/69227573/1712937 > Group Owner: devel+owner@edk2.groups.io > Unsubscribe: https://edk2.groups.io/g/devel/unsub [ray.ni@intel.com] > -=3D-=3D-=3D-=3D-=3D-=3D