Re: [PATCH 10/18] arm64: convert native/compat syscall entry to C

[Mark Rutland <mark.rutland@arm.com>]

On Mon, May 14, 2018 at 03:43:36PM +0100, Dave Martin wrote:
> On Mon, May 14, 2018 at 12:58:05PM +0100, Mark Rutland wrote:
> > On Mon, May 14, 2018 at 12:07:30PM +0100, Dave Martin wrote:
> > > On Mon, May 14, 2018 at 10:46:32AM +0100, Mark Rutland wrote:
> > > > +{
> > > > +	if (!system_supports_sve())
> > > > +		return;
> > > > +
> > > > +	/*
> > > > +	 * task_fpsimd_load() won't be called to update CPACR_EL1 in
> > > > +	 * ret_to_user unless TIF_FOREIGN_FPSTATE is still set, which only
> > > > +	 * happens if a context switch or kernel_neon_begin() or context
> > > > +	 * modification (sigreturn, ptrace) intervenes.
> > > > +	 * So, ensure that CPACR_EL1 is already correct for the fast-path case.
> > > > +	 */
> > > > +	if (test_and_clear_thread_flag(TIF_SVE))
> > > > +		sve_user_disable();
> > > 
> > > sve_user_disable() is already inline, and incorporates the if()
> > > internally via sysreg_clear_set().
> > > 
> > > So, should this just be
> > > 
> > > 	clear_thread_flag(TIF_SVE);
> > > 	sve_user_disable();
> > 
> > Sure. That does mean we'll unconditionally read cpacr_el1, but I assume
> > you're happy with that. I'll note the difference in the commit message.
> 
> This is what the code does today, conditioned no system_supports_sve().
> 
> I'm assuming that reading CPACR_EL1 is cheap ... or have you come across
> counterexamples to that?

I have no data either way. :)

> > > > +}
> > > > +
> > > > +extern syscall_fn_t sys_call_table[];
> > > > +
> > > > +asmlinkage void el0_svc_handler(struct pt_regs *regs)
> > > > +{
> > > 
> > > if (system_supports_sve()) ?
> > > 
> > > > +	sve_user_disable();
> > > 
> > > Or should this be replaced by a call to sve_user_reset()?
> > > 
> > > I suspect the latter, since we do want to be clearing TIF_SVE here too.
> > 
> > Yes, this was mean to be sve_user_reset().
> 
> OK.  Just to be clear, I think there should be a system_supports_sve()
> check here (in case that wasn't obvious from my previous reply).

I understood that; the check is inside sve_user_reset(), which I had
mean to call here.

With your above comments, I now have the following:

static inline void sve_user_reset(void)
{
	if (!system_supports_sve())
		return;

	/*
	 * task_fpsimd_load() won't be called to update CPACR_EL1 in
	 * ret_to_user unless TIF_FOREIGN_FPSTATE is still set, which only
	 * happens if a context switch or kernel_neon_begin() or context
	 * modification (sigreturn, ptrace) intervenes.
	 * So, ensure that CPACR_EL1 is already correct for the fast-path case.
	 */
	clear_thread_flag(TIF_SVE);
	sve_user_disable();
}

asmlinkage void el0_svc_handler(struct pt_regs *regs)
{
	sve_user_reset();
	el0_svc_common(regs, regs->regs[8], __NR_syscalls, sys_call_table);
}

... which I think alleviates that concern?

Thanks,
Mark.