Re: [patch 20/24] x86/speculation: Split out TIF update

[Ingo Molnar]

* Thomas Gleixner <tglx@xxxxxxxxxxxxx> wrote:

> The update of the TIF_SSBD flag and the conditional speculation control MSR
> update is done in the ssb_prctl_set() function directly. The upcoming prctl
> support for controlling indirect branch speculation via STIBP needs the
> same mechanism.
> 
> Split the code out and make it reusable.
> 
> Signed-off-by: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
> ---
>  arch/x86/kernel/cpu/bugs.c |   31 +++++++++++++++++++------------
>  1 file changed, 19 insertions(+), 12 deletions(-)
> 
> --- a/arch/x86/kernel/cpu/bugs.c
> +++ b/arch/x86/kernel/cpu/bugs.c
> @@ -703,10 +703,25 @@ static void ssb_select_mitigation(void)
>  #undef pr_fmt
>  #define pr_fmt(fmt)     "Speculation prctl: " fmt
>  
> -static int ssb_prctl_set(struct task_struct *task, unsigned long ctrl)
> +static void task_update_spec_tif(struct task_struct *tsk, int tifbit, bool on)
>  {
>  	bool update;
>  
> +	if (on)
> +		update = !test_and_set_tsk_thread_flag(tsk, tifbit);
> +	else
> +		update = test_and_clear_tsk_thread_flag(tsk, tifbit);
> +
> +	/*
> +	 * If being set on non-current task, delay setting the CPU
> +	 * mitigation until it is scheduled next.
> +	 */
> +	if (tsk == current && update)
> +		speculation_ctrl_update_current();

Had to read this twice, because the comment and the code are both correct 
but deal with the inverse case. This might have helped:

	/*
	 * Immediately update the speculation MSRs on the current task,
	 * but for non-current tasks delay setting the CPU mitigation 
	 * until it is scheduled next.
	 */
	if (tsk == current && update)
		speculation_ctrl_update_current();

But can the target task ever be non-current here? I don't think so: the 
two callers are prctl and seccomp, and both are passing in the current 
task pointer.

If so then it would be nice to rename all these task variable names from 
'task' to 'curr' or such, to make this property apparent.

Then we can also remove the condition and the comment, and update 
unconditionally, and maybe add:

	WARN_ON_ONCE(curr != current);

... or so?

Thanks,

	Ingo