On 2/20/24 04:41, Wu, Jiaxin wrote:
>> 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.
>>
>
> I think here data race won't cause the undefined behavior:
>
> The read operation in one processor might see the 1) old value (MAX_UINT32)
> or 2) the new value (CpuIndex), but both behaviors are explicable:
>
> If case 1, that's ok continue do the lock comxchg since BspIndex hasn't been
> elected.
> If case 2, which means another processor has already atomic invalid or write
> the BspIndex, then existing processor absolutely can skip the lock comxchg.
>
> if (mSmmMpSyncData->BspIndex == MAX_UINT32) {
> InterlockedCompareExchange32 (
> (UINT32 *)&mSmmMpSyncData->BspIndex,
> MAX_UINT32,
> (UINT32)CpuIndex
> );
> }
>
>> If you consider PiSmmCpuDxeSmm's usage of "volatile" to be "atomic",
>> then that would remove the data race; but volatile is not necessarily
>> atomic.
>>
>
> I *never* do the assumption: "volatile" is "atomic".
> BspIndex is volatile, I think it only can ensure the compiler behavior, not
> processor itself:
> 1. Compiler will not optimize this variable. All reads and writes to this
> variable will be done directly to memory, not using cached values in
> registers. But it doesn't prevent the CPU from caching that variable in its
> own internal caches.
> 2. "volatile" can prevent the compiler from optimizing and reordering
> instructions, it can't prevent the processor from reordering instructions,
> and can't guarantee the atomicity of operations.
>
> For processor reordering, I think Ray explained that reordering won't happen.
>
>> 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.
>
> Here, the case is different to the above three "Double-checked locking".
> Cache coherency can make sure the read value for check is only 2 cases as I
> explained above.
> The only possible impact is the AP behavior: AP won't pending on lock
> comxchg, while it will continue do the following code logic after if check:
> for example performs the APHandler. But it won't has the negative-impact
> since it has the timeout BSP check in APHandler. And even failure of that, we
> still has the error handling to sync out the existing AP due to don't know
> the BSP:
>
> SmmCpuSyncCheckOutCpu (mSmmMpSyncData->SyncContext, CpuIndex);
> return;
I don't have other objections; feel free to proceed.
Thanks
Laszlo
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#115660): https://edk2.groups.io/g/devel/message/115660
Mute This Topic: https://groups.io/mt/104094808/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
