They are wrappers of BUILD_BUG_ON_ZERO(NCAPINTS != n), which is already
present in corresponding *_MASK_BIT_SET. And fill the missing period in
head comments by the way.
Signed-off-by: Cao jin <[email protected]>
---
arch/x86/include/asm/cpufeature.h | 2 --
arch/x86/include/asm/disabled-features.h | 1 -
arch/x86/include/asm/required-features.h | 3 +--
3 files changed, 1 insertion(+), 5 deletions(-)
diff --git a/arch/x86/include/asm/cpufeature.h b/arch/x86/include/asm/cpufeature.h
index 58acda503817..232ffb88039c 100644
--- a/arch/x86/include/asm/cpufeature.h
+++ b/arch/x86/include/asm/cpufeature.h
@@ -81,7 +81,6 @@ extern const char * const x86_bug_flags[NBUGINTS*32];
CHECK_BIT_IN_MASK_WORD(REQUIRED_MASK, 16, feature_bit) || \
CHECK_BIT_IN_MASK_WORD(REQUIRED_MASK, 17, feature_bit) || \
CHECK_BIT_IN_MASK_WORD(REQUIRED_MASK, 18, feature_bit) || \
- REQUIRED_MASK_CHECK || \
BUILD_BUG_ON_ZERO(NCAPINTS != 19))
#define DISABLED_MASK_BIT_SET(feature_bit) \
@@ -104,7 +103,6 @@ extern const char * const x86_bug_flags[NBUGINTS*32];
CHECK_BIT_IN_MASK_WORD(DISABLED_MASK, 16, feature_bit) || \
CHECK_BIT_IN_MASK_WORD(DISABLED_MASK, 17, feature_bit) || \
CHECK_BIT_IN_MASK_WORD(DISABLED_MASK, 18, feature_bit) || \
- DISABLED_MASK_CHECK || \
BUILD_BUG_ON_ZERO(NCAPINTS != 19))
#define cpu_has(c, bit) \
diff --git a/arch/x86/include/asm/disabled-features.h b/arch/x86/include/asm/disabled-features.h
index a5ea841cc6d2..8a2eafa86739 100644
--- a/arch/x86/include/asm/disabled-features.h
+++ b/arch/x86/include/asm/disabled-features.h
@@ -84,6 +84,5 @@
#define DISABLED_MASK16 (DISABLE_PKU|DISABLE_OSPKE|DISABLE_LA57|DISABLE_UMIP)
#define DISABLED_MASK17 0
#define DISABLED_MASK18 0
-#define DISABLED_MASK_CHECK BUILD_BUG_ON_ZERO(NCAPINTS != 19)
#endif /* _ASM_X86_DISABLED_FEATURES_H */
diff --git a/arch/x86/include/asm/required-features.h b/arch/x86/include/asm/required-features.h
index 6847d85400a8..cb98b66d3e81 100644
--- a/arch/x86/include/asm/required-features.h
+++ b/arch/x86/include/asm/required-features.h
@@ -1,7 +1,7 @@
#ifndef _ASM_X86_REQUIRED_FEATURES_H
#define _ASM_X86_REQUIRED_FEATURES_H
-/* Define minimum CPUID feature set for kernel These bits are checked
+/* Define minimum CPUID feature set for kernel. These bits are checked
really early to actually display a visible error message before the
kernel dies. Make sure to assign features to the proper mask!
@@ -101,6 +101,5 @@
#define REQUIRED_MASK16 0
#define REQUIRED_MASK17 0
#define REQUIRED_MASK18 0
-#define REQUIRED_MASK_CHECK BUILD_BUG_ON_ZERO(NCAPINTS != 19)
#endif /* _ASM_X86_REQUIRED_FEATURES_H */
--
2.17.0
On Tue, Aug 27, 2019 at 03:05:50PM +0800, Cao jin wrote:
> They are wrappers of BUILD_BUG_ON_ZERO(NCAPINTS != n), which is already
> present in corresponding *_MASK_BIT_SET. And fill the missing period in
> head comments by the way.
>
> Signed-off-by: Cao jin <[email protected]>
> ---
> arch/x86/include/asm/cpufeature.h | 2 --
> arch/x86/include/asm/disabled-features.h | 1 -
> arch/x86/include/asm/required-features.h | 3 +--
> 3 files changed, 1 insertion(+), 5 deletions(-)
>
> diff --git a/arch/x86/include/asm/cpufeature.h b/arch/x86/include/asm/cpufeature.h
> index 58acda503817..232ffb88039c 100644
> --- a/arch/x86/include/asm/cpufeature.h
> +++ b/arch/x86/include/asm/cpufeature.h
> @@ -81,7 +81,6 @@ extern const char * const x86_bug_flags[NBUGINTS*32];
> CHECK_BIT_IN_MASK_WORD(REQUIRED_MASK, 16, feature_bit) || \
> CHECK_BIT_IN_MASK_WORD(REQUIRED_MASK, 17, feature_bit) || \
> CHECK_BIT_IN_MASK_WORD(REQUIRED_MASK, 18, feature_bit) || \
> - REQUIRED_MASK_CHECK || \
> BUILD_BUG_ON_ZERO(NCAPINTS != 19))
>
> #define DISABLED_MASK_BIT_SET(feature_bit) \
> @@ -104,7 +103,6 @@ extern const char * const x86_bug_flags[NBUGINTS*32];
> CHECK_BIT_IN_MASK_WORD(DISABLED_MASK, 16, feature_bit) || \
> CHECK_BIT_IN_MASK_WORD(DISABLED_MASK, 17, feature_bit) || \
> CHECK_BIT_IN_MASK_WORD(DISABLED_MASK, 18, feature_bit) || \
> - DISABLED_MASK_CHECK || \
> BUILD_BUG_ON_ZERO(NCAPINTS != 19))
>
> #define cpu_has(c, bit) \
If you do a little bit of git archeology:
$ git annotate arch/x86/include/asm/cpufeature.h
after a while, you'll see that this was added in:
1e61f78baf89 ("x86/cpufeature: Make sure DISABLED/REQUIRED macros are updated")
and then you could Cc Dave and ask what he was thinking then?
Leaving in the rest for reference.
> diff --git a/arch/x86/include/asm/disabled-features.h b/arch/x86/include/asm/disabled-features.h
> index a5ea841cc6d2..8a2eafa86739 100644
> --- a/arch/x86/include/asm/disabled-features.h
> +++ b/arch/x86/include/asm/disabled-features.h
> @@ -84,6 +84,5 @@
> #define DISABLED_MASK16 (DISABLE_PKU|DISABLE_OSPKE|DISABLE_LA57|DISABLE_UMIP)
> #define DISABLED_MASK17 0
> #define DISABLED_MASK18 0
> -#define DISABLED_MASK_CHECK BUILD_BUG_ON_ZERO(NCAPINTS != 19)
>
> #endif /* _ASM_X86_DISABLED_FEATURES_H */
> diff --git a/arch/x86/include/asm/required-features.h b/arch/x86/include/asm/required-features.h
> index 6847d85400a8..cb98b66d3e81 100644
> --- a/arch/x86/include/asm/required-features.h
> +++ b/arch/x86/include/asm/required-features.h
> @@ -1,7 +1,7 @@
> #ifndef _ASM_X86_REQUIRED_FEATURES_H
> #define _ASM_X86_REQUIRED_FEATURES_H
>
> -/* Define minimum CPUID feature set for kernel These bits are checked
> +/* Define minimum CPUID feature set for kernel. These bits are checked
> really early to actually display a visible error message before the
> kernel dies. Make sure to assign features to the proper mask!
>
> @@ -101,6 +101,5 @@
> #define REQUIRED_MASK16 0
> #define REQUIRED_MASK17 0
> #define REQUIRED_MASK18 0
> -#define REQUIRED_MASK_CHECK BUILD_BUG_ON_ZERO(NCAPINTS != 19)
>
> #endif /* _ASM_X86_REQUIRED_FEATURES_H */
> --
> 2.17.0
--
Regards/Gruss,
Boris.
Good mailing practices for 400: avoid top-posting and trim the reply.
On 8/27/19 12:41 AM, Borislav Petkov wrote:
>> #define DISABLED_MASK_BIT_SET(feature_bit) \
>> @@ -104,7 +103,6 @@ extern const char * const x86_bug_flags[NBUGINTS*32];
>> CHECK_BIT_IN_MASK_WORD(DISABLED_MASK, 16, feature_bit) || \
>> CHECK_BIT_IN_MASK_WORD(DISABLED_MASK, 17, feature_bit) || \
>> CHECK_BIT_IN_MASK_WORD(DISABLED_MASK, 18, feature_bit) || \
>> - DISABLED_MASK_CHECK || \
>> BUILD_BUG_ON_ZERO(NCAPINTS != 19))
>>
>> #define cpu_has(c, bit) \
> If you do a little bit of git archeology:
>
> $ git annotate arch/x86/include/asm/cpufeature.h
>
> after a while, you'll see that this was added in:
>
> 1e61f78baf89 ("x86/cpufeature: Make sure DISABLED/REQUIRED macros are updated")
>
> and then you could Cc Dave and ask what he was thinking then?
>
> Leaving in the rest for reference.
The point was that there are 5 files in the code that need to be changed
if you change NCAPINTS:
1. arch/x86/include/asm/required-features.h
2. arch/x86/include/asm/disabled-features.h
3. tools/arch/x86/include/asm/disabled-features.h
4. tools/arch/x86/include/asm/required-features.h
5. arch/x86/include/asm/cpufeature.h (2 sites)
Each of those sites has a compile-time check for NCAPINTS. The problem
is that the *-features.h code doesn't get compiled directly so a
BUILD_BUG_ON() doesn't work by itself. So, for the sites there, we put
it somewhere that *will* get compiled: the macros that actually check
the bits.
It looks weird, but the end effect is good: If you change NCAPINTS, you
get compile errors in 5 files and have to go edit those 5 files to fix
it. Your patch makes it easier to introduce errors and miss one of
those sites.
On Tue, Aug 27, 2019 at 09:33:11AM -0700, Dave Hansen wrote:
> The point was that there are 5 files in the code that need to be changed
> if you change NCAPINTS:
>
> 1. arch/x86/include/asm/required-features.h
> 2. arch/x86/include/asm/disabled-features.h
> 3. tools/arch/x86/include/asm/disabled-features.h
> 4. tools/arch/x86/include/asm/required-features.h
> 5. arch/x86/include/asm/cpufeature.h (2 sites)
>
> Each of those sites has a compile-time check for NCAPINTS. The problem
> is that the *-features.h code doesn't get compiled directly so a
> BUILD_BUG_ON() doesn't work by itself. So, for the sites there, we put
> it somewhere that *will* get compiled: the macros that actually check
> the bits.
>
> It looks weird, but the end effect is good: If you change NCAPINTS, you
> get compile errors in 5 files and have to go edit those 5 files to fix
> it. Your patch makes it easier to introduce errors and miss one of
> those sites.
... and we wouldn't want to debug any strange bugs from missing a case.
So, Cao, I wouldn't mind having the gist of that above somewhere around
there in a comment explicitly.
Thx.
--
Regards/Gruss,
Boris.
Good mailing practices for 400: avoid top-posting and trim the reply.
On 8/28/19 1:20 AM, Borislav Petkov wrote:
> On Tue, Aug 27, 2019 at 09:33:11AM -0700, Dave Hansen wrote:
>> The point was that there are 5 files in the code that need to be changed
>> if you change NCAPINTS:
>>
>> 1. arch/x86/include/asm/required-features.h
>> 2. arch/x86/include/asm/disabled-features.h
>> 3. tools/arch/x86/include/asm/disabled-features.h
>> 4. tools/arch/x86/include/asm/required-features.h
>> 5. arch/x86/include/asm/cpufeature.h (2 sites)
>>
>> Each of those sites has a compile-time check for NCAPINTS. The problem
>> is that the *-features.h code doesn't get compiled directly so a
>> BUILD_BUG_ON() doesn't work by itself. So, for the sites there, we put
>> it somewhere that *will* get compiled: the macros that actually check
>> the bits.
>>
>> It looks weird, but the end effect is good: If you change NCAPINTS, you
>> get compile errors in 5 files and have to go edit those 5 files to fix
>> it. Your patch makes it easier to introduce errors and miss one of
>> those sites.
>
> ... and we wouldn't want to debug any strange bugs from missing a case.
> So, Cao, I wouldn't mind having the gist of that above somewhere around
> there in a comment explicitly.
>
A subtle issue hard to detect by eyes. Sure, on the way.
--
Sincerely,
Cao jin