From: "Ni, Ruiyu" <ruiyu.ni@intel.com>
To: "Dong, Eric" <eric.dong@intel.com>,
"edk2-devel@lists.01.org" <edk2-devel@lists.01.org>
Cc: Laszlo Ersek <lersek@redhat.com>
Subject: Re: [Patch] UefiCpuPkg/RegisterCpuFeaturesLib: Support combo CPU feature style.
Date: Fri, 26 Oct 2018 04:16:08 +0000 [thread overview]
Message-ID: <734D49CCEBEEF84792F5B80ED585239D5BEDEABC@SHSMSX104.ccr.corp.intel.com> (raw)
In-Reply-To: <20181025065022.25884-1-eric.dong@intel.com>
Reviewed-by: Ruiyu Ni <ruiyu.ni@intel.com>
> -----Original Message-----
> From: Dong, Eric
> Sent: Thursday, October 25, 2018 2:50 PM
> To: edk2-devel@lists.01.org
> Cc: Ni, Ruiyu <ruiyu.ni@intel.com>; Laszlo Ersek <lersek@redhat.com>
> Subject: [Patch] UefiCpuPkg/RegisterCpuFeaturesLib: Support combo CPU
> feature style.
>
> Current code assume only one dependence (before or after) for one feature.
> Enhance code logic to support feature has two dependence (before and after)
> type.
>
> Cc: Ruiyu Ni <ruiyu.ni@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Eric Dong <eric.dong@intel.com>
> ---
> .../RegisterCpuFeaturesLib/CpuFeaturesInitialize.c | 5 +-
> .../RegisterCpuFeaturesLib/RegisterCpuFeatures.h | 8 +-
> .../RegisterCpuFeaturesLib.c | 99 ++++++++--------------
> 3 files changed, 45 insertions(+), 67 deletions(-)
>
> diff --git a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize.c
> b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize.c
> index 173f2edbea..bc372a338f 100644
> --- a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize.c
> +++ b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/CpuFeaturesInitialize.c
> @@ -671,10 +671,11 @@ AnalysisProcessorFeatures (
> // 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);
> if (NextCpuFeatureInOrder != NULL) {
> - AfterDep = DetectFeatureScope (NextCpuFeatureInOrder, FALSE);
> + BeforeDep = DetectFeatureScope (CpuFeatureInOrder, TRUE,
> NextCpuFeatureInOrder->FeatureMask);
> + AfterDep = DetectFeatureScope (NextCpuFeatureInOrder, FALSE,
> + CpuFeatureInOrder->FeatureMask);
> } else {
> + BeforeDep = DetectFeatureScope (CpuFeatureInOrder, TRUE,
> + NULL);
> AfterDep = NoneDepType;
> }
> //
> diff --git a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeatures.h
> b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeatures.h
> index 42a3f91fbf..b5fe8fbce1 100644
> --- a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeatures.h
> +++ b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeatures.h
> @@ -193,15 +193,17 @@ DumpCpuFeature (
> /**
> Return feature dependence result.
>
> - @param[in] CpuFeature Pointer to CPU feature.
> - @param[in] Before Check before dependence or after.
> + @param[in] CpuFeature Pointer to CPU feature.
> + @param[in] Before Check before dependence or after.
> + @param[in] NextCpuFeatureMask Pointer to next CPU feature Mask.
>
> @retval return the dependence result.
> **/
> CPU_FEATURE_DEPENDENCE_TYPE
> DetectFeatureScope (
> IN CPU_FEATURES_ENTRY *CpuFeature,
> - IN BOOLEAN Before
> + IN BOOLEAN Before,
> + IN CHAR8 *NextCpuFeatureMask
> );
>
> /**
> diff --git
> a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c
> b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c
> index b6e108b8ad..9a66bc49ff 100644
> --- a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c
> +++ b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c
> @@ -115,90 +115,69 @@ IsBitMaskMatchCheck (
> /**
> Return feature dependence result.
>
> - @param[in] CpuFeature Pointer to CPU feature.
> - @param[in] Before Check before dependence or after.
> + @param[in] CpuFeature Pointer to CPU feature.
> + @param[in] Before Check before dependence or after.
> + @param[in] NextCpuFeatureMask Pointer to next CPU feature Mask.
>
> @retval return the dependence result.
> **/
> CPU_FEATURE_DEPENDENCE_TYPE
> DetectFeatureScope (
> IN CPU_FEATURES_ENTRY *CpuFeature,
> - IN BOOLEAN Before
> + IN BOOLEAN Before,
> + IN CHAR8 *NextCpuFeatureMask
> )
> {
> + //
> + // if need to check before type dependence but the feature after
> + current feature is not // exist, means this before type dependence not valid,
> just return NoneDepType.
> + // Just like Feature A has a dependence of feature B, but Feature B
> + not installed, so // Feature A maybe insert to the last entry of the
> + list. In this case, for below code, // Featrure A has depend of
> + feature B, but it is the last entry of the list, so the //
> + NextCpuFeatureMask is NULL, so the dependence for feature A here is useless
> and code // just return NoneDepType.
> + //
> + if (NextCpuFeatureMask == NULL) {
> + return NoneDepType;
> + }
> +
> if (Before) {
> - if (CpuFeature->PackageBeforeFeatureBitMask != NULL) {
> + if ((CpuFeature->PackageBeforeFeatureBitMask != NULL) &&
> + IsBitMaskMatchCheck (NextCpuFeatureMask,
> + CpuFeature->PackageBeforeFeatureBitMask)) {
> return PackageDepType;
> }
>
> - if (CpuFeature->CoreBeforeFeatureBitMask != NULL) {
> + if ((CpuFeature->CoreBeforeFeatureBitMask != NULL) &&
> + IsBitMaskMatchCheck (NextCpuFeatureMask,
> + CpuFeature->CoreBeforeFeatureBitMask)) {
> return CoreDepType;
> }
>
> - if (CpuFeature->BeforeFeatureBitMask != NULL) {
> + if ((CpuFeature->BeforeFeatureBitMask != NULL) &&
> + IsBitMaskMatchCheck (NextCpuFeatureMask,
> + CpuFeature->BeforeFeatureBitMask)) {
> return ThreadDepType;
> }
>
> return NoneDepType;
> }
>
> - if (CpuFeature->PackageAfterFeatureBitMask != NULL) {
> + if ((CpuFeature->PackageAfterFeatureBitMask != NULL) &&
> + IsBitMaskMatchCheck (NextCpuFeatureMask,
> + CpuFeature->PackageAfterFeatureBitMask)) {
> return PackageDepType;
> }
>
> - if (CpuFeature->CoreAfterFeatureBitMask != NULL) {
> + if ((CpuFeature->CoreAfterFeatureBitMask != NULL) &&
> + IsBitMaskMatchCheck (NextCpuFeatureMask,
> + CpuFeature->CoreAfterFeatureBitMask)) {
> return CoreDepType;
> }
>
> - if (CpuFeature->AfterFeatureBitMask != NULL) {
> + if ((CpuFeature->AfterFeatureBitMask != NULL) &&
> + IsBitMaskMatchCheck (NextCpuFeatureMask,
> + CpuFeature->AfterFeatureBitMask)) {
> return ThreadDepType;
> }
>
> return NoneDepType;
> }
>
> -/**
> - Clear dependence for the specified type.
> -
> - @param[in] CpuFeature Cpu feature need to clear.
> - @param[in] Before Before or after dependence relationship.
> -
> -**/
> -VOID
> -ClearFeatureScope (
> - IN CPU_FEATURES_ENTRY *CpuFeature,
> - IN BOOLEAN Before
> - )
> -{
> - if (Before) {
> - if (CpuFeature->BeforeFeatureBitMask != NULL) {
> - FreePool (CpuFeature->BeforeFeatureBitMask);
> - CpuFeature->BeforeFeatureBitMask = NULL;
> - }
> - if (CpuFeature->CoreBeforeFeatureBitMask != NULL) {
> - FreePool (CpuFeature->CoreBeforeFeatureBitMask);
> - CpuFeature->CoreBeforeFeatureBitMask = NULL;
> - }
> - if (CpuFeature->PackageBeforeFeatureBitMask != NULL) {
> - FreePool (CpuFeature->PackageBeforeFeatureBitMask);
> - CpuFeature->PackageBeforeFeatureBitMask = NULL;
> - }
> - } else {
> - if (CpuFeature->PackageAfterFeatureBitMask != NULL) {
> - FreePool (CpuFeature->PackageAfterFeatureBitMask);
> - CpuFeature->PackageAfterFeatureBitMask = NULL;
> - }
> - if (CpuFeature->CoreAfterFeatureBitMask != NULL) {
> - FreePool (CpuFeature->CoreAfterFeatureBitMask);
> - CpuFeature->CoreAfterFeatureBitMask = NULL;
> - }
> - if (CpuFeature->AfterFeatureBitMask != NULL) {
> - FreePool (CpuFeature->AfterFeatureBitMask);
> - CpuFeature->AfterFeatureBitMask = NULL;
> - }
> - }
> -}
> -
> /**
> Base on dependence relationship to asjust feature dependence.
>
> @@ -209,6 +188,7 @@ ClearFeatureScope (
>
> @param[in, out] PreviousFeature CPU feature current before the find one.
> @param[in, out] CurrentFeature Cpu feature need to adjust.
> + @param[in] FindFeature Cpu feature which current feature depends.
> @param[in] Before Before or after dependence relationship.
>
> @retval TRUE means the current feature dependence has been adjusted.
> @@ -221,14 +201,15 @@ BOOLEAN
> AdjustFeaturesDependence (
> IN OUT CPU_FEATURES_ENTRY *PreviousFeature,
> IN OUT CPU_FEATURES_ENTRY *CurrentFeature,
> + IN CPU_FEATURES_ENTRY *FindFeature,
> IN BOOLEAN Before
> )
> {
> CPU_FEATURE_DEPENDENCE_TYPE PreDependType;
> CPU_FEATURE_DEPENDENCE_TYPE CurrentDependType;
>
> - PreDependType = DetectFeatureScope(PreviousFeature, Before);
> - CurrentDependType = DetectFeatureScope(CurrentFeature, Before);
> + PreDependType = DetectFeatureScope(PreviousFeature, Before,
> FindFeature->FeatureMask);
> + CurrentDependType = DetectFeatureScope(CurrentFeature, Before,
> + FindFeature->FeatureMask);
>
> //
> // If previous feature has no dependence with the find featue.
> @@ -243,10 +224,8 @@ AdjustFeaturesDependence (
> // processors and clear the dependence for the other one.
> //
> if (PreDependType >= CurrentDependType) {
> - ClearFeatureScope (CurrentFeature, Before);
> return TRUE;
> } else {
> - ClearFeatureScope (PreviousFeature, Before);
> return FALSE;
> }
> }
> @@ -271,6 +250,7 @@ AdjustEntry (
> LIST_ENTRY *PreviousEntry;
> CPU_FEATURES_ENTRY *PreviousFeature;
> CPU_FEATURES_ENTRY *CurrentFeature;
> + CPU_FEATURES_ENTRY *FindFeature;
>
> //
> // For CPU feature which has core or package type dependence, later code
> need to insert @@ -308,8 +288,9 @@ AdjustEntry (
> // If exist the previous or next entry, need to check it before insert curent
> entry.
> //
> PreviousFeature = CPU_FEATURE_ENTRY_FROM_LINK (PreviousEntry);
> + FindFeature = CPU_FEATURE_ENTRY_FROM_LINK (FindEntry);
>
> - if (AdjustFeaturesDependence (PreviousFeature, CurrentFeature, Before)) {
> + if (AdjustFeaturesDependence (PreviousFeature, CurrentFeature,
> + FindFeature, Before)) {
> //
> // Return TRUE means current feature dependence has been cleared and the
> previous
> // feature dependence has been kept and used. So insert current feature
> before (or after) @@ -486,7 +467,6 @@ CheckCpuFeaturesDependency (
> if (CpuFeature->BeforeFeatureBitMask != NULL) {
> Swapped = InsertToBeforeEntry (FeatureList, CurrentEntry, CpuFeature-
> >BeforeFeatureBitMask);
> if (Swapped) {
> - CurrentEntry = NextEntry;
> continue;
> }
> }
> @@ -494,7 +474,6 @@ CheckCpuFeaturesDependency (
> if (CpuFeature->AfterFeatureBitMask != NULL) {
> Swapped = InsertToAfterEntry (FeatureList, CurrentEntry, CpuFeature-
> >AfterFeatureBitMask);
> if (Swapped) {
> - CurrentEntry = NextEntry;
> continue;
> }
> }
> @@ -502,7 +481,6 @@ CheckCpuFeaturesDependency (
> if (CpuFeature->CoreBeforeFeatureBitMask != NULL) {
> Swapped = InsertToBeforeEntry (FeatureList, CurrentEntry, CpuFeature-
> >CoreBeforeFeatureBitMask);
> if (Swapped) {
> - CurrentEntry = NextEntry;
> continue;
> }
> }
> @@ -510,7 +488,6 @@ CheckCpuFeaturesDependency (
> if (CpuFeature->CoreAfterFeatureBitMask != NULL) {
> Swapped = InsertToAfterEntry (FeatureList, CurrentEntry, CpuFeature-
> >CoreAfterFeatureBitMask);
> if (Swapped) {
> - CurrentEntry = NextEntry;
> continue;
> }
> }
> @@ -518,7 +495,6 @@ CheckCpuFeaturesDependency (
> if (CpuFeature->PackageBeforeFeatureBitMask != NULL) {
> Swapped = InsertToBeforeEntry (FeatureList, CurrentEntry, CpuFeature-
> >PackageBeforeFeatureBitMask);
> if (Swapped) {
> - CurrentEntry = NextEntry;
> continue;
> }
> }
> @@ -526,7 +502,6 @@ CheckCpuFeaturesDependency (
> if (CpuFeature->PackageAfterFeatureBitMask != NULL) {
> Swapped = InsertToAfterEntry (FeatureList, CurrentEntry, CpuFeature-
> >PackageAfterFeatureBitMask);
> if (Swapped) {
> - CurrentEntry = NextEntry;
> continue;
> }
> }
> --
> 2.15.0.windows.1
prev parent reply other threads:[~2018-10-26 4:16 UTC|newest]
Thread overview: 2+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-10-25 6:50 [Patch] UefiCpuPkg/RegisterCpuFeaturesLib: Support combo CPU feature style Eric Dong
2018-10-26 4:16 ` Ni, Ruiyu [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-list from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=734D49CCEBEEF84792F5B80ED585239D5BEDEABC@SHSMSX104.ccr.corp.intel.com \
--to=devel@edk2.groups.io \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox