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 AC8AF211B5090 for ; Wed, 9 Jan 2019 02:35:16 -0800 (PST) Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.phx2.redhat.com [10.5.11.13]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 0D35FC07456A; Wed, 9 Jan 2019 10:35:16 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-11.rdu2.redhat.com [10.10.120.11]) by smtp.corp.redhat.com (Postfix) with ESMTP id 59DF4608DA; Wed, 9 Jan 2019 10:35:14 +0000 (UTC) To: "Dong, Eric" Cc: "Brian J. Johnson" , "Yao, Jiewen" , "Ni, Ray" , "edk2-devel@lists.01.org" , "Kinney, Michael D" 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> From: Laszlo Ersek Message-ID: Date: Wed, 9 Jan 2019 11:35:13 +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: X-Scanned-By: MIMEDefang 2.79 on 10.5.11.13 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.32]); Wed, 09 Jan 2019 10:35:16 +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: Wed, 09 Jan 2019 10:35:16 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit 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. (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 .) 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 >