[v5,15/30] arm64/sve: Signal handling support

Message ID 1509465082-30427-16-git-send-email-Dave.Martin@arm.com
State New
Headers show
Series
  • ARM Scalable Vector Extension (SVE)
Related show

Commit Message

Dave Martin Oct. 31, 2017, 3:51 p.m.
This patch implements support for saving and restoring the SVE
registers around signals.

A fixed-size header struct sve_context is always included in the
signal frame encoding the thread's vector length at the time of
signal delivery, optionally followed by a variable-layout structure
encoding the SVE registers.

Because of the need to preserve backwards compatibility, the FPSIMD
view of the SVE registers is always dumped as a struct
fpsimd_context in the usual way, in addition to any sve_context.

The SVE vector registers are dumped in full, including bits 127:0
of each register which alias the corresponding FPSIMD vector
registers in the hardware.  To avoid any ambiguity about which
alias to restore during sigreturn, the kernel always restores bits
127:0 of each SVE vector register from the fpsimd_context in the
signal frame (which must be present): userspace needs to take this
into account if it wants to modify the SVE vector register contents
on return from a signal.

FPSR and FPCR, which are used by both FPSIMD and SVE, are not
included in sve_context because they are always present in
fpsimd_context anyway.

For signal delivery, a new helper
fpsimd_signal_preserve_current_state() is added to update _both_
the FPSIMD and SVE views in the task struct, to make it easier to
populate this information into the signal frame.  Because of the
redundancy between the two views of the state, only one is updated
otherwise.

Signed-off-by: Dave Martin <Dave.Martin@arm.com>
Cc: Alex Bennée <alex.bennee@linaro.org>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Will Deacon <will.deacon@arm.com>

---

**Dropped** Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>
(Non-trivial changes.)

Changes since v4
----------------

Requested by Will Deacon:

 * Fix inconsistent return semantics in restore_sve_fpsimd_context().

   Currently a nonzero return value from __copy_from_user() is passed
   back as-is to the caller or restore_sve_fpsimd_context(), rather than
   translating it do an error code as is done elsewhere.

   Callers of restore_sve_fpsimd_context() only care whether the return
   value is 0 or not, so this isn't a big deal, but it is more
   consistent to return proper error codes on failure in case we grow a
   use for them later.

   This patch returns -EFAULT in the __copy_from_user() error cases
   that weren't explicitly handled.

Miscellaneous:

 * Change inconsistent copy_to_user() calls to __copy_to_user() in
   preserve_sve_context().

   There are already __put_user_error() calls here.

   The whole extended signal frame is already checked for
   access_ok(VERIFY_WRITE) in get_sigframe().
---
 arch/arm64/include/asm/fpsimd.h |   1 +
 arch/arm64/kernel/fpsimd.c      |  55 ++++++++++---
 arch/arm64/kernel/signal.c      | 167 ++++++++++++++++++++++++++++++++++++++--
 arch/arm64/kernel/signal32.c    |   2 +-
 4 files changed, 206 insertions(+), 19 deletions(-)

Comments

Catalin Marinas Nov. 1, 2017, 2:33 p.m. | #1
On Tue, Oct 31, 2017 at 03:51:07PM +0000, Dave P Martin wrote:
> This patch implements support for saving and restoring the SVE
> registers around signals.
> 
> A fixed-size header struct sve_context is always included in the
> signal frame encoding the thread's vector length at the time of
> signal delivery, optionally followed by a variable-layout structure
> encoding the SVE registers.
> 
> Because of the need to preserve backwards compatibility, the FPSIMD
> view of the SVE registers is always dumped as a struct
> fpsimd_context in the usual way, in addition to any sve_context.
> 
> The SVE vector registers are dumped in full, including bits 127:0
> of each register which alias the corresponding FPSIMD vector
> registers in the hardware.  To avoid any ambiguity about which
> alias to restore during sigreturn, the kernel always restores bits
> 127:0 of each SVE vector register from the fpsimd_context in the
> signal frame (which must be present): userspace needs to take this
> into account if it wants to modify the SVE vector register contents
> on return from a signal.
> 
> FPSR and FPCR, which are used by both FPSIMD and SVE, are not
> included in sve_context because they are always present in
> fpsimd_context anyway.
> 
> For signal delivery, a new helper
> fpsimd_signal_preserve_current_state() is added to update _both_
> the FPSIMD and SVE views in the task struct, to make it easier to
> populate this information into the signal frame.  Because of the
> redundancy between the two views of the state, only one is updated
> otherwise.
> 
> Signed-off-by: Dave Martin <Dave.Martin@arm.com>
> Cc: Alex Bennée <alex.bennee@linaro.org>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Will Deacon <will.deacon@arm.com>

Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>
Alex Bennée Nov. 7, 2017, 1:22 p.m. | #2
Dave Martin <Dave.Martin@arm.com> writes:

> This patch implements support for saving and restoring the SVE
> registers around signals.
>
> A fixed-size header struct sve_context is always included in the
> signal frame encoding the thread's vector length at the time of
> signal delivery, optionally followed by a variable-layout structure
> encoding the SVE registers.
>
> Because of the need to preserve backwards compatibility, the FPSIMD
> view of the SVE registers is always dumped as a struct
> fpsimd_context in the usual way, in addition to any sve_context.
>
> The SVE vector registers are dumped in full, including bits 127:0
> of each register which alias the corresponding FPSIMD vector
> registers in the hardware.  To avoid any ambiguity about which
> alias to restore during sigreturn, the kernel always restores bits
> 127:0 of each SVE vector register from the fpsimd_context in the
> signal frame (which must be present): userspace needs to take this
> into account if it wants to modify the SVE vector register contents
> on return from a signal.
>
> FPSR and FPCR, which are used by both FPSIMD and SVE, are not
> included in sve_context because they are always present in
> fpsimd_context anyway.
>
> For signal delivery, a new helper
> fpsimd_signal_preserve_current_state() is added to update _both_
> the FPSIMD and SVE views in the task struct, to make it easier to
> populate this information into the signal frame.  Because of the
> redundancy between the two views of the state, only one is updated
> otherwise.
>
> Signed-off-by: Dave Martin <Dave.Martin@arm.com>
> Cc: Alex Bennée <alex.bennee@linaro.org>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Will Deacon <will.deacon@arm.com>

After adding SVE support for RISU I was able to confirm consistent
signal delivery with a decent context over multiple runs and after
hacking echo "32" > /proc/sys/abi/sve_default_vector_length to change
the default length the tests would fail (as expected).

Tested-by: Alex Bennée <alex.bennee@linaro.org>

>
> ---
>
> **Dropped** Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>
> (Non-trivial changes.)
>
> Changes since v4
> ----------------
>
> Requested by Will Deacon:
>
>  * Fix inconsistent return semantics in restore_sve_fpsimd_context().
>
>    Currently a nonzero return value from __copy_from_user() is passed
>    back as-is to the caller or restore_sve_fpsimd_context(), rather than
>    translating it do an error code as is done elsewhere.
>
>    Callers of restore_sve_fpsimd_context() only care whether the return
>    value is 0 or not, so this isn't a big deal, but it is more
>    consistent to return proper error codes on failure in case we grow a
>    use for them later.
>
>    This patch returns -EFAULT in the __copy_from_user() error cases
>    that weren't explicitly handled.
>
> Miscellaneous:
>
>  * Change inconsistent copy_to_user() calls to __copy_to_user() in
>    preserve_sve_context().
>
>    There are already __put_user_error() calls here.
>
>    The whole extended signal frame is already checked for
>    access_ok(VERIFY_WRITE) in get_sigframe().
> ---
>  arch/arm64/include/asm/fpsimd.h |   1 +
>  arch/arm64/kernel/fpsimd.c      |  55 ++++++++++---
>  arch/arm64/kernel/signal.c      | 167 ++++++++++++++++++++++++++++++++++++++--
>  arch/arm64/kernel/signal32.c    |   2 +-
>  4 files changed, 206 insertions(+), 19 deletions(-)
>
> diff --git a/arch/arm64/include/asm/fpsimd.h b/arch/arm64/include/asm/fpsimd.h
> index 5655fe1..9bbd74c 100644
> --- a/arch/arm64/include/asm/fpsimd.h
> +++ b/arch/arm64/include/asm/fpsimd.h
> @@ -63,6 +63,7 @@ extern void fpsimd_load_state(struct fpsimd_state *state);
>  extern void fpsimd_thread_switch(struct task_struct *next);
>  extern void fpsimd_flush_thread(void);
>
> +extern void fpsimd_signal_preserve_current_state(void);
>  extern void fpsimd_preserve_current_state(void);
>  extern void fpsimd_restore_current_state(void);
>  extern void fpsimd_update_current_state(struct fpsimd_state *state);
> diff --git a/arch/arm64/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c
> index c91be8e..e0b5ef5 100644
> --- a/arch/arm64/kernel/fpsimd.c
> +++ b/arch/arm64/kernel/fpsimd.c
> @@ -299,6 +299,32 @@ static void fpsimd_to_sve(struct task_struct *task)
>  		       sizeof(fst->vregs[i]));
>  }
>
> +/*
> + * Transfer the SVE state in task->thread.sve_state to
> + * task->thread.fpsimd_state.
> + *
> + * Task can be a non-runnable task, or current.  In the latter case,
> + * softirqs (and preemption) must be disabled.
> + * task->thread.sve_state must point to at least sve_state_size(task)
> + * bytes of allocated kernel memory.
> + * task->thread.sve_state must be up to date before calling this function.
> + */
> +static void sve_to_fpsimd(struct task_struct *task)
> +{
> +	unsigned int vq;
> +	void const *sst = task->thread.sve_state;
> +	struct fpsimd_state *fst = &task->thread.fpsimd_state;
> +	unsigned int i;
> +
> +	if (!system_supports_sve())
> +		return;
> +
> +	vq = sve_vq_from_vl(task->thread.sve_vl);
> +	for (i = 0; i < 32; ++i)
> +		memcpy(&fst->vregs[i], ZREG(sst, vq, i),
> +		       sizeof(fst->vregs[i]));
> +}
> +
>  #ifdef CONFIG_ARM64_SVE
>
>  /*
> @@ -500,9 +526,6 @@ void fpsimd_flush_thread(void)
>  /*
>   * Save the userland FPSIMD state of 'current' to memory, but only if the state
>   * currently held in the registers does in fact belong to 'current'
> - *
> - * Currently, SVE tasks can't exist, so just WARN in that case.
> - * Subsequent patches will add full SVE support here.
>   */
>  void fpsimd_preserve_current_state(void)
>  {
> @@ -510,16 +533,23 @@ void fpsimd_preserve_current_state(void)
>  		return;
>
>  	local_bh_disable();
> -
> -	if (!test_thread_flag(TIF_FOREIGN_FPSTATE))
> -		fpsimd_save_state(&current->thread.fpsimd_state);
> -
> -	WARN_ON_ONCE(test_and_clear_thread_flag(TIF_SVE));
> -
> +	task_fpsimd_save();
>  	local_bh_enable();
>  }
>
>  /*
> + * Like fpsimd_preserve_current_state(), but ensure that
> + * current->thread.fpsimd_state is updated so that it can be copied to
> + * the signal frame.
> + */
> +void fpsimd_signal_preserve_current_state(void)
> +{
> +	fpsimd_preserve_current_state();
> +	if (system_supports_sve() && test_thread_flag(TIF_SVE))
> +		sve_to_fpsimd(current);
> +}
> +
> +/*
>   * Load the userland FPSIMD state of 'current' from memory, but only if the
>   * FPSIMD state already held in the registers is /not/ the most recent FPSIMD
>   * state of 'current'
> @@ -554,7 +584,12 @@ void fpsimd_update_current_state(struct fpsimd_state *state)
>
>  	local_bh_disable();
>
> -	fpsimd_load_state(state);
> +	if (system_supports_sve() && test_thread_flag(TIF_SVE)) {
> +		current->thread.fpsimd_state = *state;
> +		fpsimd_to_sve(current);
> +	}
> +	task_fpsimd_load();
> +
>  	if (test_and_clear_thread_flag(TIF_FOREIGN_FPSTATE)) {
>  		struct fpsimd_state *st = &current->thread.fpsimd_state;
>
> diff --git a/arch/arm64/kernel/signal.c b/arch/arm64/kernel/signal.c
> index 4716729..64abf88 100644
> --- a/arch/arm64/kernel/signal.c
> +++ b/arch/arm64/kernel/signal.c
> @@ -63,6 +63,7 @@ struct rt_sigframe_user_layout {
>
>  	unsigned long fpsimd_offset;
>  	unsigned long esr_offset;
> +	unsigned long sve_offset;
>  	unsigned long extra_offset;
>  	unsigned long end_offset;
>  };
> @@ -179,9 +180,6 @@ static int preserve_fpsimd_context(struct fpsimd_context __user *ctx)
>  	struct fpsimd_state *fpsimd = &current->thread.fpsimd_state;
>  	int err;
>
> -	/* dump the hardware registers to the fpsimd_state structure */
> -	fpsimd_preserve_current_state();
> -
>  	/* copy the FP and status/control registers */
>  	err = __copy_to_user(ctx->vregs, fpsimd->vregs, sizeof(fpsimd->vregs));
>  	__put_user_error(fpsimd->fpsr, &ctx->fpsr, err);
> @@ -214,6 +212,8 @@ static int restore_fpsimd_context(struct fpsimd_context __user *ctx)
>  	__get_user_error(fpsimd.fpsr, &ctx->fpsr, err);
>  	__get_user_error(fpsimd.fpcr, &ctx->fpcr, err);
>
> +	clear_thread_flag(TIF_SVE);
> +
>  	/* load the hardware registers from the fpsimd_state structure */
>  	if (!err)
>  		fpsimd_update_current_state(&fpsimd);
> @@ -221,10 +221,118 @@ static int restore_fpsimd_context(struct fpsimd_context __user *ctx)
>  	return err ? -EFAULT : 0;
>  }
>
> +
>  struct user_ctxs {
>  	struct fpsimd_context __user *fpsimd;
> +	struct sve_context __user *sve;
>  };
>
> +#ifdef CONFIG_ARM64_SVE
> +
> +static int preserve_sve_context(struct sve_context __user *ctx)
> +{
> +	int err = 0;
> +	u16 reserved[ARRAY_SIZE(ctx->__reserved)];
> +	unsigned int vl = current->thread.sve_vl;
> +	unsigned int vq = 0;
> +
> +	if (test_thread_flag(TIF_SVE))
> +		vq = sve_vq_from_vl(vl);
> +
> +	memset(reserved, 0, sizeof(reserved));
> +
> +	__put_user_error(SVE_MAGIC, &ctx->head.magic, err);
> +	__put_user_error(round_up(SVE_SIG_CONTEXT_SIZE(vq), 16),
> +			 &ctx->head.size, err);
> +	__put_user_error(vl, &ctx->vl, err);
> +	BUILD_BUG_ON(sizeof(ctx->__reserved) != sizeof(reserved));
> +	err |= __copy_to_user(&ctx->__reserved, reserved, sizeof(reserved));
> +
> +	if (vq) {
> +		/*
> +		 * This assumes that the SVE state has already been saved to
> +		 * the task struct by calling preserve_fpsimd_context().
> +		 */
> +		err |= __copy_to_user((char __user *)ctx + SVE_SIG_REGS_OFFSET,
> +				      current->thread.sve_state,
> +				      SVE_SIG_REGS_SIZE(vq));
> +	}
> +
> +	return err ? -EFAULT : 0;
> +}
> +
> +static int restore_sve_fpsimd_context(struct user_ctxs *user)
> +{
> +	int err;
> +	unsigned int vq;
> +	struct fpsimd_state fpsimd;
> +	struct sve_context sve;
> +
> +	if (__copy_from_user(&sve, user->sve, sizeof(sve)))
> +		return -EFAULT;
> +
> +	if (sve.vl != current->thread.sve_vl)
> +		return -EINVAL;
> +
> +	if (sve.head.size <= sizeof(*user->sve)) {
> +		clear_thread_flag(TIF_SVE);
> +		goto fpsimd_only;
> +	}
> +
> +	vq = sve_vq_from_vl(sve.vl);
> +
> +	if (sve.head.size < SVE_SIG_CONTEXT_SIZE(vq))
> +		return -EINVAL;
> +
> +	/*
> +	 * Careful: we are about __copy_from_user() directly into
> +	 * thread.sve_state with preemption enabled, so protection is
> +	 * needed to prevent a racing context switch from writing stale
> +	 * registers back over the new data.
> +	 */
> +
> +	fpsimd_flush_task_state(current);
> +	barrier();
> +	/* From now, fpsimd_thread_switch() won't clear TIF_FOREIGN_FPSTATE */
> +
> +	set_thread_flag(TIF_FOREIGN_FPSTATE);
> +	barrier();
> +	/* From now, fpsimd_thread_switch() won't touch thread.sve_state */
> +
> +	sve_alloc(current);
> +	err = __copy_from_user(current->thread.sve_state,
> +			       (char __user const *)user->sve +
> +					SVE_SIG_REGS_OFFSET,
> +			       SVE_SIG_REGS_SIZE(vq));
> +	if (err)
> +		return -EFAULT;
> +
> +	set_thread_flag(TIF_SVE);
> +
> +fpsimd_only:
> +	/* copy the FP and status/control registers */
> +	/* restore_sigframe() already checked that user->fpsimd != NULL. */
> +	err = __copy_from_user(fpsimd.vregs, user->fpsimd->vregs,
> +			       sizeof(fpsimd.vregs));
> +	__get_user_error(fpsimd.fpsr, &user->fpsimd->fpsr, err);
> +	__get_user_error(fpsimd.fpcr, &user->fpsimd->fpcr, err);
> +
> +	/* load the hardware registers from the fpsimd_state structure */
> +	if (!err)
> +		fpsimd_update_current_state(&fpsimd);
> +
> +	return err ? -EFAULT : 0;
> +}
> +
> +#else /* ! CONFIG_ARM64_SVE */
> +
> +/* Turn any non-optimised out attempts to use these into a link error: */
> +extern int preserve_sve_context(void __user *ctx);
> +extern int restore_sve_fpsimd_context(struct user_ctxs *user);
> +
> +#endif /* ! CONFIG_ARM64_SVE */
> +
> +
>  static int parse_user_sigframe(struct user_ctxs *user,
>  			       struct rt_sigframe __user *sf)
>  {
> @@ -237,6 +345,7 @@ static int parse_user_sigframe(struct user_ctxs *user,
>  	char const __user *const sfp = (char const __user *)sf;
>
>  	user->fpsimd = NULL;
> +	user->sve = NULL;
>
>  	if (!IS_ALIGNED((unsigned long)base, 16))
>  		goto invalid;
> @@ -287,6 +396,19 @@ static int parse_user_sigframe(struct user_ctxs *user,
>  			/* ignore */
>  			break;
>
> +		case SVE_MAGIC:
> +			if (!system_supports_sve())
> +				goto invalid;
> +
> +			if (user->sve)
> +				goto invalid;
> +
> +			if (size < sizeof(*user->sve))
> +				goto invalid;
> +
> +			user->sve = (struct sve_context __user *)head;
> +			break;
> +
>  		case EXTRA_MAGIC:
>  			if (have_extra_context)
>  				goto invalid;
> @@ -363,9 +485,6 @@ static int parse_user_sigframe(struct user_ctxs *user,
>  	}
>
>  done:
> -	if (!user->fpsimd)
> -		goto invalid;
> -
>  	return 0;
>
>  invalid:
> @@ -399,8 +518,19 @@ static int restore_sigframe(struct pt_regs *regs,
>  	if (err == 0)
>  		err = parse_user_sigframe(&user, sf);
>
> -	if (err == 0)
> -		err = restore_fpsimd_context(user.fpsimd);
> +	if (err == 0) {
> +		if (!user.fpsimd)
> +			return -EINVAL;
> +
> +		if (user.sve) {
> +			if (!system_supports_sve())
> +				return -EINVAL;
> +
> +			err = restore_sve_fpsimd_context(&user);
> +		} else {
> +			err = restore_fpsimd_context(user.fpsimd);
> +		}
> +	}
>
>  	return err;
>  }
> @@ -459,6 +589,18 @@ static int setup_sigframe_layout(struct rt_sigframe_user_layout *user)
>  			return err;
>  	}
>
> +	if (system_supports_sve()) {
> +		unsigned int vq = 0;
> +
> +		if (test_thread_flag(TIF_SVE))
> +			vq = sve_vq_from_vl(current->thread.sve_vl);
> +
> +		err = sigframe_alloc(user, &user->sve_offset,
> +				     SVE_SIG_CONTEXT_SIZE(vq));
> +		if (err)
> +			return err;
> +	}
> +
>  	return sigframe_alloc_end(user);
>  }
>
> @@ -500,6 +642,13 @@ static int setup_sigframe(struct rt_sigframe_user_layout *user,
>  		__put_user_error(current->thread.fault_code, &esr_ctx->esr, err);
>  	}
>
> +	/* Scalable Vector Extension state, if present */
> +	if (system_supports_sve() && err == 0 && user->sve_offset) {
> +		struct sve_context __user *sve_ctx =
> +			apply_user_offset(user, user->sve_offset);
> +		err |= preserve_sve_context(sve_ctx);
> +	}
> +
>  	if (err == 0 && user->extra_offset) {
>  		char __user *sfp = (char __user *)user->sigframe;
>  		char __user *userp =
> @@ -599,6 +748,8 @@ static int setup_rt_frame(int usig, struct ksignal *ksig, sigset_t *set,
>  	struct rt_sigframe __user *frame;
>  	int err = 0;
>
> +	fpsimd_signal_preserve_current_state();
> +
>  	if (get_sigframe(&user, ksig, regs))
>  		return 1;
>
> diff --git a/arch/arm64/kernel/signal32.c b/arch/arm64/kernel/signal32.c
> index e09bf5d..22711ee 100644
> --- a/arch/arm64/kernel/signal32.c
> +++ b/arch/arm64/kernel/signal32.c
> @@ -239,7 +239,7 @@ static int compat_preserve_vfp_context(struct compat_vfp_sigframe __user *frame)
>  	 * Note that this also saves V16-31, which aren't visible
>  	 * in AArch32.
>  	 */
> -	fpsimd_preserve_current_state();
> +	fpsimd_signal_preserve_current_state();
>
>  	/* Place structure header on the stack */
>  	__put_user_error(magic, &frame->magic, err);


--
Alex Bennée
Dave Martin Nov. 8, 2017, 4:11 p.m. | #3
On Tue, Nov 07, 2017 at 01:22:33PM +0000, Alex Bennée wrote:
> 
> Dave Martin <Dave.Martin@arm.com> writes:
> 
> > This patch implements support for saving and restoring the SVE
> > registers around signals.
> >
> > A fixed-size header struct sve_context is always included in the
> > signal frame encoding the thread's vector length at the time of
> > signal delivery, optionally followed by a variable-layout structure
> > encoding the SVE registers.
> >
> > Because of the need to preserve backwards compatibility, the FPSIMD
> > view of the SVE registers is always dumped as a struct
> > fpsimd_context in the usual way, in addition to any sve_context.
> >
> > The SVE vector registers are dumped in full, including bits 127:0
> > of each register which alias the corresponding FPSIMD vector
> > registers in the hardware.  To avoid any ambiguity about which
> > alias to restore during sigreturn, the kernel always restores bits
> > 127:0 of each SVE vector register from the fpsimd_context in the
> > signal frame (which must be present): userspace needs to take this
> > into account if it wants to modify the SVE vector register contents
> > on return from a signal.
> >
> > FPSR and FPCR, which are used by both FPSIMD and SVE, are not
> > included in sve_context because they are always present in
> > fpsimd_context anyway.
> >
> > For signal delivery, a new helper
> > fpsimd_signal_preserve_current_state() is added to update _both_
> > the FPSIMD and SVE views in the task struct, to make it easier to
> > populate this information into the signal frame.  Because of the
> > redundancy between the two views of the state, only one is updated
> > otherwise.
> >
> > Signed-off-by: Dave Martin <Dave.Martin@arm.com>
> > Cc: Alex Bennée <alex.bennee@linaro.org>
> > Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > Cc: Will Deacon <will.deacon@arm.com>
> 
> After adding SVE support for RISU I was able to confirm consistent
> signal delivery with a decent context over multiple runs and after
> hacking echo "32" > /proc/sys/abi/sve_default_vector_length to change
> the default length the tests would fail (as expected).
> 
> Tested-by: Alex Bennée <alex.bennee@linaro.org>

Good to have confirmation of this, thanks

[...]

Cheers
---Dave

Patch

diff --git a/arch/arm64/include/asm/fpsimd.h b/arch/arm64/include/asm/fpsimd.h
index 5655fe1..9bbd74c 100644
--- a/arch/arm64/include/asm/fpsimd.h
+++ b/arch/arm64/include/asm/fpsimd.h
@@ -63,6 +63,7 @@  extern void fpsimd_load_state(struct fpsimd_state *state);
 extern void fpsimd_thread_switch(struct task_struct *next);
 extern void fpsimd_flush_thread(void);
 
+extern void fpsimd_signal_preserve_current_state(void);
 extern void fpsimd_preserve_current_state(void);
 extern void fpsimd_restore_current_state(void);
 extern void fpsimd_update_current_state(struct fpsimd_state *state);
diff --git a/arch/arm64/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c
index c91be8e..e0b5ef5 100644
--- a/arch/arm64/kernel/fpsimd.c
+++ b/arch/arm64/kernel/fpsimd.c
@@ -299,6 +299,32 @@  static void fpsimd_to_sve(struct task_struct *task)
 		       sizeof(fst->vregs[i]));
 }
 
+/*
+ * Transfer the SVE state in task->thread.sve_state to
+ * task->thread.fpsimd_state.
+ *
+ * Task can be a non-runnable task, or current.  In the latter case,
+ * softirqs (and preemption) must be disabled.
+ * task->thread.sve_state must point to at least sve_state_size(task)
+ * bytes of allocated kernel memory.
+ * task->thread.sve_state must be up to date before calling this function.
+ */
+static void sve_to_fpsimd(struct task_struct *task)
+{
+	unsigned int vq;
+	void const *sst = task->thread.sve_state;
+	struct fpsimd_state *fst = &task->thread.fpsimd_state;
+	unsigned int i;
+
+	if (!system_supports_sve())
+		return;
+
+	vq = sve_vq_from_vl(task->thread.sve_vl);
+	for (i = 0; i < 32; ++i)
+		memcpy(&fst->vregs[i], ZREG(sst, vq, i),
+		       sizeof(fst->vregs[i]));
+}
+
 #ifdef CONFIG_ARM64_SVE
 
 /*
@@ -500,9 +526,6 @@  void fpsimd_flush_thread(void)
 /*
  * Save the userland FPSIMD state of 'current' to memory, but only if the state
  * currently held in the registers does in fact belong to 'current'
- *
- * Currently, SVE tasks can't exist, so just WARN in that case.
- * Subsequent patches will add full SVE support here.
  */
 void fpsimd_preserve_current_state(void)
 {
@@ -510,16 +533,23 @@  void fpsimd_preserve_current_state(void)
 		return;
 
 	local_bh_disable();
-
-	if (!test_thread_flag(TIF_FOREIGN_FPSTATE))
-		fpsimd_save_state(&current->thread.fpsimd_state);
-
-	WARN_ON_ONCE(test_and_clear_thread_flag(TIF_SVE));
-
+	task_fpsimd_save();
 	local_bh_enable();
 }
 
 /*
+ * Like fpsimd_preserve_current_state(), but ensure that
+ * current->thread.fpsimd_state is updated so that it can be copied to
+ * the signal frame.
+ */
+void fpsimd_signal_preserve_current_state(void)
+{
+	fpsimd_preserve_current_state();
+	if (system_supports_sve() && test_thread_flag(TIF_SVE))
+		sve_to_fpsimd(current);
+}
+
+/*
  * Load the userland FPSIMD state of 'current' from memory, but only if the
  * FPSIMD state already held in the registers is /not/ the most recent FPSIMD
  * state of 'current'
@@ -554,7 +584,12 @@  void fpsimd_update_current_state(struct fpsimd_state *state)
 
 	local_bh_disable();
 
-	fpsimd_load_state(state);
+	if (system_supports_sve() && test_thread_flag(TIF_SVE)) {
+		current->thread.fpsimd_state = *state;
+		fpsimd_to_sve(current);
+	}
+	task_fpsimd_load();
+
 	if (test_and_clear_thread_flag(TIF_FOREIGN_FPSTATE)) {
 		struct fpsimd_state *st = &current->thread.fpsimd_state;
 
diff --git a/arch/arm64/kernel/signal.c b/arch/arm64/kernel/signal.c
index 4716729..64abf88 100644
--- a/arch/arm64/kernel/signal.c
+++ b/arch/arm64/kernel/signal.c
@@ -63,6 +63,7 @@  struct rt_sigframe_user_layout {
 
 	unsigned long fpsimd_offset;
 	unsigned long esr_offset;
+	unsigned long sve_offset;
 	unsigned long extra_offset;
 	unsigned long end_offset;
 };
@@ -179,9 +180,6 @@  static int preserve_fpsimd_context(struct fpsimd_context __user *ctx)
 	struct fpsimd_state *fpsimd = &current->thread.fpsimd_state;
 	int err;
 
-	/* dump the hardware registers to the fpsimd_state structure */
-	fpsimd_preserve_current_state();
-
 	/* copy the FP and status/control registers */
 	err = __copy_to_user(ctx->vregs, fpsimd->vregs, sizeof(fpsimd->vregs));
 	__put_user_error(fpsimd->fpsr, &ctx->fpsr, err);
@@ -214,6 +212,8 @@  static int restore_fpsimd_context(struct fpsimd_context __user *ctx)
 	__get_user_error(fpsimd.fpsr, &ctx->fpsr, err);
 	__get_user_error(fpsimd.fpcr, &ctx->fpcr, err);
 
+	clear_thread_flag(TIF_SVE);
+
 	/* load the hardware registers from the fpsimd_state structure */
 	if (!err)
 		fpsimd_update_current_state(&fpsimd);
@@ -221,10 +221,118 @@  static int restore_fpsimd_context(struct fpsimd_context __user *ctx)
 	return err ? -EFAULT : 0;
 }
 
+
 struct user_ctxs {
 	struct fpsimd_context __user *fpsimd;
+	struct sve_context __user *sve;
 };
 
+#ifdef CONFIG_ARM64_SVE
+
+static int preserve_sve_context(struct sve_context __user *ctx)
+{
+	int err = 0;
+	u16 reserved[ARRAY_SIZE(ctx->__reserved)];
+	unsigned int vl = current->thread.sve_vl;
+	unsigned int vq = 0;
+
+	if (test_thread_flag(TIF_SVE))
+		vq = sve_vq_from_vl(vl);
+
+	memset(reserved, 0, sizeof(reserved));
+
+	__put_user_error(SVE_MAGIC, &ctx->head.magic, err);
+	__put_user_error(round_up(SVE_SIG_CONTEXT_SIZE(vq), 16),
+			 &ctx->head.size, err);
+	__put_user_error(vl, &ctx->vl, err);
+	BUILD_BUG_ON(sizeof(ctx->__reserved) != sizeof(reserved));
+	err |= __copy_to_user(&ctx->__reserved, reserved, sizeof(reserved));
+
+	if (vq) {
+		/*
+		 * This assumes that the SVE state has already been saved to
+		 * the task struct by calling preserve_fpsimd_context().
+		 */
+		err |= __copy_to_user((char __user *)ctx + SVE_SIG_REGS_OFFSET,
+				      current->thread.sve_state,
+				      SVE_SIG_REGS_SIZE(vq));
+	}
+
+	return err ? -EFAULT : 0;
+}
+
+static int restore_sve_fpsimd_context(struct user_ctxs *user)
+{
+	int err;
+	unsigned int vq;
+	struct fpsimd_state fpsimd;
+	struct sve_context sve;
+
+	if (__copy_from_user(&sve, user->sve, sizeof(sve)))
+		return -EFAULT;
+
+	if (sve.vl != current->thread.sve_vl)
+		return -EINVAL;
+
+	if (sve.head.size <= sizeof(*user->sve)) {
+		clear_thread_flag(TIF_SVE);
+		goto fpsimd_only;
+	}
+
+	vq = sve_vq_from_vl(sve.vl);
+
+	if (sve.head.size < SVE_SIG_CONTEXT_SIZE(vq))
+		return -EINVAL;
+
+	/*
+	 * Careful: we are about __copy_from_user() directly into
+	 * thread.sve_state with preemption enabled, so protection is
+	 * needed to prevent a racing context switch from writing stale
+	 * registers back over the new data.
+	 */
+
+	fpsimd_flush_task_state(current);
+	barrier();
+	/* From now, fpsimd_thread_switch() won't clear TIF_FOREIGN_FPSTATE */
+
+	set_thread_flag(TIF_FOREIGN_FPSTATE);
+	barrier();
+	/* From now, fpsimd_thread_switch() won't touch thread.sve_state */
+
+	sve_alloc(current);
+	err = __copy_from_user(current->thread.sve_state,
+			       (char __user const *)user->sve +
+					SVE_SIG_REGS_OFFSET,
+			       SVE_SIG_REGS_SIZE(vq));
+	if (err)
+		return -EFAULT;
+
+	set_thread_flag(TIF_SVE);
+
+fpsimd_only:
+	/* copy the FP and status/control registers */
+	/* restore_sigframe() already checked that user->fpsimd != NULL. */
+	err = __copy_from_user(fpsimd.vregs, user->fpsimd->vregs,
+			       sizeof(fpsimd.vregs));
+	__get_user_error(fpsimd.fpsr, &user->fpsimd->fpsr, err);
+	__get_user_error(fpsimd.fpcr, &user->fpsimd->fpcr, err);
+
+	/* load the hardware registers from the fpsimd_state structure */
+	if (!err)
+		fpsimd_update_current_state(&fpsimd);
+
+	return err ? -EFAULT : 0;
+}
+
+#else /* ! CONFIG_ARM64_SVE */
+
+/* Turn any non-optimised out attempts to use these into a link error: */
+extern int preserve_sve_context(void __user *ctx);
+extern int restore_sve_fpsimd_context(struct user_ctxs *user);
+
+#endif /* ! CONFIG_ARM64_SVE */
+
+
 static int parse_user_sigframe(struct user_ctxs *user,
 			       struct rt_sigframe __user *sf)
 {
@@ -237,6 +345,7 @@  static int parse_user_sigframe(struct user_ctxs *user,
 	char const __user *const sfp = (char const __user *)sf;
 
 	user->fpsimd = NULL;
+	user->sve = NULL;
 
 	if (!IS_ALIGNED((unsigned long)base, 16))
 		goto invalid;
@@ -287,6 +396,19 @@  static int parse_user_sigframe(struct user_ctxs *user,
 			/* ignore */
 			break;
 
+		case SVE_MAGIC:
+			if (!system_supports_sve())
+				goto invalid;
+
+			if (user->sve)
+				goto invalid;
+
+			if (size < sizeof(*user->sve))
+				goto invalid;
+
+			user->sve = (struct sve_context __user *)head;
+			break;
+
 		case EXTRA_MAGIC:
 			if (have_extra_context)
 				goto invalid;
@@ -363,9 +485,6 @@  static int parse_user_sigframe(struct user_ctxs *user,
 	}
 
 done:
-	if (!user->fpsimd)
-		goto invalid;
-
 	return 0;
 
 invalid:
@@ -399,8 +518,19 @@  static int restore_sigframe(struct pt_regs *regs,
 	if (err == 0)
 		err = parse_user_sigframe(&user, sf);
 
-	if (err == 0)
-		err = restore_fpsimd_context(user.fpsimd);
+	if (err == 0) {
+		if (!user.fpsimd)
+			return -EINVAL;
+
+		if (user.sve) {
+			if (!system_supports_sve())
+				return -EINVAL;
+
+			err = restore_sve_fpsimd_context(&user);
+		} else {
+			err = restore_fpsimd_context(user.fpsimd);
+		}
+	}
 
 	return err;
 }
@@ -459,6 +589,18 @@  static int setup_sigframe_layout(struct rt_sigframe_user_layout *user)
 			return err;
 	}
 
+	if (system_supports_sve()) {
+		unsigned int vq = 0;
+
+		if (test_thread_flag(TIF_SVE))
+			vq = sve_vq_from_vl(current->thread.sve_vl);
+
+		err = sigframe_alloc(user, &user->sve_offset,
+				     SVE_SIG_CONTEXT_SIZE(vq));
+		if (err)
+			return err;
+	}
+
 	return sigframe_alloc_end(user);
 }
 
@@ -500,6 +642,13 @@  static int setup_sigframe(struct rt_sigframe_user_layout *user,
 		__put_user_error(current->thread.fault_code, &esr_ctx->esr, err);
 	}
 
+	/* Scalable Vector Extension state, if present */
+	if (system_supports_sve() && err == 0 && user->sve_offset) {
+		struct sve_context __user *sve_ctx =
+			apply_user_offset(user, user->sve_offset);
+		err |= preserve_sve_context(sve_ctx);
+	}
+
 	if (err == 0 && user->extra_offset) {
 		char __user *sfp = (char __user *)user->sigframe;
 		char __user *userp =
@@ -599,6 +748,8 @@  static int setup_rt_frame(int usig, struct ksignal *ksig, sigset_t *set,
 	struct rt_sigframe __user *frame;
 	int err = 0;
 
+	fpsimd_signal_preserve_current_state();
+
 	if (get_sigframe(&user, ksig, regs))
 		return 1;
 
diff --git a/arch/arm64/kernel/signal32.c b/arch/arm64/kernel/signal32.c
index e09bf5d..22711ee 100644
--- a/arch/arm64/kernel/signal32.c
+++ b/arch/arm64/kernel/signal32.c
@@ -239,7 +239,7 @@  static int compat_preserve_vfp_context(struct compat_vfp_sigframe __user *frame)
 	 * Note that this also saves V16-31, which aren't visible
 	 * in AArch32.
 	 */
-	fpsimd_preserve_current_state();
+	fpsimd_signal_preserve_current_state();
 
 	/* Place structure header on the stack */
 	__put_user_error(magic, &frame->magic, err);