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 C5675780091 for ; Tue, 7 Nov 2023 09:47:24 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=ehT7MHYFNl6KgaAuN4pR3y2SLeLb4216s0vFxhz0x4s=; 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=1699350443; v=1; b=G5ACYGr8/NI4skqNxzEqYa8bZmbSmCKEdfarsyyyUOzpvQTnW5wQjvNbODkKXsquwRzCiPXf yNNrP/MC/u0UqP8Txzd+/p/7uWVREBqcApVPsbwdv4wq3GVu8/f+gPrLdl09M0O0g80pEro6k5C ELXrwXq22FCkHUiy0VTjjTIU= X-Received: by 127.0.0.2 with SMTP id 8DmNYY7687511xBS59BOKyzp; Tue, 07 Nov 2023 01:47:23 -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.6739.1699350442650407453 for ; Tue, 07 Nov 2023 01:47:22 -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-259-w9QHl34DP3CiCtrGRML9kA-1; Tue, 07 Nov 2023 04:47:15 -0500 X-MC-Unique: w9QHl34DP3CiCtrGRML9kA-1 X-Received: from smtp.corp.redhat.com (int-mx02.intmail.prod.int.rdu2.redhat.com [10.11.54.2]) (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 50483280C2A0; Tue, 7 Nov 2023 09:47:15 +0000 (UTC) X-Received: from [10.39.193.64] (unknown [10.39.193.64]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 12EAC40C6EB9; Tue, 7 Nov 2023 09:47:13 +0000 (UTC) Message-ID: <260f2821-fc41-9658-583a-ad2e8cbc7e7e@redhat.com> Date: Tue, 7 Nov 2023 10:47:12 +0100 MIME-Version: 1.0 Subject: Re: [edk2-devel] [PATCH v1 2/7] UefiCpuPkg/PiSmmCpuDxeSmm: Reduce times of BSP and AP Sync for SMM Exit To: devel@edk2.groups.io, jiaxin.wu@intel.com Cc: Eric Dong , Ray Ni , Zeng Star , Rahul Kumar , Gerd Hoffmann References: <20231103153012.3704-1-jiaxin.wu@intel.com> <20231103153012.3704-3-jiaxin.wu@intel.com> From: "Laszlo Ersek" In-Reply-To: <20231103153012.3704-3-jiaxin.wu@intel.com> X-Scanned-By: MIMEDefang 3.4.1 on 10.11.54.2 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: cA0zDOC8oc9HxG0EhxLFCJGJx7686176AA= 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=G5ACYGr8; 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 11/3/23 16:30, Wu, Jiaxin wrote: > After review, there are unnecessary steps for BSP and AP sync for SMM > exit. This patch is to reduce one round BSP and AP sync so as to improve > SMI performance: > BSP: WaitForAllAPs <-- AP: ReleaseBsp > BSP: ReleaseAllAPs --> AP: WaitForBsp >=20 > Change-Id: Ic33f42f3daa7ff1847e524d0c3d9cd4fcdefa61b (1) please drop this > Cc: Eric Dong > Cc: Ray Ni > Cc: Zeng Star > Cc: Rahul Kumar > Cc: Gerd Hoffmann > Signed-off-by: Jiaxin Wu > --- > UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c | 44 +++++++++++++++++++----------= ------ > 1 file changed, 24 insertions(+), 20 deletions(-) >=20 > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c b/UefiCpuPkg/PiSmmCpuD= xeSmm/MpService.c > index e96c7f51d6..5a42a5dd12 100644 > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c > @@ -665,11 +665,13 @@ BSPHandler ( > // > *mSmmMpSyncData->AllCpusInSync =3D TRUE; > ApCount =3D LockdownSemaphore (mSmmMpSyncData= ->Counter) - 1; > =20 > // > - // Wait for all APs to get ready for programming MTRRs > + // Wait for all APs: > + // 1. Make sure all Aps have set the Present. (2.1) This comment ("set the Present") is confusing. Either say "set the Present *flag*", or just "set Present". (2.2) This comment update is unrelated to the performance optimization; it just documents existent *and preserved* behavior. It's very confusing to see this in the same patch. The comment documents behavior that *precedes* the "Wait for something to happen" loop and the "Invoke the scheduled procedure" action in APHandler(), but the performance optimization is *after* that loop. (As the subject says, the optimization is for the exit path.) So please split the comment update to a separate patch. > + // 2. Get ready for programming MTRRs. > // > WaitForAllAPs (ApCount); > =20 > if (SmmCpuFeaturesNeedConfigureMtrrs ()) { > // > @@ -768,16 +770,16 @@ BSPHandler ( > // Notify all APs to exit > // > *mSmmMpSyncData->InsideSmm =3D FALSE; > ReleaseAllAPs (); > =20 > - // > - // Wait for all APs to complete their pending tasks > - // > - WaitForAllAPs (ApCount); > - > if (SmmCpuFeaturesNeedConfigureMtrrs ()) { > + // > + // Wait for all APs to complete their pending tasks > + // > + WaitForAllAPs (ApCount); > + > // > // Signal APs to restore MTRRs > // > ReleaseAllAPs (); > =20 > @@ -789,23 +791,23 @@ BSPHandler ( > =20 > // > // Wait for all APs to complete MTRR programming > // > WaitForAllAPs (ApCount); > + > + // > + // Signal APs to Reset states/semaphore for this processor > + // > + ReleaseAllAPs (); > } > =20 > // > // Stop source level debug in BSP handler, the code below will not be > // debugged. > // > InitializeDebugAgent (DEBUG_AGENT_INIT_EXIT_SMI, NULL, NULL); > =20 > - // > - // Signal APs to Reset states/semaphore for this processor > - // > - ReleaseAllAPs (); > - > // > // Perform pending operations for hot-plug > // > SmmCpuUpdate (); > =20 > @@ -941,10 +943,12 @@ APHandler ( > *(mSmmMpSyncData->CpuData[CpuIndex].Present) =3D TRUE; > =20 > if ((SyncMode =3D=3D SmmCpuSyncModeTradition) || SmmCpuFeaturesNeedCon= figureMtrrs ()) { > // > // Notify BSP of arrival at this point > + // 1. Set the Present. (3) Same as (2) (both sub-points). > + // 2. Get ready for programming MTRRs. > // > ReleaseBsp (mSmmMpSyncData->CpuData[BspIndex].Run); > } > =20 > if (SmmCpuFeaturesNeedConfigureMtrrs ()) { > @@ -1033,21 +1037,21 @@ APHandler ( > // > // Restore OS MTRRs > // > SmmCpuFeaturesReenableSmrr (); > MtrrSetAllMtrrs (&Mtrrs); > - } > =20 > - // > - // Notify BSP the readiness of this AP to Reset states/semaphore for t= his processor > - // > - ReleaseBsp (mSmmMpSyncData->CpuData[BspIndex].Run); > + // > + // Notify BSP the readiness of this AP to Reset states/semaphore for= this processor > + // > + ReleaseBsp (mSmmMpSyncData->CpuData[BspIndex].Run); > =20 > - // > - // Wait for the signal from BSP to Reset states/semaphore for this pro= cessor > - // > - WaitForBsp (mSmmMpSyncData->CpuData[CpuIndex].Run); > + // > + // Wait for the signal from BSP to Reset states/semaphore for this p= rocessor > + // > + WaitForBsp (mSmmMpSyncData->CpuData[CpuIndex].Run); > + } > =20 > // > // Reset states/semaphore for this processor > // > *(mSmmMpSyncData->CpuData[CpuIndex].Present) =3D FALSE; (4.1) If SmmCpuFeaturesNeedConfigureMtrrs() returns TRUE, then: - the AP logic in unchanged - the BSP logic changes, because InitializeDebugAgent() is invoked with the APs released (4.2) If SmmCpuFeaturesNeedConfigureMtrrs() returns FALSE, then: - the AP logic omits an empty rendezvous with the BSP - the BSP logic also omits the empty rendezvous with the APs, but it's cheating: the rendezvous on the BSP side is originally *not* empty even when there is no need to configure MTRRs -- the rendezvous protects the InitializeDebugAgent() call. (4.3) So, we need yet another precursor patch for this performance optimization. Namely, you need to show that calling InitializeDebugAgent() with DEBUG_AGENT_INIT_EXIT_SMI, with all the APs released, is safe. Put differently, a precursor patch is needed that does nothing other than *reordering* the ReleaseAllAPs() and InitializeDebugAgent() calls in BSPHandler(). You need to justify and document that change in a separate patch, and once that is in place, you can employ this performance optimization. (5) After the performance optimization, the comments on the final semaphore operations inside the MTRR branch are misleading. Those comments say Signal APs to Reset states/semaphore for this processor on the BSP side, and they say Notify BSP the readiness of this AP to Reset states/semaphore for this processor and Wait for the signal from BSP to Reset states/semaphore for this processor on the AP side. The problem is, this structuring and this wording imply that "state resetting" is *conditional* on MTRR programming, which is of course not true. State resetting must happen in all cases, it's only the *additional rendezvous* that is necessary for, and conditional on, MTRR configuration. Put differently: - Before the performance optimization, we have a rendezvous that *unconditionally* happens (it is independent of MTRR configuration). The good side of that is that we can directly tie this unconditional rendezvous to the final "state resetting", and therefore the comments on the rendezvous logic are correct, before the patch. However, the bad side (before the patch) is that this approach wastes performance, when MTRR config is unneeded. - After the performance optimization, the rendezvous occurs only when it is really required, for MTRR config. This is good. The bad side is that the original comments no longer fit -- the rendezvous is now tied to MTRR config, *not* the final state resetting! All in all, the comments on the BSP side should be: if (SmmCpuFeaturesNeedConfigureMtrrs ()) { // // Sync with the APs just before MTRR restoration // WaitForAllAPs (ApCount); ReleaseAllAPs (); // // Restore OS MTRRs // SmmCpuFeaturesReenableSmrr (); MtrrSetAllMtrrs (&Mtrrs); // // Sync with the APs just after MTRR restoration // WaitForAllAPs (ApCount); ReleaseAllAPs (); } And on the AP side: if (SmmCpuFeaturesNeedConfigureMtrrs ()) { // // Sync with the BSP just before MTRR restoration // ReleaseBsp (mSmmMpSyncData->CpuData[BspIndex].Run); WaitForBsp (mSmmMpSyncData->CpuData[CpuIndex].Run); // // Restore OS MTRRs // SmmCpuFeaturesReenableSmrr (); MtrrSetAllMtrrs (&Mtrrs); // // Sync with the BSP just after MTRR restoration // ReleaseBsp (mSmmMpSyncData->CpuData[BspIndex].Run); WaitForBsp (mSmmMpSyncData->CpuData[CpuIndex].Run); } * In summary: - First patch: update the comment in the top part of the BSP and AP handlers. - Second patch: invert the order of ReleaseAllAPs() and InitializeDebugAgent(DEBUG_AGENT_INIT_EXIT_SMI) in BSPHandler(). Furthermore, explain in the commit message that this reordering is valid. - Third patch: the perf optimization. Restrict the rendezvous to when MTRR configuration is needed. While at it, (a) explain the nature of the opimization in the commit message (i.e., that without MTRR config, we have a superfluous rendezvous), because the current commit message is useless, (b) clean up the comments on the before and after synchronizations (see proposal above). 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 (#110829): https://edk2.groups.io/g/devel/message/110829 Mute This Topic: https://groups.io/mt/102366299/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-