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

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

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/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-