Re: [PATCH V2] UefiCpuPkg: Keep library class header file definition independent

["Kinney, Michael D" <michael.d.kinney@intel.com>]

This still does not work because a library instance that
registers a new CPU feature may want to use a new feature
bit larger than MAX.  This change moves the define 
from the RegisterCpuFeaturesLib library class to the 
implementation of the RegisterCpuFeaturesLibs.

The components that know the maximum bit number are the
NULL lib instances that register CPU features (e.g. 
CpuCommonFeaturesLib) and the platform developer that
that provides the set of NULL lib instances in a platform
DSC file and the associated PCDs.  For example, the 
following DSC fragment only specifies a single NULL lib
instance that registers CPU features.  However, it is
legal to provide additional NULL lib instances from
Si packages.

  UefiCpuPkg/CpuFeatures/CpuFeaturesDxe.inf {
    <LibraryClasses>
      NULL|UefiCpuPkg/Library/CpuCommonFeaturesLib/CpuCommonFeaturesLib.inf
  }

The maximum size of the bitmask for CPU features is known 
by the size of the PCD called PcdCpuFeaturesSupport that
has a DEC default value based on CpuCommonFeaturesLib but
can be set to a larger size based on the maximum bit used
by all the NULL lib instances.

So maybe the valid check should just verify that the bit
number passed in is < PcdGetSize (PcdCpuFeaturesSupport) * 8.
This eliminates the use of the #define value and uses an
existing PCD.

Thanks,

Mike

> -----Original Message-----
> From: Song, BinX
> Sent: Sunday, December 17, 2017 9:37 PM
> To: edk2-devel@lists.01.org
> Cc: Dong, Eric <eric.dong@intel.com>;
> lersek@redhat.com; Ni, Ruiyu <ruiyu.ni@intel.com>;
> Kinney, Michael D <michael.d.kinney@intel.com>
> Subject: [PATCH V2] UefiCpuPkg: Keep library class
> header file definition independent
> 
> V2:
> Move CPU_FEATURE_MAX definition from header file to C
> file.
> V1:
> Keep library class header file definition independent
> 
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Contributed-under: TianoCore Contribution Agreement 1.1
> Signed-off-by: Bell Song <binx.song@intel.com>
> ---
>  UefiCpuPkg/Include/Library/RegisterCpuFeaturesLib.h
> |  5 -----
> 
> .../Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesL
> ib.c   | 11 ++++++-----
>  2 files changed, 6 insertions(+), 10 deletions(-)
> 
> diff --git
> a/UefiCpuPkg/Include/Library/RegisterCpuFeaturesLib.h
> b/UefiCpuPkg/Include/Library/RegisterCpuFeaturesLib.h
> index fc3ccda..9331e49 100644
> ---
> a/UefiCpuPkg/Include/Library/RegisterCpuFeaturesLib.h
> +++
> b/UefiCpuPkg/Include/Library/RegisterCpuFeaturesLib.h
> @@ -71,11 +71,6 @@
>  #define CPU_FEATURE_APIC_TPR_UPDATE_MESSAGE
> (32+9)
>  #define CPU_FEATURE_ENERGY_PERFORMANCE_BIAS
> (32+10)
>  #define CPU_FEATURE_PPIN
> (32+11)
> -//
> -// Currently, CPU_FEATURE_PROC_TRACE is the MAX
> feature we support.
> -// If you define a feature bigger than it, please also
> replace it
> -// in RegisterCpuFeatureLibIsFeatureValid function.
> -//
>  #define CPU_FEATURE_PROC_TRACE
> (32+12)
> 
>  #define CPU_FEATURE_BEFORE_ALL
> BIT27
> diff --git
> a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpu
> FeaturesLib.c
> b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpu
> FeaturesLib.c
> index 6ec26e1..afc424c 100644
> ---
> a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpu
> FeaturesLib.c
> +++
> b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpu
> FeaturesLib.c
> @@ -13,6 +13,10 @@
>  **/
> 
>  #include "RegisterCpuFeatures.h"
> +//
> +// Please keep CPU_FEATURE_MAX as the max CPU feature
> +//
> +#define CPU_FEATURE_MAX              (32+12)
> 
>  /**
>    Checks if two CPU feature bit masks are equal.
> @@ -97,11 +101,8 @@ RegisterCpuFeatureLibIsFeatureValid
> (
> 
>    Data = Feature;
>    Data &= ~(CPU_FEATURE_BEFORE | CPU_FEATURE_AFTER |
> CPU_FEATURE_BEFORE_ALL | CPU_FEATURE_AFTER_ALL);
> -  //
> -  // Currently, CPU_FEATURE_PROC_TRACE is the MAX
> feature we support.
> -  // If you define a feature bigger than it, please
> replace it at below.
> -  //
> -  if (Data > CPU_FEATURE_PROC_TRACE) {
> +
> +  if (Data > CPU_FEATURE_MAX) {
>      DEBUG ((DEBUG_ERROR, "Invalid CPU feature: 0x%x ",
> Feature));
>      return FALSE;
>    }
> --
> 2.10.2.windows.1