1. Patchew
  2. EDK2
  3. View series

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

Song, BinX posted 1 patch 6 years, 4 months ago
Failed in applying to current master (apply log)
UefiCpuPkg/Include/Library/RegisterCpuFeaturesLib.h           |  5 -----
.../Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c   | 11 ++++++-----
2 files changed, 6 insertions(+), 10 deletions(-)
[edk2] [PATCH V2] UefiCpuPkg: Keep library class header file definition independent
Posted by Song, BinX 6 years, 4 months ago 
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/RegisterCpuFeaturesLib.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/RegisterCpuFeaturesLib.c b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c
index 6ec26e1..afc424c 100644
--- a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c
+++ b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.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

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH V2] UefiCpuPkg: Keep library class header file definition independent
Posted by Kinney, Michael D 6 years, 4 months ago 
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

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH V2] UefiCpuPkg: Keep library class header file definition independent
Posted by Song, BinX 6 years, 4 months ago 
Hi Mike,

Thanks for your suggestion.
After discussion with Eric, I know there is a function named IsCpuFeatureSupported() to do the feature valid/invalid check.
User should do IsCpuFeatureSupported() check first and then RegisterCpuFeature().
So there is no need to add any code to do the same work, I will roll back the patch which has been checked in before.

Best Regards,
Bell Song


> -----Original Message-----
> From: Kinney, Michael D
> Sent: Tuesday, December 19, 2017 7:21 AM
> To: Song, BinX <binx.song@intel.com>; edk2-devel@lists.01.org; Kinney,
> Michael D <michael.d.kinney@intel.com>
> Cc: Dong, Eric <eric.dong@intel.com>; lersek@redhat.com; Ni, Ruiyu
> <ruiyu.ni@intel.com>
> Subject: RE: [PATCH V2] UefiCpuPkg: Keep library class header file definition
> independent
> 
> 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

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.