From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=2a00:1450:4864:20::342; helo=mail-wm1-x342.google.com; envelope-from=leif.lindholm@linaro.org; receiver=edk2-devel@lists.01.org Received: from mail-wm1-x342.google.com (mail-wm1-x342.google.com [IPv6:2a00:1450:4864:20::342]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id CEFA1211575FE for ; Sat, 10 Nov 2018 02:00:11 -0800 (PST) Received: by mail-wm1-x342.google.com with SMTP id 124-v6so4157633wmw.0 for ; Sat, 10 Nov 2018 02:00:11 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=jD28QD7CGv0Ujf+Y2NLkBoS+jC17n58hNlDsHfAl2Bc=; b=fQfQhckjZ5ZhyA1zv6B9cZm7omfkeiKJMhaUr3n0BZ9q4s6BQYLXMItv3ieGfh6VEl ex407yK6RezXkSan+E93DSdERnrqmbunXgUCH4jDLtE/zbVNJoLUkJdijT/M0RaxGZSW oOHrOAY5y4jV67wEczV2CQf9tqqrPj/d/At8Y= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=jD28QD7CGv0Ujf+Y2NLkBoS+jC17n58hNlDsHfAl2Bc=; b=J0mqTW2pnJTDloepOwKH4f66xqB9iwu4/N/IvS0o7fU2dLRTEsYaHu2N+eicF/Qxps LcKKZVv7i7oC7TgrES82ZlzbmAsUOf/pmA+LvFQrqRBWtB2L45Dy+zTHANq0VP/QekD8 YxUz9ZFYdWBqYYr53uNhuADJQCWL1fj9dUi8t481zrg7RZLGA5X98ONUfHyRovN3uBw7 DKFsN//QanO1VnkQPvB/sM59bTmMpi5NDOaGgUPhwLEoKAyNUG8cAr1DlCBbOpmo8WZO SXnjcOMleK0eOZd9foc1SWBtO9/EtFfAaR7rWCjqt6t3dseXERXHXzxM8HcgGrDdcqOh KF1w== X-Gm-Message-State: AGRZ1gKQ5xG4NO0fMhcDEcqD7ZJW/2GskBbILtVLEiTcz58nNmlf8yYf R2gQX2zvuel2/2GNEIaRxi7Yhg== X-Google-Smtp-Source: AJdET5dpbKbMDRz6ncSVBOXIwFxeOet3ycEXHcziuacp9hIEFuJDvzCQvsui7hyFLE1oZydqWlRC9A== X-Received: by 2002:a1c:f313:: with SMTP id q19-v6mr2012960wmq.87.1541844009338; Sat, 10 Nov 2018 02:00:09 -0800 (PST) Received: from bivouac.eciton.net (bivouac.eciton.net. [2a00:1098:0:86:1000:23:0:2]) by smtp.gmail.com with ESMTPSA id d15-v6sm7215876wre.25.2018.11.10.02.00.07 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Sat, 10 Nov 2018 02:00:07 -0800 (PST) Date: Sat, 10 Nov 2018 10:00:05 +0000 From: Leif Lindholm To: "Dong, Eric" Cc: "Laszlo Ersek (lersek@redhat.com)" , "Kinney, Michael D" , "afish@apple.com" , "Ni, Ruiyu" , "edk2-devel@lists.01.org" Message-ID: <20181110100005.u2apchcfynttcyhj@bivouac.eciton.net> References: <20181109052041.14816-1-eric.dong@intel.com> <734D49CCEBEEF84792F5B80ED585239D5BEFCE90@SHSMSX104.ccr.corp.intel.com> MIME-Version: 1.0 In-Reply-To: User-Agent: NeoMutt/20170113 (1.7.2) Subject: Re: [Patch] UefiCpuPkg/RegisterCpuFeaturesLib: Adjust Order. 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: Sat, 10 Nov 2018 10:00:12 -0000 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Hi Eric, I have no concerns over pushing this bugfix. However, having lookes at it, I do have a couple of style comments. On Sat, Nov 10, 2018 at 03:16:26AM +0000, Dong, Eric wrote: > Hi Stewards: > > Since this is a bug fix, and the risk for this release is small. I > plan to push this change before edk2-stable201811 tag. > > If you have any concern, please raise here. > > Thanks, > Eric > > > -----Original Message----- > > From: Ni, Ruiyu > > Sent: Friday, November 9, 2018 10:55 PM > > To: Dong, Eric ; edk2-devel@lists.01.org > > Cc: Laszlo Ersek > > Subject: RE: [edk2] [Patch] UefiCpuPkg/RegisterCpuFeaturesLib: Adjust Order. > > > > Eric, > > Thanks for fixing the new found issues. > > > > Reviewed-by: Ruiyu Ni > > > > > -----Original Message----- > > > From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of > > > Eric Dong > > > Sent: Friday, November 9, 2018 1:21 PM > > > To: edk2-devel@lists.01.org > > > Cc: Ni, Ruiyu ; Laszlo Ersek > > > Subject: [edk2] [Patch] UefiCpuPkg/RegisterCpuFeaturesLib: Adjust Order. > > > > > > V2 changes: > > > V1 change has regression which caused by change feature order. > > > V2 changes logic to detect dependence not only for the neighborhood > > > features. It need to check all features in the list. > > > > > > V1 Changes: The patch version information does not belong in the patch description. Please include it under "---" and make the description refer only to what the patch does. > > > In current code logic, only adjust feature position if current CPU > > > feature position not follow the request order. Just like Feature A > > > need to be executed before feature B, but current feature A registers > > > after feature B. So code will adjust the position for feature A, move > > > it to just before feature B. If the position already met the > > > requirement, code will not adjust the position. > > > > > > This logic has issue when met all below cases: > > > 1. feature A has core or package level dependence with feature B. > > > 2. feature A is register before feature B. > > > 3. Also exist other features exist between feature A and B. > > > > > > Root cause is driver ignores the dependence for this case, so threads > > > may execute not follow the dependence order. > > > > > > Fix this issue by change code logic to adjust feature position for CPU > > > features which has dependence relationship. > > > > > > Cc: Laszlo Ersek > > > Cc: Ruiyu Ni > > > Contributed-under: TianoCore Contribution Agreement 1.1 > > > Signed-off-by: Eric Dong > > > --- > > > .../RegisterCpuFeaturesLib/CpuFeaturesInitialize.c | 71 ++++++++---- > > > .../RegisterCpuFeaturesLib/RegisterCpuFeatures.h | 16 +++ > > > .../RegisterCpuFeaturesLib.c | 122 +++++++++++++++++++++ > > > 3 files changed, 189 insertions(+), 20 deletions(-) > > > > > > diff --git > > > a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize.c > > > b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize.c > > > index 4bed0ce3a4..69e2c04daf 100644 > > > --- > > > a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize.c > > > +++ b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize. > > > +++ c > > > @@ -534,6 +534,28 @@ DumpRegisterTableOnProcessor ( > > > } > > > } > > > > > > +/** > > > + Get the biggest dependence type. > > > + PackageDepType > CoreDepType > ThreadDepType > NoneDepType. > > > + > > > + @param[in] BeforeDep Before dependence type. > > > + @param[in] AfterDep After dependence type. > > > + @param[in] NoneNeibBeforeDep Before dependence type for not > > > neighborhood features. > > > + @param[in] NoneNeibAfterDep After dependence type for not > > > neighborhood features. > > > + > > > + @retval Return the biggest dependence type. > > > +**/ > > > +CPU_FEATURE_DEPENDENCE_TYPE > > > +BiggestDep ( > > > + IN CPU_FEATURE_DEPENDENCE_TYPE BeforeDep, > > > + IN CPU_FEATURE_DEPENDENCE_TYPE AfterDep, > > > + IN CPU_FEATURE_DEPENDENCE_TYPE NoneNeibBeforeDep, > > > + IN CPU_FEATURE_DEPENDENCE_TYPE NoneNeibAfterDep > > > + ) > > > +{ > > > + return MAX(MAX (MAX(BeforeDep, AfterDep), NoneNeibBeforeDep), > > > NoneNeibAfterDep); And please make use of some temporary variables here. It will make no difference to code generation in any compiler designed since 1995, but will make it human readable. / Leif > > > +} > > > + > > > /** > > > Analysis register CPU features on each processor and save CPU > > > setting in CPU register table. > > > > > > @@ -558,6 +580,8 @@ AnalysisProcessorFeatures ( > > > BOOLEAN Success; > > > CPU_FEATURE_DEPENDENCE_TYPE BeforeDep; > > > CPU_FEATURE_DEPENDENCE_TYPE AfterDep; > > > + CPU_FEATURE_DEPENDENCE_TYPE NoneNeibBeforeDep; > > > + CPU_FEATURE_DEPENDENCE_TYPE NoneNeibAfterDep; > > > > > > CpuFeaturesData = GetCpuFeaturesData (); > > > CpuFeaturesData->CapabilityPcd = AllocatePool (CpuFeaturesData- > > > >BitMaskSize); > > > @@ -634,14 +658,9 @@ AnalysisProcessorFeatures ( > > > // > > > CpuInfo = &CpuFeaturesData->InitOrder[ProcessorNumber].CpuInfo; > > > Entry = GetFirstNode (&CpuInitOrder->OrderList); > > > - NextEntry = Entry->ForwardLink; > > > while (!IsNull (&CpuInitOrder->OrderList, Entry)) { > > > CpuFeatureInOrder = CPU_FEATURE_ENTRY_FROM_LINK (Entry); > > > - if (!IsNull (&CpuInitOrder->OrderList, NextEntry)) { > > > - NextCpuFeatureInOrder = CPU_FEATURE_ENTRY_FROM_LINK > > (NextEntry); > > > - } else { > > > - NextCpuFeatureInOrder = NULL; > > > - } > > > + > > > Success = FALSE; > > > if (IsBitMaskMatch (CpuFeatureInOrder->FeatureMask, > > > CpuFeaturesData- > > > >SettingPcd)) { > > > Status = CpuFeatureInOrder->InitializeFunc (ProcessorNumber, > > > CpuInfo, > > > CpuFeatureInOrder->ConfigData, TRUE); > > > @@ -674,31 +693,43 @@ AnalysisProcessorFeatures ( > > > } > > > > > > if (Success) { > > > - // > > > - // If feature has dependence with the next feature (ONLY care > > > core/package dependency). > > > - // and feature initialize succeed, add sync semaphere here. > > > - // > > > - if (NextCpuFeatureInOrder != NULL) { > > > + NextEntry = Entry->ForwardLink; > > > + if (!IsNull (&CpuInitOrder->OrderList, NextEntry)) { > > > + NextCpuFeatureInOrder = CPU_FEATURE_ENTRY_FROM_LINK > > > (NextEntry); > > > + > > > + // > > > + // If feature has dependence with the next feature (ONLY > > > + care > > > core/package dependency). > > > + // and feature initialize succeed, add sync semaphere here. > > > + // > > > BeforeDep = DetectFeatureScope (CpuFeatureInOrder, TRUE, > > > NextCpuFeatureInOrder->FeatureMask); > > > AfterDep = DetectFeatureScope (NextCpuFeatureInOrder, > > > FALSE, > > > CpuFeatureInOrder->FeatureMask); > > > + // > > > + // Check whether next feature has After type dependence > > > + with not > > > neighborhood CPU > > > + // Features in former CPU features. > > > + // > > > + NoneNeibAfterDep = > > > DetectNoneNeighborhoodFeatureScope(NextCpuFeatureInOrder, FALSE, > > > &CpuInitOrder->OrderList); > > > } else { > > > - BeforeDep = DetectFeatureScope (CpuFeatureInOrder, TRUE, NULL); > > > - AfterDep = NoneDepType; > > > + BeforeDep = NoneDepType; > > > + AfterDep = NoneDepType; > > > + NoneNeibAfterDep = NoneDepType; > > > } > > > // > > > - // Assume only one of the depend is valid. > > > + // Check whether current feature has Before type dependence > > > + with none > > > neighborhood > > > + // CPU features in after Cpu features. > > > // > > > - ASSERT (!(BeforeDep > ThreadDepType && AfterDep > ThreadDepType)); > > > + NoneNeibBeforeDep = > > > DetectNoneNeighborhoodFeatureScope(CpuFeatureInOrder, TRUE, > > > &CpuInitOrder->OrderList); > > > + > > > + // > > > + // Get the biggest dependence and add semaphore for it. > > > + // PackageDepType > CoreDepType > ThreadDepType > NoneDepType. > > > + // > > > + BeforeDep = BiggestDep(BeforeDep, AfterDep, > > > + NoneNeibBeforeDep, > > > NoneNeibAfterDep); > > > if (BeforeDep > ThreadDepType) { > > > CPU_REGISTER_TABLE_WRITE32 (ProcessorNumber, Semaphore, 0, > > > BeforeDep); > > > } > > > - if (AfterDep > ThreadDepType) { > > > - CPU_REGISTER_TABLE_WRITE32 (ProcessorNumber, Semaphore, 0, > > > AfterDep); > > > - } > > > } > > > > > > - Entry = Entry->ForwardLink; > > > - NextEntry = Entry->ForwardLink; > > > + Entry = Entry->ForwardLink; > > > } > > > > > > // > > > diff --git > > > a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeatures.h > > > b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeatures.h > > > index 4898a80827..cf3da84837 100644 > > > --- a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeatures.h > > > +++ b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeatures.h > > > @@ -207,6 +207,22 @@ DetectFeatureScope ( > > > IN UINT8 *NextCpuFeatureMask > > > ); > > > > > > +/** > > > + Return feature dependence result. > > > + > > > + @param[in] CpuFeature Pointer to CPU feature. > > > + @param[in] Before Check before dependence or after. > > > + @param[in] FeatureList Pointer to CPU feature list. > > > + > > > + @retval return the dependence result. > > > +**/ > > > +CPU_FEATURE_DEPENDENCE_TYPE > > > +DetectNoneNeighborhoodFeatureScope ( > > > + IN CPU_FEATURES_ENTRY *CpuFeature, > > > + IN BOOLEAN Before, > > > + IN LIST_ENTRY *FeatureList > > > + ); > > > + > > > /** > > > Programs registers for the calling processor. > > > > > > diff --git > > > a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c > > > b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c > > > index 394695baf2..ed8d526325 100644 > > > --- > > > a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c > > > +++ b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib > > > +++ .c > > > @@ -112,6 +112,75 @@ IsBitMaskMatchCheck ( > > > return FALSE; > > > } > > > > > > +/** > > > + Try to find the specify cpu featuren in former/after feature list. > > > + > > > + @param[in] FeatureList Pointer to dependent CPU feature list > > > + @param[in] CurrentEntry Pointer to current CPU feature entry. > > > + @param[in] SearchFormer Find in former feature or after features. > > > + @param[in] FeatureMask Pointer to CPU feature bit mask > > > + > > > + @retval TRUE The feature bit mask is in dependent CPU feature bit > > > + mask > > > buffer. > > > + @retval FALSE The feature bit mask is not in dependent CPU feature > > > + bit mask > > > buffer. > > > +**/ > > > +BOOLEAN > > > +FindSpecifyFeature ( > > > + IN LIST_ENTRY *FeatureList, > > > + IN LIST_ENTRY *CurrentEntry, > > > + IN BOOLEAN SearchFormer, > > > + IN UINT8 *FeatureMask > > > + ) > > > +{ > > > + CPU_FEATURES_ENTRY *CpuFeature; > > > + LIST_ENTRY *NextEntry; > > > + > > > + // > > > + // Check whether exist the not neighborhood entry first. > > > + // If not exist, return FALSE means not found status. > > > + // > > > + if (SearchFormer) { > > > + NextEntry = CurrentEntry->BackLink; > > > + if (IsNull (FeatureList, NextEntry)) { > > > + return FALSE; > > > + } > > > + > > > + NextEntry = NextEntry->BackLink; > > > + if (IsNull (FeatureList, NextEntry)) { > > > + return FALSE; > > > + } > > > + > > > + NextEntry = CurrentEntry->BackLink->BackLink; } else { > > > + NextEntry = CurrentEntry->ForwardLink; > > > + if (IsNull (FeatureList, NextEntry)) { > > > + return FALSE; > > > + } > > > + > > > + NextEntry = NextEntry->ForwardLink; > > > + if (IsNull (FeatureList, NextEntry)) { > > > + return FALSE; > > > + } > > > + > > > + NextEntry = CurrentEntry->ForwardLink->ForwardLink; > > > + } > > > + > > > + while (!IsNull (FeatureList, NextEntry)) { > > > + CpuFeature = CPU_FEATURE_ENTRY_FROM_LINK (NextEntry); > > > + > > > + if (IsBitMaskMatchCheck (FeatureMask, CpuFeature->FeatureMask)) { > > > + return TRUE; > > > + } > > > + > > > + if (SearchFormer) { > > > + NextEntry = NextEntry->BackLink; > > > + } else { > > > + NextEntry = NextEntry->ForwardLink; > > > + } > > > + } > > > + > > > + return FALSE; > > > +} > > > + > > > /** > > > Return feature dependence result. > > > > > > @@ -178,6 +247,59 @@ DetectFeatureScope ( > > > return NoneDepType; > > > } > > > > > > +/** > > > + Return feature dependence result. > > > + > > > + @param[in] CpuFeature Pointer to CPU feature. > > > + @param[in] Before Check before dependence or after. > > > + @param[in] FeatureList Pointer to CPU feature list. > > > + > > > + @retval return the dependence result. > > > +**/ > > > +CPU_FEATURE_DEPENDENCE_TYPE > > > +DetectNoneNeighborhoodFeatureScope ( > > > + IN CPU_FEATURES_ENTRY *CpuFeature, > > > + IN BOOLEAN Before, > > > + IN LIST_ENTRY *FeatureList > > > + ) > > > +{ > > > + if (Before) { > > > + if ((CpuFeature->PackageBeforeFeatureBitMask != NULL) && > > > + FindSpecifyFeature(FeatureList, &CpuFeature->Link, FALSE, > > > +CpuFeature- > > > >PackageBeforeFeatureBitMask)) { > > > + return PackageDepType; > > > + } > > > + > > > + if ((CpuFeature->CoreBeforeFeatureBitMask != NULL) && > > > + FindSpecifyFeature(FeatureList, &CpuFeature->Link, FALSE, > > > + CpuFeature- > > > >CoreBeforeFeatureBitMask)) { > > > + return CoreDepType; > > > + } > > > + > > > + if ((CpuFeature->BeforeFeatureBitMask != NULL) && > > > + FindSpecifyFeature(FeatureList, &CpuFeature->Link, FALSE, > > > + CpuFeature- > > > >BeforeFeatureBitMask)) { > > > + return ThreadDepType; > > > + } > > > + > > > + return NoneDepType; > > > + } > > > + > > > + if ((CpuFeature->PackageAfterFeatureBitMask != NULL) && > > > + FindSpecifyFeature(FeatureList, &CpuFeature->Link, TRUE, > > > + CpuFeature- > > > >PackageAfterFeatureBitMask)) { > > > + return PackageDepType; > > > + } > > > + > > > + if ((CpuFeature->CoreAfterFeatureBitMask != NULL) && > > > + FindSpecifyFeature(FeatureList, &CpuFeature->Link, TRUE, > > > + CpuFeature- > > > >CoreAfterFeatureBitMask)) { > > > + return CoreDepType; > > > + } > > > + > > > + if ((CpuFeature->AfterFeatureBitMask != NULL) && > > > + FindSpecifyFeature(FeatureList, &CpuFeature->Link, TRUE, > > > + CpuFeature- > > > >AfterFeatureBitMask)) { > > > + return ThreadDepType; > > > + } > > > + > > > + return NoneDepType; > > > +} > > > + > > > /** > > > Base on dependence relationship to asjust feature dependence. > > > > > > -- > > > 2.15.0.windows.1 > > > > > > _______________________________________________ > > > edk2-devel mailing list > > > edk2-devel@lists.01.org > > > https://lists.01.org/mailman/listinfo/edk2-devel