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.web12.5675.1570829487649292597 for ; Fri, 11 Oct 2019 14:31:27 -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 E4B1D883C32; Fri, 11 Oct 2019 21:31:26 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-177.rdu2.redhat.com [10.10.120.177]) by smtp.corp.redhat.com (Postfix) with ESMTP id A716A5D6C8; Fri, 11 Oct 2019 21:31:25 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH v2 2/2] UefiCpuPkg/MpInitLib: honor the platform's boot CPU count in AP detection To: devel@edk2.groups.io, ray.ni@intel.com Cc: "Dong, Eric" References: <20191010112952.7187-1-lersek@redhat.com> <20191010112952.7187-3-lersek@redhat.com> <734D49CCEBEEF84792F5B80ED585239D5C31D4A7@SHSMSX104.ccr.corp.intel.com> From: "Laszlo Ersek" Message-ID: Date: Fri, 11 Oct 2019 23:31:24 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <734D49CCEBEEF84792F5B80ED585239D5C31D4A7@SHSMSX104.ccr.corp.intel.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.15 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.6.2 (mx1.redhat.com [10.5.110.69]); Fri, 11 Oct 2019 21:31:27 +0000 (UTC) Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 10/11/19 10:22, Ni, Ray wrote: > Laszlo, the comments couldn't be better! Thanks!! > > Reviewed-by: Ray Ni Thanks, Ray :) Series pushed as commit range a7e2d20193e8..778832bcad33. Cheers! Laszlo > >> -----Original Message----- >> From: Laszlo Ersek >> Sent: Thursday, October 10, 2019 7:30 PM >> To: edk2-devel-groups-io >> Cc: Dong, Eric ; Ni, Ray >> Subject: [PATCH v2 2/2] UefiCpuPkg/MpInitLib: honor the platform's boot >> CPU count in AP detection >> >> - 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=1515 >> 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, >> PcdsDynamic, PcdsDynamicEx] >> ## Specifies timeout value in microseconds for the BSP to detect all APs for >> the first time. >> # @Prompt Timeout for the BSP to detect all APs for the first time. >> >> gUefiCpuPkgTokenSpaceGuid.PcdCpuApInitTimeOutInMicroSeconds|50000| >> UINT32|0x00000004 >> + ## Specifies the number of Logical Processors that are available in >> + the # preboot environment after platform reset, including BSP and >> + APs. Possible # values:

# zero (default) - >> + PcdCpuBootLogicalProcessorNumber is ignored, and >> + # PcdCpuApInitTimeOutInMicroSeconds limits the initial AP >> + # detection by the BSP.
>> + # nonzero - PcdCpuApInitTimeOutInMicroSeconds is ignored. The >> initial >> + # AP detection finishes only when the detected CPU count >> + # (BSP plus APs) reaches the value of >> + # PcdCpuBootLogicalProcessorNumber, regardless of how long >> + # that takes.
>> + # @Prompt Number of Logical Processors available after platform reset. >> + >> + >> gUefiCpuPkgTokenSpaceGuid.PcdCpuBootLogicalProcessorNumber|0|UINT >> 32|0x >> + 00000008 >> ## Specifies the base address of the first microcode Patch in the microcode >> Region. >> # @Prompt Microcode Region base address. >> >> gUefiCpuPkgTokenSpaceGuid.PcdCpuMicrocodePatchAddress|0x0|UINT64| >> 0x00000005 >> 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 @@ >> >> #string >> STR_gUefiCpuPkgTokenSpaceGuid_PcdCpuApInitTimeOutInMicroSeconds_ >> HELP #language en-US "Specifies timeout value in microseconds for the BSP >> to detect all APs for the first time." >> >> +#string >> STR_gUefiCpuPkgTokenSpaceGuid_PcdCpuBootLogicalProcessorNumber_PR >> OMPT #language en-US "Number of Logical Processors available after >> platform reset." >> + >> +#string >> STR_gUefiCpuPkgTokenSpaceGuid_PcdCpuBootLogicalProcessorNumber_HE >> LP #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_PROMP >> T #language en-US "Microcode Region base address." >> >> #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/Library/MpInitLib/DxeMpInitLib.inf >> index 37b3f64e578a..cd912ab0c5ee 100644 >> --- a/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf >> +++ b/UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf >> @@ -61,6 +61,7 @@ [Guids] >> >> [Pcd] >> gUefiCpuPkgTokenSpaceGuid.PcdCpuMaxLogicalProcessorNumber ## >> CONSUMES >> + gUefiCpuPkgTokenSpaceGuid.PcdCpuBootLogicalProcessorNumber ## >> CONSUMES >> gUefiCpuPkgTokenSpaceGuid.PcdCpuApInitTimeOutInMicroSeconds ## >> SOMETIMES_CONSUMES >> gUefiCpuPkgTokenSpaceGuid.PcdCpuApStackSize ## >> CONSUMES >> gUefiCpuPkgTokenSpaceGuid.PcdCpuMicrocodePatchAddress ## >> CONSUMES >> diff --git a/UefiCpuPkg/Library/MpInitLib/PeiMpInitLib.inf >> b/UefiCpuPkg/Library/MpInitLib/PeiMpInitLib.inf >> index 82b77b63ea87..1538185ef99a 100644 >> --- a/UefiCpuPkg/Library/MpInitLib/PeiMpInitLib.inf >> +++ b/UefiCpuPkg/Library/MpInitLib/PeiMpInitLib.inf >> @@ -53,7 +53,8 @@ [LibraryClasses] >> >> [Pcd] >> gUefiCpuPkgTokenSpaceGuid.PcdCpuMaxLogicalProcessorNumber ## >> CONSUMES >> - gUefiCpuPkgTokenSpaceGuid.PcdCpuApInitTimeOutInMicroSeconds ## >> CONSUMES >> + gUefiCpuPkgTokenSpaceGuid.PcdCpuBootLogicalProcessorNumber ## >> CONSUMES >> + gUefiCpuPkgTokenSpaceGuid.PcdCpuApInitTimeOutInMicroSeconds ## >> SOMETIMES_CONSUMES >> gUefiCpuPkgTokenSpaceGuid.PcdCpuApStackSize ## >> CONSUMES >> gUefiCpuPkgTokenSpaceGuid.PcdCpuMicrocodePatchAddress ## >> CONSUMES >> gUefiCpuPkgTokenSpaceGuid.PcdCpuMicrocodePatchRegionSize ## >> CONSUMES >> diff --git a/UefiCpuPkg/Library/MpInitLib/MpLib.c >> b/UefiCpuPkg/Library/MpInitLib/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->BufferStart); >> } >> if (CpuMpData->InitFlag == ApInitConfig) { >> - // >> - // The AP enumeration algorithm below is suitable for two use cases. >> - // >> - // (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 all >> - // 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 will 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 initialization, >> - // 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 boot 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 reaches >> + // (PcdCpuBootLogicalProcessorNumber - 1), regardless of how long >> that >> + // takes. If at least one AP fails to check in (meaning a platform >> + // 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 cases. >> + // >> + // (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 all >> + // 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 will 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 initialization, >> + // 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 boot 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) >> + ); >> >> - while (CpuMpData->MpCpuExchangeInfo->NumApsExecuting != 0) { >> - CpuPause(); >> + while (CpuMpData->MpCpuExchangeInfo->NumApsExecuting != 0) { >> + CpuPause(); >> + } >> } >> } else { >> // >> -- >> 2.19.1.3.g30247aa5d201 > > > >