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 4B0B094135C for ; Tue, 7 Nov 2023 08:28:47 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=zC2rtKEn26haSdQ0WobqfY0o/zDzUAQhFR6ygt+yQC0=; 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=1699345725; v=1; b=M0BWXAoBo8ePmSe75prDdrxp0gjkzTocqLtxSYb80EcERI+SfoCOCN/hdIYzkLYS3JsihT89 TbaB+m1Hm2ecIOQmc9hdggaP4KD7BCGoZBDSAzDa0FJ+h4uIvbh3nnZ+YxNIr7QAJ6dL5wZhXan JEuRNhZWT/y1DAqiRtNoANms= X-Received: by 127.0.0.2 with SMTP id NrnWYY7687511x0OswuznJFI; Tue, 07 Nov 2023 00:28:45 -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.5892.1699345725078548761 for ; Tue, 07 Nov 2023 00:28:45 -0800 X-Received: from mimecast-mx02.redhat.com (mimecast-mx02.redhat.com [66.187.233.88]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-526-xg6dSRhvNsKZOzKUcTj8og-1; Tue, 07 Nov 2023 03:28:41 -0500 X-MC-Unique: xg6dSRhvNsKZOzKUcTj8og-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 89476847726; Tue, 7 Nov 2023 08:28:40 +0000 (UTC) X-Received: from [10.39.193.64] (unknown [10.39.193.64]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 5230F2026D66; Tue, 7 Nov 2023 08:28:39 +0000 (UTC) Message-ID: <78fae77f-de4d-528b-cbaf-65e8969a04ee@redhat.com> Date: Tue, 7 Nov 2023 09:28:37 +0100 MIME-Version: 1.0 Subject: Re: [edk2-devel] [PATCH v1 1/7] UefiCpuPkg/PiSmmCpuDxeSmm: Optimize Semaphore Sync between BSP and AP 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-2-jiaxin.wu@intel.com> From: "Laszlo Ersek" In-Reply-To: <20231103153012.3704-2-jiaxin.wu@intel.com> 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: XXsm9WbpkNgFsVL6CmWJaOlVx7686176AA= 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=M0BWXAoB; 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: > This patch is to define 3 new functions (WaitForBsp & ReleaseBsp & > ReleaseOneAp) used for the semaphore sync between BSP & AP. With the > change, BSP and AP Sync flow will be easy understand as below: > BSP: ReleaseAllAPs or ReleaseOneAp --> AP: WaitForBsp > BSP: WaitForAllAPs <-- AP: ReleaseBsp >=20 > Change-Id: I0fb25e26e1015e918800f4d8d62e5276dcd5b5b1 (1) please remove this; not useful upstream > Cc: Eric Dong > Cc: Ray Ni > Cc: Zeng Star > Cc: Rahul Kumar > Cc: Gerd Hoffmann > Signed-off-by: Jiaxin Wu > --- > UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c | 72 ++++++++++++++++++++++++++++-= ------ > 1 file changed, 58 insertions(+), 14 deletions(-) >=20 > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c b/UefiCpuPkg/PiSmmCpuD= xeSmm/MpService.c > index 25d058c5b9..e96c7f51d6 100644 > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/MpService.c > @@ -120,10 +120,11 @@ LockdownSemaphore ( > =20 > return Value; > } > =20 > /** > + Used for BSP to wait all APs. > Wait all APs to performs an atomic compare exchange operation to relea= se semaphore. > =20 > @param NumberOfAPs AP number > =20 > **/ > @@ -139,10 +140,11 @@ WaitForAllAPs ( > WaitForSemaphore (mSmmMpSyncData->CpuData[BspIndex].Run); > } > } > =20 > /** > + Used for BSP to release all APs. > Performs an atomic compare exchange operation to release semaphore > for each AP. > =20 > **/ > VOID > @@ -157,10 +159,52 @@ ReleaseAllAPs ( > ReleaseSemaphore (mSmmMpSyncData->CpuData[Index].Run); > } > } > } > =20 > +/** > + Used for BSP to release one AP. > + > + @param ApSem IN: 32-bit unsigned integer > + OUT: original integer + 1 > +**/ > +VOID > +ReleaseOneAp ( > + IN OUT volatile UINT32 *ApSem > + ) > +{ > + ReleaseSemaphore (ApSem); > +} > + > +/** > + Used for AP to wait BSP. > + > + @param ApSem IN: 32-bit unsigned integer > + OUT: original integer 0 (2) wrong comment; WaitForSemaphore() says "OUT: original integer - 1". > +**/ > +VOID > +WaitForBsp ( > + IN OUT volatile UINT32 *ApSem > + ) > +{ > + WaitForSemaphore (ApSem); > +} > + > +/** > + Used for AP to release BSP. > + > + @param BspSem IN: 32-bit unsigned integer > + OUT: original integer + 1 > +**/ > +VOID > +ReleaseBsp ( > + IN OUT volatile UINT32 *BspSem > + ) > +{ > + ReleaseSemaphore (BspSem); > +} > + > /** > Check whether the index of CPU perform the package level register > programming during System Management Mode initialization. > =20 > The index of Processor specified by mPackageFirstThreadIndex[PackageIn= dex] > @@ -632,11 +676,11 @@ BSPHandler ( > // Signal all APs it's time for backup MTRRs > // > ReleaseAllAPs (); > =20 > // > - // WaitForSemaphore() may wait for ever if an AP happens to enter = SMM at > + // WaitForBsp() may wait for ever if an AP happens to enter SMM at > // exactly this point. Please make sure PcdCpuSmmMaxSyncLoops has = been set > // to a large enough value to avoid this situation. > // Note: For HT capable CPUs, threads within a core share the same= set of MTRRs. > // We do the backup first and then set MTRR to avoid race conditio= n for threads > // in the same core. > @@ -652,11 +696,11 @@ BSPHandler ( > // Let all processors program SMM MTRRs together > // > ReleaseAllAPs (); > =20 > // > - // WaitForSemaphore() may wait for ever if an AP happens to enter = SMM at > + // WaitForBsp() may wait for ever if an AP happens to enter SMM at > // exactly this point. Please make sure PcdCpuSmmMaxSyncLoops has = been set > // to a large enough value to avoid this situation. > // > ReplaceOSMtrrs (CpuIndex); > =20 (3) I believe (but am not fully sure) that the above comment updates are wrong. Both contexts belong to BSPHandler(), where the BSP orchestrates MTRR programming on all processors (which must occur on all processors at the same time). The comments explain that the BSP releases the APs to do their jobs, and then waits for them to finish. We have two WaitForAllAPs() calls. IOW, the BSP does not wait for the BSP, but the APs. Thus, the updated comments should say WaitForAllAPs(), not WaitForBsp(). > @@ -898,50 +942,50 @@ APHandler ( > =20 > if ((SyncMode =3D=3D SmmCpuSyncModeTradition) || SmmCpuFeaturesNeedCon= figureMtrrs ()) { > // > // Notify BSP of arrival at this point > // > - ReleaseSemaphore (mSmmMpSyncData->CpuData[BspIndex].Run); > + ReleaseBsp (mSmmMpSyncData->CpuData[BspIndex].Run); > } > =20 > if (SmmCpuFeaturesNeedConfigureMtrrs ()) { > // > // Wait for the signal from BSP to backup MTRRs > // > - WaitForSemaphore (mSmmMpSyncData->CpuData[CpuIndex].Run); > + WaitForBsp (mSmmMpSyncData->CpuData[CpuIndex].Run); > =20 > // > // Backup OS MTRRs > // > MtrrGetAllMtrrs (&Mtrrs); > =20 > // > // Signal BSP the completion of this AP > // > - ReleaseSemaphore (mSmmMpSyncData->CpuData[BspIndex].Run); > + ReleaseBsp (mSmmMpSyncData->CpuData[BspIndex].Run); > =20 > // > // Wait for BSP's signal to program MTRRs > // > - WaitForSemaphore (mSmmMpSyncData->CpuData[CpuIndex].Run); > + WaitForBsp (mSmmMpSyncData->CpuData[CpuIndex].Run); > =20 > // > // Replace OS MTRRs with SMI MTRRs > // > ReplaceOSMtrrs (CpuIndex); > =20 > // > // Signal BSP the completion of this AP > // > - ReleaseSemaphore (mSmmMpSyncData->CpuData[BspIndex].Run); > + ReleaseBsp (mSmmMpSyncData->CpuData[BspIndex].Run); > } > =20 > while (TRUE) { > // > // Wait for something to happen > // > - WaitForSemaphore (mSmmMpSyncData->CpuData[CpuIndex].Run); > + WaitForBsp (mSmmMpSyncData->CpuData[CpuIndex].Run); > =20 > // > // Check if BSP wants to exit SMM > // > if (!(*mSmmMpSyncData->InsideSmm)) { > @@ -977,16 +1021,16 @@ APHandler ( > =20 > if (SmmCpuFeaturesNeedConfigureMtrrs ()) { > // > // Notify BSP the readiness of this AP to program MTRRs > // > - ReleaseSemaphore (mSmmMpSyncData->CpuData[BspIndex].Run); > + ReleaseBsp (mSmmMpSyncData->CpuData[BspIndex].Run); > =20 > // > // Wait for the signal from BSP to program MTRRs > // > - WaitForSemaphore (mSmmMpSyncData->CpuData[CpuIndex].Run); > + WaitForBsp (mSmmMpSyncData->CpuData[CpuIndex].Run); > =20 > // > // Restore OS MTRRs > // > SmmCpuFeaturesReenableSmrr (); > @@ -994,26 +1038,26 @@ APHandler ( > } > =20 > // > // Notify BSP the readiness of this AP to Reset states/semaphore for t= his processor > // > - ReleaseSemaphore (mSmmMpSyncData->CpuData[BspIndex].Run); > + ReleaseBsp (mSmmMpSyncData->CpuData[BspIndex].Run); > =20 > // > // Wait for the signal from BSP to Reset states/semaphore for this pro= cessor > // > - WaitForSemaphore (mSmmMpSyncData->CpuData[CpuIndex].Run); > + WaitForBsp (mSmmMpSyncData->CpuData[CpuIndex].Run); > =20 > // > // Reset states/semaphore for this processor > // > *(mSmmMpSyncData->CpuData[CpuIndex].Present) =3D FALSE; > =20 > // > // Notify BSP the readiness of this AP to exit SMM > // > - ReleaseSemaphore (mSmmMpSyncData->CpuData[BspIndex].Run); > + ReleaseBsp (mSmmMpSyncData->CpuData[BspIndex].Run); > } > =20 > /** > Checks whether the input token is the current used token. > =20 > @@ -1277,11 +1321,11 @@ InternalSmmStartupThisAp ( > mSmmMpSyncData->CpuData[CpuIndex].Status =3D CpuStatus; > if (mSmmMpSyncData->CpuData[CpuIndex].Status !=3D NULL) { > *mSmmMpSyncData->CpuData[CpuIndex].Status =3D EFI_NOT_READY; > } > =20 > - ReleaseSemaphore (mSmmMpSyncData->CpuData[CpuIndex].Run); > + ReleaseOneAp (mSmmMpSyncData->CpuData[CpuIndex].Run); > =20 > if (Token =3D=3D NULL) { > AcquireSpinLock (mSmmMpSyncData->CpuData[CpuIndex].Busy); > ReleaseSpinLock (mSmmMpSyncData->CpuData[CpuIndex].Busy); > } Looks OK to me otherwise. 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 (#110822): https://edk2.groups.io/g/devel/message/110822 Mute This Topic: https://groups.io/mt/102366297/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-