Re: MemoryFence()


Laszlo Ersek
 

On 02/05/21 18:34, Paolo Bonzini wrote:
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.
This is why I'd like someone else to write the primitives :)

- Where is this documented?

- Is it architecture-dependent?

- Is it alignment-dependent?

etc... :)

(I'm not challenging your statement of course; I know I should have
known, just not sure where from...)

Thanks,
Laszlo

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