Dave Martin <Dave.Martin@xxxxxxx> 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. In order to avoid racing with a pending discard of the
> SVE state, this flush is hoisted before the sigframe layout phase,
> so that the layout and population phases see a consistent view of
> the thread.
>
> Signed-off-by: Dave Martin <Dave.Martin@xxxxxxx>
> ---
> arch/arm64/include/asm/fpsimd.h | 1 +
> arch/arm64/kernel/fpsimd.c | 23 ++++--
> arch/arm64/kernel/signal.c | 169 ++++++++++++++++++++++++++++++++++++++--
> arch/arm64/kernel/signal32.c | 2 +-
> 4 files changed, 179 insertions(+), 16 deletions(-)
>
> diff --git a/arch/arm64/include/asm/fpsimd.h b/arch/arm64/include/asm/fpsimd.h
> index 72090a1..7efd04e 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 80ecb2d..e8674f6 100644
> --- a/arch/arm64/kernel/fpsimd.c
> +++ b/arch/arm64/kernel/fpsimd.c
> @@ -148,8 +148,6 @@ static void change_cpacr(u64 old, u64 new)
> write_sysreg(new, CPACR_EL1);
> }
>
> -#ifdef CONFIG_ARM64_SVE
> -
> #define ZREG(sve_state, vq, n) ((char *)(sve_state) + \
> (SVE_SIG_ZREG_OFFSET(vq, n) - SVE_SIG_REGS_OFFSET))
>
> @@ -191,6 +189,8 @@ static void fpsimd_to_sve(struct task_struct *task)
> sizeof(fst->vregs[i]));
> }
>
> +#ifdef CONFIG_ARM64_SVE
> +
Hmm have sve_to_fpsimd and fpsimd_to_sve only just started being used by
the generic code here?
> size_t sve_state_size(struct task_struct const *task)
> {
> unsigned int vl = task->thread.sve_vl;
> @@ -431,13 +431,17 @@ void fpsimd_preserve_current_state(void)
> return;
>
> local_bh_disable();
> -
> - if (!test_thread_flag(TIF_FOREIGN_FPSTATE))
> - fpsimd_save_state(¤t->thread.fpsimd_state);
> -
> + task_fpsimd_save();
> local_bh_enable();
> }
>
> +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
> @@ -473,7 +477,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 4991e87..2694143 100644
> --- a/arch/arm64/kernel/signal.c
> +++ b/arch/arm64/kernel/signal.c
> @@ -62,6 +62,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;
> };
> @@ -178,9 +179,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);
> @@ -213,6 +211,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);
> @@ -220,10 +220,113 @@ 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;
> +
> + BUG_ON(!sve_vl_valid(vl));
> + 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().
> + */
> + BUG_ON(SVE_SIG_REGS_SIZE(vq) !=
> sve_state_size(current));
I think others have mentioned the excessive BUG_ON()s here but I think
you are planning on cleaning some up on the next version. Assuming
sve_vq_from_vl() can't give you an invalid answer from a
sve_vl_valid(vl) then I wouldn't expect this test to add much.
> + 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;
> + }
> +
> + BUG_ON(!sve_vl_valid(sve.vl));
> + vq = sve_vq_from_vl(sve.vl);
> +
> + if (sve.head.size < SVE_SIG_CONTEXT_SIZE(vq))
> + return -EINVAL;
> +
> + fpsimd_flush_task_state(current);
> + barrier();
> + set_thread_flag(TIF_FOREIGN_FPSTATE);
> + barrier();
What are you trying to achieve with barriers here? Is there a potential
interaction between flushing the state and setting the flag that the
compiler can't see? A comment should be added at least.
> +
> + sve_alloc(current);
> + BUG_ON(SVE_SIG_REGS_SIZE(vq) != sve_state_size(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 err;
> +
> + barrier();
> + set_thread_flag(TIF_SVE);
Hmm and again. If this is about visibility of context when the thread
flag is read by other CPUs a barrier() on it's own is not enough as it
only stop local code re-organisation - do you actually mean smp_mb()?
Either way you need to document the potential race in a comment so the
reason can be understood.
> +
> +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;
> +}
> +
> +#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)
> {
> @@ -236,6 +339,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;
> @@ -286,6 +390,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;
> @@ -358,9 +475,6 @@ static int parse_user_sigframe(struct user_ctxs *user,
> }
>
> done:
> - if (!user->fpsimd)
> - goto invalid;
> -
> return 0;
>
> invalid:
> @@ -394,8 +508,18 @@ 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;
> }
> @@ -454,6 +578,20 @@ 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)) {
> + BUG_ON(!sve_vl_valid(current->thread.sve_vl));
> + 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);
> }
>
> @@ -495,6 +633,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 =
> @@ -594,6 +739,14 @@ static int setup_rt_frame(int usig, struct ksignal *ksig, sigset_t *set,
> struct rt_sigframe __user *frame;
> int err = 0;
>
> + /*
> + * Ensure FPSIMD/SVE state in task_struct is up-to-date.
> + * This is needed here in order to complete any pending SVE discard:
> + * otherwise, discard may occur between deciding on the sigframe
> + * layout and dumping the register data.
> + */
> + 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 4e5a664..202337d 100644
> --- a/arch/arm64/kernel/signal32.c
> +++ b/arch/arm64/kernel/signal32.c
> @@ -244,7 +244,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
