Re: [edk2-devel] [PATCH v3 3/6] UefiCpuPkg: Implements SmmCpuSyncLib library instance

["Wu, Jiaxin" <jiaxin.wu@intel.com>]

> > The code will be changed to:
> >
> >   if ((INT32)InternalWaitForSemaphore (Context->CpuCount) < 0) {
> >     return RETURN_ABORTED;
> >   }
> 
> I find this quite ugly. In the "semaphore post" operation, we already
> have code that prevents incrementing if the semaphore is "locked". Can
> we perhaps create a "semaphore pend" operation that does the same?
> 
> How about this:
> 
> diff --git a/UefiCpuPkg/Library/SmmCpuSyncLib/SmmCpuSyncLib.c
> b/UefiCpuPkg/Library/SmmCpuSyncLib/SmmCpuSyncLib.c
> index 3c2835f8def6..5d7fc58ef23f 100644
> --- a/UefiCpuPkg/Library/SmmCpuSyncLib/SmmCpuSyncLib.c
> +++ b/UefiCpuPkg/Library/SmmCpuSyncLib/SmmCpuSyncLib.c
> @@ -91,35 +91,38 @@ UINT32
>  InternalWaitForSemaphore (
>    IN OUT  volatile UINT32  *Sem
>    )
>  {
>    UINT32  Value;
> 
>    for ( ; ;) {
>      Value = *Sem;
> +    if (Value == MAX_UINT32) {
> +      return Value;
> +    }
>      if ((Value != 0) &&
>          (InterlockedCompareExchange32 (
>             (UINT32 *)Sem,
>             Value,
>             Value - 1
>             ) == Value))
>      {
>        break;
>      }
> 
>      CpuPause ();
>    }
> 
>    return Value - 1;
>  }
> 
> Note, I'm just brainstorming here, I've not thought it through. Just to
> illustrate the direction I'm thinking of.
> 
> This change should be mostly OK. InternalWaitForSemaphore() returns the
> decremented value. So, for InternalWaitForSemaphore() to return
> MAX_UINT32 *without* this update, the function would have to decrement
> the semaphore when the semaphore is zero. But in that case, the function
> *blocks*. Thus, a return value of MAX_UINT32 is not possible without
> this extension; ergo, if MAX_UINT32 is returned (with this extension),

Yes, that's for the semaphore sync usage, we have to block the sem if it's zero, decrease it when return. That's why I said - it's naturally make sure the Run is reset after all ready to exit.  Then it can achieve the below flow:
    BSP: ReleaseOneAp  -->  AP: WaitForBsp
    BSP: WaitForAPs    <--  AP: ReleaseBsp


For locked case, I just copy the existing logic from SMM cpu driver (as I document in the commit message: The instance refers the existing SMM CPU driver (PiSmmCpuDxeSmm) sync implementation and behavior):
existing ReleaseSemaphore() prevents increase the semaphore, but it still return the original semaphore value +1; --> that's why we have to check the return value is  0 or not in SmmCpuSyncCheckInCpu()
existing WaitForSemaphore() allow decrease the semaphore if locked, and it also return the original semaphore value -1;  --> that's why we have to check the return value is  < 0 or not in SmmCpuSyncCheckOutCpu()
 
so, do you want to align the behavior as below?

ReleaseSemaphore() prevents increase the semaphore if locked, and it should return the locked value (MAX_UINT32);  --> then we can check the return value is  MAX_UINT32 or not in SmmCpuSyncCheckInCpu(), and sem itself won't be changed.
WaitForSemaphore() prevents decrease the semaphore if locked, and it should return the locked value (MAX_UINT32); --> then we can check the return value is  MAX_UINT32 or not in SmmCpuSyncCheckOutCpu (), and sem itself won't be changed.

I think:
for ReleaseSemaphore, it must meet below 2 cases usage:
1. for semaphore sync usage (Run), it doesn't care the lock case, and returned value is not cared. Just check the semaphore itself.
2. for Rendezvous case (Counter), it not only needs to check locked or not from return value, but also require "only increase the semaphore if not locked".

for WaitForSemaphore, it must meet below 2 cases usage:
1. for semaphore sync usage (Run), it doesn't care the lock case, and returned value is not cared. But for the semaphore itself, it need block at 0, and decrease when return.
2. for Rendezvous case (Counter), it only needs to check locked or not from return value. semaphore itself is not cared.

So, based on above, I think, yes, we can do the change to align the lock behavior:  

/**
  Performs an atomic compare exchange operation to get semaphore.
  The compare exchange operation must be performed using MP safe
  mechanisms.

  @param[in,out]  Sem    IN:  32-bit unsigned integer
                         OUT: original integer - 1 if Sem is not locked.
                         OUT: original integer (MAX_UINT32) if Sem is locked.

  @retval     Original integer - 1 if Sem is not locked.
              Original integer (MAX_UINT32) if Sem is locked.

**/
STATIC
UINT32
InternalWaitForSemaphore (
  IN OUT  volatile UINT32  *Sem
  )
{
  UINT32  Value;

  for ( ; ;) {
    Value = *Sem;
    if (Value == MAX_UINT32) {
      return Value;
    }

    if ((Value != 0) &&
        (InterlockedCompareExchange32 (
           (UINT32 *)Sem,
           Value,
           Value - 1
           ) == Value))
    {
      break;
    }

    CpuPause ();
  }

  return Value - 1;
}

/**
  Performs an atomic compare exchange operation to release semaphore.
  The compare exchange operation must be performed using MP safe
  mechanisms.

  @param[in,out]  Sem    IN:  32-bit unsigned integer
                         OUT: original integer + 1 if Sem is not locked.
                         OUT: original integer (MAX_UINT32) if Sem is locked.

  @retval    Original integer + 1 if Sem is not locked.
             Original integer (MAX_UINT32) if Sem is locked.

**/
STATIC
UINT32
InternalReleaseSemaphore (
  IN OUT  volatile UINT32  *Sem
  )
{
  UINT32  Value;

  do {
    Value = *Sem;
  } while (Value + 1 != 0 &&
           InterlockedCompareExchange32 (
             (UINT32 *)Sem,
             Value,
             Value + 1
             ) != Value);

  if (Value == MAX_UINT32) {
    return Value;
  }

  return Value + 1;
}

I haven't see any issue with this change. 

> we know the door was locked earlier (and the semaphore is not changed).
> 
> At the same time, we might want to update InternalReleaseSemaphore() as
> well, so that it cannot validly increment the semaphore value to MAX_UINT32.
> 
> 
> 
> >
> >



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#112542): https://edk2.groups.io/g/devel/message/112542
Mute This Topic: https://groups.io/mt/103010165/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-