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; ler...@redhat.com
> Cc: Dong, Eric <eric.d...@intel.com>; Ni, Ray <ray...@intel.com>; Zeng, Star
> <star.z...@intel.com>; Gerd Hoffmann <kra...@redhat.com>; Kumar, Rahul R
> <rahul.r.ku...@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/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
