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 8EB20740045 for ; Wed, 25 Oct 2023 10:36:59 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=eNhApbsom+uMpxdQv3rw3Cs6yjER0VitynCV5t5kUWQ=; 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=1698230218; v=1; b=D0sWYxVuffy+RpQKP1P8KLcHbMkHGM2NBLMu1OnmyUgqzJW6MZizpLr+MgfpO/5dO3KmGUHP 4xz8l5DD8vA8MNsDvj07qVvYf1k7aqtSAjSUj1s5C/ms0JjCQEpHlE6I431q/P3YH5oQXr4sS9E 8sr6LDYvgFhWNPQmynEu9bik= X-Received: by 127.0.0.2 with SMTP id 5wSoYY7687511xKf7WH3xAQ9; Wed, 25 Oct 2023 03:36:58 -0700 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.40471.1698230217678410182 for ; Wed, 25 Oct 2023 03:36:57 -0700 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-448-7vDb15uRMKqXYNJDRsgF_w-1; Wed, 25 Oct 2023 06:36:50 -0400 X-MC-Unique: 7vDb15uRMKqXYNJDRsgF_w-1 X-Received: from smtp.corp.redhat.com (int-mx07.intmail.prod.int.rdu2.redhat.com [10.11.54.7]) (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 347B83C0C4A1; Wed, 25 Oct 2023 10:36:50 +0000 (UTC) X-Received: from [10.39.192.250] (unknown [10.39.192.250]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 2EAC61C060AE; Wed, 25 Oct 2023 10:36:49 +0000 (UTC) Message-ID: Date: Wed, 25 Oct 2023 12:36:47 +0200 MIME-Version: 1.0 Subject: Re: [edk2-devel] [PATCH] UefiCpuPkg: Remove ASSERT checking if FinishedCount equal to CpuCount-1 To: devel@edk2.groups.io, yuanhao.xie@intel.com Cc: Ray Ni , Eric Dong , Rahul Kumar , Tom Lendacky References: <20231025100719.2640-1-yuanhao.xie@intel.com> From: "Laszlo Ersek" In-Reply-To: <20231025100719.2640-1-yuanhao.xie@intel.com> X-Scanned-By: MIMEDefang 3.4.1 on 10.11.54.7 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: PzHT25cTv6eLz2kDeuVlXuGsx7686176AA= 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=D0sWYxVu; 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 10/25/23 12:07, Yuanhao Xie wrote: > The purpose is to fix an assertion with applying the following patch > series: >=20 > UefiCpuPkg: Refactor the logic for placing APs in HltLoop. > UefiCpuPkg: Refactor the logic for placing APs in Mwait/Runloop. > UefiCpuPkg: Create MpHandOff. > UefiCpuPkg: ApWakeupFunction directly use CpuMpData. > UefiCpuPkg: Eliminate the second INIT-SIPI-SIPI sequence. > UefiCpuPkg: Decouple the SEV-ES functionality. >=20 > The assertion arises from a timing discrepancy between BSP completing > its startup signal check and the APs incrementing the FinishedCount. >=20 > Instead of assertion, use while loop to waits until all the APs have > incremented the FinishedCount. >=20 > Cc: Ray Ni > Cc: Eric Dong > Cc: Rahul Kumar > Cc: Tom Lendacky > Signed-off-by: Yuanhao Xie > --- > UefiCpuPkg/Library/MpInitLib/MpLib.c | 9 +++++++-- > 1 file changed, 7 insertions(+), 2 deletions(-) >=20 > diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c b/UefiCpuPkg/Library/Mp= InitLib/MpLib.c > index 6f1456cfe1..9a6ec5db5c 100644 > --- a/UefiCpuPkg/Library/MpInitLib/MpLib.c > +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c > @@ -913,8 +913,8 @@ DxeApEntryPoint ( > UINTN ProcessorNumber; > =20 > GetProcessorNumber (CpuMpData, &ProcessorNumber); > - InterlockedIncrement ((UINT32 *)&CpuMpData->FinishedCount); > RestoreVolatileRegisters (&CpuMpData->CpuData[0].VolatileRegisters, FA= LSE); > + InterlockedIncrement ((UINT32 *)&CpuMpData->FinishedCount); > PlaceAPInMwaitLoopOrRunLoop ( > CpuMpData->ApLoopMode, > CpuMpData->CpuData[ProcessorNumber].StartupApSignal, > @@ -2201,7 +2201,12 @@ MpInitLibInitialize ( > // looping process there. > // > SwitchApContext (MpHandOff); > - ASSERT (CpuMpData->FinishedCount =3D=3D (CpuMpData->CpuCount - 1))= ; > + // > + // Wait for all APs finished initialization > + // > + while (CpuMpData->FinishedCount < (CpuMpData->CpuCount - 1)) { > + CpuPause (); > + } > =20 > // > // Set Apstate as Idle, otherwise Aps cannot be waken-up again. The second hunk makes sense. SwitchApContext() returns after all APs are "live", but that doesn't guarantee that all APs are also "done" by the time the BSP reaches the FinishedCount check. (1) What is the justification for the first hunk? I understand that we may want to report "finished" from an AP as late as possible. Is that the only (general) reason for the first hunk, or is there a specific reason= ? Either way, the reason for the first hunk should be documented in the commit message. (2) The subject line should be UefiCpuPkg/MpInitLib: wait for all APs to finish initialization because - we should also state the component name within UefiCpuPkg, - "remove assert" is just a natural language expression of the direct code change, so it's not useful; what's useful is naming the *goal* that we're achieving. (3) I suggest appending Fixes: 964a4f032dcd to the commit message, just above your Signed-off-by. 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 (#110034): https://edk2.groups.io/g/devel/message/110034 Mute This Topic: https://groups.io/mt/102174874/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-