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 D8883D802C8 for ; Mon, 13 Nov 2023 17:18:34 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=p/f4nMYhmmGnBlW0Rj/kKs8o2KAvZKwDuNeX7EJxP90=; 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=1699895913; v=1; b=El+UvG0l3pZFq0OR122e68QxjIBIeElruWYEej6sEY2ccQOCCaO3rABjLX12o+l70G7tqlhR CZ3wQ9D0cEyylzdJgc04992cQs3SVBeeDtSOvlax+0pzTL9z/dqIE44xzo+ryGjvGMZ64my/LJM yU1YPS4OFreakFEQhT2pYSUA= X-Received: by 127.0.0.2 with SMTP id xZsRYY7687511xIkR140WvN5; Mon, 13 Nov 2023 09:18:33 -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.1077.1699895912824851675 for ; Mon, 13 Nov 2023 09:18:32 -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-461-GA9ez093OnuArv4RkEr0sg-1; Mon, 13 Nov 2023 12:18:30 -0500 X-MC-Unique: GA9ez093OnuArv4RkEr0sg-1 X-Received: from smtp.corp.redhat.com (int-mx10.intmail.prod.int.rdu2.redhat.com [10.11.54.10]) (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 2D1E7185A780; Mon, 13 Nov 2023 17:18:30 +0000 (UTC) X-Received: from [10.39.192.220] (unknown [10.39.192.220]) by smtp.corp.redhat.com (Postfix) with ESMTPS id A9AF3492BFD; Mon, 13 Nov 2023 17:18:28 +0000 (UTC) Message-ID: Date: Mon, 13 Nov 2023 18:18:27 +0100 MIME-Version: 1.0 Subject: Re: [edk2-devel] [Patch V2] UefiCpuPkg/PiSmmCpuDxeSmm: SmmCpuRendezvous ensure all Aps in Present. To: devel@edk2.groups.io, yuanhao.xie@intel.com Cc: Zhihao Li , Ray Ni , Rahul Kumar , Gerd Hoffmann References: <20231113054714.1729-1-yuanhao.xie@intel.com> From: "Laszlo Ersek" In-Reply-To: <20231113054714.1729-1-yuanhao.xie@intel.com> X-Scanned-By: MIMEDefang 3.4.1 on 10.11.54.10 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: UQoRD0ktDrPsPGpfFt5Ss0Jex7686176AA= 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=El+UvG0l; 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 11/13/23 06:47, Yuanhao Xie wrote: > SMM read save state requires AP must be present. > This patch aim to avoid the AP not ready for save state check. >=20 > Signed-off-by: Zhihao Li > Cc: Ray Ni > Cc: Rahul Kumar > Cc: Gerd Hoffmann > --- > UefiCpuPkg/PiSmmCpuDxeSmm/CpuService.c | 25 ++++++++++++++++++++++++= + > UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h | 13 +++++++++++++ > 2 files changed, 38 insertions(+) >=20 > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/CpuService.c b/UefiCpuPkg/PiSmmCpu= DxeSmm/CpuService.c > index 391b64e9f2..cdc7021ee9 100644 > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/CpuService.c > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/CpuService.c > @@ -406,8 +406,15 @@ SmmCpuRendezvous ( > IN BOOLEAN BlockingMode > ) > { > + UINTN Index; > + UINTN PresentCount; > + UINT32 BlockedCount; > + UINT32 DisabledCount; > EFI_STATUS Status; > =20 > + BlockedCount =3D 0; > + DisabledCount =3D 0; > + > // > // Return success immediately if all CPUs are already synchronized. > // > @@ -426,6 +433,24 @@ SmmCpuRendezvous ( > // There are some APs outside SMM, Wait for all avaiable APs to arri= ve. > // > SmmWaitForApArrival (); > + > + // > + // Make sure all APs have their Present flag set > + // > + while (TRUE) { > + PresentCount =3D 0; > + for (Index =3D 0; Index < mMaxNumberOfCpus; Index++) { > + if (*(mSmmMpSyncData->CpuData[Index].Present)) { > + PresentCount++; > + } > + } > + > + GetSmmDelayedBlockedDisabledCount (NULL, &BlockedCount, &DisabledC= ount); > + if (PresentCount =3D=3D mMaxNumberOfCpus - BlockedCount - Disabled= Count ) { > + break; > + } > + } > + > Status =3D mSmmMpSyncData->AllApArrivedWithException ? EFI_SUCCESS := EFI_TIMEOUT; > } else { > // (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? (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. Thanks Laszlo > diff --git a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h b/UefiCpuPkg/PiSm= mCpuDxeSmm/PiSmmCpuDxeSmm.h > index 20ada465c2..b5aa5f99d7 100644 > --- a/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h > +++ b/UefiCpuPkg/PiSmmCpuDxeSmm/PiSmmCpuDxeSmm.h > @@ -1552,6 +1552,19 @@ SmmWaitForApArrival ( > VOID > ); > =20 > +/** > + Returns the Number of SMM Delayed & Blocked & Disabled Thread Count. > + @param[in,out] DelayedCount The Number of SMM Delayed Thread Count. > + @param[in,out] BlockedCount The Number of SMM Blocked Thread Count. > + @param[in,out] DisabledCount The Number of SMM Disabled Thread Count. > +**/ > +VOID > +GetSmmDelayedBlockedDisabledCount ( > + IN OUT UINT32 *DelayedCount, > + IN OUT UINT32 *BlockedCount, > + IN OUT UINT32 *DisabledCount > + ); > + > /** > Write unprotect read-only pages if Cr0.Bits.WP is 1. > =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 (#111168): https://edk2.groups.io/g/devel/message/111168 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-