Re: [PATCH v1 3/3] x86/entry: Set FRED RSP0 on return to userspace instead of context switch

[Thomas Gleixner <tglx@linutronix.de>]

On Tue, Aug 06 2024 at 22:47, Xin Li wrote:
> --- a/arch/x86/include/asm/entry-common.h
> +++ b/arch/x86/include/asm/entry-common.h
> @@ -51,6 +51,11 @@ static inline void arch_exit_to_user_mode_prepare(struct pt_regs *regs,
>  		if (ti_work & _TIF_USER_RETURN_NOTIFY)
>  			fire_user_return_notifiers();
>  
> +		if (cpu_feature_enabled(X86_FEATURE_FRED) &&
> +		    (ti_work & _TIF_LOAD_USER_STATES))
> +			wrmsrns(MSR_IA32_FRED_RSP0,
> +				(unsigned long)task_stack_page(current) + THREAD_SIZE);

Eyes are bleeding. If you do the inline in patch 1 then this becomes

	if (cpu_feature_enabled(X86_FEATURE_FRED) && (ti_work & _TIF_LOAD_USER_STATES))
		wrmsrns(MSR_IA32_FRED_RSP0, (unsigned long)task_stack_page(current) + THREAD_SIZE);

which is in the 100 character limit and readable.

Though I wonder if this should not have a per CPU cache for that.

        if (cpu_feature_enabled(X86_FEATURE_FRED) {
        	unsigned long rsp0 = (unsigned long) task_stack_page(current) + THREAD_SIZE;

		if (__this_cpu_read(cpu_fred_rsp0) != rsp0) {
			wrmsrns(MSR_IA32_FRED_RSP0, fred_rsp0);
                        this_cpu_write(cpu_fred_rsp0, rsp0);
                }
        }	

Hmm?

>  		if (unlikely(ti_work & _TIF_IO_BITMAP))
>  			tss_update_io_bitmap();
>  
> diff --git a/arch/x86/include/asm/switch_to.h b/arch/x86/include/asm/switch_to.h
> index c3bd0c0758c9..a31ea544cc0e 100644
> --- a/arch/x86/include/asm/switch_to.h
> +++ b/arch/x86/include/asm/switch_to.h
> @@ -71,8 +71,7 @@ static inline void update_task_stack(struct task_struct *task)
>  	this_cpu_write(cpu_tss_rw.x86_tss.sp1, task->thread.sp0);
>  #else
>  	if (cpu_feature_enabled(X86_FEATURE_FRED)) {
> -		/* WRMSRNS is a baseline feature for FRED. */

This comment is wrong when using the alternative WRMSR fallback and
should be remove by the alternatives patch.

> -		wrmsrns(MSR_IA32_FRED_RSP0, (unsigned long)task_stack_page(task) + THREAD_SIZE);
> +		set_thread_flag(TIF_LOAD_USER_STATES);

Thanks,

        tglx