| Message ID | 1502280338-23002-14-git-send-email-Dave.Martin@arm.com |
|---|---|
| State | New |
| Headers | show |
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. 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@arm.com> > --- > 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
On Wed, Aug 23, 2017 at 10:38:51AM +0100, 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. 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@arm.com> > > --- [...] > > 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? Yes, the signal code now makes use of these via fpsimd_signal_preserve_current_state() and fpsimd_update_current_state(), with this patch. [...] > > 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 [...] > > +#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. Agreed. Some BUG_ON()s test for things that are now obviously true by construction, which was not initially the case during development. This one can go away. I'm very interested in any BUG_ON() that you think can actually fire. There _shouldn't_ be any of those, but the reasons are non-trivial in some cases. > > + 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. This is a tricky one -- I suppose I should definitely add comments, since it took me 2-3 weeks to debug this and get it right the first time around... I'm trying to protect against a context switch during the copy_from_user rewriting stale register data over the data we're copying from the user signal frame. In most places, I now disable preemption when writing into thread_struct, to keep this can of worms firmly closed. Here though, the code writes directly into thread_struct with preemption enabled: in this one case the copy is a copy_from_user(), and we can't disable preemption for that. Other than this, the reasons for this code being the way it is derive from Ard's deferred fpsimd reload machinery (the TIF_FOREIGN_FPSTATE stuff), which is described in more detail in fpsimd.c. The reason for the difference from the old fpsimd code here is that the SVE state may be unreasonably large to bounce via a buffer on the stack, and the copy-in occurs while the thread is running. In the ptrace case (later on) the thread is guaranteed stopped, whereas for fpsimd the state is small enough that we _can_ bounce it via the stack. TIF_FOREIGN_FPSTATE may be cleared by the context switch code, but only if fpsimd_flush_task_state() wasn't called since the last kernel entry for current. (See fpsimd_thread_switch().) The context switch code may write the CPU registers over thread_struct, but only if TIF_FOREIGN_FPSTATE is not set at sched-out time. (fpsimd_thread_switch() again). These dependencies are strictly intra-thread, since context-switch is part of the same thread, so fpsimd_flush_task_state(); barrier(); set_thread_flag(TIF_FOREIGN_FPSTATE); barrier(); copy_from_user(current->thread.sve_state, ...); should be sufficient. I prefer to avoid spuriously saving the SVE regs into thread_struct when they may not already be live, so I don't set TIF_SVE until after TIF_FOREIGN_FPSTATE is set. (This is a cost saving rather than a security issue -- only current's state can be in the registers, but the extra SVE bits are definitely not worth saving. In any case, it only makes a difference to context switches in this narrow window.) > > + > > + 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. The final barrier() isn't really needed, and the ordering of TIF_SVE with the copy_from_user is unimportant unless I've missed something ... so I may get rid of that. TIF_SVE would affect context switch, but only if TIF_FOREIGN_FPSTATE is clear or a ret_to_user occurs, neither of which is possible after the second barrier(). Re smp_mb(), the only cross-thread ordering guarantees needed are for ptrace (later in the series), since that runs in some thread operating on a stopped thread that may have last run on some other CPU. But this is a general ptrace issue, not specifically related to SVE. IIRC some interaction between the ptrace core code and the scheduler's manipulation of the task state and runqueue locks ensures safety in that case without need for extra smp_mb() all over the place. > > + > > +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; > > +} [...] Cheers ---Dave
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 + 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)); + 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(); + + 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); + +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);
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@arm.com> --- 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(-)