From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=148.163.143.35; helo=mx0b-002e3701.pphosted.com; envelope-from=brian.johnson@hpe.com; receiver=edk2-devel@lists.01.org Received: from mx0b-002e3701.pphosted.com (mx0b-002e3701.pphosted.com [148.163.143.35]) (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 974EA211A2D8E for ; Thu, 20 Dec 2018 08:36:10 -0800 (PST) Received: from pps.filterd (m0134423.ppops.net [127.0.0.1]) by mx0b-002e3701.pphosted.com (8.16.0.27/8.16.0.27) with SMTP id wBKGZfWH022808; Thu, 20 Dec 2018 16:36:08 GMT Received: from g4t3426.houston.hpe.com (g4t3426.houston.hpe.com [15.241.140.75]) by mx0b-002e3701.pphosted.com with ESMTP id 2pgdug8ghs-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=NOT); Thu, 20 Dec 2018 16:36:08 +0000 Received: from g4t3433.houston.hpecorp.net (g4t3433.houston.hpecorp.net [16.208.49.245]) by g4t3426.houston.hpe.com (Postfix) with ESMTP id AC1555A; Thu, 20 Dec 2018 16:36:07 +0000 (UTC) Received: from [10.33.152.19] (bjj-laptop2.americas.hpqcorp.net [10.33.152.19]) by g4t3433.houston.hpecorp.net (Postfix) with ESMTP id 5E57E45; Thu, 20 Dec 2018 16:36:07 +0000 (UTC) To: "Yao, Jiewen" , "Ni, Ruiyu" , "Dong, Eric" , "edk2-devel@lists.01.org" Cc: Laszlo Ersek 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> From: "Brian J. Johnson" Message-ID: <993916e9-86cf-0e3c-19e4-b334de5214e7@hpe.com> Date: Thu, 20 Dec 2018 10:36:17 -0600 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.2.1 MIME-Version: 1.0 In-Reply-To: <74D8A39837DF1E4DA445A8C0B3885C503F4610D1@shsmsx102.ccr.corp.intel.com> X-HPE-SCL: -1 X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:, , definitions=2018-12-20_08:, , signatures=0 X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 priorityscore=1501 malwarescore=0 suspectscore=0 phishscore=0 bulkscore=0 spamscore=0 clxscore=1011 lowpriorityscore=0 mlxscore=0 impostorscore=0 mlxlogscore=999 adultscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.0.1-1810050000 definitions=main-1812200135 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, 20 Dec 2018 16:36:11 -0000 Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit 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