Dave Martin <dave.mar...@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.mar...@arm.com>
> Cc: Alex Bennée <alex.ben...@linaro.org>
> Cc: Ard Biesheuvel <ard.biesheu...@linaro.org>
> Cc: Will Deacon <will.dea...@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.ben...@linaro.org>
>
> ---
>
> **Dropped** Reviewed-by: Catalin Marinas <catalin.mari...@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(¤t->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 = ¤t->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 = ¤t->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
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
