Re: [Patch 1/3] UefiCpuPkg/RegisterCpuFeaturesLib: Avoid AP calls PeiService.

["Dong, Eric" <eric.dong@intel.com>]

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
> >