> > 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/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
