Re: MemoryFence()


Paolo Bonzini <pbonzini@...>
 

On 05/02/21 18:31, Laszlo Ersek wrote:
On 02/05/21 18:21, Paolo Bonzini wrote:
On 05/02/21 17:56, Laszlo Ersek wrote:
    221  SPIN_LOCK *
    222  EFIAPI
    223  ReleaseSpinLock (
    224    IN OUT  SPIN_LOCK                 *SpinLock
    225    )
    226  {
    227    SPIN_LOCK    LockValue;
    228
    229    ASSERT (SpinLock != NULL);
    230
    231    LockValue = *SpinLock;
    232    ASSERT (LockValue == SPIN_LOCK_ACQUIRED || LockValue ==
SPIN_LOCK_RELEASED);
    233
    234    _ReadWriteBarrier ();
    235    *SpinLock = SPIN_LOCK_RELEASED;
    236    _ReadWriteBarrier ();
    237
    238    return SpinLock;
    239  }

Fishy. I would have implemented it with another
InterlockedCompareExchangePointer(), and maybe ASSERT()ed on the
original value returned by the InterlockedCompareExchangePointer().
That would be overkill.  However, it *is* buggy because it is missing a
(processor) barrier on non-x86 architectures and has a useless barrier
after the store.  Instead it should be just this:

    ReleaseMemoryFence ();
    *SpinLock = SPIN_LOCK_RELEASED;
No concern that the store might not be atomic?
Not as long as it's a pointer or smaller.

Paolo

Join rfc@edk2.groups.io to automatically receive all group messages.