Re: [edk2-devel] [PATCH v2 2/3] UefiCpuPkg/RegisterCpuFeaturesLib: Rename [Before|After]FeatureBitMask

["Zeng, Star" <star.zeng@intel.com>]

Minor comment added.

> -----Original Message-----
> From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of
> Ray Ni
> Sent: Tuesday, November 26, 2019 2:16 PM
> To: devel@edk2.groups.io
> Cc: Ni, Ray <ray.ni@intel.com>; Dong, Eric <eric.dong@intel.com>
> Subject: [edk2-devel] [PATCH v2 2/3] UefiCpuPkg/RegisterCpuFeaturesLib:
> Rename [Before|After]FeatureBitMask
> 
> From: Ray Ni <ray.ni@intel.com>
> 
> The patch doesn't have any functionality impact.
> 
> Signed-off-by: Ray Ni <ray.ni@intel.com>
> Cc: Eric Dong <eric.dong@intel.com>
> ---
>  .../RegisterCpuFeatures.h                     |  4 +-
>  .../RegisterCpuFeaturesLib.c                  | 68 +++++++++++--------
>  2 files changed, 40 insertions(+), 32 deletions(-)
> 
> diff --git

[Trimmed]

> 
>    FeatureMask                 = NULL;
> -  BeforeFeatureBitMask        = NULL;
> -  AfterFeatureBitMask         = NULL;
> +  ThreadBeforeFeatureBitMask  = NULL;
> +  ThreadAfterFeatureBitMask   = NULL;
>    CoreBeforeFeatureBitMask    = NULL;
>    CoreAfterFeatureBitMask     = NULL;
>    PackageBeforeFeatureBitMask = NULL;
> @@ -850,10 +850,18 @@ RegisterCpuFeature (
>    VA_START (Marker, InitializeFunc);
>    Feature = VA_ARG (Marker, UINT32);
>    while (Feature != CPU_FEATURE_END) {
> -    ASSERT ((Feature & (CPU_FEATURE_BEFORE | CPU_FEATURE_AFTER))
> -                    != (CPU_FEATURE_BEFORE | CPU_FEATURE_AFTER));
> +    //
> +    // It's invalid to require a feature is before AND after all other features.
> +    //
>      ASSERT ((Feature & (CPU_FEATURE_BEFORE_ALL |
> CPU_FEATURE_AFTER_ALL))
>                      != (CPU_FEATURE_BEFORE_ALL | CPU_FEATURE_AFTER_ALL));
> +
> +    //
> +    // It's invalid to require feature A is before AND after before feature B,

"after before" should be just "after", right?

With it corrected, Reviewed-by: Star Zeng <star.zeng@intel.com>

Thanks,
Star

> +    // either in thread level, core level or package level.
> +    //
> +    ASSERT ((Feature & (CPU_FEATURE_THREAD_BEFORE |
> CPU_FEATURE_THREAD_AFTER))
> +                    != (CPU_FEATURE_THREAD_BEFORE |
> CPU_FEATURE_THREAD_AFTER));
>      ASSERT ((Feature & (CPU_FEATURE_CORE_BEFORE |
> CPU_FEATURE_CORE_AFTER))
>                      != (CPU_FEATURE_CORE_BEFORE | CPU_FEATURE_CORE_AFTER));
>      ASSERT ((Feature & (CPU_FEATURE_PACKAGE_BEFORE |
> CPU_FEATURE_PACKAGE_AFTER))
> @@ -865,9 +873,9 @@ RegisterCpuFeature (
>        ASSERT (FeatureMask == NULL);
>        SetCpuFeaturesBitMask (&FeatureMask, Feature, CpuFeaturesData-
> >BitMaskSize);
>      } else if ((Feature & CPU_FEATURE_THREAD_BEFORE) != 0) {
> -      SetCpuFeaturesBitMask (&BeforeFeatureBitMask, Feature &
> ~CPU_FEATURE_THREAD_BEFORE, CpuFeaturesData->BitMaskSize);
> +      SetCpuFeaturesBitMask (&ThreadBeforeFeatureBitMask, Feature &
> ~CPU_FEATURE_THREAD_BEFORE, CpuFeaturesData->BitMaskSize);
>      } else if ((Feature & CPU_FEATURE_THREAD_AFTER) != 0) {
> -      SetCpuFeaturesBitMask (&AfterFeatureBitMask, Feature &
> ~CPU_FEATURE_THREAD_AFTER, CpuFeaturesData->BitMaskSize);
> +      SetCpuFeaturesBitMask (&ThreadAfterFeatureBitMask, Feature &
> ~CPU_FEATURE_THREAD_AFTER, CpuFeaturesData->BitMaskSize);
>      } else if ((Feature & CPU_FEATURE_CORE_BEFORE) != 0) {
>        SetCpuFeaturesBitMask (&CoreBeforeFeatureBitMask, Feature &
> ~CPU_FEATURE_CORE_BEFORE, CpuFeaturesData->BitMaskSize);
>      } else if ((Feature & CPU_FEATURE_CORE_AFTER) != 0) {
> @@ -885,8 +893,8 @@ RegisterCpuFeature (
>    ASSERT (CpuFeature != NULL);
>    CpuFeature->Signature                   = CPU_FEATURE_ENTRY_SIGNATURE;
>    CpuFeature->FeatureMask                 = FeatureMask;
> -  CpuFeature->BeforeFeatureBitMask        = BeforeFeatureBitMask;
> -  CpuFeature->AfterFeatureBitMask         = AfterFeatureBitMask;
> +  CpuFeature->ThreadBeforeFeatureBitMask  =
> ThreadBeforeFeatureBitMask;
> +  CpuFeature->ThreadAfterFeatureBitMask   = ThreadAfterFeatureBitMask;
>    CpuFeature->CoreBeforeFeatureBitMask    = CoreBeforeFeatureBitMask;
>    CpuFeature->CoreAfterFeatureBitMask     = CoreAfterFeatureBitMask;
>    CpuFeature->PackageBeforeFeatureBitMask =
> PackageBeforeFeatureBitMask;
> --
> 2.21.0.windows.1
> 
> 
>