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

[Laszlo Ersek <lersek@redhat.com>]

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