From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [216.205.24.124]) by mx.groups.io with SMTP id smtpd.web10.10121.1611654335226713481 for ; Tue, 26 Jan 2021 01:45:35 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=fOSChqHi; spf=pass (domain: redhat.com, ip: 216.205.24.124, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1611654334; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=ciMtX8xehtraIbc27ba0aejj8oqIfNVTw4SYKT6grcg=; b=fOSChqHiJPegqCZSZbvft9xLE9HAZzeAYYRsUs74fqVYTme6WzU+E+l5p8bNCALnw/XpLL I+MQi754wIiUCebvR4/Zny4OqJUnJTE+10mF2JMWrged4wqSwZVYTvIUEZr/QPH9Rt11PT IMb3rgBxy9eAWnOzAid/uTQawyfmwmk= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-35-MZ1_9T8MP0iR56bOEFSlcg-1; Tue, 26 Jan 2021 04:45:30 -0500 X-MC-Unique: MZ1_9T8MP0iR56bOEFSlcg-1 Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.phx2.redhat.com [10.5.11.15]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 3BE76107ACE8; Tue, 26 Jan 2021 09:45:29 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-114-177.ams2.redhat.com [10.36.114.177]) by smtp.corp.redhat.com (Postfix) with ESMTP id 6A87E5D752; Tue, 26 Jan 2021 09:45:28 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH] UefiCpuPkg/MpInitLib: Don't increase CpuCount in ApWakeupFunction To: devel@edk2.groups.io, ray.ni@intel.com References: <20210126055015.633-1-ray.ni@intel.com> From: "Laszlo Ersek" Message-ID: <146018a4-95c9-05c2-9a5d-978bb996ae03@redhat.com> Date: Tue, 26 Jan 2021 10:45:27 +0100 MIME-Version: 1.0 In-Reply-To: <20210126055015.633-1-ray.ni@intel.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.15 Authentication-Results: relay.mimecast.com; auth=pass smtp.auth=CUSA124A263 smtp.mailfrom=lersek@redhat.com X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 01/26/21 06:50, Ni, Ray wrote: > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3179 > > When BSP first time wakes all APs, each AP atomically increases > CpuMpData->CpuCount and CpuMpData->FinishedCount. > Each AP atomically increases CpuMpData->NumApsExecuting > in early assembly code and decreases it before it enters to HLT or > MWAIT state. > > Putting them together, the 3 variables are changed in the following order: > 1. NumApsExecuting++ // in assembly > 2. CpuCpunt++ > 4. FinishedCount++ > 3. NumApsExecuting-- // in C > > BSP waits for a certain timeout and then polls NumApsExecuting > until it drops to zero. It assumes all APs are waken up concurrently > and NumApsExecuting only drops to zero when all APs have checked in. > > Then it additionally waits for FinishedCount == CpuCount - 1. > (FinishedCount doesn't include BSP while CpuCount includes BSP.) > > There is no need to additionally wait for > FinishedCount == CpuCount - 1 because when NumApsExecuting == 0, > the number of increments of FinishedCount and CpuCount should equal. > > This patch simplifies the code to remove "CpuCount++" in > ApWakeupFunction() and assigns FinishedCount + 1 to CpuCount after > WakeUpAP(). > > Signed-off-by: Ray Ni > --- > UefiCpuPkg/Library/MpInitLib/MpLib.c | 16 +++++----------- > 1 file changed, 5 insertions(+), 11 deletions(-) > > diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c b/UefiCpuPkg/Library/MpInitLib/MpLib.c > index 8b1f7f84ba..2568986d8c 100644 > --- a/UefiCpuPkg/Library/MpInitLib/MpLib.c > +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c > @@ -1,7 +1,7 @@ > /** @file > CPU MP Initialize Library common functions. > > - Copyright (c) 2016 - 2020, Intel Corporation. All rights reserved.
> + Copyright (c) 2016 - 2021, Intel Corporation. All rights reserved.
> Copyright (c) 2020, AMD Inc. All rights reserved.
> > SPDX-License-Identifier: BSD-2-Clause-Patent > @@ -485,14 +485,12 @@ CollectProcessorCount ( > CpuMpData->InitFlag = ApInitConfig; > WakeUpAP (CpuMpData, TRUE, 0, NULL, NULL, TRUE); > CpuMpData->InitFlag = ApInitDone; > - ASSERT (CpuMpData->CpuCount <= PcdGet32 (PcdCpuMaxLogicalProcessorNumber)); > // > - // Wait for all APs finished the initialization > + // When InitFlag == ApInitConfig, WakeUpAP () guarantees all APs are checked in. > + // FinishedCount is the number of check-in APs. > // > - while (CpuMpData->FinishedCount < (CpuMpData->CpuCount - 1)) { > - CpuPause (); > - } > - > + CpuMpData->CpuCount = CpuMpData->FinishedCount + 1; > + ASSERT (CpuMpData->CpuCount <= PcdGet32 (PcdCpuMaxLogicalProcessorNumber)); > > // > // Enable x2APIC mode if > @@ -751,10 +749,6 @@ ApWakeupFunction ( > CurrentApicMode = GetApicMode (); > while (TRUE) { > if (CpuMpData->InitFlag == ApInitConfig) { > - // > - // Add CPU number > - // > - InterlockedIncrement ((UINT32 *) &CpuMpData->CpuCount); > ProcessorNumber = ApIndex; > // > // This is first time AP wakeup, get BIST information from AP stack > In ApWakeupFunction(), there is a stretch of code (pre-patch) where CpuCount has been incremented, but FinishedCount hasn't, yet. In that part of the AP code, the PAUSE loop in CollectProcessorCount() is waiting (running on the BSP). But, said part of the AP code is (more broadly) bracketed by the NumApsExecuting increment / decrement as well. And NumApsExecuting==0 is waited-for in WakeUpAP(). So this patch looks OK to me. I didn't try to verify the patch more thoroughly than described above; OTOH, on QEMU, the *other* branch in WakeUpAP() is supposed to be active (i.e. the one where "PcdCpuBootLogicalProcessorNumber" is nonzero). In that case, TimedWaitForApFinish() will wait until FinishedCount reaches (PcdGet32 (PcdCpuBootLogicalProcessorNumber) - 1) exactly, so the NumApsExecuting==0 check is not relevant in the first place. IOW, I think this patch cannot regress behavior on QEMU. Acked-by: Laszlo Ersek Thanks Laszlo