From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) by mx.groups.io with SMTP id smtpd.web10.4641.1570706999510398024 for ; Thu, 10 Oct 2019 04:29:59 -0700 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: redhat.com, ip: 209.132.183.28, mailfrom: lersek@redhat.com) 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 mx1.redhat.com (Postfix) with ESMTPS id 2A063C05686D; Thu, 10 Oct 2019 11:29:59 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-48.rdu2.redhat.com [10.10.120.48]) by smtp.corp.redhat.com (Postfix) with ESMTP id 269325D6A5; Thu, 10 Oct 2019 11:29:57 +0000 (UTC) From: "Laszlo Ersek" To: edk2-devel-groups-io Cc: Eric Dong , Ray Ni Subject: [PATCH v2 2/2] UefiCpuPkg/MpInitLib: honor the platform's boot CPU count in AP detection Date: Thu, 10 Oct 2019 13:29:52 +0200 Message-Id: <20191010112952.7187-3-lersek@redhat.com> In-Reply-To: <20191010112952.7187-1-lersek@redhat.com> References: <20191010112952.7187-1-lersek@redhat.com> MIME-Version: 1.0 X-Scanned-By: MIMEDefang 2.79 on 10.5.11.15 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.32]); Thu, 10 Oct 2019 11:29:59 +0000 (UTC) Content-Transfer-Encoding: quoted-printable - If a platform boots such that the boot CPU count is smaller than PcdCpuMaxLogicalProcessorNumber, then the platform cannot use the "fast AP detection" logic added in commit 6e1987f19af7. (Which has been documented as a subset of use case (2) in the previous patch.) Said logic depends on the boot CPU count being equal to PcdCpuMaxLogicalProcessorNumber. If the equality does not hold, the platform either has to wait too long, or risk missing APs due to an early timeout. - The platform may not be able to use the variant added in commit 0594ec417c89 either. (Which has been documented as use case (1) in the previous patch.) See commit 861218740d6d. When OVMF runs on QEMU/KVM, APs may check in with the BSP in arbitrary order, plus the individual AP may take arbitrarily long to check-in. If "NumApsExecuting" falls to zero mid-enumeration, APs will be missed. Allow platforms to specify the exact boot CPU count, independently of PcdCpuMaxLogicalProcessorNumber. In this mode, the BSP waits for all APs to check-in regardless of timeout. If at least one AP fails to check-in, then the AP enumeration hangs forever. That is the desired behavior when the exact boot CPU count is known in advance. (A hung boot is better than an AP checking-in after timeout, and executing code from released storage.) Cc: Eric Dong Cc: Ray Ni Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3D1515 Signed-off-by: Laszlo Ersek --- Notes: v2: - update commit message - update docs in DEC file - add code comments - no functional changes UefiCpuPkg/UefiCpuPkg.dec | 13 +++ UefiCpuPkg/UefiCpuPkg.uni | 4 + UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf | 1 + UefiCpuPkg/Library/MpInitLib/PeiMpInitLib.inf | 3 +- UefiCpuPkg/Library/MpInitLib/MpLib.c | 99 ++++++++++++-------- 5 files changed, 80 insertions(+), 40 deletions(-) diff --git a/UefiCpuPkg/UefiCpuPkg.dec b/UefiCpuPkg/UefiCpuPkg.dec index 031a2ccd680a..12f4413ea5b0 100644 --- a/UefiCpuPkg/UefiCpuPkg.dec +++ b/UefiCpuPkg/UefiCpuPkg.dec @@ -227,6 +227,19 @@ [PcdsFixedAtBuild, PcdsPatchableInModule, PcdsDynami= c, PcdsDynamicEx] ## Specifies timeout value in microseconds for the BSP to detect all A= Ps for the first time. # @Prompt Timeout for the BSP to detect all APs for the first time. gUefiCpuPkgTokenSpaceGuid.PcdCpuApInitTimeOutInMicroSeconds|50000|UINT= 32|0x00000004 + ## Specifies the number of Logical Processors that are available in th= e + # preboot environment after platform reset, including BSP and APs. Po= ssible + # values:

+ # zero (default) - PcdCpuBootLogicalProcessorNumber is ignored, and + # PcdCpuApInitTimeOutInMicroSeconds limits the initi= al AP + # detection by the BSP.
+ # nonzero - PcdCpuApInitTimeOutInMicroSeconds is ignored. The = initial + # AP detection finishes only when the detected CPU c= ount + # (BSP plus APs) reaches the value of + # PcdCpuBootLogicalProcessorNumber, regardless of ho= w long + # that takes.
+ # @Prompt Number of Logical Processors available after platform reset. + gUefiCpuPkgTokenSpaceGuid.PcdCpuBootLogicalProcessorNumber|0|UINT32|0x= 00000008 ## Specifies the base address of the first microcode Patch in the micr= ocode Region. # @Prompt Microcode Region base address. gUefiCpuPkgTokenSpaceGuid.PcdCpuMicrocodePatchAddress|0x0|UINT64|0x000= 00005 diff --git a/UefiCpuPkg/UefiCpuPkg.uni b/UefiCpuPkg/UefiCpuPkg.uni index fbf768072668..a7e279c5cb14 100644 --- a/UefiCpuPkg/UefiCpuPkg.uni +++ b/UefiCpuPkg/UefiCpuPkg.uni @@ -37,6 +37,10 @@ =20 #string STR_gUefiCpuPkgTokenSpaceGuid_PcdCpuApInitTimeOutInMicroSeconds_= HELP #language en-US "Specifies timeout value in microseconds for the BS= P to detect all APs for the first time." =20 +#string STR_gUefiCpuPkgTokenSpaceGuid_PcdCpuBootLogicalProcessorNumber_P= ROMPT #language en-US "Number of Logical Processors available after plat= form reset." + +#string STR_gUefiCpuPkgTokenSpaceGuid_PcdCpuBootLogicalProcessorNumber_H= ELP #language en-US "Specifies the number of Logical Processors that are= available in the preboot environment after platform reset, including BSP= and APs." + #string STR_gUefiCpuPkgTokenSpaceGuid_PcdCpuMicrocodePatchAddress_PROMPT= #language en-US "Microcode Region base address." =20 #string STR_gUefiCpuPkgTokenSpaceGuid_PcdCpuMicrocodePatchAddress_HELP = #language en-US "Specifies the base address of the first microcode Patch = in the microcode Region." diff --git a/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf b/UefiCpuPkg/L= ibrary/MpInitLib/DxeMpInitLib.inf index 37b3f64e578a..cd912ab0c5ee 100644 --- a/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf +++ b/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf @@ -61,6 +61,7 @@ [Guids] =20 [Pcd] gUefiCpuPkgTokenSpaceGuid.PcdCpuMaxLogicalProcessorNumber ## CO= NSUMES + gUefiCpuPkgTokenSpaceGuid.PcdCpuBootLogicalProcessorNumber ## CO= NSUMES gUefiCpuPkgTokenSpaceGuid.PcdCpuApInitTimeOutInMicroSeconds ## SO= METIMES_CONSUMES gUefiCpuPkgTokenSpaceGuid.PcdCpuApStackSize ## CO= NSUMES gUefiCpuPkgTokenSpaceGuid.PcdCpuMicrocodePatchAddress ## CO= NSUMES diff --git a/UefiCpuPkg/Library/MpInitLib/PeiMpInitLib.inf b/UefiCpuPkg/L= ibrary/MpInitLib/PeiMpInitLib.inf index 82b77b63ea87..1538185ef99a 100644 --- a/UefiCpuPkg/Library/MpInitLib/PeiMpInitLib.inf +++ b/UefiCpuPkg/Library/MpInitLib/PeiMpInitLib.inf @@ -53,7 +53,8 @@ [LibraryClasses] =20 [Pcd] gUefiCpuPkgTokenSpaceGuid.PcdCpuMaxLogicalProcessorNumber ## CO= NSUMES - gUefiCpuPkgTokenSpaceGuid.PcdCpuApInitTimeOutInMicroSeconds ## CO= NSUMES + gUefiCpuPkgTokenSpaceGuid.PcdCpuBootLogicalProcessorNumber ## CO= NSUMES + gUefiCpuPkgTokenSpaceGuid.PcdCpuApInitTimeOutInMicroSeconds ## SO= METIMES_CONSUMES gUefiCpuPkgTokenSpaceGuid.PcdCpuApStackSize ## CO= NSUMES gUefiCpuPkgTokenSpaceGuid.PcdCpuMicrocodePatchAddress ## CO= NSUMES gUefiCpuPkgTokenSpaceGuid.PcdCpuMicrocodePatchRegionSize ## CO= NSUMES diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c b/UefiCpuPkg/Library/Mp= InitLib/MpLib.c index 594a035d8b92..622b70ca3c4e 100644 --- a/UefiCpuPkg/Library/MpInitLib/MpLib.c +++ b/UefiCpuPkg/Library/MpInitLib/MpLib.c @@ -1044,46 +1044,67 @@ WakeUpAP ( SendInitSipiSipiAllExcludingSelf ((UINT32) ExchangeInfo->BufferSta= rt); } if (CpuMpData->InitFlag =3D=3D ApInitConfig) { - // - // The AP enumeration algorithm below is suitable for two use case= s. - // - // (1) The check-in time for an individual AP is bounded, and APs = run - // through their initialization routines strongly concurrently= . In - // particular, the number of concurrently running APs - // ("NumApsExecuting") is never expected to fall to zero - // *temporarily* -- it is expected to fall to zero only when a= ll - // APs have checked-in. - // - // In this case, the platform is supposed to set - // PcdCpuApInitTimeOutInMicroSeconds to a low-ish value (just = long - // enough for one AP to start initialization). The timeout wil= l be - // reached soon, and remaining APs are collected by watching - // NumApsExecuting fall to zero. If NumApsExecuting falls to z= ero - // mid-process, while some APs have not completed initializati= on, - // the behavior is undefined. - // - // (2) The check-in time for an individual AP is unbounded, and/or= APs - // may complete their initializations widely spread out. In - // particular, some APs may finish initialization before some = APs - // even start. - // - // In this case, the platform is supposed to set - // PcdCpuApInitTimeOutInMicroSeconds to a high-ish value. The = AP - // enumeration will always take that long (except when the boo= t CPU - // count happens to be maximal, that is, - // PcdCpuMaxLogicalProcessorNumber). All APs are expected to - // check-in before the timeout, and NumApsExecuting is assumed= zero - // at timeout. APs that miss the time-out may cause undefined - // behavior. - // - TimedWaitForApFinish ( - CpuMpData, - PcdGet32 (PcdCpuMaxLogicalProcessorNumber) - 1, - PcdGet32 (PcdCpuApInitTimeOutInMicroSeconds) - ); + if (PcdGet32 (PcdCpuBootLogicalProcessorNumber) > 0) { + // + // The AP enumeration algorithm below is suitable only when the + // platform can tell us the *exact* boot CPU count in advance. + // + // The wait below finishes only when the detected AP count reach= es + // (PcdCpuBootLogicalProcessorNumber - 1), regardless of how lon= g that + // takes. If at least one AP fails to check in (meaning a platfo= rm + // hardware bug), the detection hangs forever, by design. If the= actual + // boot CPU count in the system is higher than + // PcdCpuBootLogicalProcessorNumber (meaning a platform + // misconfiguration), then some APs may complete initialization = after + // the wait finishes, and cause undefined behavior. + // + TimedWaitForApFinish ( + CpuMpData, + PcdGet32 (PcdCpuBootLogicalProcessorNumber) - 1, + MAX_UINT32 // approx. 71 minutes + ); + } else { + // + // The AP enumeration algorithm below is suitable for two use ca= ses. + // + // (1) The check-in time for an individual AP is bounded, and AP= s run + // through their initialization routines strongly concurrent= ly. In + // particular, the number of concurrently running APs + // ("NumApsExecuting") is never expected to fall to zero + // *temporarily* -- it is expected to fall to zero only when= all + // APs have checked-in. + // + // In this case, the platform is supposed to set + // PcdCpuApInitTimeOutInMicroSeconds to a low-ish value (jus= t long + // enough for one AP to start initialization). The timeout w= ill be + // reached soon, and remaining APs are collected by watching + // NumApsExecuting fall to zero. If NumApsExecuting falls to= zero + // mid-process, while some APs have not completed initializa= tion, + // the behavior is undefined. + // + // (2) The check-in time for an individual AP is unbounded, and/= or APs + // may complete their initializations widely spread out. In + // particular, some APs may finish initialization before som= e APs + // even start. + // + // In this case, the platform is supposed to set + // PcdCpuApInitTimeOutInMicroSeconds to a high-ish value. Th= e AP + // enumeration will always take that long (except when the b= oot CPU + // count happens to be maximal, that is, + // PcdCpuMaxLogicalProcessorNumber). All APs are expected to + // check-in before the timeout, and NumApsExecuting is assum= ed zero + // at timeout. APs that miss the time-out may cause undefine= d + // behavior. + // + TimedWaitForApFinish ( + CpuMpData, + PcdGet32 (PcdCpuMaxLogicalProcessorNumber) - 1, + PcdGet32 (PcdCpuApInitTimeOutInMicroSeconds) + ); =20 - while (CpuMpData->MpCpuExchangeInfo->NumApsExecuting !=3D 0) { - CpuPause(); + while (CpuMpData->MpCpuExchangeInfo->NumApsExecuting !=3D 0) { + CpuPause(); + } } } else { // --=20 2.19.1.3.g30247aa5d201