Re: [patch V2 07/28] x86/speculation: Reorganize speculation control MSRs update
On Sun, Nov 25, 2018 at 07:33:35PM +0100, Thomas Gleixner wrote:
> The logic to detect whether there's a change in the previous and next
> task's flag relevant to update speculation control MSRs are spread out
> across multiple functions.
>
> Consolidate all checks needed for updating speculation control MSRs into
> the new __speculation_ctrl_update() helper function.
>
> This makes it easy to pick the right speculation control MSR and the bits
> in the MSR that needs updating based on TIF flags changes.
>
> Originally-by: Thomas Lendacky
> Signed-off-by: Tim Chen
> Signed-off-by: Thomas Gleixner
Reviewed-by: Konrad Rzeszutek Wilk
.. and I also have two tiny comments below - feel free to
incorporate or not them in.
>
> ---
> arch/x86/kernel/process.c | 42 --
> 1 file changed, 32 insertions(+), 10 deletions(-)
>
> --- a/arch/x86/kernel/process.c
> +++ b/arch/x86/kernel/process.c
> @@ -397,25 +397,48 @@ static __always_inline void amd_set_ssb_
>
> static __always_inline void spec_ctrl_update_msr(unsigned long tifn)
> {
> - u64 msr = x86_spec_ctrl_base | ssbd_tif_to_spec_ctrl(tifn);
> + u64 msr = x86_spec_ctrl_base;
> +
> + /*
> + * If X86_FEATURE_SSBD is not set, the SSBD bit is not to be
> + * touched.
> + */
I had a bit of hard time parsing that. Could it perhaps be changed to:
"If X86_FEATURE_SSBD is off (not set), we MUST leave the SSBD bit alone"
> + if (static_cpu_has(X86_FEATURE_SSBD))
> + msr |= ssbd_tif_to_spec_ctrl(tifn);
>
> wrmsrl(MSR_IA32_SPEC_CTRL, msr);
> }
>
> -static __always_inline void __speculation_ctrl_update(unsigned long tifn)
> +/*
> + * Update the MSRs managing speculation control, during context switch.
> + *
> + * tifp: Previous task's thread flags
> + * tifn: Next task's thread flags
> + */
> +static __always_inline void __speculation_ctrl_update(unsigned long tifp,
> + unsigned long tifn)
> {
> - if (static_cpu_has(X86_FEATURE_VIRT_SSBD))
> - amd_set_ssb_virt_state(tifn);
> - else if (static_cpu_has(X86_FEATURE_LS_CFG_SSBD))
> - amd_set_core_ssb_state(tifn);
> - else
> + bool updmsr = false;
> +
> + /* If TIF_SSBD is different, select the proper mitigation method */
> + if ((tifp ^ tifn) & _TIF_SSBD) {
> + if (static_cpu_has(X86_FEATURE_VIRT_SSBD))
> + amd_set_ssb_virt_state(tifn);
> + else if (static_cpu_has(X86_FEATURE_LS_CFG_SSBD))
> + amd_set_core_ssb_state(tifn);
> + else if (static_cpu_has(X86_FEATURE_SSBD))
> + updmsr = true;
^
Nothing really big, but you have an extra space here.
> + }
> +
> + if (updmsr)
> spec_ctrl_update_msr(tifn);
> }
>
> void speculation_ctrl_update(unsigned long tif)
> {
> + /* Forced update. Make sure all relevant TIF flags are different */
> preempt_disable();
> - __speculation_ctrl_update(tif);
> + __speculation_ctrl_update(~tif, tif);
> preempt_enable();
> }
>
> @@ -451,8 +474,7 @@ void __switch_to_xtra(struct task_struct
> if ((tifp ^ tifn) & _TIF_NOCPUID)
> set_cpuid_faulting(!!(tifn & _TIF_NOCPUID));
>
> - if ((tifp ^ tifn) & _TIF_SSBD)
> - __speculation_ctrl_update(tifn);
> + __speculation_ctrl_update(tifp, tifn);
> }
>
> /*
>
>
Re: [patch V2 07/28] x86/speculation: Reorganize speculation control MSRs update
On Sun, Nov 25, 2018 at 07:33:35PM +0100, Thomas Gleixner wrote:
> The logic to detect whether there's a change in the previous and next
> task's flag relevant to update speculation control MSRs are spread out
> across multiple functions.
>
> Consolidate all checks needed for updating speculation control MSRs into
> the new __speculation_ctrl_update() helper function.
>
> This makes it easy to pick the right speculation control MSR and the bits
> in the MSR that needs updating based on TIF flags changes.
>
> Originally-by: Thomas Lendacky
> Signed-off-by: Tim Chen
> Signed-off-by: Thomas Gleixner
Reviewed-by: Konrad Rzeszutek Wilk
.. and I also have two tiny comments below - feel free to
incorporate or not them in.
>
> ---
> arch/x86/kernel/process.c | 42 --
> 1 file changed, 32 insertions(+), 10 deletions(-)
>
> --- a/arch/x86/kernel/process.c
> +++ b/arch/x86/kernel/process.c
> @@ -397,25 +397,48 @@ static __always_inline void amd_set_ssb_
>
> static __always_inline void spec_ctrl_update_msr(unsigned long tifn)
> {
> - u64 msr = x86_spec_ctrl_base | ssbd_tif_to_spec_ctrl(tifn);
> + u64 msr = x86_spec_ctrl_base;
> +
> + /*
> + * If X86_FEATURE_SSBD is not set, the SSBD bit is not to be
> + * touched.
> + */
I had a bit of hard time parsing that. Could it perhaps be changed to:
"If X86_FEATURE_SSBD is off (not set), we MUST leave the SSBD bit alone"
> + if (static_cpu_has(X86_FEATURE_SSBD))
> + msr |= ssbd_tif_to_spec_ctrl(tifn);
>
> wrmsrl(MSR_IA32_SPEC_CTRL, msr);
> }
>
> -static __always_inline void __speculation_ctrl_update(unsigned long tifn)
> +/*
> + * Update the MSRs managing speculation control, during context switch.
> + *
> + * tifp: Previous task's thread flags
> + * tifn: Next task's thread flags
> + */
> +static __always_inline void __speculation_ctrl_update(unsigned long tifp,
> + unsigned long tifn)
> {
> - if (static_cpu_has(X86_FEATURE_VIRT_SSBD))
> - amd_set_ssb_virt_state(tifn);
> - else if (static_cpu_has(X86_FEATURE_LS_CFG_SSBD))
> - amd_set_core_ssb_state(tifn);
> - else
> + bool updmsr = false;
> +
> + /* If TIF_SSBD is different, select the proper mitigation method */
> + if ((tifp ^ tifn) & _TIF_SSBD) {
> + if (static_cpu_has(X86_FEATURE_VIRT_SSBD))
> + amd_set_ssb_virt_state(tifn);
> + else if (static_cpu_has(X86_FEATURE_LS_CFG_SSBD))
> + amd_set_core_ssb_state(tifn);
> + else if (static_cpu_has(X86_FEATURE_SSBD))
> + updmsr = true;
^
Nothing really big, but you have an extra space here.
> + }
> +
> + if (updmsr)
> spec_ctrl_update_msr(tifn);
> }
>
> void speculation_ctrl_update(unsigned long tif)
> {
> + /* Forced update. Make sure all relevant TIF flags are different */
> preempt_disable();
> - __speculation_ctrl_update(tif);
> + __speculation_ctrl_update(~tif, tif);
> preempt_enable();
> }
>
> @@ -451,8 +474,7 @@ void __switch_to_xtra(struct task_struct
> if ((tifp ^ tifn) & _TIF_NOCPUID)
> set_cpuid_faulting(!!(tifn & _TIF_NOCPUID));
>
> - if ((tifp ^ tifn) & _TIF_SSBD)
> - __speculation_ctrl_update(tifn);
> + __speculation_ctrl_update(tifp, tifn);
> }
>
> /*
>
>
Re: [patch V2 07/28] x86/speculation: Reorganize speculation control MSRs update
On Sun, Nov 25, 2018 at 07:33:35PM +0100, Thomas Gleixner wrote: > The logic to detect whether there's a change in the previous and next > task's flag relevant to update speculation control MSRs are spread out s/are/is/ > across multiple functions. > > Consolidate all checks needed for updating speculation control MSRs into > the new __speculation_ctrl_update() helper function. > > This makes it easy to pick the right speculation control MSR and the bits > in the MSR that needs updating based on TIF flags changes. s/needs/need/ -- Regards/Gruss, Boris. Good mailing practices for 400: avoid top-posting and trim the reply.
Re: [patch V2 07/28] x86/speculation: Reorganize speculation control MSRs update
On Sun, Nov 25, 2018 at 07:33:35PM +0100, Thomas Gleixner wrote: > The logic to detect whether there's a change in the previous and next > task's flag relevant to update speculation control MSRs are spread out s/are/is/ > across multiple functions. > > Consolidate all checks needed for updating speculation control MSRs into > the new __speculation_ctrl_update() helper function. > > This makes it easy to pick the right speculation control MSR and the bits > in the MSR that needs updating based on TIF flags changes. s/needs/need/ -- Regards/Gruss, Boris. Good mailing practices for 400: avoid top-posting and trim the reply.
[patch V2 07/28] x86/speculation: Reorganize speculation control MSRs update
The logic to detect whether there's a change in the previous and next
task's flag relevant to update speculation control MSRs are spread out
across multiple functions.
Consolidate all checks needed for updating speculation control MSRs into
the new __speculation_ctrl_update() helper function.
This makes it easy to pick the right speculation control MSR and the bits
in the MSR that needs updating based on TIF flags changes.
Originally-by: Thomas Lendacky
Signed-off-by: Tim Chen
Signed-off-by: Thomas Gleixner
---
arch/x86/kernel/process.c | 42 --
1 file changed, 32 insertions(+), 10 deletions(-)
--- a/arch/x86/kernel/process.c
+++ b/arch/x86/kernel/process.c
@@ -397,25 +397,48 @@ static __always_inline void amd_set_ssb_
static __always_inline void spec_ctrl_update_msr(unsigned long tifn)
{
- u64 msr = x86_spec_ctrl_base | ssbd_tif_to_spec_ctrl(tifn);
+ u64 msr = x86_spec_ctrl_base;
+
+ /*
+* If X86_FEATURE_SSBD is not set, the SSBD bit is not to be
+* touched.
+*/
+ if (static_cpu_has(X86_FEATURE_SSBD))
+ msr |= ssbd_tif_to_spec_ctrl(tifn);
wrmsrl(MSR_IA32_SPEC_CTRL, msr);
}
-static __always_inline void __speculation_ctrl_update(unsigned long tifn)
+/*
+ * Update the MSRs managing speculation control, during context switch.
+ *
+ * tifp: Previous task's thread flags
+ * tifn: Next task's thread flags
+ */
+static __always_inline void __speculation_ctrl_update(unsigned long tifp,
+ unsigned long tifn)
{
- if (static_cpu_has(X86_FEATURE_VIRT_SSBD))
- amd_set_ssb_virt_state(tifn);
- else if (static_cpu_has(X86_FEATURE_LS_CFG_SSBD))
- amd_set_core_ssb_state(tifn);
- else
+ bool updmsr = false;
+
+ /* If TIF_SSBD is different, select the proper mitigation method */
+ if ((tifp ^ tifn) & _TIF_SSBD) {
+ if (static_cpu_has(X86_FEATURE_VIRT_SSBD))
+ amd_set_ssb_virt_state(tifn);
+ else if (static_cpu_has(X86_FEATURE_LS_CFG_SSBD))
+ amd_set_core_ssb_state(tifn);
+ else if (static_cpu_has(X86_FEATURE_SSBD))
+ updmsr = true;
+ }
+
+ if (updmsr)
spec_ctrl_update_msr(tifn);
}
void speculation_ctrl_update(unsigned long tif)
{
+ /* Forced update. Make sure all relevant TIF flags are different */
preempt_disable();
- __speculation_ctrl_update(tif);
+ __speculation_ctrl_update(~tif, tif);
preempt_enable();
}
@@ -451,8 +474,7 @@ void __switch_to_xtra(struct task_struct
if ((tifp ^ tifn) & _TIF_NOCPUID)
set_cpuid_faulting(!!(tifn & _TIF_NOCPUID));
- if ((tifp ^ tifn) & _TIF_SSBD)
- __speculation_ctrl_update(tifn);
+ __speculation_ctrl_update(tifp, tifn);
}
/*
[patch V2 07/28] x86/speculation: Reorganize speculation control MSRs update
The logic to detect whether there's a change in the previous and next
task's flag relevant to update speculation control MSRs are spread out
across multiple functions.
Consolidate all checks needed for updating speculation control MSRs into
the new __speculation_ctrl_update() helper function.
This makes it easy to pick the right speculation control MSR and the bits
in the MSR that needs updating based on TIF flags changes.
Originally-by: Thomas Lendacky
Signed-off-by: Tim Chen
Signed-off-by: Thomas Gleixner
---
arch/x86/kernel/process.c | 42 --
1 file changed, 32 insertions(+), 10 deletions(-)
--- a/arch/x86/kernel/process.c
+++ b/arch/x86/kernel/process.c
@@ -397,25 +397,48 @@ static __always_inline void amd_set_ssb_
static __always_inline void spec_ctrl_update_msr(unsigned long tifn)
{
- u64 msr = x86_spec_ctrl_base | ssbd_tif_to_spec_ctrl(tifn);
+ u64 msr = x86_spec_ctrl_base;
+
+ /*
+* If X86_FEATURE_SSBD is not set, the SSBD bit is not to be
+* touched.
+*/
+ if (static_cpu_has(X86_FEATURE_SSBD))
+ msr |= ssbd_tif_to_spec_ctrl(tifn);
wrmsrl(MSR_IA32_SPEC_CTRL, msr);
}
-static __always_inline void __speculation_ctrl_update(unsigned long tifn)
+/*
+ * Update the MSRs managing speculation control, during context switch.
+ *
+ * tifp: Previous task's thread flags
+ * tifn: Next task's thread flags
+ */
+static __always_inline void __speculation_ctrl_update(unsigned long tifp,
+ unsigned long tifn)
{
- if (static_cpu_has(X86_FEATURE_VIRT_SSBD))
- amd_set_ssb_virt_state(tifn);
- else if (static_cpu_has(X86_FEATURE_LS_CFG_SSBD))
- amd_set_core_ssb_state(tifn);
- else
+ bool updmsr = false;
+
+ /* If TIF_SSBD is different, select the proper mitigation method */
+ if ((tifp ^ tifn) & _TIF_SSBD) {
+ if (static_cpu_has(X86_FEATURE_VIRT_SSBD))
+ amd_set_ssb_virt_state(tifn);
+ else if (static_cpu_has(X86_FEATURE_LS_CFG_SSBD))
+ amd_set_core_ssb_state(tifn);
+ else if (static_cpu_has(X86_FEATURE_SSBD))
+ updmsr = true;
+ }
+
+ if (updmsr)
spec_ctrl_update_msr(tifn);
}
void speculation_ctrl_update(unsigned long tif)
{
+ /* Forced update. Make sure all relevant TIF flags are different */
preempt_disable();
- __speculation_ctrl_update(tif);
+ __speculation_ctrl_update(~tif, tif);
preempt_enable();
}
@@ -451,8 +474,7 @@ void __switch_to_xtra(struct task_struct
if ((tifp ^ tifn) & _TIF_NOCPUID)
set_cpuid_faulting(!!(tifn & _TIF_NOCPUID));
- if ((tifp ^ tifn) & _TIF_SSBD)
- __speculation_ctrl_update(tifn);
+ __speculation_ctrl_update(tifp, tifn);
}
/*
