diff mbox series

[v8,09/33] x86/cpu: add X86_CR4_FRED macro

Message ID 20230410081438.1750-10-xin3.li@intel.com
State New
Headers
Series x86: enable FRED for x86-64 |

Commit Message

Li, Xin3 April 10, 2023, 8:14 a.m. UTC 
  From: "H. Peter Anvin (Intel)" <hpa@zytor.com>

Add X86_CR4_FRED macro for the FRED bit in %cr4. This bit should be a
pinned bit, not to be changed after initialization.

Signed-off-by: H. Peter Anvin (Intel) <hpa@zytor.com>
Tested-by: Shan Kang <shan.kang@intel.com>
Signed-off-by: Xin Li <xin3.li@intel.com>
---
 arch/x86/include/uapi/asm/processor-flags.h |  2 ++
 arch/x86/kernel/cpu/common.c                | 11 ++++++++---
 2 files changed, 10 insertions(+), 3 deletions(-)

Comments

Thomas Gleixner June 5, 2023, 12:01 p.m. UTC | #1 
On Mon, Apr 10 2023 at 01:14, Xin Li wrote:

> From: "H. Peter Anvin (Intel)" <hpa@zytor.com>
>
> Add X86_CR4_FRED macro for the FRED bit in %cr4. This bit should be a

s/should/must/ no?

> +/*
> + * These bits should not change their value after CPU init is finished.
> + * The explicit cast to unsigned long suppresses a warning on i386 for
> + * x86-64 only feature bits >= 32.
> + */
>  static const unsigned long cr4_pinned_mask =
> -	X86_CR4_SMEP | X86_CR4_SMAP | X86_CR4_UMIP |
> -	X86_CR4_FSGSBASE | X86_CR4_CET;
> +	(unsigned long)

That type cast is required because:

+#define X86_CR4_FRED		_BITULL(X86_CR4_FRED_BIT)

Bah. Fred is 64 bit only. So why defining this as 1ULL << 32
unconditionally and stripping the bit off on 32bit via the type cast?

#ifdef CONFIG_X86_64
#define X86_CR4_FRED		_BITUL(X86_CR4_FRED_BIT)
#else
#define X86_CR4_FRED		0
#endif

would be too obvious, right?

> +	(X86_CR4_SMEP | X86_CR4_SMAP | X86_CR4_UMIP |
> +	 X86_CR4_FSGSBASE | X86_CR4_CET | X86_CR4_FRED);

Thanks,

        tglx
H. Peter Anvin June 5, 2023, 5:06 p.m. UTC | #2 
On 6/5/23 05:01, Thomas Gleixner wrote:
> On Mon, Apr 10 2023 at 01:14, Xin Li wrote:
> 
>> From: "H. Peter Anvin (Intel)" <hpa@zytor.com>
>>
>> Add X86_CR4_FRED macro for the FRED bit in %cr4. This bit should be a
> 
> s/should/must/ no?
> 

Technically no bit "must" be set in the fixed bit variable, but it would 
obviously be insane not to. But it makes it a "should", both in 
dictionary and RFC 2119 definitions.

Incidentally, I strongly advice everyone to use the RFC 2119 definitions 
of technical requirement terms when possible.

	-hpa
H. Peter Anvin June 5, 2023, 5:19 p.m. UTC | #3 
On 6/5/23 05:01, Thomas Gleixner wrote:
> On Mon, Apr 10 2023 at 01:14, Xin Li wrote:
> 
>> From: "H. Peter Anvin (Intel)" <hpa@zytor.com>
>>
>> Add X86_CR4_FRED macro for the FRED bit in %cr4. This bit should be a
> 
> s/should/must/ no?
> 
>> +/*
>> + * These bits should not change their value after CPU init is finished.
>> + * The explicit cast to unsigned long suppresses a warning on i386 for
>> + * x86-64 only feature bits >= 32.
>> + */
>>   static const unsigned long cr4_pinned_mask =
>> -	X86_CR4_SMEP | X86_CR4_SMAP | X86_CR4_UMIP |
>> -	X86_CR4_FSGSBASE | X86_CR4_CET;
>> +	(unsigned long)
> 
> That type cast is required because:
> 
> +#define X86_CR4_FRED		_BITULL(X86_CR4_FRED_BIT)
> 
> Bah. Fred is 64 bit only. So why defining this as 1ULL << 32
> unconditionally and stripping the bit off on 32bit via the type cast?
> 
> #ifdef CONFIG_X86_64
> #define X86_CR4_FRED		_BITUL(X86_CR4_FRED_BIT)
> #else
> #define X86_CR4_FRED		0
> #endif
> 
> would be too obvious, right?
> 

It also adds an #ifdef mess to avoid a simple typecast. Is that the 
right tradeoff?

I'm not saying it is or it isn't, it is an open question.

	-hpa
diff mbox series

Patch

diff --git a/arch/x86/include/uapi/asm/processor-flags.h b/arch/x86/include/uapi/asm/processor-flags.h
index c47cc7f2feeb..a90933f1ff41 100644
--- a/arch/x86/include/uapi/asm/processor-flags.h
+++ b/arch/x86/include/uapi/asm/processor-flags.h
@@ -132,6 +132,8 @@ 
 #define X86_CR4_PKE		_BITUL(X86_CR4_PKE_BIT)
 #define X86_CR4_CET_BIT		23 /* enable Control-flow Enforcement Technology */
 #define X86_CR4_CET		_BITUL(X86_CR4_CET_BIT)
+#define X86_CR4_FRED_BIT	32 /* enable FRED kernel entry */
+#define X86_CR4_FRED		_BITULL(X86_CR4_FRED_BIT)
 
 /*
  * x86-64 Task Priority Register, CR8
diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
index 8cd4126d8253..e8cf6f4cfb52 100644
--- a/arch/x86/kernel/cpu/common.c
+++ b/arch/x86/kernel/cpu/common.c
@@ -412,10 +412,15 @@  static __always_inline void setup_umip(struct cpuinfo_x86 *c)
 	cr4_clear_bits(X86_CR4_UMIP);
 }
 
-/* These bits should not change their value after CPU init is finished. */
+/*
+ * These bits should not change their value after CPU init is finished.
+ * The explicit cast to unsigned long suppresses a warning on i386 for
+ * x86-64 only feature bits >= 32.
+ */
 static const unsigned long cr4_pinned_mask =
-	X86_CR4_SMEP | X86_CR4_SMAP | X86_CR4_UMIP |
-	X86_CR4_FSGSBASE | X86_CR4_CET;
+	(unsigned long)
+	(X86_CR4_SMEP | X86_CR4_SMAP | X86_CR4_UMIP |
+	 X86_CR4_FSGSBASE | X86_CR4_CET | X86_CR4_FRED);
 static DEFINE_STATIC_KEY_FALSE_RO(cr_pinning);
 static unsigned long cr4_pinned_bits __ro_after_init;