* [PATCH] UefiCpuPkg: Check invalid RegisterCpuFeature parameter
@ 2017-12-11 8:16 Song, BinX
2017-12-11 8:23 ` Dong, Eric
2017-12-11 9:40 ` Ni, Ruiyu
0 siblings, 2 replies; 6+ messages in thread
From: Song, BinX @ 2017-12-11 8:16 UTC (permalink / raw)
To: edk2-devel@lists.01.org; +Cc: Dong, Eric, lersek@redhat.com
Check and assert invalid RegisterCpuFeature function parameter
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>
---
.../Include/Library/RegisterCpuFeaturesLib.h | 4 ++++
.../RegisterCpuFeaturesLib.c | 28 ++++++++++++++++++++++
2 files changed, 32 insertions(+)
diff --git a/UefiCpuPkg/Include/Library/RegisterCpuFeaturesLib.h b/UefiCpuPkg/Include/Library/RegisterCpuFeaturesLib.h
index 9331e49..54244cd 100644
--- a/UefiCpuPkg/Include/Library/RegisterCpuFeaturesLib.h
+++ b/UefiCpuPkg/Include/Library/RegisterCpuFeaturesLib.h
@@ -72,6 +72,10 @@
#define CPU_FEATURE_ENERGY_PERFORMANCE_BIAS (32+10)
#define CPU_FEATURE_PPIN (32+11)
+//
+// When you add new CPU features, please also replace the minor CPU feature
+// with the max CPU feature in the IsFeatureValidCheck() function.
+//
#define CPU_FEATURE_PROC_TRACE (32+12)
#define CPU_FEATURE_BEFORE_ALL BIT27
#define CPU_FEATURE_AFTER_ALL BIT28
diff --git a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c
index dd6a82b..f75d900 100644
--- a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c
+++ b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c
@@ -81,6 +81,33 @@ DumpCpuFeature (
}
/**
+ Determines if the CPU feature is valid.
+
+ @param[in] Feature Pointer to CPU feature
+
+ @retval TRUE The CPU feature is valid.
+ @retval FALSE The CPU feature is invalid.
+**/
+BOOLEAN
+IsFeatureValidCheck (
+ IN UINT32 Feature
+ )
+{
+ UINT32 Data;
+
+ Data = Feature;
+ Data &= ~(CPU_FEATURE_BEFORE | CPU_FEATURE_AFTER | CPU_FEATURE_BEFORE_ALL | CPU_FEATURE_AFTER_ALL);
+ //
+ // Please replace CPU feature below with the MAX one if have.
+ //
+ if (Data > CPU_FEATURE_PROC_TRACE) {
+ DEBUG ((DEBUG_ERROR, "Invalid CPU feature: 0x%x ", Feature));
+ return FALSE;
+ }
+ return TRUE;
+}
+
+/**
Determines if the feature bit mask is in dependent CPU feature bit mask buffer.
@param[in] FeatureMask Pointer to CPU feature bit mask
@@ -444,6 +471,7 @@ RegisterCpuFeature (
VA_START (Marker, InitializeFunc);
Feature = VA_ARG (Marker, UINT32);
+ ASSERT (IsFeatureValidCheck(Feature));
while (Feature != CPU_FEATURE_END) {
ASSERT ((Feature & (CPU_FEATURE_BEFORE | CPU_FEATURE_AFTER))
!= (CPU_FEATURE_BEFORE | CPU_FEATURE_AFTER));
--
2.10.2.windows.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] UefiCpuPkg: Check invalid RegisterCpuFeature parameter
2017-12-11 8:16 [PATCH] UefiCpuPkg: Check invalid RegisterCpuFeature parameter Song, BinX
@ 2017-12-11 8:23 ` Dong, Eric
2017-12-11 9:40 ` Ni, Ruiyu
1 sibling, 0 replies; 6+ messages in thread
From: Dong, Eric @ 2017-12-11 8:23 UTC (permalink / raw)
To: Song, BinX, edk2-devel@lists.01.org; +Cc: lersek@redhat.com
Reviewed-by: Eric Dong <eric.dong@intel.com>
-----Original Message-----
From: Song, BinX
Sent: Monday, December 11, 2017 4:16 PM
To: edk2-devel@lists.01.org
Cc: Dong, Eric; lersek@redhat.com
Subject: [PATCH] UefiCpuPkg: Check invalid RegisterCpuFeature parameter
Check and assert invalid RegisterCpuFeature function parameter
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>
---
.../Include/Library/RegisterCpuFeaturesLib.h | 4 ++++
.../RegisterCpuFeaturesLib.c | 28 ++++++++++++++++++++++
2 files changed, 32 insertions(+)
diff --git a/UefiCpuPkg/Include/Library/RegisterCpuFeaturesLib.h b/UefiCpuPkg/Include/Library/RegisterCpuFeaturesLib.h
index 9331e49..54244cd 100644
--- a/UefiCpuPkg/Include/Library/RegisterCpuFeaturesLib.h
+++ b/UefiCpuPkg/Include/Library/RegisterCpuFeaturesLib.h
@@ -72,6 +72,10 @@
#define CPU_FEATURE_ENERGY_PERFORMANCE_BIAS (32+10)
#define CPU_FEATURE_PPIN (32+11)
+//
+// When you add new CPU features, please also replace the minor CPU
+feature // with the max CPU feature in the IsFeatureValidCheck() function.
+//
#define CPU_FEATURE_PROC_TRACE (32+12)
#define CPU_FEATURE_BEFORE_ALL BIT27
#define CPU_FEATURE_AFTER_ALL BIT28
diff --git a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c
index dd6a82b..f75d900 100644
--- a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c
+++ b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c
@@ -81,6 +81,33 @@ DumpCpuFeature (
}
/**
+ Determines if the CPU feature is valid.
+
+ @param[in] Feature Pointer to CPU feature
+
+ @retval TRUE The CPU feature is valid.
+ @retval FALSE The CPU feature is invalid.
+**/
+BOOLEAN
+IsFeatureValidCheck (
+ IN UINT32 Feature
+ )
+{
+ UINT32 Data;
+
+ Data = Feature;
+ Data &= ~(CPU_FEATURE_BEFORE | CPU_FEATURE_AFTER |
+CPU_FEATURE_BEFORE_ALL | CPU_FEATURE_AFTER_ALL);
+ //
+ // Please replace CPU feature below with the MAX one if have.
+ //
+ if (Data > CPU_FEATURE_PROC_TRACE) {
+ DEBUG ((DEBUG_ERROR, "Invalid CPU feature: 0x%x ", Feature));
+ return FALSE;
+ }
+ return TRUE;
+}
+
+/**
Determines if the feature bit mask is in dependent CPU feature bit mask buffer.
@param[in] FeatureMask Pointer to CPU feature bit mask
@@ -444,6 +471,7 @@ RegisterCpuFeature (
VA_START (Marker, InitializeFunc);
Feature = VA_ARG (Marker, UINT32);
+ ASSERT (IsFeatureValidCheck(Feature));
while (Feature != CPU_FEATURE_END) {
ASSERT ((Feature & (CPU_FEATURE_BEFORE | CPU_FEATURE_AFTER))
!= (CPU_FEATURE_BEFORE | CPU_FEATURE_AFTER));
--
2.10.2.windows.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] UefiCpuPkg: Check invalid RegisterCpuFeature parameter
2017-12-11 8:16 [PATCH] UefiCpuPkg: Check invalid RegisterCpuFeature parameter Song, BinX
2017-12-11 8:23 ` Dong, Eric
@ 2017-12-11 9:40 ` Ni, Ruiyu
2017-12-11 10:00 ` Song, BinX
1 sibling, 1 reply; 6+ messages in thread
From: Ni, Ruiyu @ 2017-12-11 9:40 UTC (permalink / raw)
To: Song, BinX, edk2-devel@lists.01.org; +Cc: lersek@redhat.com, Dong, Eric
On 12/11/2017 4:16 PM, Song, BinX wrote:
> Check and assert invalid RegisterCpuFeature function parameter
>
> 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>
> ---
> .../Include/Library/RegisterCpuFeaturesLib.h | 4 ++++
> .../RegisterCpuFeaturesLib.c | 28 ++++++++++++++++++++++
> 2 files changed, 32 insertions(+)
>
> diff --git a/UefiCpuPkg/Include/Library/RegisterCpuFeaturesLib.h b/UefiCpuPkg/Include/Library/RegisterCpuFeaturesLib.h
> index 9331e49..54244cd 100644
> --- a/UefiCpuPkg/Include/Library/RegisterCpuFeaturesLib.h
> +++ b/UefiCpuPkg/Include/Library/RegisterCpuFeaturesLib.h
> @@ -72,6 +72,10 @@
> #define CPU_FEATURE_ENERGY_PERFORMANCE_BIAS (32+10)
> #define CPU_FEATURE_PPIN (32+11)
> +//
> +// When you add new CPU features, please also replace the minor CPU feature
> +// with the max CPU feature in the IsFeatureValidCheck() function.
> +//
> #define CPU_FEATURE_PROC_TRACE (32+12)
>
> #define CPU_FEATURE_BEFORE_ALL BIT27
> #define CPU_FEATURE_AFTER_ALL BIT28
> diff --git a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c
> index dd6a82b..f75d900 100644
> --- a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c
> +++ b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c
> @@ -81,6 +81,33 @@ DumpCpuFeature (
> }
>
> /**
> + Determines if the CPU feature is valid.
> +
> + @param[in] Feature Pointer to CPU feature
> +
> + @retval TRUE The CPU feature is valid.
> + @retval FALSE The CPU feature is invalid.
> +**/
> +BOOLEAN
> +IsFeatureValidCheck (
Can we rename this function name to
"RegisterCpuFeatureLibIsFeatureValid"?
> + IN UINT32 Feature
> + )
> +{
> + UINT32 Data;
> +
> + Data = Feature;
> + Data &= ~(CPU_FEATURE_BEFORE | CPU_FEATURE_AFTER | CPU_FEATURE_BEFORE_ALL | CPU_FEATURE_AFTER_ALL);
> + //
> + // Please replace CPU feature below with the MAX one if have.
Can we just say "CPU_FEATURE_PROC_TRACE" is the MAX feature we support?
> + //
> + if (Data > CPU_FEATURE_PROC_TRACE) {
> + DEBUG ((DEBUG_ERROR, "Invalid CPU feature: 0x%x ", Feature));
> + return FALSE;
> + }
> + return TRUE;
> +}
> +
> +/**
> Determines if the feature bit mask is in dependent CPU feature bit mask buffer.
>
> @param[in] FeatureMask Pointer to CPU feature bit mask
> @@ -444,6 +471,7 @@ RegisterCpuFeature (
>
> VA_START (Marker, InitializeFunc);
> Feature = VA_ARG (Marker, UINT32);
> + ASSERT (IsFeatureValidCheck(Feature));
> while (Feature != CPU_FEATURE_END) {
> ASSERT ((Feature & (CPU_FEATURE_BEFORE | CPU_FEATURE_AFTER))
> != (CPU_FEATURE_BEFORE | CPU_FEATURE_AFTER));
>
--
Thanks,
Ray
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] UefiCpuPkg: Check invalid RegisterCpuFeature parameter
2017-12-11 9:40 ` Ni, Ruiyu
@ 2017-12-11 10:00 ` Song, BinX
2017-12-12 8:43 ` Ni, Ruiyu
0 siblings, 1 reply; 6+ messages in thread
From: Song, BinX @ 2017-12-11 10:00 UTC (permalink / raw)
To: Ni, Ruiyu, edk2-devel@lists.01.org; +Cc: lersek@redhat.com, Dong, Eric
Hi Ray,
Below is my opinions for your 2 questions:
1. Can we rename this function name to "RegisterCpuFeatureLibIsFeatureValid"?
[Bell]: In content of RegisterCpuFeaturesLib.c, there is a function named IsBitMaskMatchCheck(), it's my function's base, they have similar function - a small valid/invalid check,
So I think it is better to keep them align.
2. Can we just say "CPU_FEATURE_PROC_TRACE" is the MAX feature we support?
[Bell]: Discussed with Eric before, we should not define this as a MAX feature for future extension purpose.
Best Regards,
Bell Song
> -----Original Message-----
> From: Ni, Ruiyu
> Sent: Monday, December 11, 2017 5:40 PM
> To: Song, BinX <binx.song@intel.com>; edk2-devel@lists.01.org
> Cc: lersek@redhat.com; Dong, Eric <eric.dong@intel.com>
> Subject: Re: [edk2] [PATCH] UefiCpuPkg: Check invalid RegisterCpuFeature
> parameter
>
> On 12/11/2017 4:16 PM, Song, BinX wrote:
> > Check and assert invalid RegisterCpuFeature function parameter
> >
> > 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>
> > ---
> > .../Include/Library/RegisterCpuFeaturesLib.h | 4 ++++
> > .../RegisterCpuFeaturesLib.c | 28 ++++++++++++++++++++++
> > 2 files changed, 32 insertions(+)
> >
> > diff --git a/UefiCpuPkg/Include/Library/RegisterCpuFeaturesLib.h
> b/UefiCpuPkg/Include/Library/RegisterCpuFeaturesLib.h
> > index 9331e49..54244cd 100644
> > --- a/UefiCpuPkg/Include/Library/RegisterCpuFeaturesLib.h
> > +++ b/UefiCpuPkg/Include/Library/RegisterCpuFeaturesLib.h
> > @@ -72,6 +72,10 @@
> > #define CPU_FEATURE_ENERGY_PERFORMANCE_BIAS (32+10)
> > #define CPU_FEATURE_PPIN (32+11)
> > +//
> > +// When you add new CPU features, please also replace the minor CPU
> feature
> > +// with the max CPU feature in the IsFeatureValidCheck() function.
> > +//
> > #define CPU_FEATURE_PROC_TRACE (32+12)
> >
> > #define CPU_FEATURE_BEFORE_ALL BIT27
> > #define CPU_FEATURE_AFTER_ALL BIT28
> > diff --git
> a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c
> b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c
> > index dd6a82b..f75d900 100644
> > ---
> a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c
> > +++
> b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c
> > @@ -81,6 +81,33 @@ DumpCpuFeature (
> > }
> >
> > /**
> > + Determines if the CPU feature is valid.
> > +
> > + @param[in] Feature Pointer to CPU feature
> > +
> > + @retval TRUE The CPU feature is valid.
> > + @retval FALSE The CPU feature is invalid.
> > +**/
> > +BOOLEAN
> > +IsFeatureValidCheck (
> Can we rename this function name to
> "RegisterCpuFeatureLibIsFeatureValid"?
>
>
> > + IN UINT32 Feature
> > + )
> > +{
> > + UINT32 Data;
> > +
> > + Data = Feature;
> > + Data &= ~(CPU_FEATURE_BEFORE | CPU_FEATURE_AFTER |
> CPU_FEATURE_BEFORE_ALL | CPU_FEATURE_AFTER_ALL);
> > + //
> > + // Please replace CPU feature below with the MAX one if have.
> Can we just say "CPU_FEATURE_PROC_TRACE" is the MAX feature we
> support?
>
>
> > + //
> > + if (Data > CPU_FEATURE_PROC_TRACE) {
> > + DEBUG ((DEBUG_ERROR, "Invalid CPU feature: 0x%x ", Feature));
> > + return FALSE;
> > + }
> > + return TRUE;
> > +}
> > +
> > +/**
> > Determines if the feature bit mask is in dependent CPU feature bit mask
> buffer.
> >
> > @param[in] FeatureMask Pointer to CPU feature bit mask
> > @@ -444,6 +471,7 @@ RegisterCpuFeature (
> >
> > VA_START (Marker, InitializeFunc);
> > Feature = VA_ARG (Marker, UINT32);
> > + ASSERT (IsFeatureValidCheck(Feature));
> > while (Feature != CPU_FEATURE_END) {
> > ASSERT ((Feature & (CPU_FEATURE_BEFORE | CPU_FEATURE_AFTER))
> > != (CPU_FEATURE_BEFORE | CPU_FEATURE_AFTER));
> >
>
>
> --
> Thanks,
> Ray
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] UefiCpuPkg: Check invalid RegisterCpuFeature parameter
2017-12-11 10:00 ` Song, BinX
@ 2017-12-12 8:43 ` Ni, Ruiyu
2017-12-13 1:54 ` Song, BinX
0 siblings, 1 reply; 6+ messages in thread
From: Ni, Ruiyu @ 2017-12-12 8:43 UTC (permalink / raw)
To: Song, BinX, edk2-devel@lists.01.org; +Cc: lersek@redhat.com, Dong, Eric
> -----Original Message-----
> From: Song, BinX
> Sent: Monday, December 11, 2017 6:00 PM
> To: Ni, Ruiyu <ruiyu.ni@intel.com>; edk2-devel@lists.01.org
> Cc: lersek@redhat.com; Dong, Eric <eric.dong@intel.com>
> Subject: RE: [edk2] [PATCH] UefiCpuPkg: Check invalid RegisterCpuFeature
> parameter
>
> Hi Ray,
>
> Below is my opinions for your 2 questions:
> 1. Can we rename this function name to
> "RegisterCpuFeatureLibIsFeatureValid"?
> [Bell]: In content of RegisterCpuFeaturesLib.c, there is a function named
> IsBitMaskMatchCheck(), it's my function's base, they have similar function - a
> small valid/invalid check, So I think it is better to keep them align.
The original function name IsXXXXCheck() is not good. Please do not follow the
same naming style.
> 2. Can we just say "CPU_FEATURE_PROC_TRACE" is the MAX feature we
> support?
> [Bell]: Discussed with Eric before, we should not define this as a MAX feature
> for future extension purpose.
I didn't mean to define a new MAX macro.
You just need to update the comments.
>
> Best Regards,
> Bell Song
>
>
> > -----Original Message-----
> > From: Ni, Ruiyu
> > Sent: Monday, December 11, 2017 5:40 PM
> > To: Song, BinX <binx.song@intel.com>; edk2-devel@lists.01.org
> > Cc: lersek@redhat.com; Dong, Eric <eric.dong@intel.com>
> > Subject: Re: [edk2] [PATCH] UefiCpuPkg: Check invalid
> > RegisterCpuFeature parameter
> >
> > On 12/11/2017 4:16 PM, Song, BinX wrote:
> > > Check and assert invalid RegisterCpuFeature function parameter
> > >
> > > 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>
> > > ---
> > > .../Include/Library/RegisterCpuFeaturesLib.h | 4 ++++
> > > .../RegisterCpuFeaturesLib.c | 28
> ++++++++++++++++++++++
> > > 2 files changed, 32 insertions(+)
> > >
> > > diff --git a/UefiCpuPkg/Include/Library/RegisterCpuFeaturesLib.h
> > b/UefiCpuPkg/Include/Library/RegisterCpuFeaturesLib.h
> > > index 9331e49..54244cd 100644
> > > --- a/UefiCpuPkg/Include/Library/RegisterCpuFeaturesLib.h
> > > +++ b/UefiCpuPkg/Include/Library/RegisterCpuFeaturesLib.h
> > > @@ -72,6 +72,10 @@
> > > #define CPU_FEATURE_ENERGY_PERFORMANCE_BIAS (32+10)
> > > #define CPU_FEATURE_PPIN (32+11)
> > > +//
> > > +// When you add new CPU features, please also replace the minor CPU
> > feature
> > > +// with the max CPU feature in the IsFeatureValidCheck() function.
> > > +//
> > > #define CPU_FEATURE_PROC_TRACE (32+12)
> > >
> > > #define CPU_FEATURE_BEFORE_ALL BIT27
> > > #define CPU_FEATURE_AFTER_ALL BIT28
> > > diff --git
> > a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c
> > b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c
> > > index dd6a82b..f75d900 100644
> > > ---
> > a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c
> > > +++
> > b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c
> > > @@ -81,6 +81,33 @@ DumpCpuFeature (
> > > }
> > >
> > > /**
> > > + Determines if the CPU feature is valid.
> > > +
> > > + @param[in] Feature Pointer to CPU feature
> > > +
> > > + @retval TRUE The CPU feature is valid.
> > > + @retval FALSE The CPU feature is invalid.
> > > +**/
> > > +BOOLEAN
> > > +IsFeatureValidCheck (
> > Can we rename this function name to
> > "RegisterCpuFeatureLibIsFeatureValid"?
> >
> >
> > > + IN UINT32 Feature
> > > + )
> > > +{
> > > + UINT32 Data;
> > > +
> > > + Data = Feature;
> > > + Data &= ~(CPU_FEATURE_BEFORE | CPU_FEATURE_AFTER |
> > CPU_FEATURE_BEFORE_ALL | CPU_FEATURE_AFTER_ALL);
> > > + //
> > > + // Please replace CPU feature below with the MAX one if have.
> > Can we just say "CPU_FEATURE_PROC_TRACE" is the MAX feature we
> > support?
> >
> >
> > > + //
> > > + if (Data > CPU_FEATURE_PROC_TRACE) {
> > > + DEBUG ((DEBUG_ERROR, "Invalid CPU feature: 0x%x ", Feature));
> > > + return FALSE;
> > > + }
> > > + return TRUE;
> > > +}
> > > +
> > > +/**
> > > Determines if the feature bit mask is in dependent CPU feature
> > > bit mask
> > buffer.
> > >
> > > @param[in] FeatureMask Pointer to CPU feature bit mask
> > > @@ -444,6 +471,7 @@ RegisterCpuFeature (
> > >
> > > VA_START (Marker, InitializeFunc);
> > > Feature = VA_ARG (Marker, UINT32);
> > > + ASSERT (IsFeatureValidCheck(Feature));
> > > while (Feature != CPU_FEATURE_END) {
> > > ASSERT ((Feature & (CPU_FEATURE_BEFORE | CPU_FEATURE_AFTER))
> > > != (CPU_FEATURE_BEFORE | CPU_FEATURE_AFTER));
> > >
> >
> >
> > --
> > Thanks,
> > Ray
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] UefiCpuPkg: Check invalid RegisterCpuFeature parameter
2017-12-12 8:43 ` Ni, Ruiyu
@ 2017-12-13 1:54 ` Song, BinX
0 siblings, 0 replies; 6+ messages in thread
From: Song, BinX @ 2017-12-13 1:54 UTC (permalink / raw)
To: Ni, Ruiyu, edk2-devel@lists.01.org; +Cc: lersek@redhat.com, Dong, Eric
Hi Ray,
Got it, I will update a V2 patch.
Best Regards,
Bell Song
> -----Original Message-----
> From: Ni, Ruiyu
> Sent: Tuesday, December 12, 2017 4:44 PM
> To: Song, BinX <binx.song@intel.com>; edk2-devel@lists.01.org
> Cc: lersek@redhat.com; Dong, Eric <eric.dong@intel.com>
> Subject: RE: [edk2] [PATCH] UefiCpuPkg: Check invalid RegisterCpuFeature
> parameter
>
> > -----Original Message-----
> > From: Song, BinX
> > Sent: Monday, December 11, 2017 6:00 PM
> > To: Ni, Ruiyu <ruiyu.ni@intel.com>; edk2-devel@lists.01.org
> > Cc: lersek@redhat.com; Dong, Eric <eric.dong@intel.com>
> > Subject: RE: [edk2] [PATCH] UefiCpuPkg: Check invalid RegisterCpuFeature
> > parameter
> >
> > Hi Ray,
> >
> > Below is my opinions for your 2 questions:
> > 1. Can we rename this function name to
> > "RegisterCpuFeatureLibIsFeatureValid"?
> > [Bell]: In content of RegisterCpuFeaturesLib.c, there is a function named
> > IsBitMaskMatchCheck(), it's my function's base, they have similar function -
> a
> > small valid/invalid check, So I think it is better to keep them align.
> The original function name IsXXXXCheck() is not good. Please do not follow
> the
> same naming style.
>
> > 2. Can we just say "CPU_FEATURE_PROC_TRACE" is the MAX feature we
> > support?
> > [Bell]: Discussed with Eric before, we should not define this as a MAX
> feature
> > for future extension purpose.
> I didn't mean to define a new MAX macro.
> You just need to update the comments.
> >
> > Best Regards,
> > Bell Song
> >
> >
> > > -----Original Message-----
> > > From: Ni, Ruiyu
> > > Sent: Monday, December 11, 2017 5:40 PM
> > > To: Song, BinX <binx.song@intel.com>; edk2-devel@lists.01.org
> > > Cc: lersek@redhat.com; Dong, Eric <eric.dong@intel.com>
> > > Subject: Re: [edk2] [PATCH] UefiCpuPkg: Check invalid
> > > RegisterCpuFeature parameter
> > >
> > > On 12/11/2017 4:16 PM, Song, BinX wrote:
> > > > Check and assert invalid RegisterCpuFeature function parameter
> > > >
> > > > 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>
> > > > ---
> > > > .../Include/Library/RegisterCpuFeaturesLib.h | 4 ++++
> > > > .../RegisterCpuFeaturesLib.c | 28
> > ++++++++++++++++++++++
> > > > 2 files changed, 32 insertions(+)
> > > >
> > > > diff --git a/UefiCpuPkg/Include/Library/RegisterCpuFeaturesLib.h
> > > b/UefiCpuPkg/Include/Library/RegisterCpuFeaturesLib.h
> > > > index 9331e49..54244cd 100644
> > > > --- a/UefiCpuPkg/Include/Library/RegisterCpuFeaturesLib.h
> > > > +++ b/UefiCpuPkg/Include/Library/RegisterCpuFeaturesLib.h
> > > > @@ -72,6 +72,10 @@
> > > > #define CPU_FEATURE_ENERGY_PERFORMANCE_BIAS (32+10)
> > > > #define CPU_FEATURE_PPIN (32+11)
> > > > +//
> > > > +// When you add new CPU features, please also replace the minor
> CPU
> > > feature
> > > > +// with the max CPU feature in the IsFeatureValidCheck() function.
> > > > +//
> > > > #define CPU_FEATURE_PROC_TRACE (32+12)
> > > >
> > > > #define CPU_FEATURE_BEFORE_ALL BIT27
> > > > #define CPU_FEATURE_AFTER_ALL BIT28
> > > > diff --git
> > > a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c
> > > b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c
> > > > index dd6a82b..f75d900 100644
> > > > ---
> > > a/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c
> > > > +++
> > > b/UefiCpuPkg/Library/RegisterCpuFeaturesLib/RegisterCpuFeaturesLib.c
> > > > @@ -81,6 +81,33 @@ DumpCpuFeature (
> > > > }
> > > >
> > > > /**
> > > > + Determines if the CPU feature is valid.
> > > > +
> > > > + @param[in] Feature Pointer to CPU feature
> > > > +
> > > > + @retval TRUE The CPU feature is valid.
> > > > + @retval FALSE The CPU feature is invalid.
> > > > +**/
> > > > +BOOLEAN
> > > > +IsFeatureValidCheck (
> > > Can we rename this function name to
> > > "RegisterCpuFeatureLibIsFeatureValid"?
> > >
> > >
> > > > + IN UINT32 Feature
> > > > + )
> > > > +{
> > > > + UINT32 Data;
> > > > +
> > > > + Data = Feature;
> > > > + Data &= ~(CPU_FEATURE_BEFORE | CPU_FEATURE_AFTER |
> > > CPU_FEATURE_BEFORE_ALL | CPU_FEATURE_AFTER_ALL);
> > > > + //
> > > > + // Please replace CPU feature below with the MAX one if have.
> > > Can we just say "CPU_FEATURE_PROC_TRACE" is the MAX feature we
> > > support?
> > >
> > >
> > > > + //
> > > > + if (Data > CPU_FEATURE_PROC_TRACE) {
> > > > + DEBUG ((DEBUG_ERROR, "Invalid CPU feature: 0x%x ", Feature));
> > > > + return FALSE;
> > > > + }
> > > > + return TRUE;
> > > > +}
> > > > +
> > > > +/**
> > > > Determines if the feature bit mask is in dependent CPU feature
> > > > bit mask
> > > buffer.
> > > >
> > > > @param[in] FeatureMask Pointer to CPU feature bit mask
> > > > @@ -444,6 +471,7 @@ RegisterCpuFeature (
> > > >
> > > > VA_START (Marker, InitializeFunc);
> > > > Feature = VA_ARG (Marker, UINT32);
> > > > + ASSERT (IsFeatureValidCheck(Feature));
> > > > while (Feature != CPU_FEATURE_END) {
> > > > ASSERT ((Feature & (CPU_FEATURE_BEFORE |
> CPU_FEATURE_AFTER))
> > > > != (CPU_FEATURE_BEFORE | CPU_FEATURE_AFTER));
> > > >
> > >
> > >
> > > --
> > > Thanks,
> > > Ray
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2017-12-13 1:50 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-12-11 8:16 [PATCH] UefiCpuPkg: Check invalid RegisterCpuFeature parameter Song, BinX
2017-12-11 8:23 ` Dong, Eric
2017-12-11 9:40 ` Ni, Ruiyu
2017-12-11 10:00 ` Song, BinX
2017-12-12 8:43 ` Ni, Ruiyu
2017-12-13 1:54 ` Song, BinX
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox