* [Patch 0/3] Avoid AP calls PeiService @ 2018-12-20 1:15 Eric Dong 2018-12-20 1:15 ` [Patch 1/3] UefiCpuPkg/RegisterCpuFeaturesLib: " Eric Dong ` (2 more replies) 0 siblings, 3 replies; 15+ messages in thread From: Eric Dong @ 2018-12-20 1:15 UTC (permalink / raw) To: edk2-devel AP should not use PeiServices. This patches fixed two issues which both caused by AP try to call PeiServices. Eric Dong (3): UefiCpuPkg/RegisterCpuFeaturesLib: Avoid AP calls PeiService. UefiCpuPkg/RegisterCpuFeaturesLib: Avoid AP calls PeiService. UefiCpuPkg/RegisterCpuFeaturesLib: Remove useless function. .../RegisterCpuFeaturesLib/CpuFeaturesInitialize.c | 9 +++- .../DxeRegisterCpuFeaturesLib.c | 6 ++- .../PeiRegisterCpuFeaturesLib.c | 56 ++++++++++------------ .../RegisterCpuFeaturesLib/RegisterCpuFeatures.h | 6 ++- 4 files changed, 40 insertions(+), 37 deletions(-) -- 2.15.0.windows.1 ^ permalink raw reply [flat|nested] 15+ messages in thread
* [Patch 1/3] UefiCpuPkg/RegisterCpuFeaturesLib: Avoid AP calls PeiService. 2018-12-20 1:15 [Patch 0/3] Avoid AP calls PeiService Eric Dong @ 2018-12-20 1:15 ` Eric Dong 2018-12-20 1:19 ` Yao, Jiewen 2018-12-20 1:15 ` [Patch 2/3] " Eric Dong 2018-12-20 1:15 ` [Patch 3/3] UefiCpuPkg/RegisterCpuFeaturesLib: Remove useless function Eric Dong 2 siblings, 1 reply; 15+ messages in thread From: Eric Dong @ 2018-12-20 1:15 UTC (permalink / raw) To: edk2-devel; +Cc: Ruiyu Ni, Laszlo Ersek In AcquireSpinLock function, it calls GetPerformanceCounter which final calls PeiService service. This patch avoid to call AcquireSpinLock function. BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=1411 Cc: Ruiyu Ni <ruiyu.ni@intel.com> Cc: Laszlo Ersek <lersek@redhat.com> Contributed-under: TianoCore Contribution Agreement 1.1 Signed-off-by: Eric Dong <eric.dong@intel.com> --- UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize.c b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize.c index 624ddee055..a64326239f 100644 --- a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize.c +++ b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize.c @@ -832,7 +832,12 @@ ProgramProcessorRegister ( RegisterTableEntry = &RegisterTableEntryHead[Index]; DEBUG_CODE_BEGIN (); - AcquireSpinLock (&CpuFlags->ConsoleLogLock); + // + // Wait for the AP to release the MSR spin lock. + // + while (!AcquireSpinLockOrFail (&CpuFlags->ConsoleLogLock)) { + CpuPause (); + } ThreadIndex = ApLocation->Package * CpuStatus->MaxCoreCount * CpuStatus->MaxThreadCount + ApLocation->Core * CpuStatus->MaxThreadCount + ApLocation->Thread; -- 2.15.0.windows.1 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [Patch 1/3] UefiCpuPkg/RegisterCpuFeaturesLib: Avoid AP calls PeiService. 2018-12-20 1:15 ` [Patch 1/3] UefiCpuPkg/RegisterCpuFeaturesLib: " Eric Dong @ 2018-12-20 1:19 ` Yao, Jiewen 2018-12-20 1:22 ` Dong, Eric 0 siblings, 1 reply; 15+ messages in thread From: Yao, Jiewen @ 2018-12-20 1:19 UTC (permalink / raw) To: Dong, Eric, edk2-devel@lists.01.org; +Cc: Ni, Ruiyu, Laszlo Ersek Hi If we think below code is generic, can we have an API for that? + // + // Wait for the AP to release the MSR spin lock. + // + while (!AcquireSpinLockOrFail (&CpuFlags->ConsoleLogLock)) { + CpuPause (); + } > -----Original Message----- > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of > Eric Dong > Sent: Thursday, December 20, 2018 9:16 AM > To: edk2-devel@lists.01.org > Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Laszlo Ersek <lersek@redhat.com> > Subject: [edk2] [Patch 1/3] UefiCpuPkg/RegisterCpuFeaturesLib: Avoid AP > calls PeiService. > > In AcquireSpinLock function, it calls GetPerformanceCounter which > final calls PeiService service. This patch avoid to call > AcquireSpinLock function. > > BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=1411 > > Cc: Ruiyu Ni <ruiyu.ni@intel.com> > Cc: Laszlo Ersek <lersek@redhat.com> > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Eric Dong <eric.dong@intel.com> > --- > UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize.c | 7 > ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) > > diff --git > a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize.c > b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize.c > index 624ddee055..a64326239f 100644 > --- a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize.c > +++ b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize.c > @@ -832,7 +832,12 @@ ProgramProcessorRegister ( > RegisterTableEntry = &RegisterTableEntryHead[Index]; > > DEBUG_CODE_BEGIN (); > - AcquireSpinLock (&CpuFlags->ConsoleLogLock); > + // > + // Wait for the AP to release the MSR spin lock. > + // > + while (!AcquireSpinLockOrFail (&CpuFlags->ConsoleLogLock)) { > + CpuPause (); > + } > ThreadIndex = ApLocation->Package * CpuStatus->MaxCoreCount * > CpuStatus->MaxThreadCount + > ApLocation->Core * CpuStatus->MaxThreadCount + > ApLocation->Thread; > -- > 2.15.0.windows.1 > > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.01.org > https://lists.01.org/mailman/listinfo/edk2-devel ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Patch 1/3] UefiCpuPkg/RegisterCpuFeaturesLib: Avoid AP calls PeiService. 2018-12-20 1:19 ` Yao, Jiewen @ 2018-12-20 1:22 ` Dong, Eric 2018-12-20 2:06 ` Ni, Ruiyu 0 siblings, 1 reply; 15+ messages in thread From: Dong, Eric @ 2018-12-20 1:22 UTC (permalink / raw) To: Yao, Jiewen, edk2-devel@lists.01.org; +Cc: Ni, Ruiyu, Laszlo Ersek Agreed, Maybe it's time to add a new API like AcquireSpinLockWithoutTimeOut? Thanks, Eric > -----Original Message----- > From: Yao, Jiewen > Sent: Thursday, December 20, 2018 9:19 AM > To: Dong, Eric <eric.dong@intel.com>; edk2-devel@lists.01.org > Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Laszlo Ersek <lersek@redhat.com> > Subject: RE: [edk2] [Patch 1/3] UefiCpuPkg/RegisterCpuFeaturesLib: Avoid > AP calls PeiService. > > Hi > If we think below code is generic, can we have an API for that? > > + // > + // Wait for the AP to release the MSR spin lock. > + // > + while (!AcquireSpinLockOrFail (&CpuFlags->ConsoleLogLock)) { > + CpuPause (); > + } > > > > > > -----Original Message----- > > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of > > Eric Dong > > Sent: Thursday, December 20, 2018 9:16 AM > > To: edk2-devel@lists.01.org > > Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Laszlo Ersek <lersek@redhat.com> > > Subject: [edk2] [Patch 1/3] UefiCpuPkg/RegisterCpuFeaturesLib: Avoid > > AP calls PeiService. > > > > In AcquireSpinLock function, it calls GetPerformanceCounter which > > final calls PeiService service. This patch avoid to call > > AcquireSpinLock function. > > > > BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=1411 > > > > Cc: Ruiyu Ni <ruiyu.ni@intel.com> > > Cc: Laszlo Ersek <lersek@redhat.com> > > Contributed-under: TianoCore Contribution Agreement 1.1 > > Signed-off-by: Eric Dong <eric.dong@intel.com> > > --- > > UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize.c | 7 > > ++++++- > > 1 file changed, 6 insertions(+), 1 deletion(-) > > > > diff --git > > a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize.c > > b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize.c > > index 624ddee055..a64326239f 100644 > > --- > > a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize.c > > +++ b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize. > > +++ c > > @@ -832,7 +832,12 @@ ProgramProcessorRegister ( > > RegisterTableEntry = &RegisterTableEntryHead[Index]; > > > > DEBUG_CODE_BEGIN (); > > - AcquireSpinLock (&CpuFlags->ConsoleLogLock); > > + // > > + // Wait for the AP to release the MSR spin lock. > > + // > > + while (!AcquireSpinLockOrFail (&CpuFlags->ConsoleLogLock)) { > > + CpuPause (); > > + } > > ThreadIndex = ApLocation->Package * CpuStatus->MaxCoreCount * > > CpuStatus->MaxThreadCount + > > ApLocation->Core * CpuStatus->MaxThreadCount + > > ApLocation->Thread; > > -- > > 2.15.0.windows.1 > > > > _______________________________________________ > > edk2-devel mailing list > > edk2-devel@lists.01.org > > https://lists.01.org/mailman/listinfo/edk2-devel ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Patch 1/3] UefiCpuPkg/RegisterCpuFeaturesLib: Avoid AP calls PeiService. 2018-12-20 1:22 ` Dong, Eric @ 2018-12-20 2:06 ` Ni, Ruiyu 2018-12-20 2:08 ` Yao, Jiewen 0 siblings, 1 reply; 15+ messages in thread From: Ni, Ruiyu @ 2018-12-20 2:06 UTC (permalink / raw) To: Dong, Eric, Yao, Jiewen, edk2-devel@lists.01.org; +Cc: Laszlo Ersek Can you just change the AcquireSpinLock() behavior to remove the Timeout PCD consumption? I haven't seen a real case that the timed acquisition of spin lock is needed. Thanks/Ray > -----Original Message----- > From: Dong, Eric <eric.dong@intel.com> > Sent: Thursday, December 20, 2018 9:23 AM > To: Yao, Jiewen <jiewen.yao@intel.com>; edk2-devel@lists.01.org > Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Laszlo Ersek <lersek@redhat.com> > Subject: RE: [edk2] [Patch 1/3] UefiCpuPkg/RegisterCpuFeaturesLib: Avoid > AP calls PeiService. > > > Agreed, Maybe it's time to add a new API like > AcquireSpinLockWithoutTimeOut? > > Thanks, > Eric > > -----Original Message----- > > From: Yao, Jiewen > > Sent: Thursday, December 20, 2018 9:19 AM > > To: Dong, Eric <eric.dong@intel.com>; edk2-devel@lists.01.org > > Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Laszlo Ersek <lersek@redhat.com> > > Subject: RE: [edk2] [Patch 1/3] UefiCpuPkg/RegisterCpuFeaturesLib: > > Avoid AP calls PeiService. > > > > Hi > > If we think below code is generic, can we have an API for that? > > > > + // > > + // Wait for the AP to release the MSR spin lock. > > + // > > + while (!AcquireSpinLockOrFail (&CpuFlags->ConsoleLogLock)) { > > + CpuPause (); > > + } > > > > > > > > > > > -----Original Message----- > > > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf > > > Of Eric Dong > > > Sent: Thursday, December 20, 2018 9:16 AM > > > To: edk2-devel@lists.01.org > > > Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Laszlo Ersek <lersek@redhat.com> > > > Subject: [edk2] [Patch 1/3] UefiCpuPkg/RegisterCpuFeaturesLib: Avoid > > > AP calls PeiService. > > > > > > In AcquireSpinLock function, it calls GetPerformanceCounter which > > > final calls PeiService service. This patch avoid to call > > > AcquireSpinLock function. > > > > > > BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=1411 > > > > > > Cc: Ruiyu Ni <ruiyu.ni@intel.com> > > > Cc: Laszlo Ersek <lersek@redhat.com> > > > Contributed-under: TianoCore Contribution Agreement 1.1 > > > Signed-off-by: Eric Dong <eric.dong@intel.com> > > > --- > > > UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize.c | > > > 7 > > > ++++++- > > > 1 file changed, 6 insertions(+), 1 deletion(-) > > > > > > diff --git > > > a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize.c > > > b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize.c > > > index 624ddee055..a64326239f 100644 > > > --- > > > a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize.c > > > +++ b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize. > > > +++ c > > > @@ -832,7 +832,12 @@ ProgramProcessorRegister ( > > > RegisterTableEntry = &RegisterTableEntryHead[Index]; > > > > > > DEBUG_CODE_BEGIN (); > > > - AcquireSpinLock (&CpuFlags->ConsoleLogLock); > > > + // > > > + // Wait for the AP to release the MSR spin lock. > > > + // > > > + while (!AcquireSpinLockOrFail (&CpuFlags->ConsoleLogLock)) { > > > + CpuPause (); > > > + } > > > ThreadIndex = ApLocation->Package * CpuStatus->MaxCoreCount * > > > CpuStatus->MaxThreadCount + > > > ApLocation->Core * CpuStatus->MaxThreadCount + > > > ApLocation->Thread; > > > -- > > > 2.15.0.windows.1 > > > > > > _______________________________________________ > > > edk2-devel mailing list > > > edk2-devel@lists.01.org > > > https://lists.01.org/mailman/listinfo/edk2-devel ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Patch 1/3] UefiCpuPkg/RegisterCpuFeaturesLib: Avoid AP calls PeiService. 2018-12-20 2:06 ` Ni, Ruiyu @ 2018-12-20 2:08 ` Yao, Jiewen 2018-12-20 16:36 ` Brian J. Johnson 0 siblings, 1 reply; 15+ messages in thread From: Yao, Jiewen @ 2018-12-20 2:08 UTC (permalink / raw) To: Ni, Ruiyu, Dong, Eric, edk2-devel@lists.01.org; +Cc: Laszlo Ersek Yes, I agree, if we don't have any real case. > -----Original Message----- > From: Ni, Ruiyu > Sent: Thursday, December 20, 2018 10:07 AM > To: Dong, Eric <eric.dong@intel.com>; Yao, Jiewen > <jiewen.yao@intel.com>; edk2-devel@lists.01.org > Cc: Laszlo Ersek <lersek@redhat.com> > Subject: RE: [edk2] [Patch 1/3] UefiCpuPkg/RegisterCpuFeaturesLib: Avoid > AP calls PeiService. > > Can you just change the AcquireSpinLock() behavior to remove the Timeout > PCD consumption? > > I haven't seen a real case that the timed acquisition of spin lock is needed. > > > Thanks/Ray > > > -----Original Message----- > > From: Dong, Eric <eric.dong@intel.com> > > Sent: Thursday, December 20, 2018 9:23 AM > > To: Yao, Jiewen <jiewen.yao@intel.com>; edk2-devel@lists.01.org > > Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Laszlo Ersek <lersek@redhat.com> > > Subject: RE: [edk2] [Patch 1/3] UefiCpuPkg/RegisterCpuFeaturesLib: Avoid > > AP calls PeiService. > > > > > > Agreed, Maybe it's time to add a new API like > > AcquireSpinLockWithoutTimeOut? > > > > Thanks, > > Eric > > > -----Original Message----- > > > From: Yao, Jiewen > > > Sent: Thursday, December 20, 2018 9:19 AM > > > To: Dong, Eric <eric.dong@intel.com>; edk2-devel@lists.01.org > > > Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Laszlo Ersek <lersek@redhat.com> > > > Subject: RE: [edk2] [Patch 1/3] UefiCpuPkg/RegisterCpuFeaturesLib: > > > Avoid AP calls PeiService. > > > > > > Hi > > > If we think below code is generic, can we have an API for that? > > > > > > + // > > > + // Wait for the AP to release the MSR spin lock. > > > + // > > > + while (!AcquireSpinLockOrFail (&CpuFlags->ConsoleLogLock)) { > > > + CpuPause (); > > > + } > > > > > > > > > > > > > > > > -----Original Message----- > > > > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf > > > > Of Eric Dong > > > > Sent: Thursday, December 20, 2018 9:16 AM > > > > To: edk2-devel@lists.01.org > > > > Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Laszlo Ersek <lersek@redhat.com> > > > > Subject: [edk2] [Patch 1/3] UefiCpuPkg/RegisterCpuFeaturesLib: Avoid > > > > AP calls PeiService. > > > > > > > > In AcquireSpinLock function, it calls GetPerformanceCounter which > > > > final calls PeiService service. This patch avoid to call > > > > AcquireSpinLock function. > > > > > > > > BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=1411 > > > > > > > > Cc: Ruiyu Ni <ruiyu.ni@intel.com> > > > > Cc: Laszlo Ersek <lersek@redhat.com> > > > > Contributed-under: TianoCore Contribution Agreement 1.1 > > > > Signed-off-by: Eric Dong <eric.dong@intel.com> > > > > --- > > > > UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize.c | > > > > 7 > > > > ++++++- > > > > 1 file changed, 6 insertions(+), 1 deletion(-) > > > > > > > > diff --git > > > > a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize.c > > > > b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize.c > > > > index 624ddee055..a64326239f 100644 > > > > --- > > > > a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize.c > > > > +++ > b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize. > > > > +++ c > > > > @@ -832,7 +832,12 @@ ProgramProcessorRegister ( > > > > RegisterTableEntry = &RegisterTableEntryHead[Index]; > > > > > > > > DEBUG_CODE_BEGIN (); > > > > - AcquireSpinLock (&CpuFlags->ConsoleLogLock); > > > > + // > > > > + // Wait for the AP to release the MSR spin lock. > > > > + // > > > > + while (!AcquireSpinLockOrFail (&CpuFlags->ConsoleLogLock)) { > > > > + CpuPause (); > > > > + } > > > > ThreadIndex = ApLocation->Package * > CpuStatus->MaxCoreCount * > > > > CpuStatus->MaxThreadCount + > > > > ApLocation->Core * CpuStatus->MaxThreadCount + > > > > ApLocation->Thread; > > > > -- > > > > 2.15.0.windows.1 > > > > > > > > _______________________________________________ > > > > edk2-devel mailing list > > > > edk2-devel@lists.01.org > > > > https://lists.01.org/mailman/listinfo/edk2-devel ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Patch 1/3] UefiCpuPkg/RegisterCpuFeaturesLib: Avoid AP calls PeiService. 2018-12-20 2:08 ` Yao, Jiewen @ 2018-12-20 16:36 ` Brian J. Johnson 2018-12-21 2:06 ` Dong, Eric 2019-01-09 5:26 ` Dong, Eric 0 siblings, 2 replies; 15+ messages in thread From: Brian J. Johnson @ 2018-12-20 16:36 UTC (permalink / raw) To: Yao, Jiewen, Ni, Ruiyu, Dong, Eric, edk2-devel@lists.01.org; +Cc: Laszlo Ersek Agreed. We've seen issues on real platforms with timed-out spinlocks in DXE causing calls to GetPerformanceCounter and DebugAssert. (DXE has the same code, with the same issues.) Note that it's possible to set PcdSpinLockTimeout=0 to work around the issue on a particular platform, or in a particular module. But if you have to do that for every module which uses APs, and hence could contend for a spinlock, it kind of defeats the point.... We're better off removing the timeout code. Thanks, Brian On 12/19/18 8:08 PM, Yao, Jiewen wrote: > Yes, I agree, if we don't have any real case. > > >> -----Original Message----- >> From: Ni, Ruiyu >> Sent: Thursday, December 20, 2018 10:07 AM >> To: Dong, Eric <eric.dong@intel.com>; Yao, Jiewen >> <jiewen.yao@intel.com>; edk2-devel@lists.01.org >> Cc: Laszlo Ersek <lersek@redhat.com> >> Subject: RE: [edk2] [Patch 1/3] UefiCpuPkg/RegisterCpuFeaturesLib: Avoid >> AP calls PeiService. >> >> Can you just change the AcquireSpinLock() behavior to remove the Timeout >> PCD consumption? >> >> I haven't seen a real case that the timed acquisition of spin lock is needed. >> >> >> Thanks/Ray >> >>> -----Original Message----- >>> From: Dong, Eric <eric.dong@intel.com> >>> Sent: Thursday, December 20, 2018 9:23 AM >>> To: Yao, Jiewen <jiewen.yao@intel.com>; edk2-devel@lists.01.org >>> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Laszlo Ersek <lersek@redhat.com> >>> Subject: RE: [edk2] [Patch 1/3] UefiCpuPkg/RegisterCpuFeaturesLib: Avoid >>> AP calls PeiService. >>> >>> >>> Agreed, Maybe it's time to add a new API like >>> AcquireSpinLockWithoutTimeOut? >>> >>> Thanks, >>> Eric >>>> -----Original Message----- >>>> From: Yao, Jiewen >>>> Sent: Thursday, December 20, 2018 9:19 AM >>>> To: Dong, Eric <eric.dong@intel.com>; edk2-devel@lists.01.org >>>> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Laszlo Ersek <lersek@redhat.com> >>>> Subject: RE: [edk2] [Patch 1/3] UefiCpuPkg/RegisterCpuFeaturesLib: >>>> Avoid AP calls PeiService. >>>> >>>> Hi >>>> If we think below code is generic, can we have an API for that? >>>> >>>> + // >>>> + // Wait for the AP to release the MSR spin lock. >>>> + // >>>> + while (!AcquireSpinLockOrFail (&CpuFlags->ConsoleLogLock)) { >>>> + CpuPause (); >>>> + } >>>> >>>> >>>> >>>> >>>>> -----Original Message----- >>>>> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf >>>>> Of Eric Dong >>>>> Sent: Thursday, December 20, 2018 9:16 AM >>>>> To: edk2-devel@lists.01.org >>>>> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Laszlo Ersek <lersek@redhat.com> >>>>> Subject: [edk2] [Patch 1/3] UefiCpuPkg/RegisterCpuFeaturesLib: Avoid >>>>> AP calls PeiService. >>>>> >>>>> In AcquireSpinLock function, it calls GetPerformanceCounter which >>>>> final calls PeiService service. This patch avoid to call >>>>> AcquireSpinLock function. >>>>> >>>>> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=1411 >>>>> >>>>> Cc: Ruiyu Ni <ruiyu.ni@intel.com> >>>>> Cc: Laszlo Ersek <lersek@redhat.com> >>>>> Contributed-under: TianoCore Contribution Agreement 1.1 >>>>> Signed-off-by: Eric Dong <eric.dong@intel.com> >>>>> --- >>>>> UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize.c | >>>>> 7 >>>>> ++++++- >>>>> 1 file changed, 6 insertions(+), 1 deletion(-) >>>>> >>>>> diff --git >>>>> a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize.c >>>>> b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize.c >>>>> index 624ddee055..a64326239f 100644 >>>>> --- >>>>> a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize.c >>>>> +++ >> b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize. >>>>> +++ c >>>>> @@ -832,7 +832,12 @@ ProgramProcessorRegister ( >>>>> RegisterTableEntry = &RegisterTableEntryHead[Index]; >>>>> >>>>> DEBUG_CODE_BEGIN (); >>>>> - AcquireSpinLock (&CpuFlags->ConsoleLogLock); >>>>> + // >>>>> + // Wait for the AP to release the MSR spin lock. >>>>> + // >>>>> + while (!AcquireSpinLockOrFail (&CpuFlags->ConsoleLogLock)) { >>>>> + CpuPause (); >>>>> + } >>>>> ThreadIndex = ApLocation->Package * >> CpuStatus->MaxCoreCount * >>>>> CpuStatus->MaxThreadCount + >>>>> ApLocation->Core * CpuStatus->MaxThreadCount + >>>>> ApLocation->Thread; >>>>> -- >>>>> 2.15.0.windows.1 >>>>> >>>>> _______________________________________________ >>>>> edk2-devel mailing list >>>>> edk2-devel@lists.01.org >>>>> https://lists.01.org/mailman/listinfo/edk2-devel > _______________________________________________ > edk2-devel mailing list > edk2-devel@lists.01.org > https://lists.01.org/mailman/listinfo/edk2-devel > -- Brian J. Johnson Enterprise X86 Lab Hewlett Packard Enterprise brian.johnson@hpe.com ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Patch 1/3] UefiCpuPkg/RegisterCpuFeaturesLib: Avoid AP calls PeiService. 2018-12-20 16:36 ` Brian J. Johnson @ 2018-12-21 2:06 ` Dong, Eric 2019-01-09 5:26 ` Dong, Eric 1 sibling, 0 replies; 15+ messages in thread From: Dong, Eric @ 2018-12-21 2:06 UTC (permalink / raw) To: Brian J. Johnson, Yao, Jiewen, Ni, Ruiyu, edk2-devel@lists.01.org Cc: Laszlo Ersek Hi, Thanks all for your comments. It's valuable to me. Seems like we all agree to remove the PcdSpinLockTimeout in AcquireSpinLock. I have submit BZ for it: https://bugzilla.tianocore.org/show_bug.cgi?id=1419 For my patch, I will avoid this code change and set PcdSpinLockTimeout=0 for my modules. I will send V2 patch soon. Thanks, Eric > -----Original Message----- > From: Brian J. Johnson [mailto:brian.johnson@hpe.com] > Sent: Friday, December 21, 2018 12:36 AM > To: Yao, Jiewen <jiewen.yao@intel.com>; Ni, Ruiyu <ruiyu.ni@intel.com>; > Dong, Eric <eric.dong@intel.com>; edk2-devel@lists.01.org > Cc: Laszlo Ersek <lersek@redhat.com> > Subject: Re: [edk2] [Patch 1/3] UefiCpuPkg/RegisterCpuFeaturesLib: Avoid > AP calls PeiService. > > Agreed. We've seen issues on real platforms with timed-out spinlocks in DXE > causing calls to GetPerformanceCounter and DebugAssert. (DXE has the > same code, with the same issues.) > > Note that it's possible to set PcdSpinLockTimeout=0 to work around the issue > on a particular platform, or in a particular module. But if you have to do that > for every module which uses APs, and hence could contend for a spinlock, it > kind of defeats the point.... We're better off removing the timeout code. > > Thanks, > Brian > > On 12/19/18 8:08 PM, Yao, Jiewen wrote: > > Yes, I agree, if we don't have any real case. > > > > > >> -----Original Message----- > >> From: Ni, Ruiyu > >> Sent: Thursday, December 20, 2018 10:07 AM > >> To: Dong, Eric <eric.dong@intel.com>; Yao, Jiewen > >> <jiewen.yao@intel.com>; edk2-devel@lists.01.org > >> Cc: Laszlo Ersek <lersek@redhat.com> > >> Subject: RE: [edk2] [Patch 1/3] UefiCpuPkg/RegisterCpuFeaturesLib: > >> Avoid AP calls PeiService. > >> > >> Can you just change the AcquireSpinLock() behavior to remove the > >> Timeout PCD consumption? > >> > >> I haven't seen a real case that the timed acquisition of spin lock is needed. > >> > >> > >> Thanks/Ray > >> > >>> -----Original Message----- > >>> From: Dong, Eric <eric.dong@intel.com> > >>> Sent: Thursday, December 20, 2018 9:23 AM > >>> To: Yao, Jiewen <jiewen.yao@intel.com>; edk2-devel@lists.01.org > >>> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Laszlo Ersek <lersek@redhat.com> > >>> Subject: RE: [edk2] [Patch 1/3] UefiCpuPkg/RegisterCpuFeaturesLib: > >>> Avoid AP calls PeiService. > >>> > >>> > >>> Agreed, Maybe it's time to add a new API like > >>> AcquireSpinLockWithoutTimeOut? > >>> > >>> Thanks, > >>> Eric > >>>> -----Original Message----- > >>>> From: Yao, Jiewen > >>>> Sent: Thursday, December 20, 2018 9:19 AM > >>>> To: Dong, Eric <eric.dong@intel.com>; edk2-devel@lists.01.org > >>>> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Laszlo Ersek > >>>> <lersek@redhat.com> > >>>> Subject: RE: [edk2] [Patch 1/3] UefiCpuPkg/RegisterCpuFeaturesLib: > >>>> Avoid AP calls PeiService. > >>>> > >>>> Hi > >>>> If we think below code is generic, can we have an API for that? > >>>> > >>>> + // > >>>> + // Wait for the AP to release the MSR spin lock. > >>>> + // > >>>> + while (!AcquireSpinLockOrFail (&CpuFlags->ConsoleLogLock)) { > >>>> + CpuPause (); > >>>> + } > >>>> > >>>> > >>>> > >>>> > >>>>> -----Original Message----- > >>>>> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On > >>>>> Behalf Of Eric Dong > >>>>> Sent: Thursday, December 20, 2018 9:16 AM > >>>>> To: edk2-devel@lists.01.org > >>>>> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Laszlo Ersek > >>>>> <lersek@redhat.com> > >>>>> Subject: [edk2] [Patch 1/3] UefiCpuPkg/RegisterCpuFeaturesLib: > >>>>> Avoid AP calls PeiService. > >>>>> > >>>>> In AcquireSpinLock function, it calls GetPerformanceCounter which > >>>>> final calls PeiService service. This patch avoid to call > >>>>> AcquireSpinLock function. > >>>>> > >>>>> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=1411 > >>>>> > >>>>> Cc: Ruiyu Ni <ruiyu.ni@intel.com> > >>>>> Cc: Laszlo Ersek <lersek@redhat.com> > >>>>> Contributed-under: TianoCore Contribution Agreement 1.1 > >>>>> Signed-off-by: Eric Dong <eric.dong@intel.com> > >>>>> --- > >>>>> > >>>>> UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize.c > >>>>> | > >>>>> 7 > >>>>> ++++++- > >>>>> 1 file changed, 6 insertions(+), 1 deletion(-) > >>>>> > >>>>> diff --git > >>>>> a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize. > >>>>> c > >>>>> b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize. > >>>>> c index 624ddee055..a64326239f 100644 > >>>>> --- > >>>>> a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize. > >>>>> c > >>>>> +++ > >> b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize. > >>>>> +++ c > >>>>> @@ -832,7 +832,12 @@ ProgramProcessorRegister ( > >>>>> RegisterTableEntry = &RegisterTableEntryHead[Index]; > >>>>> > >>>>> DEBUG_CODE_BEGIN (); > >>>>> - AcquireSpinLock (&CpuFlags->ConsoleLogLock); > >>>>> + // > >>>>> + // Wait for the AP to release the MSR spin lock. > >>>>> + // > >>>>> + while (!AcquireSpinLockOrFail (&CpuFlags->ConsoleLogLock)) { > >>>>> + CpuPause (); > >>>>> + } > >>>>> ThreadIndex = ApLocation->Package * > >> CpuStatus->MaxCoreCount * > >>>>> CpuStatus->MaxThreadCount + > >>>>> ApLocation->Core * CpuStatus->MaxThreadCount + > >>>>> ApLocation->Thread; > >>>>> -- > >>>>> 2.15.0.windows.1 > >>>>> > >>>>> _______________________________________________ > >>>>> edk2-devel mailing list > >>>>> edk2-devel@lists.01.org > >>>>> https://lists.01.org/mailman/listinfo/edk2-devel > > _______________________________________________ > > edk2-devel mailing list > > edk2-devel@lists.01.org > > https://lists.01.org/mailman/listinfo/edk2-devel > > > > > -- > Brian J. Johnson > Enterprise X86 Lab > > Hewlett Packard Enterprise > > brian.johnson@hpe.com ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Patch 1/3] UefiCpuPkg/RegisterCpuFeaturesLib: Avoid AP calls PeiService. 2018-12-20 16:36 ` Brian J. Johnson 2018-12-21 2:06 ` Dong, Eric @ 2019-01-09 5:26 ` Dong, Eric 2019-01-09 10:35 ` Laszlo Ersek 1 sibling, 1 reply; 15+ messages in thread From: Dong, Eric @ 2019-01-09 5:26 UTC (permalink / raw) To: Brian J. Johnson, Yao, Jiewen, Ni, Ray, edk2-devel@lists.01.org Cc: Laszlo Ersek, Kinney, Michael D, Dong, Eric Hi all, We got some feedback about this BZ. Someone think this timeout is valuable for the debug purpose, and oppose to remove it. https://bugzilla.tianocore.org/show_bug.cgi?id=1419 So I'm back to here and want to still use this change. I not use "update PcdSpinLockTimeout to 0 in platform" solution because I think core driver depends on platform policy is not a good design. Do you guys have any other concern? Thanks, Eric > -----Original Message----- > From: Brian J. Johnson [mailto:brian.johnson@hpe.com] > Sent: Friday, December 21, 2018 12:36 AM > To: Yao, Jiewen <jiewen.yao@intel.com>; Ni, Ruiyu <ruiyu.ni@intel.com>; > Dong, Eric <eric.dong@intel.com>; edk2-devel@lists.01.org > Cc: Laszlo Ersek <lersek@redhat.com> > Subject: Re: [edk2] [Patch 1/3] UefiCpuPkg/RegisterCpuFeaturesLib: Avoid > AP calls PeiService. > > Agreed. We've seen issues on real platforms with timed-out spinlocks in DXE > causing calls to GetPerformanceCounter and DebugAssert. (DXE has the > same code, with the same issues.) > > Note that it's possible to set PcdSpinLockTimeout=0 to work around the issue > on a particular platform, or in a particular module. But if you have to do that > for every module which uses APs, and hence could contend for a spinlock, it > kind of defeats the point.... We're better off removing the timeout code. > > Thanks, > Brian > > On 12/19/18 8:08 PM, Yao, Jiewen wrote: > > Yes, I agree, if we don't have any real case. > > > > > >> -----Original Message----- > >> From: Ni, Ruiyu > >> Sent: Thursday, December 20, 2018 10:07 AM > >> To: Dong, Eric <eric.dong@intel.com>; Yao, Jiewen > >> <jiewen.yao@intel.com>; edk2-devel@lists.01.org > >> Cc: Laszlo Ersek <lersek@redhat.com> > >> Subject: RE: [edk2] [Patch 1/3] UefiCpuPkg/RegisterCpuFeaturesLib: > >> Avoid AP calls PeiService. > >> > >> Can you just change the AcquireSpinLock() behavior to remove the > >> Timeout PCD consumption? > >> > >> I haven't seen a real case that the timed acquisition of spin lock is needed. > >> > >> > >> Thanks/Ray > >> > >>> -----Original Message----- > >>> From: Dong, Eric <eric.dong@intel.com> > >>> Sent: Thursday, December 20, 2018 9:23 AM > >>> To: Yao, Jiewen <jiewen.yao@intel.com>; edk2-devel@lists.01.org > >>> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Laszlo Ersek <lersek@redhat.com> > >>> Subject: RE: [edk2] [Patch 1/3] UefiCpuPkg/RegisterCpuFeaturesLib: > >>> Avoid AP calls PeiService. > >>> > >>> > >>> Agreed, Maybe it's time to add a new API like > >>> AcquireSpinLockWithoutTimeOut? > >>> > >>> Thanks, > >>> Eric > >>>> -----Original Message----- > >>>> From: Yao, Jiewen > >>>> Sent: Thursday, December 20, 2018 9:19 AM > >>>> To: Dong, Eric <eric.dong@intel.com>; edk2-devel@lists.01.org > >>>> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Laszlo Ersek > >>>> <lersek@redhat.com> > >>>> Subject: RE: [edk2] [Patch 1/3] UefiCpuPkg/RegisterCpuFeaturesLib: > >>>> Avoid AP calls PeiService. > >>>> > >>>> Hi > >>>> If we think below code is generic, can we have an API for that? > >>>> > >>>> + // > >>>> + // Wait for the AP to release the MSR spin lock. > >>>> + // > >>>> + while (!AcquireSpinLockOrFail (&CpuFlags->ConsoleLogLock)) { > >>>> + CpuPause (); > >>>> + } > >>>> > >>>> > >>>> > >>>> > >>>>> -----Original Message----- > >>>>> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On > >>>>> Behalf Of Eric Dong > >>>>> Sent: Thursday, December 20, 2018 9:16 AM > >>>>> To: edk2-devel@lists.01.org > >>>>> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Laszlo Ersek > >>>>> <lersek@redhat.com> > >>>>> Subject: [edk2] [Patch 1/3] UefiCpuPkg/RegisterCpuFeaturesLib: > >>>>> Avoid AP calls PeiService. > >>>>> > >>>>> In AcquireSpinLock function, it calls GetPerformanceCounter which > >>>>> final calls PeiService service. This patch avoid to call > >>>>> AcquireSpinLock function. > >>>>> > >>>>> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=1411 > >>>>> > >>>>> Cc: Ruiyu Ni <ruiyu.ni@intel.com> > >>>>> Cc: Laszlo Ersek <lersek@redhat.com> > >>>>> Contributed-under: TianoCore Contribution Agreement 1.1 > >>>>> Signed-off-by: Eric Dong <eric.dong@intel.com> > >>>>> --- > >>>>> > >>>>> UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize.c > >>>>> | > >>>>> 7 > >>>>> ++++++- > >>>>> 1 file changed, 6 insertions(+), 1 deletion(-) > >>>>> > >>>>> diff --git > >>>>> a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize. > >>>>> c > >>>>> b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize. > >>>>> c index 624ddee055..a64326239f 100644 > >>>>> --- > >>>>> a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize. > >>>>> c > >>>>> +++ > >> b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize. > >>>>> +++ c > >>>>> @@ -832,7 +832,12 @@ ProgramProcessorRegister ( > >>>>> RegisterTableEntry = &RegisterTableEntryHead[Index]; > >>>>> > >>>>> DEBUG_CODE_BEGIN (); > >>>>> - AcquireSpinLock (&CpuFlags->ConsoleLogLock); > >>>>> + // > >>>>> + // Wait for the AP to release the MSR spin lock. > >>>>> + // > >>>>> + while (!AcquireSpinLockOrFail (&CpuFlags->ConsoleLogLock)) { > >>>>> + CpuPause (); > >>>>> + } > >>>>> ThreadIndex = ApLocation->Package * > >> CpuStatus->MaxCoreCount * > >>>>> CpuStatus->MaxThreadCount + > >>>>> ApLocation->Core * CpuStatus->MaxThreadCount + > >>>>> ApLocation->Thread; > >>>>> -- > >>>>> 2.15.0.windows.1 > >>>>> > >>>>> _______________________________________________ > >>>>> edk2-devel mailing list > >>>>> edk2-devel@lists.01.org > >>>>> https://lists.01.org/mailman/listinfo/edk2-devel > > _______________________________________________ > > edk2-devel mailing list > > edk2-devel@lists.01.org > > https://lists.01.org/mailman/listinfo/edk2-devel > > > > > -- > Brian J. Johnson > Enterprise X86 Lab > > Hewlett Packard Enterprise > > brian.johnson@hpe.com ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Patch 1/3] UefiCpuPkg/RegisterCpuFeaturesLib: Avoid AP calls PeiService. 2019-01-09 5:26 ` Dong, Eric @ 2019-01-09 10:35 ` Laszlo Ersek 2019-01-10 5:53 ` Dong, Eric [not found] ` <734D49CCEBEEF84792F5B80ED585239D5BFAE3BF@SHSMSX103.ccr.corp.intel.com> 0 siblings, 2 replies; 15+ messages in thread From: Laszlo Ersek @ 2019-01-09 10:35 UTC (permalink / raw) To: Dong, Eric Cc: Brian J. Johnson, Yao, Jiewen, Ni, Ray, edk2-devel@lists.01.org, Kinney, Michael D Hi Eric, On 01/09/19 06:26, Dong, Eric wrote: > Hi all, > > We got some feedback about this BZ. Someone think this timeout is valuable for the debug purpose, and oppose to remove it. > https://bugzilla.tianocore.org/show_bug.cgi?id=1419 > > So I'm back to here and want to still use this change. I not use "update PcdSpinLockTimeout to 0 in platform" solution because I think core driver depends on platform policy is not a good design. > > Do you guys have any other concern? sorry, I don't understand. (1) In <https://bugzilla.tianocore.org/show_bug.cgi?id=1419>, where currently comment #2 is the last comment, I don't see any request for keeping the timeout facility. (2) On the mailing list as well, you seem to have received comments only in favor of removing the timeout facility. "Someone think this timeout is valuable for the debug purpose" doesn't cut it, for open development. I don't care about the identity of the person that wants to preserve the feature, but I certainly care about their use case. You shouldn't have to mediate and describe their use case for them, on the list; that's always a lossy process. Regarding the PCD: I think zeroing "PcdSpinLockTimeout" to disable the timeout case is a valid approach, it's just that we should change the default value in the DEC file to zero. Then the PCD setting will become a burden only for those platforms and those use cases that want to use the timeout feature (such as for debugging). In general, PCD default values in DEC files have to be considered carefully, but in some cases, such changes are the right thing. Another example was 509f8425b75d ("UefiCpuPkg: change PcdCpuSmmStackGuard default to TRUE", 2016-06-06). (You made the same point at the end of <https://bugzilla.tianocore.org/show_bug.cgi?id=1419#c2>.) In addition to changing the default value to zero, I'd suggest moving "PcdSpinLockTimeout" from section [PcdsFixedAtBuild,PcdsPatchableInModule] to section [PcdsFixedAtBuild, PcdsPatchableInModule, PcdsDynamic, PcdsDynamicEx] so that platforms can enable the "debug" feature (i.e. set a nonzero value) more flexibly. Thanks Laszlo >> -----Original Message----- >> From: Brian J. Johnson [mailto:brian.johnson@hpe.com] >> Sent: Friday, December 21, 2018 12:36 AM >> To: Yao, Jiewen <jiewen.yao@intel.com>; Ni, Ruiyu <ruiyu.ni@intel.com>; >> Dong, Eric <eric.dong@intel.com>; edk2-devel@lists.01.org >> Cc: Laszlo Ersek <lersek@redhat.com> >> Subject: Re: [edk2] [Patch 1/3] UefiCpuPkg/RegisterCpuFeaturesLib: Avoid >> AP calls PeiService. >> >> Agreed. We've seen issues on real platforms with timed-out spinlocks in DXE >> causing calls to GetPerformanceCounter and DebugAssert. (DXE has the >> same code, with the same issues.) >> >> Note that it's possible to set PcdSpinLockTimeout=0 to work around the issue >> on a particular platform, or in a particular module. But if you have to do that >> for every module which uses APs, and hence could contend for a spinlock, it >> kind of defeats the point.... We're better off removing the timeout code. >> >> Thanks, >> Brian >> >> On 12/19/18 8:08 PM, Yao, Jiewen wrote: >>> Yes, I agree, if we don't have any real case. >>> >>> >>>> -----Original Message----- >>>> From: Ni, Ruiyu >>>> Sent: Thursday, December 20, 2018 10:07 AM >>>> To: Dong, Eric <eric.dong@intel.com>; Yao, Jiewen >>>> <jiewen.yao@intel.com>; edk2-devel@lists.01.org >>>> Cc: Laszlo Ersek <lersek@redhat.com> >>>> Subject: RE: [edk2] [Patch 1/3] UefiCpuPkg/RegisterCpuFeaturesLib: >>>> Avoid AP calls PeiService. >>>> >>>> Can you just change the AcquireSpinLock() behavior to remove the >>>> Timeout PCD consumption? >>>> >>>> I haven't seen a real case that the timed acquisition of spin lock is needed. >>>> >>>> >>>> Thanks/Ray >>>> >>>>> -----Original Message----- >>>>> From: Dong, Eric <eric.dong@intel.com> >>>>> Sent: Thursday, December 20, 2018 9:23 AM >>>>> To: Yao, Jiewen <jiewen.yao@intel.com>; edk2-devel@lists.01.org >>>>> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Laszlo Ersek <lersek@redhat.com> >>>>> Subject: RE: [edk2] [Patch 1/3] UefiCpuPkg/RegisterCpuFeaturesLib: >>>>> Avoid AP calls PeiService. >>>>> >>>>> >>>>> Agreed, Maybe it's time to add a new API like >>>>> AcquireSpinLockWithoutTimeOut? >>>>> >>>>> Thanks, >>>>> Eric >>>>>> -----Original Message----- >>>>>> From: Yao, Jiewen >>>>>> Sent: Thursday, December 20, 2018 9:19 AM >>>>>> To: Dong, Eric <eric.dong@intel.com>; edk2-devel@lists.01.org >>>>>> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Laszlo Ersek >>>>>> <lersek@redhat.com> >>>>>> Subject: RE: [edk2] [Patch 1/3] UefiCpuPkg/RegisterCpuFeaturesLib: >>>>>> Avoid AP calls PeiService. >>>>>> >>>>>> Hi >>>>>> If we think below code is generic, can we have an API for that? >>>>>> >>>>>> + // >>>>>> + // Wait for the AP to release the MSR spin lock. >>>>>> + // >>>>>> + while (!AcquireSpinLockOrFail (&CpuFlags->ConsoleLogLock)) { >>>>>> + CpuPause (); >>>>>> + } >>>>>> >>>>>> >>>>>> >>>>>> >>>>>>> -----Original Message----- >>>>>>> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On >>>>>>> Behalf Of Eric Dong >>>>>>> Sent: Thursday, December 20, 2018 9:16 AM >>>>>>> To: edk2-devel@lists.01.org >>>>>>> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Laszlo Ersek >>>>>>> <lersek@redhat.com> >>>>>>> Subject: [edk2] [Patch 1/3] UefiCpuPkg/RegisterCpuFeaturesLib: >>>>>>> Avoid AP calls PeiService. >>>>>>> >>>>>>> In AcquireSpinLock function, it calls GetPerformanceCounter which >>>>>>> final calls PeiService service. This patch avoid to call >>>>>>> AcquireSpinLock function. >>>>>>> >>>>>>> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=1411 >>>>>>> >>>>>>> Cc: Ruiyu Ni <ruiyu.ni@intel.com> >>>>>>> Cc: Laszlo Ersek <lersek@redhat.com> >>>>>>> Contributed-under: TianoCore Contribution Agreement 1.1 >>>>>>> Signed-off-by: Eric Dong <eric.dong@intel.com> >>>>>>> --- >>>>>>> >>>>>>> UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize.c >>>>>>> | >>>>>>> 7 >>>>>>> ++++++- >>>>>>> 1 file changed, 6 insertions(+), 1 deletion(-) >>>>>>> >>>>>>> diff --git >>>>>>> a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize. >>>>>>> c >>>>>>> b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize. >>>>>>> c index 624ddee055..a64326239f 100644 >>>>>>> --- >>>>>>> a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize. >>>>>>> c >>>>>>> +++ >>>> b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize. >>>>>>> +++ c >>>>>>> @@ -832,7 +832,12 @@ ProgramProcessorRegister ( >>>>>>> RegisterTableEntry = &RegisterTableEntryHead[Index]; >>>>>>> >>>>>>> DEBUG_CODE_BEGIN (); >>>>>>> - AcquireSpinLock (&CpuFlags->ConsoleLogLock); >>>>>>> + // >>>>>>> + // Wait for the AP to release the MSR spin lock. >>>>>>> + // >>>>>>> + while (!AcquireSpinLockOrFail (&CpuFlags->ConsoleLogLock)) { >>>>>>> + CpuPause (); >>>>>>> + } >>>>>>> ThreadIndex = ApLocation->Package * >>>> CpuStatus->MaxCoreCount * >>>>>>> CpuStatus->MaxThreadCount + >>>>>>> ApLocation->Core * CpuStatus->MaxThreadCount + >>>>>>> ApLocation->Thread; >>>>>>> -- >>>>>>> 2.15.0.windows.1 >>>>>>> >>>>>>> _______________________________________________ >>>>>>> edk2-devel mailing list >>>>>>> edk2-devel@lists.01.org >>>>>>> https://lists.01.org/mailman/listinfo/edk2-devel >>> _______________________________________________ >>> edk2-devel mailing list >>> edk2-devel@lists.01.org >>> https://lists.01.org/mailman/listinfo/edk2-devel >>> >> >> >> -- >> Brian J. Johnson >> Enterprise X86 Lab >> >> Hewlett Packard Enterprise >> >> brian.johnson@hpe.com > ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Patch 1/3] UefiCpuPkg/RegisterCpuFeaturesLib: Avoid AP calls PeiService. 2019-01-09 10:35 ` Laszlo Ersek @ 2019-01-10 5:53 ` Dong, Eric [not found] ` <734D49CCEBEEF84792F5B80ED585239D5BFAE3BF@SHSMSX103.ccr.corp.intel.com> 1 sibling, 0 replies; 15+ messages in thread From: Dong, Eric @ 2019-01-10 5:53 UTC (permalink / raw) To: Laszlo Ersek Cc: Brian J. Johnson, Yao, Jiewen, Ni, Ray, edk2-devel@lists.01.org, Kinney, Michael D, Dong, Eric Hi Laszlo, > -----Original Message----- > From: Laszlo Ersek [mailto:lersek@redhat.com] > Sent: Wednesday, January 9, 2019 6:35 PM > To: Dong, Eric <eric.dong@intel.com> > Cc: Brian J. Johnson <brian.johnson@hpe.com>; Yao, Jiewen > <jiewen.yao@intel.com>; Ni, Ray <ray.ni@intel.com>; edk2- > devel@lists.01.org; Kinney, Michael D <michael.d.kinney@intel.com> > Subject: Re: [edk2] [Patch 1/3] UefiCpuPkg/RegisterCpuFeaturesLib: Avoid > AP calls PeiService. > > Hi Eric, > > On 01/09/19 06:26, Dong, Eric wrote: > > Hi all, > > > > We got some feedback about this BZ. Someone think this timeout is > valuable for the debug purpose, and oppose to remove it. > > https://bugzilla.tianocore.org/show_bug.cgi?id=1419 > > > > So I'm back to here and want to still use this change. I not use "update > PcdSpinLockTimeout to 0 in platform" solution because I think core driver > depends on platform policy is not a good design. > > > > Do you guys have any other concern? > > sorry, I don't understand. > > (1) In <https://bugzilla.tianocore.org/show_bug.cgi?id=1419>, where > currently comment #2 is the last comment, I don't see any request for > keeping the timeout facility. Please check Ray's comments I discussed with him offline. > > (2) On the mailing list as well, you seem to have received comments only in > favor of removing the timeout facility. > > "Someone think this timeout is valuable for the debug purpose" doesn't cut > it, for open development. I don't care about the identity of the person that > wants to preserve the feature, but I certainly care about their use case. You > shouldn't have to mediate and describe their use case for them, on the list; > that's always a lossy process. > Please check Ray's comments I discussed with him offline. > Regarding the PCD: I think zeroing "PcdSpinLockTimeout" to disable the > timeout case is a valid approach, it's just that we should change the default > value in the DEC file to zero. Then the PCD setting will become a burden only > for those platforms and those use cases that want to use the timeout > feature (such as for debugging). > > In general, PCD default values in DEC files have to be considered carefully, > but in some cases, such changes are the right thing. Another example was > 509f8425b75d ("UefiCpuPkg: change PcdCpuSmmStackGuard default to > TRUE", 2016-06-06). > > (You made the same point at the end of > <https://bugzilla.tianocore.org/show_bug.cgi?id=1419#c2>.) > > > In addition to changing the default value to zero, I'd suggest moving > "PcdSpinLockTimeout" from section > > [PcdsFixedAtBuild,PcdsPatchableInModule] > > to section > > [PcdsFixedAtBuild, PcdsPatchableInModule, PcdsDynamic, PcdsDynamicEx] > > so that platforms can enable the "debug" feature (i.e. set a nonzero > value) more flexibly. Agree to do both changes, will submit BZ for it if we get the agreement in this mailing list. Thanks, Eric > > Thanks > Laszlo > > >> -----Original Message----- > >> From: Brian J. Johnson [mailto:brian.johnson@hpe.com] > >> Sent: Friday, December 21, 2018 12:36 AM > >> To: Yao, Jiewen <jiewen.yao@intel.com>; Ni, Ruiyu > >> <ruiyu.ni@intel.com>; Dong, Eric <eric.dong@intel.com>; > >> edk2-devel@lists.01.org > >> Cc: Laszlo Ersek <lersek@redhat.com> > >> Subject: Re: [edk2] [Patch 1/3] UefiCpuPkg/RegisterCpuFeaturesLib: > >> Avoid AP calls PeiService. > >> > >> Agreed. We've seen issues on real platforms with timed-out spinlocks > >> in DXE causing calls to GetPerformanceCounter and DebugAssert. (DXE > >> has the same code, with the same issues.) > >> > >> Note that it's possible to set PcdSpinLockTimeout=0 to work around > >> the issue on a particular platform, or in a particular module. But > >> if you have to do that for every module which uses APs, and hence > >> could contend for a spinlock, it kind of defeats the point.... We're better > off removing the timeout code. > >> > >> Thanks, > >> Brian > >> > >> On 12/19/18 8:08 PM, Yao, Jiewen wrote: > >>> Yes, I agree, if we don't have any real case. > >>> > >>> > >>>> -----Original Message----- > >>>> From: Ni, Ruiyu > >>>> Sent: Thursday, December 20, 2018 10:07 AM > >>>> To: Dong, Eric <eric.dong@intel.com>; Yao, Jiewen > >>>> <jiewen.yao@intel.com>; edk2-devel@lists.01.org > >>>> Cc: Laszlo Ersek <lersek@redhat.com> > >>>> Subject: RE: [edk2] [Patch 1/3] UefiCpuPkg/RegisterCpuFeaturesLib: > >>>> Avoid AP calls PeiService. > >>>> > >>>> Can you just change the AcquireSpinLock() behavior to remove the > >>>> Timeout PCD consumption? > >>>> > >>>> I haven't seen a real case that the timed acquisition of spin lock is > needed. > >>>> > >>>> > >>>> Thanks/Ray > >>>> > >>>>> -----Original Message----- > >>>>> From: Dong, Eric <eric.dong@intel.com> > >>>>> Sent: Thursday, December 20, 2018 9:23 AM > >>>>> To: Yao, Jiewen <jiewen.yao@intel.com>; edk2-devel@lists.01.org > >>>>> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Laszlo Ersek > >>>>> <lersek@redhat.com> > >>>>> Subject: RE: [edk2] [Patch 1/3] UefiCpuPkg/RegisterCpuFeaturesLib: > >>>>> Avoid AP calls PeiService. > >>>>> > >>>>> > >>>>> Agreed, Maybe it's time to add a new API like > >>>>> AcquireSpinLockWithoutTimeOut? > >>>>> > >>>>> Thanks, > >>>>> Eric > >>>>>> -----Original Message----- > >>>>>> From: Yao, Jiewen > >>>>>> Sent: Thursday, December 20, 2018 9:19 AM > >>>>>> To: Dong, Eric <eric.dong@intel.com>; edk2-devel@lists.01.org > >>>>>> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Laszlo Ersek > >>>>>> <lersek@redhat.com> > >>>>>> Subject: RE: [edk2] [Patch 1/3] UefiCpuPkg/RegisterCpuFeaturesLib: > >>>>>> Avoid AP calls PeiService. > >>>>>> > >>>>>> Hi > >>>>>> If we think below code is generic, can we have an API for that? > >>>>>> > >>>>>> + // > >>>>>> + // Wait for the AP to release the MSR spin lock. > >>>>>> + // > >>>>>> + while (!AcquireSpinLockOrFail (&CpuFlags->ConsoleLogLock)) { > >>>>>> + CpuPause (); > >>>>>> + } > >>>>>> > >>>>>> > >>>>>> > >>>>>> > >>>>>>> -----Original Message----- > >>>>>>> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On > >>>>>>> Behalf Of Eric Dong > >>>>>>> Sent: Thursday, December 20, 2018 9:16 AM > >>>>>>> To: edk2-devel@lists.01.org > >>>>>>> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Laszlo Ersek > >>>>>>> <lersek@redhat.com> > >>>>>>> Subject: [edk2] [Patch 1/3] UefiCpuPkg/RegisterCpuFeaturesLib: > >>>>>>> Avoid AP calls PeiService. > >>>>>>> > >>>>>>> In AcquireSpinLock function, it calls GetPerformanceCounter > >>>>>>> which final calls PeiService service. This patch avoid to call > >>>>>>> AcquireSpinLock function. > >>>>>>> > >>>>>>> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=1411 > >>>>>>> > >>>>>>> Cc: Ruiyu Ni <ruiyu.ni@intel.com> > >>>>>>> Cc: Laszlo Ersek <lersek@redhat.com> > >>>>>>> Contributed-under: TianoCore Contribution Agreement 1.1 > >>>>>>> Signed-off-by: Eric Dong <eric.dong@intel.com> > >>>>>>> --- > >>>>>>> > >>>>>>> UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize. > >>>>>>> c > >>>>>>> | > >>>>>>> 7 > >>>>>>> ++++++- > >>>>>>> 1 file changed, 6 insertions(+), 1 deletion(-) > >>>>>>> > >>>>>>> diff --git > >>>>>>> > a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize. > >>>>>>> c > >>>>>>> > b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize. > >>>>>>> c index 624ddee055..a64326239f 100644 > >>>>>>> --- > >>>>>>> > a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize. > >>>>>>> c > >>>>>>> +++ > >>>> b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize. > >>>>>>> +++ c > >>>>>>> @@ -832,7 +832,12 @@ ProgramProcessorRegister ( > >>>>>>> RegisterTableEntry = &RegisterTableEntryHead[Index]; > >>>>>>> > >>>>>>> DEBUG_CODE_BEGIN (); > >>>>>>> - AcquireSpinLock (&CpuFlags->ConsoleLogLock); > >>>>>>> + // > >>>>>>> + // Wait for the AP to release the MSR spin lock. > >>>>>>> + // > >>>>>>> + while (!AcquireSpinLockOrFail (&CpuFlags->ConsoleLogLock)) { > >>>>>>> + CpuPause (); > >>>>>>> + } > >>>>>>> ThreadIndex = ApLocation->Package * > >>>> CpuStatus->MaxCoreCount * > >>>>>>> CpuStatus->MaxThreadCount + > >>>>>>> ApLocation->Core * CpuStatus->MaxThreadCount + > >>>>>>> ApLocation->Thread; > >>>>>>> -- > >>>>>>> 2.15.0.windows.1 > >>>>>>> > >>>>>>> _______________________________________________ > >>>>>>> edk2-devel mailing list > >>>>>>> edk2-devel@lists.01.org > >>>>>>> https://lists.01.org/mailman/listinfo/edk2-devel > >>> _______________________________________________ > >>> edk2-devel mailing list > >>> edk2-devel@lists.01.org > >>> https://lists.01.org/mailman/listinfo/edk2-devel > >>> > >> > >> > >> -- > >> Brian J. Johnson > >> Enterprise X86 Lab > >> > >> Hewlett Packard Enterprise > >> > >> brian.johnson@hpe.com > > ^ permalink raw reply [flat|nested] 15+ messages in thread
[parent not found: <734D49CCEBEEF84792F5B80ED585239D5BFAE3BF@SHSMSX103.ccr.corp.intel.com>]
* Re: [Patch 1/3] UefiCpuPkg/RegisterCpuFeaturesLib: Avoid AP calls PeiService. [not found] ` <734D49CCEBEEF84792F5B80ED585239D5BFAE3BF@SHSMSX103.ccr.corp.intel.com> @ 2019-01-10 12:51 ` Laszlo Ersek 0 siblings, 0 replies; 15+ messages in thread From: Laszlo Ersek @ 2019-01-10 12:51 UTC (permalink / raw) To: Ni, Ray, Dong, Eric, Kinney, Michael D, Brian J. Johnson Cc: Yao, Jiewen, edk2-devel@lists.01.org On 01/10/19 05:51, Ni, Ray wrote: > > >> -----Original Message----- >> From: Laszlo Ersek <lersek@redhat.com> >> Sent: Wednesday, January 9, 2019 6:35 PM >> To: Dong, Eric <eric.dong@intel.com> >> Cc: Brian J. Johnson <brian.johnson@hpe.com>; Yao, Jiewen >> <jiewen.yao@intel.com>; Ni, Ray <ray.ni@intel.com>; edk2- >> devel@lists.01.org; Kinney, Michael D <michael.d.kinney@intel.com> >> Subject: Re: [edk2] [Patch 1/3] UefiCpuPkg/RegisterCpuFeaturesLib: Avoid >> AP calls PeiService. >> >> Hi Eric, >> >> On 01/09/19 06:26, Dong, Eric wrote: >>> Hi all, >>> >>> We got some feedback about this BZ. Someone think this timeout is >> valuable for the debug purpose, and oppose to remove it. >>> https://bugzilla.tianocore.org/show_bug.cgi?id=1419 >>> >>> So I'm back to here and want to still use this change. I not use "update >> PcdSpinLockTimeout to 0 in platform" solution because I think core driver >> depends on platform policy is not a good design. >>> >>> Do you guys have any other concern? >> >> sorry, I don't understand. >> >> (1) In <https://bugzilla.tianocore.org/show_bug.cgi?id=1419>, where >> currently comment #2 is the last comment, I don't see any request for >> keeping the timeout facility. > > The comment "timeout is valuable for debugging" is raised in bug scrub meeting. > (Sorry that's not a public meeting yet. I think there is effort to make it public, not sure). > >> >> (2) On the mailing list as well, you seem to have received comments only in >> favor of removing the timeout facility. >> >> "Someone think this timeout is valuable for the debug purpose" doesn't cut >> it, for open development. I don't care about the identity of the person that >> wants to preserve the feature, but I certainly care about their use case. You >> shouldn't have to mediate and describe their use case for them, on the list; >> that's always a lossy process. > > Use case of the timeout: > When someone mis-uses the spin lock that causes dead lock, timeout assertion > debug message can help the one to know something wrong happens. > Instead of the system just hangs without any debug message. > > But I doubt whether the benefit of timeout is bigger than the potential issue it > brings. Potential issues are: > 1. User may mis-use time lib in AP procedure causing assertion. > 2. Not sure HPE Brian's issue is similar to #1. @Brian > > Basically, I also don' t like the idea that a BASE Synchronization library depends on > a non-BASE timer lib. It makes the Synchronization library a non-BASE lib in most > of the case. Or platform developer needs some change to make it BASE. Changes > could be: > 1. Setting the timeout PCD to 0 > 2. Link to a NULL timer lib. > > But for the purpose of avoiding AP calls PeiService, I agree with Eric's change. > I don't think the timer lib dependency blocks Eric's change. > Agree? Yes, I do. Replacing the AcquireSpinLock() call with a looped AcquireSpinLockOrFail() call amounts to acquiring the spinlock, but without depending on the timeout PCD or on the TimerLib class. Thanks, Laszlo >> Regarding the PCD: I think zeroing "PcdSpinLockTimeout" to disable the >> timeout case is a valid approach, it's just that we should change the default >> value in the DEC file to zero. Then the PCD setting will become a burden only >> for those platforms and those use cases that want to use the timeout >> feature (such as for debugging). >> >> In general, PCD default values in DEC files have to be considered carefully, >> but in some cases, such changes are the right thing. Another example was >> 509f8425b75d ("UefiCpuPkg: change PcdCpuSmmStackGuard default to >> TRUE", 2016-06-06). >> >> (You made the same point at the end of >> <https://bugzilla.tianocore.org/show_bug.cgi?id=1419#c2>.) >> >> >> In addition to changing the default value to zero, I'd suggest moving >> "PcdSpinLockTimeout" from section >> >> [PcdsFixedAtBuild,PcdsPatchableInModule] >> >> to section >> >> [PcdsFixedAtBuild, PcdsPatchableInModule, PcdsDynamic, PcdsDynamicEx] >> >> so that platforms can enable the "debug" feature (i.e. set a nonzero >> value) more flexibly. >> >> Thanks >> Laszlo >> >>>> -----Original Message----- >>>> From: Brian J. Johnson [mailto:brian.johnson@hpe.com] >>>> Sent: Friday, December 21, 2018 12:36 AM >>>> To: Yao, Jiewen <jiewen.yao@intel.com>; Ni, Ruiyu >>>> <ruiyu.ni@intel.com>; Dong, Eric <eric.dong@intel.com>; >>>> edk2-devel@lists.01.org >>>> Cc: Laszlo Ersek <lersek@redhat.com> >>>> Subject: Re: [edk2] [Patch 1/3] UefiCpuPkg/RegisterCpuFeaturesLib: >>>> Avoid AP calls PeiService. >>>> >>>> Agreed. We've seen issues on real platforms with timed-out spinlocks >>>> in DXE causing calls to GetPerformanceCounter and DebugAssert. (DXE >>>> has the same code, with the same issues.) >>>> >>>> Note that it's possible to set PcdSpinLockTimeout=0 to work around >>>> the issue on a particular platform, or in a particular module. But >>>> if you have to do that for every module which uses APs, and hence >>>> could contend for a spinlock, it kind of defeats the point.... We're better >> off removing the timeout code. >>>> >>>> Thanks, >>>> Brian >>>> >>>> On 12/19/18 8:08 PM, Yao, Jiewen wrote: >>>>> Yes, I agree, if we don't have any real case. >>>>> >>>>> >>>>>> -----Original Message----- >>>>>> From: Ni, Ruiyu >>>>>> Sent: Thursday, December 20, 2018 10:07 AM >>>>>> To: Dong, Eric <eric.dong@intel.com>; Yao, Jiewen >>>>>> <jiewen.yao@intel.com>; edk2-devel@lists.01.org >>>>>> Cc: Laszlo Ersek <lersek@redhat.com> >>>>>> Subject: RE: [edk2] [Patch 1/3] UefiCpuPkg/RegisterCpuFeaturesLib: >>>>>> Avoid AP calls PeiService. >>>>>> >>>>>> Can you just change the AcquireSpinLock() behavior to remove the >>>>>> Timeout PCD consumption? >>>>>> >>>>>> I haven't seen a real case that the timed acquisition of spin lock is >> needed. >>>>>> >>>>>> >>>>>> Thanks/Ray >>>>>> >>>>>>> -----Original Message----- >>>>>>> From: Dong, Eric <eric.dong@intel.com> >>>>>>> Sent: Thursday, December 20, 2018 9:23 AM >>>>>>> To: Yao, Jiewen <jiewen.yao@intel.com>; edk2-devel@lists.01.org >>>>>>> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Laszlo Ersek >>>>>>> <lersek@redhat.com> >>>>>>> Subject: RE: [edk2] [Patch 1/3] UefiCpuPkg/RegisterCpuFeaturesLib: >>>>>>> Avoid AP calls PeiService. >>>>>>> >>>>>>> >>>>>>> Agreed, Maybe it's time to add a new API like >>>>>>> AcquireSpinLockWithoutTimeOut? >>>>>>> >>>>>>> Thanks, >>>>>>> Eric >>>>>>>> -----Original Message----- >>>>>>>> From: Yao, Jiewen >>>>>>>> Sent: Thursday, December 20, 2018 9:19 AM >>>>>>>> To: Dong, Eric <eric.dong@intel.com>; edk2-devel@lists.01.org >>>>>>>> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Laszlo Ersek >>>>>>>> <lersek@redhat.com> >>>>>>>> Subject: RE: [edk2] [Patch 1/3] UefiCpuPkg/RegisterCpuFeaturesLib: >>>>>>>> Avoid AP calls PeiService. >>>>>>>> >>>>>>>> Hi >>>>>>>> If we think below code is generic, can we have an API for that? >>>>>>>> >>>>>>>> + // >>>>>>>> + // Wait for the AP to release the MSR spin lock. >>>>>>>> + // >>>>>>>> + while (!AcquireSpinLockOrFail (&CpuFlags->ConsoleLogLock)) { >>>>>>>> + CpuPause (); >>>>>>>> + } >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>>> -----Original Message----- >>>>>>>>> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On >>>>>>>>> Behalf Of Eric Dong >>>>>>>>> Sent: Thursday, December 20, 2018 9:16 AM >>>>>>>>> To: edk2-devel@lists.01.org >>>>>>>>> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Laszlo Ersek >>>>>>>>> <lersek@redhat.com> >>>>>>>>> Subject: [edk2] [Patch 1/3] UefiCpuPkg/RegisterCpuFeaturesLib: >>>>>>>>> Avoid AP calls PeiService. >>>>>>>>> >>>>>>>>> In AcquireSpinLock function, it calls GetPerformanceCounter >>>>>>>>> which final calls PeiService service. This patch avoid to call >>>>>>>>> AcquireSpinLock function. >>>>>>>>> >>>>>>>>> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=1411 >>>>>>>>> >>>>>>>>> Cc: Ruiyu Ni <ruiyu.ni@intel.com> >>>>>>>>> Cc: Laszlo Ersek <lersek@redhat.com> >>>>>>>>> Contributed-under: TianoCore Contribution Agreement 1.1 >>>>>>>>> Signed-off-by: Eric Dong <eric.dong@intel.com> >>>>>>>>> --- >>>>>>>>> >>>>>>>>> UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize. >>>>>>>>> c >>>>>>>>> | >>>>>>>>> 7 >>>>>>>>> ++++++- >>>>>>>>> 1 file changed, 6 insertions(+), 1 deletion(-) >>>>>>>>> >>>>>>>>> diff --git >>>>>>>>> >> a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize. >>>>>>>>> c >>>>>>>>> >> b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize. >>>>>>>>> c index 624ddee055..a64326239f 100644 >>>>>>>>> --- >>>>>>>>> >> a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize. >>>>>>>>> c >>>>>>>>> +++ >>>>>> b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize. >>>>>>>>> +++ c >>>>>>>>> @@ -832,7 +832,12 @@ ProgramProcessorRegister ( >>>>>>>>> RegisterTableEntry = &RegisterTableEntryHead[Index]; >>>>>>>>> >>>>>>>>> DEBUG_CODE_BEGIN (); >>>>>>>>> - AcquireSpinLock (&CpuFlags->ConsoleLogLock); >>>>>>>>> + // >>>>>>>>> + // Wait for the AP to release the MSR spin lock. >>>>>>>>> + // >>>>>>>>> + while (!AcquireSpinLockOrFail (&CpuFlags->ConsoleLogLock)) { >>>>>>>>> + CpuPause (); >>>>>>>>> + } >>>>>>>>> ThreadIndex = ApLocation->Package * >>>>>> CpuStatus->MaxCoreCount * >>>>>>>>> CpuStatus->MaxThreadCount + >>>>>>>>> ApLocation->Core * CpuStatus->MaxThreadCount + >>>>>>>>> ApLocation->Thread; >>>>>>>>> -- >>>>>>>>> 2.15.0.windows.1 >>>>>>>>> >>>>>>>>> _______________________________________________ >>>>>>>>> edk2-devel mailing list >>>>>>>>> edk2-devel@lists.01.org >>>>>>>>> https://lists.01.org/mailman/listinfo/edk2-devel >>>>> _______________________________________________ >>>>> edk2-devel mailing list >>>>> edk2-devel@lists.01.org >>>>> https://lists.01.org/mailman/listinfo/edk2-devel >>>>> >>>> >>>> >>>> -- >>>> Brian J. Johnson >>>> Enterprise X86 Lab >>>> >>>> Hewlett Packard Enterprise >>>> >>>> brian.johnson@hpe.com >>> > ^ permalink raw reply [flat|nested] 15+ messages in thread
* [Patch 2/3] UefiCpuPkg/RegisterCpuFeaturesLib: Avoid AP calls PeiService. 2018-12-20 1:15 [Patch 0/3] Avoid AP calls PeiService Eric Dong 2018-12-20 1:15 ` [Patch 1/3] UefiCpuPkg/RegisterCpuFeaturesLib: " Eric Dong @ 2018-12-20 1:15 ` Eric Dong 2018-12-20 1:15 ` [Patch 3/3] UefiCpuPkg/RegisterCpuFeaturesLib: Remove useless function Eric Dong 2 siblings, 0 replies; 15+ messages in thread From: Eric Dong @ 2018-12-20 1:15 UTC (permalink / raw) To: edk2-devel; +Cc: Ruiyu Ni, Laszlo Ersek GetProcessorIndex function calls GetMpPpi to get the MP Ppi. Ap will calls GetProcessorIndex function which final let AP calls PeiService. This patch avoid GetProcessorIndex call PeiService. BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=1411 Cc: Ruiyu Ni <ruiyu.ni@intel.com> Cc: Laszlo Ersek <lersek@redhat.com> Contributed-under: TianoCore Contribution Agreement 1.1 Signed-off-by: Eric Dong <eric.dong@intel.com> --- .../RegisterCpuFeaturesLib/CpuFeaturesInitialize.c | 2 +- .../DxeRegisterCpuFeaturesLib.c | 6 ++++-- .../PeiRegisterCpuFeaturesLib.c | 21 ++++++++++++++++----- .../RegisterCpuFeaturesLib/RegisterCpuFeatures.h | 6 +++++- 4 files changed, 26 insertions(+), 9 deletions(-) diff --git a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize.c b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize.c index a64326239f..81f4652a03 100644 --- a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize.c +++ b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize.c @@ -409,7 +409,7 @@ CollectProcessorData ( CPU_FEATURES_DATA *CpuFeaturesData; CpuFeaturesData = (CPU_FEATURES_DATA *)Buffer; - ProcessorNumber = GetProcessorIndex (); + ProcessorNumber = GetProcessorIndex (CpuFeaturesData); CpuInfo = &CpuFeaturesData->InitOrder[ProcessorNumber].CpuInfo; // // collect processor information diff --git a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/DxeRegisterCpuFeaturesLib.c b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/DxeRegisterCpuFeaturesLib.c index 926698dc95..6f3e5bd2a8 100644 --- a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/DxeRegisterCpuFeaturesLib.c +++ b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/DxeRegisterCpuFeaturesLib.c @@ -66,11 +66,13 @@ GetMpProtocol ( /** Worker function to return processor index. + @param CpuFeaturesData Cpu Feature Data structure. + @return The processor index. **/ UINTN GetProcessorIndex ( - VOID + IN CPU_FEATURES_DATA *CpuFeaturesData ) { EFI_STATUS Status; @@ -225,7 +227,7 @@ CpuFeaturesInitialize ( CpuFeaturesData = GetCpuFeaturesData (); - OldBspNumber = GetProcessorIndex(); + OldBspNumber = GetProcessorIndex(CpuFeaturesData); CpuFeaturesData->BspNumber = OldBspNumber; Status = gBS->CreateEvent ( diff --git a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/PeiRegisterCpuFeaturesLib.c b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/PeiRegisterCpuFeaturesLib.c index 0bb3dee8b6..0bbcb50181 100644 --- a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/PeiRegisterCpuFeaturesLib.c +++ b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/PeiRegisterCpuFeaturesLib.c @@ -96,20 +96,26 @@ GetMpPpi ( /** Worker function to return processor index. + @param CpuFeaturesData Cpu Feature Data structure. + @return The processor index. **/ UINTN GetProcessorIndex ( - VOID + IN CPU_FEATURES_DATA *CpuFeaturesData ) { EFI_STATUS Status; - EFI_PEI_MP_SERVICES_PPI *CpuMpPpi; UINTN ProcessorIndex; + EFI_PEI_MP_SERVICES_PPI *CpuMpPpi; - CpuMpPpi = GetMpPpi (); + ASSERT (CpuFeaturesData->CpuMpPpi != NULL); + if (CpuFeaturesData->CpuMpPpi == NULL) { + return (UINTN) (-1); + } + CpuMpPpi = (EFI_PEI_MP_SERVICES_PPI *)CpuFeaturesData->CpuMpPpi; - Status = CpuMpPpi->WhoAmI(GetPeiServicesTablePointer (), CpuMpPpi, &ProcessorIndex); + Status = CpuMpPpi->WhoAmI(NULL, CpuMpPpi, &ProcessorIndex); ASSERT_EFI_ERROR (Status); return ProcessorIndex; } @@ -286,6 +292,9 @@ GetNumberOfProcessor ( { EFI_STATUS Status; EFI_PEI_MP_SERVICES_PPI *CpuMpPpi; + CPU_FEATURES_DATA *CpuFeaturesData; + + CpuFeaturesData = GetCpuFeaturesData(); // // Get MP Services Protocol @@ -298,6 +307,8 @@ GetNumberOfProcessor ( ); ASSERT_EFI_ERROR (Status); + CpuFeaturesData->CpuMpPpi = CpuMpPpi; + // // Get the number of CPUs // @@ -329,7 +340,7 @@ CpuFeaturesInitialize ( CpuFeaturesData = GetCpuFeaturesData (); - OldBspNumber = GetProcessorIndex(); + OldBspNumber = GetProcessorIndex (CpuFeaturesData); CpuFeaturesData->BspNumber = OldBspNumber; // diff --git a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeatures.h b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeatures.h index cf3da84837..19c3420511 100644 --- a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeatures.h +++ b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeatures.h @@ -85,6 +85,8 @@ typedef struct { UINTN BspNumber; PROGRAM_CPU_REGISTER_FLAGS CpuFlags; + + VOID *CpuMpPpi; } CPU_FEATURES_DATA; #define CPU_FEATURE_ENTRY_FROM_LINK(a) \ @@ -108,11 +110,13 @@ GetCpuFeaturesData ( /** Worker function to return processor index. + @param CpuFeaturesData Cpu Feature Data structure. + @return The processor index. **/ UINTN GetProcessorIndex ( - VOID + IN CPU_FEATURES_DATA *CpuFeaturesData ); /** -- 2.15.0.windows.1 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* [Patch 3/3] UefiCpuPkg/RegisterCpuFeaturesLib: Remove useless function. 2018-12-20 1:15 [Patch 0/3] Avoid AP calls PeiService Eric Dong 2018-12-20 1:15 ` [Patch 1/3] UefiCpuPkg/RegisterCpuFeaturesLib: " Eric Dong 2018-12-20 1:15 ` [Patch 2/3] " Eric Dong @ 2018-12-20 1:15 ` Eric Dong 2018-12-20 2:03 ` Ni, Ruiyu 2 siblings, 1 reply; 15+ messages in thread From: Eric Dong @ 2018-12-20 1:15 UTC (permalink / raw) To: edk2-devel; +Cc: Ruiyu Ni, Laszlo Ersek Directly call the API instead of create function for it. BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=1411 Cc: Ruiyu Ni <ruiyu.ni@intel.com> Cc: Laszlo Ersek <lersek@redhat.com> Contributed-under: TianoCore Contribution Agreement 1.1 Signed-off-by: Eric Dong <eric.dong@intel.com> --- .../PeiRegisterCpuFeaturesLib.c | 35 +++++----------------- 1 file changed, 8 insertions(+), 27 deletions(-) diff --git a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/PeiRegisterCpuFeaturesLib.c b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/PeiRegisterCpuFeaturesLib.c index 0bbcb50181..fdd0791c89 100644 --- a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/PeiRegisterCpuFeaturesLib.c +++ b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/PeiRegisterCpuFeaturesLib.c @@ -67,32 +67,6 @@ GetCpuFeaturesData ( return CpuInitData; } -/** - Worker function to get MP PPI service pointer. - - @return PEI PPI service pointer. -**/ -EFI_PEI_MP_SERVICES_PPI * -GetMpPpi ( - VOID - ) -{ - EFI_STATUS Status; - EFI_PEI_MP_SERVICES_PPI *CpuMpPpi; - - // - // Get MP Services Protocol - // - Status = PeiServicesLocatePpi ( - &gEfiPeiMpServicesPpiGuid, - 0, - NULL, - (VOID **)&CpuMpPpi - ); - ASSERT_EFI_ERROR (Status); - return CpuMpPpi; -} - /** Worker function to return processor index. @@ -139,7 +113,14 @@ GetProcessorInformation ( EFI_PEI_MP_SERVICES_PPI *CpuMpPpi; EFI_STATUS Status; - CpuMpPpi = GetMpPpi (); + Status = PeiServicesLocatePpi ( + &gEfiPeiMpServicesPpiGuid, + 0, + NULL, + (VOID **)&CpuMpPpi + ); + ASSERT_EFI_ERROR (Status); + Status = CpuMpPpi->GetProcessorInfo ( GetPeiServicesTablePointer(), CpuMpPpi, -- 2.15.0.windows.1 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [Patch 3/3] UefiCpuPkg/RegisterCpuFeaturesLib: Remove useless function. 2018-12-20 1:15 ` [Patch 3/3] UefiCpuPkg/RegisterCpuFeaturesLib: Remove useless function Eric Dong @ 2018-12-20 2:03 ` Ni, Ruiyu 0 siblings, 0 replies; 15+ messages in thread From: Ni, Ruiyu @ 2018-12-20 2:03 UTC (permalink / raw) To: Dong, Eric, edk2-devel@lists.01.org; +Cc: Laszlo Ersek Nice cleanup. Reviewed-by: Ruiyu Ni <Ruiyu.ni@intel.com> Thanks/Ray > -----Original Message----- > From: Dong, Eric <eric.dong@intel.com> > Sent: Thursday, December 20, 2018 9:16 AM > To: edk2-devel@lists.01.org > Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Laszlo Ersek <lersek@redhat.com> > Subject: [Patch 3/3] UefiCpuPkg/RegisterCpuFeaturesLib: Remove useless > function. > > Directly call the API instead of create function for it. > > BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=1411 > > Cc: Ruiyu Ni <ruiyu.ni@intel.com> > Cc: Laszlo Ersek <lersek@redhat.com> > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Eric Dong <eric.dong@intel.com> > --- > .../PeiRegisterCpuFeaturesLib.c | 35 +++++----------------- > 1 file changed, 8 insertions(+), 27 deletions(-) > > diff --git > a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/PeiRegisterCpuFeaturesLib.c > b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/PeiRegisterCpuFeaturesLib.c > index 0bbcb50181..fdd0791c89 100644 > --- > a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/PeiRegisterCpuFeaturesLib.c > +++ > b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/PeiRegisterCpuFeaturesLib.c > @@ -67,32 +67,6 @@ GetCpuFeaturesData ( > return CpuInitData; > } > > -/** > - Worker function to get MP PPI service pointer. > - > - @return PEI PPI service pointer. > -**/ > -EFI_PEI_MP_SERVICES_PPI * > -GetMpPpi ( > - VOID > - ) > -{ > - EFI_STATUS Status; > - EFI_PEI_MP_SERVICES_PPI *CpuMpPpi; > - > - // > - // Get MP Services Protocol > - // > - Status = PeiServicesLocatePpi ( > - &gEfiPeiMpServicesPpiGuid, > - 0, > - NULL, > - (VOID **)&CpuMpPpi > - ); > - ASSERT_EFI_ERROR (Status); > - return CpuMpPpi; > -} > - > /** > Worker function to return processor index. > > @@ -139,7 +113,14 @@ GetProcessorInformation ( > EFI_PEI_MP_SERVICES_PPI *CpuMpPpi; > EFI_STATUS Status; > > - CpuMpPpi = GetMpPpi (); > + Status = PeiServicesLocatePpi ( > + &gEfiPeiMpServicesPpiGuid, > + 0, > + NULL, > + (VOID **)&CpuMpPpi > + ); > + ASSERT_EFI_ERROR (Status); > + > Status = CpuMpPpi->GetProcessorInfo ( > GetPeiServicesTablePointer(), > CpuMpPpi, > -- > 2.15.0.windows.1 ^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2019-01-10 12:51 UTC | newest] Thread overview: 15+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2018-12-20 1:15 [Patch 0/3] Avoid AP calls PeiService Eric Dong 2018-12-20 1:15 ` [Patch 1/3] UefiCpuPkg/RegisterCpuFeaturesLib: " Eric Dong 2018-12-20 1:19 ` Yao, Jiewen 2018-12-20 1:22 ` Dong, Eric 2018-12-20 2:06 ` Ni, Ruiyu 2018-12-20 2:08 ` Yao, Jiewen 2018-12-20 16:36 ` Brian J. Johnson 2018-12-21 2:06 ` Dong, Eric 2019-01-09 5:26 ` Dong, Eric 2019-01-09 10:35 ` Laszlo Ersek 2019-01-10 5:53 ` Dong, Eric [not found] ` <734D49CCEBEEF84792F5B80ED585239D5BFAE3BF@SHSMSX103.ccr.corp.intel.com> 2019-01-10 12:51 ` Laszlo Ersek 2018-12-20 1:15 ` [Patch 2/3] " Eric Dong 2018-12-20 1:15 ` [Patch 3/3] UefiCpuPkg/RegisterCpuFeaturesLib: Remove useless function Eric Dong 2018-12-20 2:03 ` Ni, Ruiyu
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox