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

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

I will the align the ReleaseSemaphore & WaitForSemaphore behavior as blow:

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'tbe changed.

Thanks,
Jiaxin 


> -----Original Message-----
> From: Wu, Jiaxin
> Sent: Thursday, December 14, 2023 11:35 PM
> To: devel@edk2.groups.io; lersek@redhat.com
> Cc: Dong, Eric <eric.dong@intel.com>; Ni, Ray <ray.ni@intel.com>; Zeng, Star
> <star.zeng@intel.com>; Gerd Hoffmann <kraxel@redhat.com>; Kumar, Rahul R
> <rahul.r.kumar@intel.com>
> Subject: RE: [edk2-devel] [PATCH v3 3/6] UefiCpuPkg: Implements
> SmmCpuSyncLib library instance
> 
> > > 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 (#112585): https://edk2.groups.io/g/devel/message/112585
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]
-=-=-=-=-=-=-=-=-=-=-=-