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 EFDF7D801B2 for ; Tue, 12 Dec 2023 00:29:55 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=N6hDF33fhFOyOyOk68Jx4giN+dODkJn5IHTA6+SSmVw=; 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=1702340994; v=1; b=br34gDX+UICthe4bZj9LgnUnJXoF3lmlzjIq62V0mTMJXXwb4QRlEWxen3duOFr0Pll7oT4h ne8uagNWt5IzPqZJC8Pr5KksQXmTjG+UzMXn/wMGRLY4hQS2hKbVkK65YDthKnIPnU5qsEjYfVY jROFneDdN9M/WaS3nhxWTEgg= X-Received: by 127.0.0.2 with SMTP id dncgYY7687511xJdwnjbZBfl; Mon, 11 Dec 2023 16:29:54 -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.web11.1023.1702340993911427317 for ; Mon, 11 Dec 2023 16:29:54 -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-691-DGs5TqShOYiJI8LgQ_ENZA-1; Mon, 11 Dec 2023 19:29:49 -0500 X-MC-Unique: DGs5TqShOYiJI8LgQ_ENZA-1 X-Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.rdu2.redhat.com [10.11.54.4]) (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 7C8373C025CC; Tue, 12 Dec 2023 00:29:48 +0000 (UTC) X-Received: from [10.39.192.110] (unknown [10.39.192.110]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 392022026D6F; Tue, 12 Dec 2023 00:29:47 +0000 (UTC) Message-ID: <0bf2e6d2-a2df-7be5-ca14-6aab2c65a71b@redhat.com> Date: Tue, 12 Dec 2023 01:29:46 +0100 MIME-Version: 1.0 Subject: Re: [edk2-devel] [Patch V2] UefiCpuPkg/PiSmmCpuDxeSmm: SmmCpuRendezvous ensure all Aps in Present. To: devel@edk2.groups.io, jiaxin.wu@intel.com, "Xie, Yuanhao" Cc: "Li, Zhihao" , "Ni, Ray" , "Kumar, Rahul R" , Gerd Hoffmann References: <20231113054714.1729-1-yuanhao.xie@intel.com> From: "Laszlo Ersek" In-Reply-To: X-Scanned-By: MIMEDefang 3.4.1 on 10.11.54.4 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: FwbmHudHhgwmQhWXb6IBWpF4x7686176AA= 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=br34gDX+; 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 12/6/23 04:35, Wu, Jiaxin wrote: >> (1) Here's why I don't like this: >> >> we already have a function that is supposed to do this, and it is >> SmmWaitForApArrival(). >> >> SmmWaitForApArrival() is called in two contexts. One, in BSPHandler(). >> Two, here. >> >> Consider the following condition: >> >> (SyncMode =3D=3D SmmCpuSyncModeTradition) || >> SmmCpuFeaturesNeedConfigureMtrrs () >> >> If this condition evaluates to true, then BSPHandler() calls >> SmmWaitForApArrival(), and SmmCpuRendezvous() doesn't. >> >> (This is what the "else" branch in SmmCpuRendezvous() states, in a >> comment, too.) >> >> And if the condition evaluates to false, then SmmCpuRendezvous() calls >> SmmWaitForApArrival(), and BSPHandler() doesn't. >> >> This patch adds extra waiting logic to *one* of both call sites. And I >> don't understand why the *other* call site (in BSPHandler()) does not >> need the exact same logic. >> >> In my opinion, this is a sign that SmmWaitForApArrival() isn't "strong >> enough". It is not doing all of the work. >> >> In my opinion, *both* call sites should receive this logic (i.e., ensure >> that all AP's are "present"), but then in turn, the additional waiting / >> checking should be pushed down into SmmWaitForApArrival(). >> >> What's your opinion on that? >=20 >=20 > Existing SmmWaitForApArrival() only make sure all Aps enter SMI except SM= I blocked & disabled Aps, not consider the "Present" state. I think this is= the original implementation purpose. It won't require all Aps must set the= Present flag. >=20 > As you also commented there is a later loop for the Present flag: > WaitForAllAPs (ApCount) >=20 > Here, i still prefer to keep existing way instead of making SmmWaitForApA= rrival return until all aps set the Present flag, because that will be dupl= icate work within SmmWaitForApArrival() & existing WaitForAllAPs (). We ca= n't delete the WaitForAllAPs for the later sync to make sure all APs to get= ready for programming MTRRs. MTRRs programming need all CPUs in the same s= tart line.=20 >=20 > WaitForAllAPs() has two purpose: > 1. Make sure all Aps have set the Present. > 2. Get ready for programming MTRRs to make sure cpus in the same start= line. >=20 > if so, that will be better as existing logic, it can also save some time = for the Present flag check in SmmWaitForApArrival OK, this argument makes sense to me. I didn't realize that WaitForAllAPs() -- called by BSPHandler() after calling SmmWaitForApArrival() -- already *effectively* ensured that the APs had their Present flag set! That happens because: (a) WaitForAllAPs() pends the "Run" semaphore of each AP. (b) The APHandler() function sets the Present flag *first*. (c) If (SyncMode =3D=3D SmmCpuSyncModeTradition) || SmmCpuFeaturesNeedConfigureMtrrs () is true, then APHandler() posts the "Run" semaphore, *second*. Therefore, once WaitForAllAPs() has acquired all AP "Run" semaphores, all AP Present flags must be set. This is not obvious at all, but it looks correct. Therefore I agree that your patch does not introduce asymmetry between SmmCpuRendezvous() and BSPHandler(). Instead, your patch eliminates asymmetry, because now SmmCpuRendezvous() will wait for the Present flags of the APs (if the above-quoted condition is false), similarly to how BSPHandler already does (if the condition is true). Now, I have not had the time yet to re-review your patch set [PATCH v3 0/6] Refine SMM CPU Sync flow and abstract SmmCpuSyncLib As far as I remember (from v1), that patch set reorganizes exactly these "Run" semaphore release/acquire patterns. (3) Can you confirm that your v3 patch set will not invalidate this discussion? I.e., can you confirm that WaitForAllAPs() will *still* ensure, via the Run semaphores, that the Present flags will have been set? (4) Please add the following to the commit message: ------- BSPHandler -> { SmmWaitForApArrival, WaitForAllAPs } already awaits that the Present flag is set for all APs, namely via the AP Run semaphores. Therefore this patch ensures symmetry between BSPHandler (when [1] is true) and SmmCpuRendezvous() (when [1] is false). [1] (SyncMode =3D=3D SmmCpuSyncModeTradition) || SmmCpuFeaturesNeedConfigureMtrrs () ------- More comments below: >=20 >> >> (2) I notice that a similar "busy loop", waiting for Present flags, is >> in BSPHandler(). >> >> Do you think we could call CpuPause() in all such "busy loops" (just >> before the end of the "while" body)? I think that's supposed to improve >> the system's throughput, considered as a whole. The function's comment >> says, >> >> Requests CPU to pause for a short period of time. Typically used in MP >> systems to prevent memory starvation while waiting for a spin lock. >> >=20 > Do you mean the below WaitForAllAPs()? There is already has the CpuPause = check within WaitForSemaphore(). >=20 > // > // Wait for all APs to get ready for programming MTRRs > // > WaitForAllAPs (ApCount); Yes, that's the pattern we should follow here. The call chain BSPHandler -> WaitForAllAPs -> WaitForSemaphore -> CpuPause already calls CpuPause() when condition [1] is true. That's the pattern we should follow. When condition [1] is false, your patch implements the wait for the Present flags in SmmCpuRendezvous(), but there we have no CpuPause(). So, we could / should add it, right at the end of the while (TRUE) loop. Summary of my requests: - the patch seems good, but please confirm that your v3 sync rework will leave the predicate intact that WaitForAllAPs() ensures the Present flags via the Run semaphores - please extend the commit message with the paragraph about symmetry between BSPHandler and SmmCpuRendezvous - please consider adding CpuPause to the end of the "while (TRUE)" loop. (BTW, the patch is not really (or usefully) testable with OVMF. OVMF sets PcdCpuSmmSyncMode to 1 (=3D SmmCpuSyncModeRelaxedAp) by default; however, OVMF's SmmControl2Dxe driver sets the PCD back to 0 (=3D SmmCpuSyncModeTradition) if QEMU supports "broadcast SMI". Given that broadcast SMI is the *only* reliable method with QEMU/KVM + OVMF, and given that this feature / method has been available for ages now, testing any SMM-related change in edk2 with that feature manually disabled in QEMU is arguably useless.) 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 (#112345): https://edk2.groups.io/g/devel/message/112345 Mute This Topic: https://groups.io/mt/102556528/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/19134562= 12/xyzzy [rebecca@openfw.io] -=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-