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 5D2E9AC07ED for ; Tue, 6 Feb 2024 12:46:23 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=KwQi50Y06i8bvU/Eyy8Kymn5/sl3Ad7QHPNS0OncmTU=; 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=1707223581; v=1; b=YVXZMYdl/raHMRQVjWgHMHrmB3rWVXWKd+V0QpfnQB0GKnBT3oEUbMHF55HEizlbFFn4TNgh uvvYOcxp8u/PZDP5nPoyOa7lNZrlbypXWsFbXoG0xCHqzqGSGtTQgX9kRNx/isd/pckSfrF+gYR UJ7dNTP9AEVDa4ePozn3iupA= X-Received: by 127.0.0.2 with SMTP id Rf6rYY7687511xNWPAX8UB9x; Tue, 06 Feb 2024 04:46:21 -0800 X-Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by mx.groups.io with SMTP id smtpd.web10.20583.1707223581061872530 for ; Tue, 06 Feb 2024 04:46:21 -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-609-Ht_Ku6vIMuuFla-cKGs33A-1; Tue, 06 Feb 2024 07:46:19 -0500 X-MC-Unique: Ht_Ku6vIMuuFla-cKGs33A-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 B49E5381644C; Tue, 6 Feb 2024 12:46:18 +0000 (UTC) X-Received: from [10.39.195.129] (unknown [10.39.195.129]) by smtp.corp.redhat.com (Postfix) with ESMTPS id AF58D1103A; Tue, 6 Feb 2024 12:46:17 +0000 (UTC) Message-ID: <1aef2255-dd9c-95c0-ab0d-100e00f8e10c@redhat.com> Date: Tue, 6 Feb 2024 13:46:16 +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, ray.ni@intel.com, "Wu, Jiaxin" 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> 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: irlxDNa1gx8s4gar5WaanAfvx7686176AA= 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=YVXZMYdl; dmarc=fail reason="SPF not aligned (relaxed), DKIM not aligned (relaxed)" header.from=redhat.com (policy=none); spf=pass (spool.mail.gandi.net: domain of bounce@groups.io designates 66.175.222.108 as permitted sender) smtp.mailfrom=bounce@groups.io 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. >=20 > Laszlo, > Test-and-set is implemented in x86 through the cmpxchg instruction. >=20 > 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 >=20 >> >> 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 >> >> >> >> >> >=20 >=20 >=20 >=20 >=20 >=20 -=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 (#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] -=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-