Re: [PATCH] UefiCpuPkg/PiSmmCpuDxeSmm: Remove dependence between APs.

["Ni, Ray" <ray.ni@intel.com>]

> 
> +  if (!Token->SingleAp) {
> 
> +    ReleaseSemaphore (&Token->FinishedApCount);

1. If the FinishedApCount is renamed to RunningApCount and
InterlockedDecrement() is called for it.

SingleAp flag is unneeded.

For StartupAllAps(), RunningApCount = mMaxNumberOfCpus - 1;
For StartupThisAps(), RunningApCount = 1;

When RunningApCount == 0, the spinlock is released.

> +    if (mSmmMpSyncData->CpuData[CpuIndex].Token != NULL) {
> 
> +      ReleaseToken (CpuIndex);

2. Can you directly pass in mSmmMpSyncData->CpuData[CpuIndex].Token?
It simplifies the ReleaseToken() and also make people understand that
ReleaseToken() will only modifies the Token but other states in CpuData[Index]
is NOT changed.

> 
> @@ -1170,10 +1120,12 @@ CreateToken (

3. With the comment #1, CreateToken() can carry additional parameter which specifies
the RunningApCount.

>    ASSERT (ProcToken != NULL);
> 
>    ProcToken->Signature = PROCEDURE_TOKEN_SIGNATURE;
> 
>    ProcToken->ProcedureToken = CpuToken;

4. ProcToken->ProcedureToken looks a bit strange.
Can we use "ProcToken->Spinlock"?

> 
> +    *Token = (MM_COMPLETION) mSmmMpSyncData-
> >CpuData[CpuIndex].Token->ProcedureToken;

5. It will become
*Token = (MM_COMPLETION) mSmmMpSyncData->CpuData[CpuIndex].Token->Spinlock;

> 
> +      ReleaseSemaphore (&ProcToken->FinishedApCount);

6. I can now understand why "FinishedApCount is directly compared against mMaxNumberOfCpus because
the FinishedApCount is already increased for BSP. It's not a comment for code change.


Thanks,
Ray