Re: [PATCH v5 13/30] arm64/sve: Core task context handling

[Dave Martin <Dave.Martin@arm.com>]

On Thu, Nov 09, 2017 at 05:16:40PM +0000, Alex Bennée wrote:
> 
> Dave Martin <Dave.Martin@arm.com> writes:
> 
> > This patch adds the core support for switching and managing the SVE
> > architectural state of user tasks.
> >
> > Calls to the existing FPSIMD low-level save/restore functions are
> > factored out as new functions task_fpsimd_{save,load}(), since SVE
> > now dynamically may or may not need to be handled at these points
> > depending on the kernel configuration, hardware features discovered
> > at boot, and the runtime state of the task.  To make these
> > decisions as fast as possible, const cpucaps are used where
> > feasible, via the system_supports_sve() helper.
> >
> > The SVE registers are only tracked for threads that have explicitly
> > used SVE, indicated by the new thread flag TIF_SVE.  Otherwise, the
> > FPSIMD view of the architectural state is stored in
> > thread.fpsimd_state as usual.
> >
> > When in use, the SVE registers are not stored directly in
> > thread_struct due to their potentially large and variable size.
> > Because the task_struct slab allocator must be configured very
> > early during kernel boot, it is also tricky to configure it
> > correctly to match the maximum vector length provided by the
> > hardware, since this depends on examining secondary CPUs as well as
> > the primary.  Instead, a pointer sve_state in thread_struct points
> > to a dynamically allocated buffer containing the SVE register data,
> > and code is added to allocate and free this buffer at appropriate
> > times.
> >
> > TIF_SVE is set when taking an SVE access trap from userspace, if
> > suitable hardware support has been detected.  This enables SVE for
> > the thread: a subsequent return to userspace will disable the trap
> > accordingly.  If such a trap is taken without sufficient system-
> > wide hardware support, SIGILL is sent to the thread instead as if
> > an undefined instruction had been executed: this may happen if
> > userspace tries to use SVE in a system where not all CPUs support
> > it for example.
> >
> > The kernel will clear TIF_SVE and disable SVE for the thread
> > whenever an explicit syscall is made by userspace.  For backwards
> > compatibility reasons and conformance with the spirit of the base
> > AArch64 procedure call standard, the subset of the SVE register
> > state that aliases the FPSIMD registers is still preserved across a
> > syscall even if this happens.  The remainder of the SVE register
> > state logically becomes zero at syscall entry, though the actual
> > zeroing work is currently deferred until the thread next tries to
> > use SVE, causing another trap to the kernel.  This implementation
> > is suboptimal: in the future, the fastpath case may be optimised
> > to zero the registers in-place and leave SVE enabled for the task,
> > where beneficial.
> >
> > TIF_SVE is also cleared in the following slowpath cases, which are
> > taken as reasonable hints that the task may no longer use SVE:
> >  * exec
> >  * fork and clone
> >
> > Code is added to sync data between thread.fpsimd_state and
> > thread.sve_state whenever enabling/disabling SVE, in a manner
> > consistent with the SVE architectural programmer's model.
> >
> > Signed-off-by: Dave Martin <Dave.Martin@arm.com>
> > Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>
> > Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > Cc: Alex Bennée <alex.bennee@linaro.org>
> >
> > ---
> >
> > Kept Catalin's Reviewed-by, since this is a trivial change.
> >
> > Changes since v4
> > ----------------
> >
> > Miscellaneous:
> >
> >  * Mark do_sve_acc() as asmlinkage.
> >
> >    (Semantic correctness only; no functional impact.)

[...]

> > diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S
> > index f5e851e..56e848f 100644
> > --- a/arch/arm64/kernel/entry.S
> > +++ b/arch/arm64/kernel/entry.S
> > @@ -607,6 +607,8 @@ el0_sync:
> >  	b.eq	el0_ia
> >  	cmp	x24, #ESR_ELx_EC_FP_ASIMD	// FP/ASIMD access
> >  	b.eq	el0_fpsimd_acc
> > +	cmp	x24, #ESR_ELx_EC_SVE		// SVE access
> > +	b.eq	el0_sve_acc
> 
> I'm guessing there is a point that this long chain of cmp instructions
> is better handled with a jump table? One for the maintainer though I
> guess?

Probably it would be worth refactoring this at some point.

There's a tradeoff between the length of this list the extra D-cache
and/or branch miss(es) that might result from using a table.

The optimimum approach would be microarchitecture dependent, but I
suspect a good compromise would be to profile this, pick the few most
common / most latency sensitive exception types and keep those as
compare-and-branch, deferring the remainder to a table lookup.

I had a vague plan to take a look at it, but for this series is
was very much in "nice-to-have" territory.

> >  	cmp	x24, #ESR_ELx_EC_FP_EXC64	// FP/ASIMD exception
> >  	b.eq	el0_fpsimd_exc
> >  	cmp	x24, #ESR_ELx_EC_SYS64		// configurable trap
> > @@ -658,6 +660,7 @@ el0_svc_compat:
> >  	/*
> >  	 * AArch32 syscall handling
> >  	 */
> > +	ldr	x16, [tsk, #TSK_TI_FLAGS]	// load thread flags
> >  	adrp	stbl, compat_sys_call_table	// load compat syscall table pointer
> >  	mov	wscno, w7			// syscall number in w7 (r7)
> >  	mov     wsc_nr, #__NR_compat_syscalls

[...]

> > @@ -835,16 +848,36 @@ ENDPROC(ret_to_user)
> >   */
> >  	.align	6
> >  el0_svc:
> > +	ldr	x16, [tsk, #TSK_TI_FLAGS]	// load thread flags
> >  	adrp	stbl, sys_call_table		// load syscall table pointer
> >  	mov	wscno, w8			// syscall number in w8
> >  	mov	wsc_nr, #__NR_syscalls
> > +
> > +#ifndef CONFIG_ARM64_SVE
> > +	b	el0_svc_naked
> 
> Hmm we've hoisted the ldr x16, [tsk, #TSK_TI_FLAGS] out of el0_svc_naked
> but we'll still be testing the bit when CONFIG_ARM64_SVE isn't enabled?

Where?  In this patch it's #ifdef'd out.  In "Detect SVE and activate
runtime support" this is converted to an asm alternative, so this should
reduce to a statically predictable branch when CONFIG_ARM64_SVE=y but
SVE support is not detected.

> I'm not clear why you couldn't keep that where it was?

Catalin wasn't keen on the duplication of work reading and writing the
thread flags, so I moved it to the common path.

> 
> > +#else
> > +	tbz	x16, #TIF_SVE, el0_svc_naked	// Skip unless TIF_SVE set:
> > +	bic	x16, x16, #_TIF_SVE		// discard SVE state
> > +	str	x16, [tsk, #TSK_TI_FLAGS]
> > +
> > +	/*
> > +	 * 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:
> > +	 */
> > +	mrs	x9, cpacr_el1
> > +	bic	x9, x9, #CPACR_EL1_ZEN_EL0EN	// disable SVE for el0
> > +	msr	cpacr_el1, x9			// synchronised by eret to el0
> > +#endif /* CONFIG_ARM64_SVE */
> > +
> >  el0_svc_naked:					// compat entry point
> >  	stp	x0, xscno, [sp, #S_ORIG_X0]	// save the original x0 and syscall number
> >  	enable_dbg_and_irq
> >  	ct_user_exit 1
> >
> > -	ldr	x16, [tsk, #TSK_TI_FLAGS]	// check for syscall hooks
> > -	tst	x16, #_TIF_SYSCALL_WORK
> > +	tst	x16, #_TIF_SYSCALL_WORK		// check for syscall hooks
> >  	b.ne	__sys_trace
> >  	cmp     wscno, wsc_nr			// check upper syscall limit
> >  	b.hs	ni_sys

[...]

> > diff --git a/arch/arm64/kernel/traps.c b/arch/arm64/kernel/traps.c
> > index 18c0290..9d3c7f0 100644
> > --- a/arch/arm64/kernel/traps.c
> > +++ b/arch/arm64/kernel/traps.c
> > @@ -310,8 +310,8 @@ static int call_undef_hook(struct pt_regs *regs)
> >  	return fn ? fn(regs, instr) : 1;
> >  }
> >
> > -static void force_signal_inject(int signal, int code, struct pt_regs *regs,
> > -				unsigned long address)
> > +void force_signal_inject(int signal, int code, struct pt_regs *regs,
> > +			 unsigned long address)
> >  {
> >  	siginfo_t info;
> >  	void __user *pc = (void __user *)instruction_pointer(regs);
> > @@ -325,7 +325,7 @@ static void force_signal_inject(int signal, int code, struct pt_regs *regs,
> >  		desc = "illegal memory access";
> >  		break;
> >  	default:
> > -		desc = "bad mode";
> > +		desc = "unknown or unrecoverable error";
> >  		break;
> >  	}
> 
> Is this a separate trivial clean-up patch? It seems like you should
> handle SIGKILL explicitly?

I considered it part of this patch, since this function is not currently
used elsewhere.

I only expect this path to be followed as a catch-all for BUG() like
conditions that can be contained by killing the user task.  Printing
out a super-descriptive message didn't seem appropriate, but "bad mode"
is especially opaque.  I think that was a paste from arch/arm -- AArch64
doesn't have "modes" as such.

Cheers
---Dave