From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail02.groups.io (mail02.groups.io [66.175.222.108]) by spool.mail.gandi.net (Postfix) with ESMTPS id 0B350D81164 for ; Tue, 20 Feb 2024 16:22:07 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=i9WAJtrK7zaul5SwxDxigUXeeR6idBS4G5fM+lFKzjo=; c=relaxed/simple; d=groups.io; h=Message-ID:Date:MIME-Version:Subject:To:Cc:References:From:In-Reply-To:Precedence:List-Subscribe:List-Help:Sender:List-Id:Mailing-List:Delivered-To:Reply-To:List-Unsubscribe-Post:List-Unsubscribe:Content-Language:Content-Type:Content-Transfer-Encoding; s=20140610; t=1708446126; v=1; b=J2bTyYFOHi3kv86X+R54WL2yidTg6l79ib6Ma+s9yzSI6zLvhwUF4EGr6/90lgEcqpXrH2cw rMju6Vn4LhcTF6tVRhJ+wuKHAiYhDZuF+/y7uCKeRpmd1tBByQz520oX8ULCkwovbgcs4kjQpPR Cijr4u9w9wKWPO5QhUqJRR+I= X-Received: by 127.0.0.2 with SMTP id Ml3yYY7687511xE4AqTAfEZx; Tue, 20 Feb 2024 08:22:06 -0800 X-Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by mx.groups.io with SMTP id smtpd.web11.17426.1708446125953116819 for ; Tue, 20 Feb 2024 08:22:06 -0800 X-Received: from mimecast-mx02.redhat.com (mx-ext.redhat.com [66.187.233.73]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-460-7_eRL5jVPxiGWbW612nHzg-1; Tue, 20 Feb 2024 11:22:02 -0500 X-MC-Unique: 7_eRL5jVPxiGWbW612nHzg-1 X-Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.rdu2.redhat.com [10.11.54.5]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id 2E0C529AC039; Tue, 20 Feb 2024 16:22:02 +0000 (UTC) X-Received: from [10.39.192.75] (unknown [10.39.192.75]) by smtp.corp.redhat.com (Postfix) with ESMTPS id CEED610F4C; Tue, 20 Feb 2024 16:22:00 +0000 (UTC) Message-ID: <9dda0ae1-6621-05ed-822c-82e48c7f1d21@redhat.com> Date: Tue, 20 Feb 2024 17:21:59 +0100 MIME-Version: 1.0 Subject: Re: [edk2-devel] [PATCH v1 2/2] UefiCpuPkg/PiSmmCpuDxeSmm: Check BspIndex first before lock cmpxchg To: devel@edk2.groups.io, jiaxin.wu@intel.com, "Ni, Ray" Cc: "Dong, Eric" , "Zeng, Star" , Gerd Hoffmann , "Kumar, Rahul R" References: <20240201112001.14416-1-jiaxin.wu@intel.com> <20240201112001.14416-3-jiaxin.wu@intel.com> <1aef2255-dd9c-95c0-ab0d-100e00f8e10c@redhat.com> From: "Laszlo Ersek" In-Reply-To: X-Scanned-By: MIMEDefang 3.4.1 on 10.11.54.5 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Precedence: Bulk List-Subscribe: List-Help: Sender: devel@edk2.groups.io List-Id: Mailing-List: list devel@edk2.groups.io; contact devel+owner@edk2.groups.io Reply-To: devel@edk2.groups.io,lersek@redhat.com List-Unsubscribe-Post: List-Unsubscribe=One-Click List-Unsubscribe: X-Gm-Message-State: QffwCWzOAR7vWi4nUp5SKelSx7686176AA= Content-Language: en-US Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable X-GND-Status: LEGIT Authentication-Results: spool.mail.gandi.net; dkim=pass header.d=groups.io header.s=20140610 header.b=J2bTyYFO; spf=pass (spool.mail.gandi.net: domain of bounce@groups.io designates 66.175.222.108 as permitted sender) smtp.mailfrom=bounce@groups.io; dmarc=fail reason="SPF not aligned (relaxed), DKIM not aligned (relaxed)" header.from=redhat.com (policy=none) 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. >> >=20 > I think here data race won't cause the undefined behavior: >=20 > The read operation in one processor might see the 1) old value (MAX_UINT3= 2) or 2) the new value (CpuIndex), but both behaviors are explicable: >=20 > If case 1, that's ok continue do the lock comxchg since BspIndex hasn't b= een elected. > If case 2, which means another processor has already atomic invalid or wr= ite the BspIndex, then existing processor absolutely can skip the lock comx= chg. >=20 > if (mSmmMpSyncData->BspIndex =3D=3D MAX_UINT32) { > InterlockedCompareExchange32 ( > (UINT32 *)&mSmmMpSyncData->BspIndex, > MAX_UINT32, > (UINT32)CpuIndex > ); > } >=20 >> If you consider PiSmmCpuDxeSmm's usage of "volatile" to be "atomic", >> then that would remove the data race; but volatile is not necessarily >> atomic. >> >=20 > I *never* do the assumption: "volatile" is "atomic". > BspIndex is volatile, I think it only can ensure the compiler behavior, n= ot 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 regis= ters. 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 ins= tructions, it can't prevent the processor from reordering instructions, and= can't guarantee the atomicity of operations.=20 >=20 > For processor reordering, I think Ray explained that reordering won't hap= pen.=20 >=20 >> 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. >=20 > 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 com= xchg, while it will continue do the following code logic after if check: fo= r example performs the APHandler. But it won't has the negative-impact sinc= e it has the timeout BSP check in APHandler. And even failure of that, we s= till has the error handling to sync out the existing AP due to don't know t= he BSP: >=20 > SmmCpuSyncCheckOutCpu (mSmmMpSyncData->SyncContext, CpuIndex); > return; I don't have other objections; feel free to proceed. Thanks Laszlo -=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D- 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] -=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-