Re: [PATCH v2 2/8] x86/enter: MACROS to set/clear IBRS

Re: [PATCH v2 2/8] x86/enter: MACROS to set/clear IBRS

[Tim Chen]

On 01/06/2018 03:05 PM, Mark Marshall wrote:
> Hi.
> 
> (I've only just subscribed and can't work out how to reply to a message from before I subscribed (on my phone), sorry)
> 
> In the macro WRMSR_ASM you seem to have lost the wrmsr?
> 
>

Yes.  That's a bug noticed by Thomas and I'll fix for update to IBRS patches.

Tim

Re: [PATCH v2 2/8] x86/enter: MACROS to set/clear IBRS

[Tim Chen]

On 01/07/2018 04:03 AM, Borislav Petkov wrote:

> 
> WRMSR as a name is good enough.
> 

It alias with wrmsr instruction and didn't
build correctly. So I'll keep it as WRMSR_ASM.

Tim

Re: [PATCH v2 2/8] x86/enter: MACROS to set/clear IBRS

[Borislav Petkov]

On Sun, Jan 07, 2018 at 09:12:21AM -0800, Tim Chen wrote:
> Currently we are not using other bits.
> When the time comes that we have other bits in this MSR used, we will
> change this.

Then please write in a short comment above it that it is ok that the
previous MSR contents get overwritten.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

Re: [PATCH v2 2/8] x86/enter: MACROS to set/clear IBRS

[Tim Chen]

On 01/07/2018 04:03 AM, Borislav Petkov wrote:
> On Fri, Jan 05, 2018 at 06:12:17PM -0800, Tim Chen wrote:
> 
>> Subject: Re: [PATCH v2 2/8] x86/enter: MACROS to set/clear IBRS
> 
> Your subject needs to have a verb and not scream:
> 
> Subject: [PATCH v2 2/8] x86/entry: Add macros to set/clear IBRS
> 
>> Create macros to control IBRS.  Use these macros to enable IBRS on kernel entry
>> paths and disable IBRS on kernel exit paths.
>>
>> The registers rax, rcx and rdx are touched when controlling IBRS
>> so they need to be saved when they can't be clobbered.
>>
>> Signed-off-by: Tim Chen <tim.c.chen@linux.intel.com>
>> ---
>>  arch/x86/entry/calling.h | 74 ++++++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 74 insertions(+)
>>
>> diff --git a/arch/x86/entry/calling.h b/arch/x86/entry/calling.h
>> index 45a63e0..09c870d 100644
>> --- a/arch/x86/entry/calling.h
>> +++ b/arch/x86/entry/calling.h
>> @@ -6,6 +6,8 @@
>>  #include <asm/percpu.h>
>>  #include <asm/asm-offsets.h>
>>  #include <asm/processor-flags.h>
>> +#include <asm/msr-index.h>
>> +#include <asm/cpufeatures.h>
>>  
>>  /*
>>  
>> @@ -347,3 +349,75 @@ For 32-bit we have the following conventions - kernel is built with
>>  .Lafter_call_\@:
>>  #endif
>>  .endm
>> +
>> +/*
>> + * IBRS related macros
>> + */
>> +.macro PUSH_MSR_REGS
>> +	pushq	%rax
>> +	pushq	%rcx
>> +	pushq	%rdx
>> +.endm
>> +
>> +.macro POP_MSR_REGS
>> +	popq	%rdx
>> +	popq	%rcx
>> +	popq	%rax
>> +.endm
>> +
>> +.macro WRMSR_ASM msr_nr:req eax_val:req
> 
> WRMSR as a name is good enough.
> 
> Also, you need edx_val:req too in case we decide to reuse that macro
> for something else later. Which I'm pretty sure we will, once it is out
> there.
> 
>> +	movl	\msr_nr, %ecx
>> +	movl	$0, %edx
> 
> ... and then
> 
> 	movl	\edx_val, %edx
> 
>> +	movl	\eax_val, %eax
>> +.endm
>> +
>> +.macro ENABLE_IBRS
>> +	ALTERNATIVE "jmp .Lskip_\@", "", X86_FEATURE_SPEC_CTRL
>> +	PUSH_MSR_REGS
>> +	WRMSR_ASM $MSR_IA32_SPEC_CTRL, $SPEC_CTRL_FEATURE_ENABLE_IBRS
> 
> This is overwriting the previous contents of the MSR. You need to read
> it and OR-in its bits [63:2] with SPEC_CTRL_FEATURE_ENABLE_IBRS and
> clear bit 0.
> 
> Unless the rest of this MSR is not going to be used for anything else.
> Then you're fine.
> 

Currently we are not using other bits.
When the time comes that we have other bits in this MSR used, we will
change this.

Tim

Re: [PATCH v2 2/8] x86/enter: MACROS to set/clear IBRS

[Borislav Petkov]

On Fri, Jan 05, 2018 at 06:12:17PM -0800, Tim Chen wrote:

> Subject: Re: [PATCH v2 2/8] x86/enter: MACROS to set/clear IBRS

Your subject needs to have a verb and not scream:

Subject: [PATCH v2 2/8] x86/entry: Add macros to set/clear IBRS

> Create macros to control IBRS.  Use these macros to enable IBRS on kernel entry
> paths and disable IBRS on kernel exit paths.
> 
> The registers rax, rcx and rdx are touched when controlling IBRS
> so they need to be saved when they can't be clobbered.
> 
> Signed-off-by: Tim Chen <tim.c.chen@linux.intel.com>
> ---
>  arch/x86/entry/calling.h | 74 ++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 74 insertions(+)
> 
> diff --git a/arch/x86/entry/calling.h b/arch/x86/entry/calling.h
> index 45a63e0..09c870d 100644
> --- a/arch/x86/entry/calling.h
> +++ b/arch/x86/entry/calling.h
> @@ -6,6 +6,8 @@
>  #include <asm/percpu.h>
>  #include <asm/asm-offsets.h>
>  #include <asm/processor-flags.h>
> +#include <asm/msr-index.h>
> +#include <asm/cpufeatures.h>
>  
>  /*
>  
> @@ -347,3 +349,75 @@ For 32-bit we have the following conventions - kernel is built with
>  .Lafter_call_\@:
>  #endif
>  .endm
> +
> +/*
> + * IBRS related macros
> + */
> +.macro PUSH_MSR_REGS
> +	pushq	%rax
> +	pushq	%rcx
> +	pushq	%rdx
> +.endm
> +
> +.macro POP_MSR_REGS
> +	popq	%rdx
> +	popq	%rcx
> +	popq	%rax
> +.endm
> +
> +.macro WRMSR_ASM msr_nr:req eax_val:req

WRMSR as a name is good enough.

Also, you need edx_val:req too in case we decide to reuse that macro
for something else later. Which I'm pretty sure we will, once it is out
there.

> +	movl	\msr_nr, %ecx
> +	movl	$0, %edx

... and then

	movl	\edx_val, %edx

> +	movl	\eax_val, %eax
> +.endm
> +
> +.macro ENABLE_IBRS
> +	ALTERNATIVE "jmp .Lskip_\@", "", X86_FEATURE_SPEC_CTRL
> +	PUSH_MSR_REGS
> +	WRMSR_ASM $MSR_IA32_SPEC_CTRL, $SPEC_CTRL_FEATURE_ENABLE_IBRS

This is overwriting the previous contents of the MSR. You need to read
it and OR-in its bits [63:2] with SPEC_CTRL_FEATURE_ENABLE_IBRS and
clear bit 0.

Unless the rest of this MSR is not going to be used for anything else.
Then you're fine.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

[PATCH v2 2/8] x86/enter: MACROS to set/clear IBRS

[Tim Chen]

Create macros to control IBRS.  Use these macros to enable IBRS on kernel entry
paths and disable IBRS on kernel exit paths.

The registers rax, rcx and rdx are touched when controlling IBRS
so they need to be saved when they can't be clobbered.

Signed-off-by: Tim Chen <tim.c.chen@linux.intel.com>
---
 arch/x86/entry/calling.h | 74 ++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 74 insertions(+)

diff --git a/arch/x86/entry/calling.h b/arch/x86/entry/calling.h
index 45a63e0..09c870d 100644
--- a/arch/x86/entry/calling.h
+++ b/arch/x86/entry/calling.h
@@ -6,6 +6,8 @@
 #include <asm/percpu.h>
 #include <asm/asm-offsets.h>
 #include <asm/processor-flags.h>
+#include <asm/msr-index.h>
+#include <asm/cpufeatures.h>
 
 /*
 
@@ -347,3 +349,75 @@ For 32-bit we have the following conventions - kernel is built with
 .Lafter_call_\@:
 #endif
 .endm
+
+/*
+ * IBRS related macros
+ */
+
+.macro PUSH_MSR_REGS
+	pushq	%rax
+	pushq	%rcx
+	pushq	%rdx
+.endm
+
+.macro POP_MSR_REGS
+	popq	%rdx
+	popq	%rcx
+	popq	%rax
+.endm
+
+.macro WRMSR_ASM msr_nr:req eax_val:req
+	movl	\msr_nr, %ecx
+	movl	$0, %edx
+	movl	\eax_val, %eax
+.endm
+
+.macro ENABLE_IBRS
+	ALTERNATIVE "jmp .Lskip_\@", "", X86_FEATURE_SPEC_CTRL
+	PUSH_MSR_REGS
+	WRMSR_ASM $MSR_IA32_SPEC_CTRL, $SPEC_CTRL_FEATURE_ENABLE_IBRS
+	POP_MSR_REGS
+.Lskip_\@:
+.endm
+
+.macro DISABLE_IBRS
+	ALTERNATIVE "jmp .Lskip_\@", "", X86_FEATURE_SPEC_CTRL
+	PUSH_MSR_REGS
+	WRMSR_ASM $MSR_IA32_SPEC_CTRL, $SPEC_CTRL_FEATURE_DISABLE_IBRS
+	POP_MSR_REGS
+.Lskip_\@:
+.endm
+
+.macro ENABLE_IBRS_CLOBBER
+	ALTERNATIVE "jmp .Lskip_\@", "", X86_FEATURE_SPEC_CTRL
+	WRMSR_ASM $MSR_IA32_SPEC_CTRL, $SPEC_CTRL_FEATURE_ENABLE_IBRS
+.Lskip_\@:
+.endm
+
+.macro DISABLE_IBRS_CLOBBER
+	ALTERNATIVE "jmp .Lskip_\@", "", X86_FEATURE_SPEC_CTRL
+	WRMSR_ASM $MSR_IA32_SPEC_CTRL, $SPEC_CTRL_FEATURE_DISABLE_IBRS
+.Lskip_\@:
+.endm
+
+.macro ENABLE_IBRS_SAVE_AND_CLOBBER save_reg:req
+	ALTERNATIVE "jmp .Lskip_\@", "", X86_FEATURE_SPEC_CTRL
+	movl	$MSR_IA32_SPEC_CTRL, %ecx
+	rdmsr
+	movl	%eax, \save_reg
+
+	movl	$0, %edx
+	movl	$SPEC_CTRL_FEATURE_ENABLE_IBRS, %eax
+	wrmsr
+.Lskip_\@:
+.endm
+
+.macro RESTORE_IBRS_CLOBBER save_reg:req
+	ALTERNATIVE "jmp .Lskip_\@", "", X86_FEATURE_SPEC_CTRL
+	/* Set IBRS to the value saved in the save_reg */
+	movl    $MSR_IA32_SPEC_CTRL, %ecx
+	movl    $0, %edx
+	movl    \save_reg, %eax
+	wrmsr
+.Lskip_\@:
+.endm
-- 
2.9.4