From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mga01.intel.com (mga01.intel.com [192.55.52.88]) by mx.groups.io with SMTP id smtpd.web11.7164.1570782145501819775 for ; Fri, 11 Oct 2019 01:22:25 -0700 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: intel.com, ip: 192.55.52.88, mailfrom: ray.ni@intel.com) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga001.jf.intel.com ([10.7.209.18]) by fmsmga101.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 11 Oct 2019 01:22:25 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.67,283,1566889200"; d="scan'208";a="278053491" Received: from fmsmsx103.amr.corp.intel.com ([10.18.124.201]) by orsmga001.jf.intel.com with ESMTP; 11 Oct 2019 01:22:24 -0700 Received: from fmsmsx119.amr.corp.intel.com (10.18.124.207) by FMSMSX103.amr.corp.intel.com (10.18.124.201) with Microsoft SMTP Server (TLS) id 14.3.439.0; Fri, 11 Oct 2019 01:22:24 -0700 Received: from shsmsx154.ccr.corp.intel.com (10.239.6.54) by FMSMSX119.amr.corp.intel.com (10.18.124.207) with Microsoft SMTP Server (TLS) id 14.3.439.0; Fri, 11 Oct 2019 01:22:23 -0700 Received: from shsmsx104.ccr.corp.intel.com ([169.254.5.166]) by SHSMSX154.ccr.corp.intel.com ([10.239.6.54]) with mapi id 14.03.0439.000; Fri, 11 Oct 2019 16:22:21 +0800 From: "Ni, Ray" To: Laszlo Ersek , edk2-devel-groups-io CC: "Dong, Eric" Subject: Re: [PATCH v2 2/2] UefiCpuPkg/MpInitLib: honor the platform's boot CPU count in AP detection Thread-Topic: [PATCH v2 2/2] UefiCpuPkg/MpInitLib: honor the platform's boot CPU count in AP detection Thread-Index: AQHVf14SUNyNkTCe3kmARQpf3qO0DqdVGnRQ Date: Fri, 11 Oct 2019 08:22:21 +0000 Message-ID: <734D49CCEBEEF84792F5B80ED585239D5C31D4A7@SHSMSX104.ccr.corp.intel.com> References: <20191010112952.7187-1-lersek@redhat.com> <20191010112952.7187-3-lersek@redhat.com> In-Reply-To: <20191010112952.7187-3-lersek@redhat.com> Accept-Language: en-US, zh-CN X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.239.127.40] MIME-Version: 1.0 Return-Path: ray.ni@intel.com Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Laszlo, the comments couldn't be better! Thanks!! Reviewed-by: Ray Ni > -----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 >=20 > - 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.) >=20 > 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. >=20 > - 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.) >=20 > 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. >=20 > 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.) >=20 > Cc: Eric Dong > Cc: Ray Ni > Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=3D1515 > Signed-off-by: Laszlo Ersek > --- >=20 > Notes: > v2: > - update commit message > - update docs in DEC file > - add code comments > - no functional changes >=20 > 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(-) >=20 > 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 A= Ps for > the first time. > # @Prompt Timeout for the BSP to detect all APs for the first time. >=20 > 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 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|UINT > 32|0x > + 00000008 > ## Specifies the base address of the first microcode Patch in the micr= ocode > Region. > # @Prompt Microcode Region base address. >=20 > 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 @@ >=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_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." >=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/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] >=20 > [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] >=20 > [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->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 { > // > -- > 2.19.1.3.g30247aa5d201