From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=209.132.183.28; helo=mx1.redhat.com; envelope-from=lersek@redhat.com; receiver=edk2-devel@lists.01.org Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 06C74211B63F1 for ; Thu, 10 Jan 2019 04:51:19 -0800 (PST) Received: from smtp.corp.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id C1E4BC067834; Thu, 10 Jan 2019 12:51:18 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-121-17.rdu2.redhat.com [10.10.121.17]) by smtp.corp.redhat.com (Postfix) with ESMTP id C11D7600C8; Thu, 10 Jan 2019 12:51:16 +0000 (UTC) To: "Ni, Ray" , "Dong, Eric" , "Kinney, Michael D" , "Brian J. Johnson" Cc: "Yao, Jiewen" , "edk2-devel@lists.01.org" References: <20181220011553.14900-1-eric.dong@intel.com> <20181220011553.14900-2-eric.dong@intel.com> <74D8A39837DF1E4DA445A8C0B3885C503F460E44@shsmsx102.ccr.corp.intel.com> <734D49CCEBEEF84792F5B80ED585239D5BF673E1@SHSMSX104.ccr.corp.intel.com> <74D8A39837DF1E4DA445A8C0B3885C503F4610D1@shsmsx102.ccr.corp.intel.com> <993916e9-86cf-0e3c-19e4-b334de5214e7@hpe.com> <734D49CCEBEEF84792F5B80ED585239D5BFAE3BF@SHSMSX103.ccr.corp.intel.com> From: Laszlo Ersek Message-ID: <6fdb2a4c-09c4-5f77-1431-871766a78b46@redhat.com> Date: Thu, 10 Jan 2019 13:51:15 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <734D49CCEBEEF84792F5B80ED585239D5BFAE3BF@SHSMSX103.ccr.corp.intel.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.11 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.32]); Thu, 10 Jan 2019 12:51:18 +0000 (UTC) Subject: Re: [Patch 1/3] UefiCpuPkg/RegisterCpuFeaturesLib: Avoid AP calls PeiService. X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 10 Jan 2019 12:51:21 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 01/10/19 05:51, Ni, Ray wrote: > > >> -----Original Message----- >> From: Laszlo Ersek >> Sent: Wednesday, January 9, 2019 6:35 PM >> To: Dong, Eric >> Cc: Brian J. Johnson ; Yao, Jiewen >> ; Ni, Ray ; edk2- >> devel@lists.01.org; Kinney, Michael D >> 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 , 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 >> .) >> >> >> 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 ; Ni, Ruiyu >>>> ; Dong, Eric ; >>>> edk2-devel@lists.01.org >>>> Cc: Laszlo Ersek >>>> 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 ; Yao, Jiewen >>>>>> ; edk2-devel@lists.01.org >>>>>> Cc: Laszlo Ersek >>>>>> 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 >>>>>>> Sent: Thursday, December 20, 2018 9:23 AM >>>>>>> To: Yao, Jiewen ; edk2-devel@lists.01.org >>>>>>> Cc: Ni, Ruiyu ; Laszlo Ersek >>>>>>> >>>>>>> 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 ; edk2-devel@lists.01.org >>>>>>>> Cc: Ni, Ruiyu ; Laszlo Ersek >>>>>>>> >>>>>>>> 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 ; Laszlo Ersek >>>>>>>>> >>>>>>>>> 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 >>>>>>>>> Cc: Laszlo Ersek >>>>>>>>> Contributed-under: TianoCore Contribution Agreement 1.1 >>>>>>>>> Signed-off-by: Eric Dong >>>>>>>>> --- >>>>>>>>> >>>>>>>>> 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 >>> >