Re: [Patch v2 2/6] UefiCpuPkg/RegisterCpuFeaturesLib.h: Add new dependence types.

["Ni, Ruiyu" <ruiyu.ni@Intel.com>]

On 10/17/2018 10:16 AM, Eric Dong wrote:
> Add new core/package dependence types which consumed by different MSRs.
> 
> 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>
> ---
>   .../Include/Library/RegisterCpuFeaturesLib.h       | 25 ++++++++++++++++++----
>   1 file changed, 21 insertions(+), 4 deletions(-)
> 
> diff --git a/UefiCpuPkg/Include/Library/RegisterCpuFeaturesLib.h b/UefiCpuPkg/Include/Library/RegisterCpuFeaturesLib.h
> index 9331e49d13..e6f0ebe4bc 100644
> --- a/UefiCpuPkg/Include/Library/RegisterCpuFeaturesLib.h
> +++ b/UefiCpuPkg/Include/Library/RegisterCpuFeaturesLib.h
> @@ -73,10 +73,17 @@
>   #define CPU_FEATURE_PPIN                            (32+11)
>   #define CPU_FEATURE_PROC_TRACE                      (32+12)
>   
> -#define CPU_FEATURE_BEFORE_ALL                      BIT27
> -#define CPU_FEATURE_AFTER_ALL                       BIT28
> -#define CPU_FEATURE_BEFORE                          BIT29
> -#define CPU_FEATURE_AFTER                           BIT30
> +#define CPU_FEATURE_BEFORE_ALL                      BIT23
> +#define CPU_FEATURE_AFTER_ALL                       BIT24


We could add comments to emphasize that CPU_FEATURE_BEFORE and 
CPU_FEATURE_AFTER only mean Thread scope before and Thread scope after.

And after the whole patch serials are checked in, I prefer to mark the 
below two macros as deprecated and avoid using them in core code any more.

> +#define CPU_FEATURE_BEFORE                          BIT25
> +#define CPU_FEATURE_AFTER                           BIT26
> +


> +#define CPU_FEATURE_THREAD_BEFORE                   CPU_FEATURE_BEFORE
> +#define CPU_FEATURE_THREAD_AFTER                    CPU_FEATURE_AFTER
> +#define CPU_FEATURE_CORE_BEFORE                     BIT27
> +#define CPU_FEATURE_CORE_AFTER                      BIT28
> +#define CPU_FEATURE_PACKAGE_BEFORE                  BIT29
> +#define CPU_FEATURE_PACKAGE_AFTER                   BIT30
>   #define CPU_FEATURE_END                             MAX_UINT32
>   /// @}
>   
> @@ -116,6 +123,16 @@ typedef struct {
>     CPUID_VERSION_INFO_EDX               CpuIdVersionInfoEdx;
>   } REGISTER_CPU_FEATURE_INFORMATION;
>   
> +//
> +// Describe the dependency type for different features.

Can you add comments to say like below?
"The value set to CPU_REGISTER_TABLE_ENTRY.Value when the REGISTER_TYPE 
is Semaphore."

And maybe move the enum definition to AcpiCpuData.h because the 
definition of CPU_REGISTER_TABLE_ENTRY and REGISTER_TYPE are all defined 
there.

> +//
> +typedef enum {
> +  NoneDepType,
> +  ThreadDepType,
> +  CoreDepType,
> +  PackageDepType
> +} CPU_FEATURE_DEPENDENCE_TYPE; > +
>   /**
>     Determines if a CPU feature is enabled in PcdCpuFeaturesSupport bit mask.
>     If a CPU feature is disabled in PcdCpuFeaturesSupport then all the code/data
> 


-- 
Thanks,
Ray