Re: [PATCH v2 8/8] x86/fpu/xstate: Restore supervisor xstates for __fpu__restore_sig()

From: Borislav Petkov <bp@alien8.de>
To: Yu-cheng Yu <yu-cheng.yu@intel.com>
Cc: linux-kernel@vger.kernel.org, x86@kernel.org,
	"H. Peter Anvin" <hpa@zytor.com>,
	Thomas Gleixner <tglx@linutronix.de>,
	Ingo Molnar <mingo@redhat.com>,
	Dave Hansen <dave.hansen@linux.intel.com>,
	Tony Luck <tony.luck@intel.com>,
	Andy Lutomirski <luto@kernel.org>,
	Rik van Riel <riel@surriel.com>,
	"Ravi V. Shankar" <ravi.v.shankar@intel.com>,
	Sebastian Andrzej Siewior <bigeasy@linutronix.de>,
	Fenghua Yu <fenghua.yu@intel.com>,
	Peter Zijlstra <peterz@infradead.org>
Subject: Re: [PATCH v2 8/8] x86/fpu/xstate: Restore supervisor xstates for __fpu__restore_sig()
Date: Fri, 28 Feb 2020 13:17:24 +0100	[thread overview]
Message-ID: <20200228121724.GA25261@zn.tnic> (raw)
In-Reply-To: <77f3841a92df5d0c819699ee3612118d566b7445.camel@intel.com>

On Thu, Feb 27, 2020 at 02:52:12PM -0800, Yu-cheng Yu wrote:
> > So the code sets TIF_NEED_FPU_LOAD unconditionally, why are you changing
> > this?
> > 
> > Why don't you simply do:
> > 
> > 		set_thread_flag(TIF_NEED_FPU_LOAD);
> > 		fpregs_lock();
> > 		if (xfeatures_mask_supervisor())
> > 			copy_xregs_to_kernel(&fpu->state.xsave);
> > 		fpregs_unlock();
> 
> If TIF_NEED_FPU_LOAD is set, then xstates are already in the xsave buffer. 
> We can skip saving them again.

Ok, then pls use test_and_set_thread_flag().

Also, in talking to Sebastian about this on IRC, he raised a valid
concern: if we are going to save supervisor states here, then
copy_xregs_to_kernel() should better save *only* supervisor states
because we're not interested in the user states - they're going to be
overwritten with the states from the stack.

So copy_xregs_to_kernel() needs to learn about a second parameter called
@mask like copy_kernel_to_xregs().

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette