Re: [edk2-devel] [PATCH v1 2/2] UefiCpuPkg/PiSmmCpuDxeSmm: Check BspIndex first before lock cmpxchg

["Laszlo Ersek" <lersek@redhat.com>]

On 2/6/24 02:40, Ni, Ray wrote:
>>
>> This patch makes me uncomfortable. I understand what it intends to do,
>> and the intent is not wrong, but we're again treating "volatile UINT32"
>> as atomic, and non-reorderable against the
>> InterlockedCompareExchange32(). This kind of "optimization" is what
>> people write cautionary tales about, later. It would be nice to see a
>> semi-formal *proof* that this cannot backfire.
> 
> Laszlo,
> Test-and-set is implemented in x86 through the cmpxchg instruction.
> 
> The patch follows https://en.wikipedia.org/wiki/Test_and_test-and-set
> and adds a "test" before "test-and-set".

That may or may not work without special additional guarantees.

From C11 "5.1.2.4 Multi-threaded executions and data races":

- paragraph 4: "Two expression evaluations conflict if one of them
modifies a memory location and the other one reads or modifies the same
memory location."

- paragraph 25: "The execution of a program contains a data race if it
contains two conflicting actions in different threads, at least one of
which is not atomic, and neither happens before the other. Any such data
race results in undefined behavior."

In this case, the outer condition (which reads the volatile UINT32
object "mSmmMpSyncData->BspIndex") is one access. It is a read access,
and it is not atomic. The other access is the
InterlockedCompareExchange32(). It is a read-write access, and it is
atomic. There is no "happens before" relationship enforced between both
accesses.

Therefore, this is a data race: the pre-check on one CPU conflicts with
the InterlockedCompareExchange32() on another CPU, the pre-check is not
atomic, and there is no happens-before between them.

A data race means undefined behavior. In particular, if there is a data
race, then there are no guarantees of sequential consistency, so the
observable values in the object could be totally inexplicable.

If you consider PiSmmCpuDxeSmm's usage of "volatile" to be "atomic",
then that would remove the data race; but volatile is not necessarily
atomic.

Since you linked a wikipedia article, I'll link three articles that
describe a similar (nearly the same) technique called "double-checked
locking". In the general case, that technique is broken.

http://www.cs.umd.edu/~pugh/java/memoryModel/DoubleCheckedLocking.html
https://en.wikipedia.org/wiki/Double-checked_locking
https://preshing.com/20130930/double-checked-locking-is-fixed-in-cpp11/

It can be made work in bare-bones C, but it requires explicit fences /
memory barriers.

Again: I'm not trying to block this patch, I just remain unconvinced
that it is safe. (And if it is safe, then it relies on such properties /
guarantees of Intel processors that should be documented in the code.)

Laszlo


> 
>>
>> Either way: I'm not trying to block the patch. If Ray is happy with it,
>> I don't object. OVMF implements PlatformSmmBspElection() in
>> "OvmfPkg/Library/SmmCpuPlatformHookLibQemu/SmmCpuPlatformHookLi
>> bQemu.c",
>> always returning EFI_SUCCESS [*], so the code that is being modified
>> here cannot be reached in OVMF.
>>
>> [*] commit 43df61878d94 ("OvmfPkg: enable SMM Monarch Election in
>> PiSmmCpuDxeSmm", 2020-03-04)
>>
>> Laszlo
>>
>>
>>
>>
>>
> 
> 
> 
> 
> 
> 



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#115157): https://edk2.groups.io/g/devel/message/115157
Mute This Topic: https://groups.io/mt/104094808/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-