On 01/10/19 05:51, Ni, Ray wrote:
>
>
>> -----Original Message-----
>> From: Laszlo Ersek <[email protected]>
>> Sent: Wednesday, January 9, 2019 6:35 PM
>> To: Dong, Eric <[email protected]>
>> Cc: Brian J. Johnson <[email protected]>; Yao, Jiewen
>> <[email protected]>; Ni, Ray <[email protected]>; edk2-
>> [email protected]; Kinney, Michael D <[email protected]>
>> 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:[email protected]]
>>>> Sent: Friday, December 21, 2018 12:36 AM
>>>> To: Yao, Jiewen <[email protected]>; Ni, Ruiyu
>>>> <[email protected]>; Dong, Eric <[email protected]>;
>>>> [email protected]
>>>> Cc: Laszlo Ersek <[email protected]>
>>>> 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 <[email protected]>; Yao, Jiewen
>>>>>> <[email protected]>; [email protected]
>>>>>> Cc: Laszlo Ersek <[email protected]>
>>>>>> 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 <[email protected]>
>>>>>>> Sent: Thursday, December 20, 2018 9:23 AM
>>>>>>> To: Yao, Jiewen <[email protected]>; [email protected]
>>>>>>> Cc: Ni, Ruiyu <[email protected]>; Laszlo Ersek
>>>>>>> <[email protected]>
>>>>>>> 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 <[email protected]>; [email protected]
>>>>>>>> Cc: Ni, Ruiyu <[email protected]>; Laszlo Ersek
>>>>>>>> <[email protected]>
>>>>>>>> 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:[email protected]] On
>>>>>>>>> Behalf Of Eric Dong
>>>>>>>>> Sent: Thursday, December 20, 2018 9:16 AM
>>>>>>>>> To: [email protected]
>>>>>>>>> Cc: Ni, Ruiyu <[email protected]>; Laszlo Ersek
>>>>>>>>> <[email protected]>
>>>>>>>>> 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 <[email protected]>
>>>>>>>>> Cc: Laszlo Ersek <[email protected]>
>>>>>>>>> Contributed-under: TianoCore Contribution Agreement 1.1
>>>>>>>>> Signed-off-by: Eric Dong <[email protected]>
>>>>>>>>> ---
>>>>>>>>>
>>>>>>>>> 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
>>>>>>>>> [email protected]
>>>>>>>>> https://lists.01.org/mailman/listinfo/edk2-devel
>>>>> _______________________________________________
>>>>> edk2-devel mailing list
>>>>> [email protected]
>>>>> https://lists.01.org/mailman/listinfo/edk2-devel
>>>>>
>>>>
>>>>
>>>> --
>>>> Brian J. Johnson
>>>> Enterprise X86 Lab
>>>>
>>>> Hewlett Packard Enterprise
>>>>
>>>> [email protected]
>>>
>
_______________________________________________
edk2-devel mailing list
[email protected]
https://lists.01.org/mailman/listinfo/edk2-devel
