| Message ID | 1504198860-12951-12-git-send-email-Dave.Martin@arm.com |
|---|---|
| State | New |
| Headers | show |
| Series | ARM Scalable Vector Extension (SVE) | expand |
On Thu, Aug 31, 2017 at 06:00:43PM +0100, Dave P Martin wrote: > +/* > + * Handle SVE state across fork(): > + * > + * dst and src must not end up with aliases of the same sve_state. > + * Because a task cannot fork except in a syscall, we can discard SVE > + * state for dst here, so long as we take care to retain the FPSIMD > + * subset of the state if SVE is in use. Reallocation of the SVE state > + * will be deferred until dst tries to use SVE. > + */ > +void fpsimd_dup_sve(struct task_struct *dst, struct task_struct const *src) > +{ > + if (test_and_clear_tsk_thread_flag(dst, TIF_SVE)) { > + WARN_ON(dst->mm && !in_syscall(task_pt_regs(dst))); > + sve_to_fpsimd(dst); > + } > + > + dst->thread.sve_state = NULL; > +} I first thought the thread flags are not visible in dst yet since dup_task_struct() calls arch_dup_task_struct() before setup_thread_stack(). However, at the end of the last year we enabled CONFIG_THREAD_INFO_IN_TASK_STRUCT. But I don't particularly like relying on this. Anyway, IIUC we don't need sve_to_fpsimd() here. The arch_dup_task_struct() already called fpsimd_preserve_current_state() for src, so the FPSIMD state (which we care about) is transferred during the *dst = *src assignment. So you'd only need the last statement, possibly with a different function name like fpsimd_erase_sve (and maybe make the function static inline in the header). [...] > int arch_dup_task_struct(struct task_struct *dst, struct task_struct *src) > @@ -246,6 +247,9 @@ int arch_dup_task_struct(struct task_struct *dst, struct task_struct *src) > if (current->mm) > fpsimd_preserve_current_state(); > *dst = *src; > + > + fpsimd_dup_sve(dst, src); > + > return 0; > }
On Thu, Aug 31, 2017 at 06:00:43PM +0100, Dave P Martin wrote: > +el0_sve_acc: > + /* > + * Scalable Vector Extension access > + */ > + enable_dbg > + ct_user_exit > + mov x0, x25 > + mov x1, sp > + bl do_sve_acc > + b ret_to_user I think do_sve_acc() runs with interrupts disabled. We may have some high latency for large SVE states. > +/* > + * Trapped SVE access > + */ > +void do_sve_acc(unsigned int esr, struct pt_regs *regs) > +{ > + /* Even if we chose not to use SVE, the hardware could still trap: */ > + if (unlikely(!system_supports_sve()) || WARN_ON(is_compat_task())) { > + force_signal_inject(SIGILL, ILL_ILLOPC, regs, 0); > + return; > + } > + > + task_fpsimd_save(); > + > + sve_alloc(current); > + fpsimd_to_sve(current); > + if (test_and_set_thread_flag(TIF_SVE)) > + WARN_ON(1); /* SVE access shouldn't have trapped */ > + > + task_fpsimd_load(); > +} When this function is entered, do we expect TIF_SVE to always be cleared? It's worth adding a comment on the expected conditions. If that's the case, task_fpsimd_save() would only save the FPSIMD state which is fine. However, you subsequently transfer the FPSIMD state to SVE, set TIF_SVE and restore the full SVE state. If we don't care about the SVE state here, can we call task_fpsimd_load() *before* setting TIF_SVE? I may as well have confused myself with the state bouncing between FPSIMD and SVE (more reasons to document the data flow better ;)).
On Wed, Sep 13, 2017 at 10:26:05AM -0700, Catalin Marinas wrote: > On Thu, Aug 31, 2017 at 06:00:43PM +0100, Dave P Martin wrote: > > +el0_sve_acc: > > + /* > > + * Scalable Vector Extension access > > + */ > > + enable_dbg > > + ct_user_exit > > + mov x0, x25 > > + mov x1, sp > > + bl do_sve_acc > > + b ret_to_user > > I think do_sve_acc() runs with interrupts disabled. We may have some > high latency for large SVE states. Historically I wanted to play safe. I meant to change this to enable_dbg_and_irq now, but it looks like I forgot to do it. I'll double-check that do_sve_acc() does nothing that makes it unsafe to enable irqs, but it should be OK. It's certainly _intended_ to be irq-safe. > > +/* > > + * Trapped SVE access > > + */ > > +void do_sve_acc(unsigned int esr, struct pt_regs *regs) > > +{ > > + /* Even if we chose not to use SVE, the hardware could still trap: */ > > + if (unlikely(!system_supports_sve()) || WARN_ON(is_compat_task())) { > > + force_signal_inject(SIGILL, ILL_ILLOPC, regs, 0); > > + return; > > + } > > + > > + task_fpsimd_save(); > > + > > + sve_alloc(current); > > + fpsimd_to_sve(current); > > + if (test_and_set_thread_flag(TIF_SVE)) > > + WARN_ON(1); /* SVE access shouldn't have trapped */ > > + > > + task_fpsimd_load(); > > +} > > When this function is entered, do we expect TIF_SVE to always be > cleared? It's worth adding a comment on the expected conditions. If Yes, and this is required for correctness, as you observe. I had a BUG_ON() here which I removed, but it makes sense to add a comment to capture the precondition here, and how it is satisfied. > that's the case, task_fpsimd_save() would only save the FPSIMD state > which is fine. However, you subsequently transfer the FPSIMD state to > SVE, set TIF_SVE and restore the full SVE state. If we don't care about > the SVE state here, can we call task_fpsimd_load() *before* setting > TIF_SVE? There should be no way to reach this code with TIF_SVE set, unless task_fpsimd_load() sets the CPACR trap bit wrongly, or the hardware is broken -- either of which is a bug. > I may as well have confused myself with the state bouncing between > FPSIMD and SVE (more reasons to document the data flow better ;)). Agreed, I think this does need more explanation... I'm currently trying to figure out the best way to describe it. Cheers ---Dave
On Wed, Sep 13, 2017 at 08:17:07PM +0100, Dave P Martin wrote: > On Wed, Sep 13, 2017 at 10:26:05AM -0700, Catalin Marinas wrote: > > On Thu, Aug 31, 2017 at 06:00:43PM +0100, Dave P Martin wrote: > > > +/* > > > + * Trapped SVE access > > > + */ > > > +void do_sve_acc(unsigned int esr, struct pt_regs *regs) > > > +{ > > > + /* Even if we chose not to use SVE, the hardware could still trap: */ > > > + if (unlikely(!system_supports_sve()) || WARN_ON(is_compat_task())) { > > > + force_signal_inject(SIGILL, ILL_ILLOPC, regs, 0); > > > + return; > > > + } > > > + > > > + task_fpsimd_save(); > > > + > > > + sve_alloc(current); > > > + fpsimd_to_sve(current); > > > + if (test_and_set_thread_flag(TIF_SVE)) > > > + WARN_ON(1); /* SVE access shouldn't have trapped */ > > > + > > > + task_fpsimd_load(); > > > +} > > > > When this function is entered, do we expect TIF_SVE to always be > > cleared? It's worth adding a comment on the expected conditions. If > > Yes, and this is required for correctness, as you observe. > > I had a BUG_ON() here which I removed, but it makes sense to add a > comment to capture the precondition here, and how it is satisfied. > > > that's the case, task_fpsimd_save() would only save the FPSIMD state > > which is fine. However, you subsequently transfer the FPSIMD state to > > SVE, set TIF_SVE and restore the full SVE state. If we don't care about > > the SVE state here, can we call task_fpsimd_load() *before* setting > > TIF_SVE? > > There should be no way to reach this code with TIF_SVE set, unless > task_fpsimd_load() sets the CPACR trap bit wrongly, or the hardware is > broken -- either of which is a bug. Thanks for confirming my assumptions. What I meant was rewriting the above function as: /* reset the SVE state (other than FPSIMD) */ task_fpsimd_save(); task_fpsimd_load(); sve_alloc(current); set_thread_flag(TIF_SVE);
On Wed, Sep 13, 2017 at 03:21:29PM -0700, Catalin Marinas wrote: > On Wed, Sep 13, 2017 at 08:17:07PM +0100, Dave P Martin wrote: > > On Wed, Sep 13, 2017 at 10:26:05AM -0700, Catalin Marinas wrote: > > > On Thu, Aug 31, 2017 at 06:00:43PM +0100, Dave P Martin wrote: > > > > +/* > > > > + * Trapped SVE access > > > > + */ > > > > +void do_sve_acc(unsigned int esr, struct pt_regs *regs) > > > > +{ > > > > + /* Even if we chose not to use SVE, the hardware could still trap: */ > > > > + if (unlikely(!system_supports_sve()) || WARN_ON(is_compat_task())) { > > > > + force_signal_inject(SIGILL, ILL_ILLOPC, regs, 0); > > > > + return; > > > > + } > > > > + > > > > + task_fpsimd_save(); > > > > + > > > > + sve_alloc(current); > > > > + fpsimd_to_sve(current); > > > > + if (test_and_set_thread_flag(TIF_SVE)) > > > > + WARN_ON(1); /* SVE access shouldn't have trapped */ > > > > + > > > > + task_fpsimd_load(); > > > > +} > > > > > > When this function is entered, do we expect TIF_SVE to always be > > > cleared? It's worth adding a comment on the expected conditions. If > > > > Yes, and this is required for correctness, as you observe. > > > > I had a BUG_ON() here which I removed, but it makes sense to add a > > comment to capture the precondition here, and how it is satisfied. > > > > > that's the case, task_fpsimd_save() would only save the FPSIMD state > > > which is fine. However, you subsequently transfer the FPSIMD state to > > > SVE, set TIF_SVE and restore the full SVE state. If we don't care about > > > the SVE state here, can we call task_fpsimd_load() *before* setting > > > TIF_SVE? > > > > There should be no way to reach this code with TIF_SVE set, unless > > task_fpsimd_load() sets the CPACR trap bit wrongly, or the hardware is > > broken -- either of which is a bug. > > Thanks for confirming my assumptions. What I meant was rewriting the > above function as: > > /* reset the SVE state (other than FPSIMD) */ > task_fpsimd_save(); > task_fpsimd_load(); I think this works, but can you explain your rationale? I think the main effect of your suggestion is that it is cheaper, due to eliminating some unnecessary load/store operations. We could go one better, and do mov v0.16b, v0.16b mov v1.16b, v1.16b // ... mov v31.16b, v31.16b which doesn't require any memory access. But I still prefer to zero p0..p15, ffr for cleanliness, even though the SVE programmer's model doesn't require this (unlike for the Z-reg high bits where we do need to zero them in order not to violate the programmer's model). Currently sve_alloc()+task_fpsimd_load() ensures that all the non-FPSIMD regs are zeroed too, in addition to the Z-reg high bits. So we might want a special-purpose helper -- if so, we can do it all with no memory access. pfalse p0.b // .. pfalse p15.b wrffr p0.b This would allow the memset-zero an sve_alloc() to be removed, but I would need to check what other code is relying on it. I guess I hadn't done this because I viewed it as an optimisation. Thoughts? Cheers ---Dave
On Wed, Sep 13, 2017 at 07:33:25AM -0700, Catalin Marinas wrote: > On Thu, Aug 31, 2017 at 06:00:43PM +0100, Dave P Martin wrote: > > +/* > > + * Handle SVE state across fork(): > > + * > > + * dst and src must not end up with aliases of the same sve_state. > > + * Because a task cannot fork except in a syscall, we can discard SVE > > + * state for dst here, so long as we take care to retain the FPSIMD > > + * subset of the state if SVE is in use. Reallocation of the SVE state > > + * will be deferred until dst tries to use SVE. > > + */ > > +void fpsimd_dup_sve(struct task_struct *dst, struct task_struct const *src) > > +{ > > + if (test_and_clear_tsk_thread_flag(dst, TIF_SVE)) { > > + WARN_ON(dst->mm && !in_syscall(task_pt_regs(dst))); > > + sve_to_fpsimd(dst); > > + } > > + > > + dst->thread.sve_state = NULL; > > +} > > I first thought the thread flags are not visible in dst yet since > dup_task_struct() calls arch_dup_task_struct() before > setup_thread_stack(). However, at the end of the last year we enabled > CONFIG_THREAD_INFO_IN_TASK_STRUCT. But I don't particularly like relying > on this. Hmmm, I see your point, but there are some sequencing issues here. > Anyway, IIUC we don't need sve_to_fpsimd() here. The > arch_dup_task_struct() already called fpsimd_preserve_current_state() I consider SVE discard as an optional side effect of task_fpsimd_save(), not something that is guaranteed to happen -- the decision about whether to do so may become more intelligent later on. So, for src, we may discard SVE (because syscall), but for dst we must NULL .sve_state (and therefore clear TIF_SVE) simply to avoid aliasing of src->sve_state and dst->sve_state. The latter requires operating on the thread_flags of dst. I'll need to check whether there's another suitable hook for updating the thread flags of dst, if we aren't confident that they will always have been initialised by the time arch_dup_task_struct() is called. Either way, there would be an intra-thread ordering requirement between the task_struct and thread_info updates here, _if_ dst were schedulable: dst:TIF_SVE must be cleared before dst->sve_state is NULLed. dst is not schedulable until fork is done though, so maybe this doesn't really matter... > for src, so the FPSIMD state (which we care about) is transferred during > the *dst = *src assignment. So you'd only need the last statement, > possibly with a different function name like fpsimd_erase_sve (and maybe > make the function static inline in the header). Not quite: TIF_SVE must be cleared so that a context switch or kernel_neon_begin() after dst is scheduled doesn't try to save state in the (NULL) dst->sve_state. > > [...] > > int arch_dup_task_struct(struct task_struct *dst, struct task_struct *src) > > @@ -246,6 +247,9 @@ int arch_dup_task_struct(struct task_struct *dst, struct task_struct *src) > > if (current->mm) > > fpsimd_preserve_current_state(); > > *dst = *src; > > + > > + fpsimd_dup_sve(dst, src); > > + > > return 0; > > } Cheers ---Dave
On Thu, Sep 14, 2017 at 08:40:41PM +0100, Dave P Martin wrote: > On Wed, Sep 13, 2017 at 03:21:29PM -0700, Catalin Marinas wrote: > > On Wed, Sep 13, 2017 at 08:17:07PM +0100, Dave P Martin wrote: > > > On Wed, Sep 13, 2017 at 10:26:05AM -0700, Catalin Marinas wrote: > > > > On Thu, Aug 31, 2017 at 06:00:43PM +0100, Dave P Martin wrote: > > > > > +/* > > > > > + * Trapped SVE access > > > > > + */ > > > > > +void do_sve_acc(unsigned int esr, struct pt_regs *regs) > > > > > +{ > > > > > + /* Even if we chose not to use SVE, the hardware could still trap: */ > > > > > + if (unlikely(!system_supports_sve()) || WARN_ON(is_compat_task())) { > > > > > + force_signal_inject(SIGILL, ILL_ILLOPC, regs, 0); > > > > > + return; > > > > > + } > > > > > + > > > > > + task_fpsimd_save(); > > > > > + > > > > > + sve_alloc(current); > > > > > + fpsimd_to_sve(current); > > > > > + if (test_and_set_thread_flag(TIF_SVE)) > > > > > + WARN_ON(1); /* SVE access shouldn't have trapped */ > > > > > + > > > > > + task_fpsimd_load(); > > > > > +} > > > > > > > > When this function is entered, do we expect TIF_SVE to always be > > > > cleared? It's worth adding a comment on the expected conditions. If > > > > > > Yes, and this is required for correctness, as you observe. > > > > > > I had a BUG_ON() here which I removed, but it makes sense to add a > > > comment to capture the precondition here, and how it is satisfied. > > > > > > > that's the case, task_fpsimd_save() would only save the FPSIMD state > > > > which is fine. However, you subsequently transfer the FPSIMD state to > > > > SVE, set TIF_SVE and restore the full SVE state. If we don't care about > > > > the SVE state here, can we call task_fpsimd_load() *before* setting > > > > TIF_SVE? > > > > > > There should be no way to reach this code with TIF_SVE set, unless > > > task_fpsimd_load() sets the CPACR trap bit wrongly, or the hardware is > > > broken -- either of which is a bug. > > > > Thanks for confirming my assumptions. What I meant was rewriting the > > above function as: > > > > /* reset the SVE state (other than FPSIMD) */ > > task_fpsimd_save(); > > task_fpsimd_load(); > > I think this works, but can you explain your rationale? > > I think the main effect of your suggestion is that it is cheaper, due > to eliminating some unnecessary load/store operations. My rationale was to avoid copying between the in-memory FPSIMD and SVE state. > We could go one better, and do > > mov v0.16b, v0.16b > mov v1.16b, v1.16b > // ... > mov v31.16b, v31.16b > > which doesn't require any memory access. Yes, that's even better. > But I still prefer to zero p0..p15, ffr for cleanliness, even though > the SVE programmer's model doesn't require this (unlike for the Z-reg > high bits where we do need to zero them in order not to violate the > programmer's model). I missed the px, ffr aspect. Can you not have a clear_sve_state() (or a better name) function to zero the predicate regs, ffr and the top bits of the vectors? > Currently sve_alloc()+task_fpsimd_load() ensures that all the non-FPSIMD > regs are zeroed too, in addition to the Z-reg high bits. Yes, just wondering if this can be implemented with less memory accesses since the SVE state is irrelevant at this stage. > > So we might want a special-purpose helper -- if so, we can do it all > with no memory access. > > pfalse p0.b > // .. > pfalse p15.b > wrffr p0.b > > This would allow the memset-zero an sve_alloc() to be removed, but I > would need to check what other code is relying on it. > > I guess I hadn't done this because I viewed it as an optimisation. It looked like some low-hanging optimisation to slightly accelerate the allocation of the SVE state on access, though I'm also worried I don't fully understand all the corner cases (like what happens if we allow interrupts during this function and get preempted). Anyway, I'm fine to leave this as it is for now and try to optimise it later with additional patches on top.
On Thu, Sep 14, 2017 at 08:55:56PM +0100, Dave P Martin wrote: > On Wed, Sep 13, 2017 at 07:33:25AM -0700, Catalin Marinas wrote: > > On Thu, Aug 31, 2017 at 06:00:43PM +0100, Dave P Martin wrote: > > > +/* > > > + * Handle SVE state across fork(): > > > + * > > > + * dst and src must not end up with aliases of the same sve_state. > > > + * Because a task cannot fork except in a syscall, we can discard SVE > > > + * state for dst here, so long as we take care to retain the FPSIMD > > > + * subset of the state if SVE is in use. Reallocation of the SVE state > > > + * will be deferred until dst tries to use SVE. > > > + */ > > > +void fpsimd_dup_sve(struct task_struct *dst, struct task_struct const *src) > > > +{ > > > + if (test_and_clear_tsk_thread_flag(dst, TIF_SVE)) { > > > + WARN_ON(dst->mm && !in_syscall(task_pt_regs(dst))); > > > + sve_to_fpsimd(dst); > > > + } > > > + > > > + dst->thread.sve_state = NULL; > > > +} > > > > I first thought the thread flags are not visible in dst yet since > > dup_task_struct() calls arch_dup_task_struct() before > > setup_thread_stack(). However, at the end of the last year we enabled > > CONFIG_THREAD_INFO_IN_TASK_STRUCT. But I don't particularly like relying > > on this. > > Hmmm, I see your point, but there are some sequencing issues here. > > > Anyway, IIUC we don't need sve_to_fpsimd() here. The > > arch_dup_task_struct() already called fpsimd_preserve_current_state() > > I consider SVE discard as an optional side effect of task_fpsimd_save(), > not something that is guaranteed to happen -- the decision about whether > to do so may become more intelligent later on. So, for src, we may > discard SVE (because syscall), but for dst we must NULL .sve_state (and > therefore clear TIF_SVE) simply to avoid aliasing of src->sve_state and > dst->sve_state. My point was that the SVE state of src is already preserved at this point and copied into dst. You don't need the sve_to_fpsimd(dst) again which basically does the same copying of the src SVE saved state into the FPSIMD one in dst. This has already been done in arch_dup_task_struct() by the combination of fpsimd_preserve_current_state() and *dst = *src (and, of course, clearing TIF_SVE in dst). I don't think the TIF_SVE clearing in src is just a side effect of task_fpsimd_save() here but rather a requirement. When returning from fork(), both src and dst would need to have the same state. However, your fpsimd_dup_sve() implementation makes it very clear that the SVE state is lost in dst. This is only allowed if we also lose it in src (as a result of a syscall). So making dst->sve_state = NULL requires that TIF_SVE is also cleared in both src and dst. Alternatively, you'd have to allocate a new state here and copy the full src SVE state across to dst, together with setting TIF_SVE (that's not necessary, however, since we get here as a result of a syscall). > > for src, so the FPSIMD state (which we care about) is transferred during > > the *dst = *src assignment. So you'd only need the last statement, > > possibly with a different function name like fpsimd_erase_sve (and maybe > > make the function static inline in the header). > > Not quite: TIF_SVE must be cleared so that a context switch or > kernel_neon_begin() after dst is scheduled doesn't try to save state in > the (NULL) dst->sve_state. Yes, TIF_SVE must also be cleared in dst when dst->sve_state = NULL (I may have forgotten to mention this).
On Wed, Sep 20, 2017 at 02:58:56PM +0100, Catalin Marinas wrote: > On Thu, Sep 14, 2017 at 08:55:56PM +0100, Dave P Martin wrote: > > On Wed, Sep 13, 2017 at 07:33:25AM -0700, Catalin Marinas wrote: > > > On Thu, Aug 31, 2017 at 06:00:43PM +0100, Dave P Martin wrote: > > > > +/* > > > > + * Handle SVE state across fork(): > > > > + * > > > > + * dst and src must not end up with aliases of the same sve_state. > > > > + * Because a task cannot fork except in a syscall, we can discard SVE > > > > + * state for dst here, so long as we take care to retain the FPSIMD > > > > + * subset of the state if SVE is in use. Reallocation of the SVE state > > > > + * will be deferred until dst tries to use SVE. > > > > + */ > > > > +void fpsimd_dup_sve(struct task_struct *dst, struct task_struct const *src) > > > > +{ > > > > + if (test_and_clear_tsk_thread_flag(dst, TIF_SVE)) { > > > > + WARN_ON(dst->mm && !in_syscall(task_pt_regs(dst))); > > > > + sve_to_fpsimd(dst); > > > > + } > > > > + > > > > + dst->thread.sve_state = NULL; > > > > +} > > > > > > I first thought the thread flags are not visible in dst yet since > > > dup_task_struct() calls arch_dup_task_struct() before > > > setup_thread_stack(). However, at the end of the last year we enabled > > > CONFIG_THREAD_INFO_IN_TASK_STRUCT. But I don't particularly like relying > > > on this. > > > > Hmmm, I see your point, but there are some sequencing issues here. > > > > > Anyway, IIUC we don't need sve_to_fpsimd() here. The > > > arch_dup_task_struct() already called fpsimd_preserve_current_state() > > > > I consider SVE discard as an optional side effect of task_fpsimd_save(), > > not something that is guaranteed to happen -- the decision about whether > > to do so may become more intelligent later on. So, for src, we may > > discard SVE (because syscall), but for dst we must NULL .sve_state (and > > therefore clear TIF_SVE) simply to avoid aliasing of src->sve_state and > > dst->sve_state. > > My point was that the SVE state of src is already preserved at this > point and copied into dst. You don't need the sve_to_fpsimd(dst) again > which basically does the same copying of the src SVE saved state into > the FPSIMD one in dst. This has already been done in > arch_dup_task_struct() by the combination of > fpsimd_preserve_current_state() and *dst = *src (and, of course, > clearing TIF_SVE in dst). > > I don't think the TIF_SVE clearing in src is just a side effect of > task_fpsimd_save() here but rather a requirement. When returning from > fork(), both src and dst would need to have the same state. However, > your fpsimd_dup_sve() implementation makes it very clear that the SVE > state is lost in dst. This is only allowed if we also lose it in src (as > a result of a syscall). So making dst->sve_state = NULL requires that > TIF_SVE is also cleared in both src and dst. Alternatively, you'd have > to allocate a new state here and copy the full src SVE state across to > dst, together with setting TIF_SVE (that's not necessary, however, since > we get here as a result of a syscall). The currently intended ABI is that the SVE bits are unspecified after a syscall, so it is legitimate (though perhaps surprising) for different things to happen in dst and src. This complicates things a lot though, just to avoid the next SVE usage exception in src after the fork. It should be simpler to do what you suggest and discard the SVE state of src unconditionally before the copy: then we really are just cloning the thread apart from the need to set dst->thread.sve_state to NULL. fpsimd_preserve_current_state() does not necessarily write back to current->thread.fpsmid_state though: at the moment, it does do this as a side effect of task_fpsimd_save() because we happen to be in a syscall (i.e., fork). What we really want is unconditional discarding of the state. This wasn't needed anywhere else yet, so there's no explicit helper for it. But it makes sense to add one. What about refactoring along these lines: fpsimd.c: /* Unconditionally discard the SVE state */ void task_sve_discard(struct task_struct *task) { if (!system_supports_sve()) return; local_bh_disable(); if (test_and_clear_tsk_thread_flag(task, TIF_SVE)) sve_to_fpsimd(task); local_bh_enable(); } process.c: int arch_dup_task_struct(sturct task_struct *dst, struct task_struct *src) { if (current->mm) { fpsimd_preserve_current_state(); task_sve_discard(src); } *dst = *src; dst->thread.sve_state = NULL; } This also avoids having to touch dst's thread flags, since now we are just cloning the task except for assigning NULL to dst->thread.sve_state. > > > for src, so the FPSIMD state (which we care about) is transferred during > > > the *dst = *src assignment. So you'd only need the last statement, > > > possibly with a different function name like fpsimd_erase_sve (and maybe > > > make the function static inline in the header). > > > > Not quite: TIF_SVE must be cleared so that a context switch or > > kernel_neon_begin() after dst is scheduled doesn't try to save state in > > the (NULL) dst->sve_state. > > Yes, TIF_SVE must also be cleared in dst when dst->sve_state = NULL (I > may have forgotten to mention this). With the above factoring, the constraint "TIF_SVE implies sve_state valid" is then never false, because TIF_SVE is already cleared before the NULL assignment. What do you think? Cheers ---Dave
On Wed, Sep 13, 2017 at 07:33:25AM -0700, Catalin Marinas wrote: > On Thu, Aug 31, 2017 at 06:00:43PM +0100, Dave P Martin wrote: > > +/* > > + * Handle SVE state across fork(): > > + * > > + * dst and src must not end up with aliases of the same sve_state. > > + * Because a task cannot fork except in a syscall, we can discard SVE > > + * state for dst here, so long as we take care to retain the FPSIMD > > + * subset of the state if SVE is in use. Reallocation of the SVE state > > + * will be deferred until dst tries to use SVE. > > + */ > > +void fpsimd_dup_sve(struct task_struct *dst, struct task_struct const *src) > > +{ > > + if (test_and_clear_tsk_thread_flag(dst, TIF_SVE)) { > > + WARN_ON(dst->mm && !in_syscall(task_pt_regs(dst))); > > + sve_to_fpsimd(dst); > > + } > > + > > + dst->thread.sve_state = NULL; > > +} > > I first thought the thread flags are not visible in dst yet since > dup_task_struct() calls arch_dup_task_struct() before > setup_thread_stack(). However, at the end of the last year we enabled > CONFIG_THREAD_INFO_IN_TASK_STRUCT. But I don't particularly like relying > on this. > > Anyway, IIUC we don't need sve_to_fpsimd() here. The > arch_dup_task_struct() already called fpsimd_preserve_current_state() > for src, so the FPSIMD state (which we care about) is transferred during > the *dst = *src assignment. So you'd only need the last statement, > possibly with a different function name like fpsimd_erase_sve (and maybe > make the function static inline in the header). Regarding the intended meanings of the thread flags, does this help? --8<-- TIF_FOREIGN_FPSTATE's meaning is expanded to cover SVE, but otherwise unchanged: * If a task is running and !TIF_FOREIGN_FPSTATE, then the the CPU registers of the CPU the task is running on contain the authoritative FPSIMD/SVE state of the task. The backing memory may be stale. * Otherwise (i.e., task not running, or task running and TIF_FOREIGN_FPSTATE), the task's FPSIMD/SVE backing memory is authoritative. If additionally per_cpu(fpsimd_last_state, task->fpsimd_state.cpu) == &task->fpsimd_state.cpu, then task->fpsimd_state.cpu's registers are also up to date for task, but not authorititive: the current FPSIMD/SVE state may be read from them, but they must not be written. The FPSIMD/SVE backing memory is selected by TIF_SVE: * TIF_SVE set: Zn (incorporating Vn in bits[127:0]), Pn and FFR are stored in task->thread.sve_state, formatted appropriately for vector length task->thread.sve_vl. task->thread.sve_state must point to a valid buffer at least sve_state_size(task) bytes in size. * TIF_SVE clear: Vn are stored in task->fpsimd_state; Zn[max : 128] are logically zero[*] but not stored anywhere; Pn, FFR are not stored and have unspecified values from userspace's point of view. task->thread.sve_state does not need to be non-null, valid or any particular size: it must not be dereferenced. In practice I don't exploit the "unspecifiedness" much. The Zn high bits, Pn and FFR are all zeroed when setting TIF_SVE again: sve_alloc() is the common path for this. * FPSR and FPCR are always stored in task->fpsimd_state irrespctive of whether TIF_SVE is clear or set, since these are not vector length dependent. [*] theoretically unspecified, which is what I've written in sve.txt. However, on any FPSIMD Vn write the upper bits of the corresponding Zn must become logically zero in order to conform to the SVE programmer's model. It's not feasible to track which Vn have been written individually because that would involve trapping every FPSIMD insn until all possible Vn have been written. So the kernel ensures that the Zn high bits become zero. Maybe we should just guarantee "zero-or-preserved" behaviour for userspace. This may close down some future optimisation opportunities, but maybe it's better to be simple. Thoughts? [...] Cheers ---Dave
On Tue, Oct 03, 2017 at 12:11:01PM +0100, Dave P Martin wrote: > On Wed, Sep 20, 2017 at 02:58:56PM +0100, Catalin Marinas wrote: > > On Thu, Sep 14, 2017 at 08:55:56PM +0100, Dave P Martin wrote: > > > On Wed, Sep 13, 2017 at 07:33:25AM -0700, Catalin Marinas wrote: > > > > On Thu, Aug 31, 2017 at 06:00:43PM +0100, Dave P Martin wrote: > > > > > +/* > > > > > + * Handle SVE state across fork(): > > > > > + * > > > > > + * dst and src must not end up with aliases of the same sve_state. > > > > > + * Because a task cannot fork except in a syscall, we can discard SVE > > > > > + * state for dst here, so long as we take care to retain the FPSIMD > > > > > + * subset of the state if SVE is in use. Reallocation of the SVE state > > > > > + * will be deferred until dst tries to use SVE. > > > > > + */ > > > > > +void fpsimd_dup_sve(struct task_struct *dst, struct task_struct const *src) > > > > > +{ > > > > > + if (test_and_clear_tsk_thread_flag(dst, TIF_SVE)) { > > > > > + WARN_ON(dst->mm && !in_syscall(task_pt_regs(dst))); > > > > > + sve_to_fpsimd(dst); > > > > > + } > > > > > + > > > > > + dst->thread.sve_state = NULL; > > > > > +} > > > > > > > > I first thought the thread flags are not visible in dst yet since > > > > dup_task_struct() calls arch_dup_task_struct() before > > > > setup_thread_stack(). However, at the end of the last year we enabled > > > > CONFIG_THREAD_INFO_IN_TASK_STRUCT. But I don't particularly like relying > > > > on this. > > > > > > Hmmm, I see your point, but there are some sequencing issues here. > > > > > > > Anyway, IIUC we don't need sve_to_fpsimd() here. The > > > > arch_dup_task_struct() already called fpsimd_preserve_current_state() > > > > > > I consider SVE discard as an optional side effect of task_fpsimd_save(), > > > not something that is guaranteed to happen -- the decision about whether > > > to do so may become more intelligent later on. So, for src, we may > > > discard SVE (because syscall), but for dst we must NULL .sve_state (and > > > therefore clear TIF_SVE) simply to avoid aliasing of src->sve_state and > > > dst->sve_state. > > > > My point was that the SVE state of src is already preserved at this > > point and copied into dst. You don't need the sve_to_fpsimd(dst) again > > which basically does the same copying of the src SVE saved state into > > the FPSIMD one in dst. This has already been done in > > arch_dup_task_struct() by the combination of > > fpsimd_preserve_current_state() and *dst = *src (and, of course, > > clearing TIF_SVE in dst). > > > > I don't think the TIF_SVE clearing in src is just a side effect of > > task_fpsimd_save() here but rather a requirement. When returning from > > fork(), both src and dst would need to have the same state. However, > > your fpsimd_dup_sve() implementation makes it very clear that the SVE > > state is lost in dst. This is only allowed if we also lose it in src (as > > a result of a syscall). So making dst->sve_state = NULL requires that > > TIF_SVE is also cleared in both src and dst. Alternatively, you'd have > > to allocate a new state here and copy the full src SVE state across to > > dst, together with setting TIF_SVE (that's not necessary, however, since > > we get here as a result of a syscall). > > The currently intended ABI is that the SVE bits are unspecified after a > syscall, so it is legitimate (though perhaps surprising) for different > things to happen in dst and src. > > This complicates things a lot though, just to avoid the next SVE usage > exception in src after the fork. > > > It should be simpler to do what you suggest and discard the SVE state of > src unconditionally before the copy: then we really are just cloning the > thread apart from the need to set dst->thread.sve_state to NULL. > > fpsimd_preserve_current_state() does not necessarily write back to > current->thread.fpsmid_state though: at the moment, it does do this as a > side effect of task_fpsimd_save() because we happen to be in a syscall > (i.e., fork). > > What we really want is unconditional discarding of the state. This > wasn't needed anywhere else yet, so there's no explicit helper for it. > But it makes sense to add one. > > What about refactoring along these lines: > > > fpsimd.c: > /* Unconditionally discard the SVE state */ > void task_sve_discard(struct task_struct *task) > { > if (!system_supports_sve()) > return; > > local_bh_disable(); > if (test_and_clear_tsk_thread_flag(task, TIF_SVE)) > sve_to_fpsimd(task); > local_bh_enable(); > } > > process.c: > int arch_dup_task_struct(sturct task_struct *dst, struct task_struct *src) > { > if (current->mm) { > fpsimd_preserve_current_state(); > task_sve_discard(src); > } > > *dst = *src; > > dst->thread.sve_state = NULL; > } > > > This also avoids having to touch dst's thread flags, since now we > are just cloning the task except for assigning NULL to > dst->thread.sve_state. This looks fine to me, the execution of task_sve_discard() is nearly a no-op with the current code. We still have some local_bh_disable/enable() calls, though I don't think it's worth optimising them now (e.g. having a fpsimd_preserve_current_state_and_discard_sve() function with a "sve_discard" argument to task_fpsimd_save() to force this).
On Tue, Oct 03, 2017 at 12:33:03PM +0100, Dave P Martin wrote: > On Wed, Sep 13, 2017 at 07:33:25AM -0700, Catalin Marinas wrote: > > On Thu, Aug 31, 2017 at 06:00:43PM +0100, Dave P Martin wrote: > > > +/* > > > + * Handle SVE state across fork(): > > > + * > > > + * dst and src must not end up with aliases of the same sve_state. > > > + * Because a task cannot fork except in a syscall, we can discard SVE > > > + * state for dst here, so long as we take care to retain the FPSIMD > > > + * subset of the state if SVE is in use. Reallocation of the SVE state > > > + * will be deferred until dst tries to use SVE. > > > + */ > > > +void fpsimd_dup_sve(struct task_struct *dst, struct task_struct const *src) > > > +{ > > > + if (test_and_clear_tsk_thread_flag(dst, TIF_SVE)) { > > > + WARN_ON(dst->mm && !in_syscall(task_pt_regs(dst))); > > > + sve_to_fpsimd(dst); > > > + } > > > + > > > + dst->thread.sve_state = NULL; > > > +} > > > > I first thought the thread flags are not visible in dst yet since > > dup_task_struct() calls arch_dup_task_struct() before > > setup_thread_stack(). However, at the end of the last year we enabled > > CONFIG_THREAD_INFO_IN_TASK_STRUCT. But I don't particularly like relying > > on this. > > > > Anyway, IIUC we don't need sve_to_fpsimd() here. The > > arch_dup_task_struct() already called fpsimd_preserve_current_state() > > for src, so the FPSIMD state (which we care about) is transferred during > > the *dst = *src assignment. So you'd only need the last statement, > > possibly with a different function name like fpsimd_erase_sve (and maybe > > make the function static inline in the header). > > Regarding the intended meanings of the thread flags, does this help? > > --8<-- > > TIF_FOREIGN_FPSTATE's meaning is expanded to cover SVE, but otherwise > unchanged: > > * If a task is running and !TIF_FOREIGN_FPSTATE, then the the CPU > registers of the CPU the task is running on contain the authoritative > FPSIMD/SVE state of the task. The backing memory may be stale. > > * Otherwise (i.e., task not running, or task running and > TIF_FOREIGN_FPSTATE), the task's FPSIMD/SVE backing memory is > authoritative. If additionally per_cpu(fpsimd_last_state, > task->fpsimd_state.cpu) == &task->fpsimd_state.cpu, then > task->fpsimd_state.cpu's registers are also up to date for task, but > not authorititive: the current FPSIMD/SVE state may be read from > them, but they must not be written. > > The FPSIMD/SVE backing memory is selected by TIF_SVE: > > * TIF_SVE set: Zn (incorporating Vn in bits[127:0]), Pn and FFR are > stored in task->thread.sve_state, formatted appropriately for vector > length task->thread.sve_vl. task->thread.sve_state must point to a > valid buffer at least sve_state_size(task) bytes in size. > > * TIF_SVE clear: Vn are stored in task->fpsimd_state; Zn[max : 128] are > logically zero[*] but not stored anywhere; Pn, FFR are not stored and > have unspecified values from userspace's point of view. > task->thread.sve_state does not need to be non-null, valid or any > particular size: it must not be dereferenced. > > In practice I don't exploit the "unspecifiedness" much. The Zn high > bits, Pn and FFR are all zeroed when setting TIF_SVE again: > sve_alloc() is the common path for this. > > * FPSR and FPCR are always stored in task->fpsimd_state irrespctive of > whether TIF_SVE is clear or set, since these are not vector length > dependent. This looks fine. I think we need to make sure (with a warning) that task_fpsimd_save() (and probably load) is always called in a non-preemptible context. > [*] theoretically unspecified, which is what I've written in sve.txt. > However, on any FPSIMD Vn write the upper bits of the corresponding Zn > must become logically zero in order to conform to the SVE programmer's > model. It's not feasible to track which Vn have been written > individually because that would involve trapping every FPSIMD insn until > all possible Vn have been written. So the kernel ensures that the Zn > high bits become zero. > > Maybe we should just guarantee "zero-or-preserved" behaviour for > userspace. This may close down some future optimisation opportunities, > but maybe it's better to be simple. Does it work if we leave it as "unspecified" in the document but just do zero-or-preserved in the kernel code? Just wondering, as an optimisation for do_sve_acc() - instead of sve_alloc() and fpsimd_to_sve(), can we not force the loading of the FPSIMD regs on the return to user via TIF_FOREIGN_FPSTATE? This would ensure the zeroing of the top SVE bits and we only need to allocate the SVE state on the saving path. This means enabling SVE for user and setting TIF_SVE without having the backing storage allocated.
On Thu, Oct 05, 2017 at 12:28:35PM +0100, Catalin Marinas wrote: > On Tue, Oct 03, 2017 at 12:33:03PM +0100, Dave P Martin wrote: > > On Wed, Sep 13, 2017 at 07:33:25AM -0700, Catalin Marinas wrote: > > > On Thu, Aug 31, 2017 at 06:00:43PM +0100, Dave P Martin wrote: > > > > +/* > > > > + * Handle SVE state across fork(): > > > > + * > > > > + * dst and src must not end up with aliases of the same sve_state. > > > > + * Because a task cannot fork except in a syscall, we can discard SVE > > > > + * state for dst here, so long as we take care to retain the FPSIMD > > > > + * subset of the state if SVE is in use. Reallocation of the SVE state > > > > + * will be deferred until dst tries to use SVE. > > > > + */ > > > > +void fpsimd_dup_sve(struct task_struct *dst, struct task_struct const *src) > > > > +{ > > > > + if (test_and_clear_tsk_thread_flag(dst, TIF_SVE)) { > > > > + WARN_ON(dst->mm && !in_syscall(task_pt_regs(dst))); > > > > + sve_to_fpsimd(dst); > > > > + } > > > > + > > > > + dst->thread.sve_state = NULL; > > > > +} > > > > > > I first thought the thread flags are not visible in dst yet since > > > dup_task_struct() calls arch_dup_task_struct() before > > > setup_thread_stack(). However, at the end of the last year we enabled > > > CONFIG_THREAD_INFO_IN_TASK_STRUCT. But I don't particularly like relying > > > on this. > > > > > > Anyway, IIUC we don't need sve_to_fpsimd() here. The > > > arch_dup_task_struct() already called fpsimd_preserve_current_state() > > > for src, so the FPSIMD state (which we care about) is transferred during > > > the *dst = *src assignment. So you'd only need the last statement, > > > possibly with a different function name like fpsimd_erase_sve (and maybe > > > make the function static inline in the header). > > > > Regarding the intended meanings of the thread flags, does this help? > > > > --8<-- > > > > TIF_FOREIGN_FPSTATE's meaning is expanded to cover SVE, but otherwise > > unchanged: > > > > * If a task is running and !TIF_FOREIGN_FPSTATE, then the the CPU > > registers of the CPU the task is running on contain the authoritative > > FPSIMD/SVE state of the task. The backing memory may be stale. > > > > * Otherwise (i.e., task not running, or task running and > > TIF_FOREIGN_FPSTATE), the task's FPSIMD/SVE backing memory is > > authoritative. If additionally per_cpu(fpsimd_last_state, > > task->fpsimd_state.cpu) == &task->fpsimd_state.cpu, then > > task->fpsimd_state.cpu's registers are also up to date for task, but > > not authorititive: the current FPSIMD/SVE state may be read from > > them, but they must not be written. > > > > The FPSIMD/SVE backing memory is selected by TIF_SVE: > > > > * TIF_SVE set: Zn (incorporating Vn in bits[127:0]), Pn and FFR are > > stored in task->thread.sve_state, formatted appropriately for vector > > length task->thread.sve_vl. task->thread.sve_state must point to a > > valid buffer at least sve_state_size(task) bytes in size. > > > > * TIF_SVE clear: Vn are stored in task->fpsimd_state; Zn[max : 128] are > > logically zero[*] but not stored anywhere; Pn, FFR are not stored and > > have unspecified values from userspace's point of view. > > task->thread.sve_state does not need to be non-null, valid or any > > particular size: it must not be dereferenced. > > > > In practice I don't exploit the "unspecifiedness" much. The Zn high > > bits, Pn and FFR are all zeroed when setting TIF_SVE again: > > sve_alloc() is the common path for this. > > > > * FPSR and FPCR are always stored in task->fpsimd_state irrespctive of > > whether TIF_SVE is clear or set, since these are not vector length > > dependent. > > This looks fine. I think we need to make sure (with a warning) that > task_fpsimd_save() (and probably load) is always called in a > non-preemptible context. > > > [*] theoretically unspecified, which is what I've written in sve.txt. > > However, on any FPSIMD Vn write the upper bits of the corresponding Zn > > must become logically zero in order to conform to the SVE programmer's > > model. It's not feasible to track which Vn have been written > > individually because that would involve trapping every FPSIMD insn until > > all possible Vn have been written. So the kernel ensures that the Zn > > high bits become zero. > > > > Maybe we should just guarantee "zero-or-preserved" behaviour for > > userspace. This may close down some future optimisation opportunities, > > but maybe it's better to be simple. > > Does it work if we leave it as "unspecified" in the document but just do > zero-or-preserved in the kernel code? Sure, that's the behaviour today in effect. I'll leave the documentation unchanged, then we can take advantage of this flexibility later if is proves to be useful. > Just wondering, as an optimisation for do_sve_acc() - instead of > sve_alloc() and fpsimd_to_sve(), can we not force the loading of the > FPSIMD regs on the return to user via TIF_FOREIGN_FPSTATE? This would > ensure the zeroing of the top SVE bits and we only need to allocate the > SVE state on the saving path. This means enabling SVE for user and > setting TIF_SVE without having the backing storage allocated. Currently the set of places where the "TIF_SVE implies sve_state valid" assumption is applied is not very constrained, so while your suggestion is reasonable I'd rather not mess with it just now, if possible. But we can do this (which is what my current fixup has): el0_sve_acc: enable_dbg_and_irq // ... bl do_sve_acc b ret_to_user void do_sve_acc(unsigned int esr, struct pt_regs *regs) { /* Even if we chose not to use SVE, the hardware could still trap: */ if (unlikely(!system_supports_sve()) || WARN_ON(is_compat_task())) { force_signal_inject(SIGILL, ILL_ILLOPC, regs, 0); return; } sve_alloc(current); local_bh_disable(); if (test_and_clear_thread_flag(TIF_FOREIGN_FPSTATE)) { task_fpsimd_load(); /* flushes high Zn bits as a side-effect */ sve_flush_pregs(); } else { sve_flush_all(); /* flush all the SVE bits in-place */ } if (test_and_set_thread_flag(TIF_SVE)) WARN_ON(1); /* SVE access shouldn't have trapped */ local_bh_enable(); } where sve_flush_all() zeroes all the high Zn bits via a series of MOV Vn, Vn instructions, and also zeroes Pn and FFR. sve_fplush_pregs() just does the latter. In the common case the sve_alloc() does a memzero, but doesn't allocate memory because more often than not, it will already have been allocated. On this path, the memzero is now pointless -- I'll need to double-check what else may be impacted by its removal (probably only ptrace, and I'm not sure if the zeroing is strictly required there). There is still a bit of room for improvement, but it's still a step up from v2. What do you think? Cheers ---Dave
On Fri, Oct 06, 2017 at 02:10:09PM +0100, Dave P Martin wrote: > On Thu, Oct 05, 2017 at 12:28:35PM +0100, Catalin Marinas wrote: > > On Tue, Oct 03, 2017 at 12:33:03PM +0100, Dave P Martin wrote: > > > TIF_FOREIGN_FPSTATE's meaning is expanded to cover SVE, but otherwise > > > unchanged: > > > > > > * If a task is running and !TIF_FOREIGN_FPSTATE, then the the CPU > > > registers of the CPU the task is running on contain the authoritative > > > FPSIMD/SVE state of the task. The backing memory may be stale. > > > > > > * Otherwise (i.e., task not running, or task running and > > > TIF_FOREIGN_FPSTATE), the task's FPSIMD/SVE backing memory is > > > authoritative. If additionally per_cpu(fpsimd_last_state, > > > task->fpsimd_state.cpu) == &task->fpsimd_state.cpu, then > > > task->fpsimd_state.cpu's registers are also up to date for task, but > > > not authorititive: the current FPSIMD/SVE state may be read from > > > them, but they must not be written. > > > > > > The FPSIMD/SVE backing memory is selected by TIF_SVE: > > > > > > * TIF_SVE set: Zn (incorporating Vn in bits[127:0]), Pn and FFR are > > > stored in task->thread.sve_state, formatted appropriately for vector > > > length task->thread.sve_vl. task->thread.sve_state must point to a > > > valid buffer at least sve_state_size(task) bytes in size. "Zn [...] stored in task->thread.sve_state" - is this still true with the changes you proposed? I guess even without these changes, you have situations where the hardware regs are out of sync with sve_state (see more below). > > > * TIF_SVE clear: Vn are stored in task->fpsimd_state; Zn[max : 128] are > > > logically zero[*] but not stored anywhere; Pn, FFR are not stored and > > > have unspecified values from userspace's point of view. > > > task->thread.sve_state does not need to be non-null, valid or any > > > particular size: it must not be dereferenced. > > > > > > In practice I don't exploit the "unspecifiedness" much. The Zn high > > > bits, Pn and FFR are all zeroed when setting TIF_SVE again: > > > sve_alloc() is the common path for this. > > > > > > * FPSR and FPCR are always stored in task->fpsimd_state irrespctive of > > > whether TIF_SVE is clear or set, since these are not vector length > > > dependent. [...] > > Just wondering, as an optimisation for do_sve_acc() - instead of > > sve_alloc() and fpsimd_to_sve(), can we not force the loading of the > > FPSIMD regs on the return to user via TIF_FOREIGN_FPSTATE? This would > > ensure the zeroing of the top SVE bits and we only need to allocate the > > SVE state on the saving path. This means enabling SVE for user and > > setting TIF_SVE without having the backing storage allocated. > > Currently the set of places where the "TIF_SVE implies sve_state valid" > assumption is applied is not very constrained, so while your suggestion > is reasonable I'd rather not mess with it just now, if possible. > > > But we can do this (which is what my current fixup has): > > el0_sve_acc: > enable_dbg_and_irq > // ... > bl do_sve_acc > b ret_to_user > > void do_sve_acc(unsigned int esr, struct pt_regs *regs) > { > /* Even if we chose not to use SVE, the hardware could still trap: */ > if (unlikely(!system_supports_sve()) || WARN_ON(is_compat_task())) { > force_signal_inject(SIGILL, ILL_ILLOPC, regs, 0); > return; > } > > sve_alloc(current); > > local_bh_disable(); > if (test_and_clear_thread_flag(TIF_FOREIGN_FPSTATE)) { > task_fpsimd_load(); /* flushes high Zn bits as a side-effect */ > sve_flush_pregs(); > } else { > sve_flush_all(); /* flush all the SVE bits in-place */ > } > > if (test_and_set_thread_flag(TIF_SVE)) > WARN_ON(1); /* SVE access shouldn't have trapped */ > local_bh_enable(); > } > > where sve_flush_all() zeroes all the high Zn bits via a series of > MOV Vn, Vn instructions, and also zeroes Pn and FFR. sve_fplush_pregs() > just does the latter. This looks fine to me but I added a comment above. IIUC, we can now have TIF_SVE set while sve_state contains stale data. I don't see an issue given that every time you enter the kernel from user space you have TIF_SVE set and the sve_state storage out of sync. Maybe tweak the TIF_SVE description above slightly.
On Fri, Oct 06, 2017 at 02:36:40PM +0100, Catalin Marinas wrote: > On Fri, Oct 06, 2017 at 02:10:09PM +0100, Dave P Martin wrote: > > On Thu, Oct 05, 2017 at 12:28:35PM +0100, Catalin Marinas wrote: > > > On Tue, Oct 03, 2017 at 12:33:03PM +0100, Dave P Martin wrote: > > > > TIF_FOREIGN_FPSTATE's meaning is expanded to cover SVE, but otherwise > > > > unchanged: > > > > > > > > * If a task is running and !TIF_FOREIGN_FPSTATE, then the the CPU > > > > registers of the CPU the task is running on contain the authoritative > > > > FPSIMD/SVE state of the task. The backing memory may be stale. > > > > > > > > * Otherwise (i.e., task not running, or task running and > > > > TIF_FOREIGN_FPSTATE), the task's FPSIMD/SVE backing memory is > > > > authoritative. If additionally per_cpu(fpsimd_last_state, > > > > task->fpsimd_state.cpu) == &task->fpsimd_state.cpu, then > > > > task->fpsimd_state.cpu's registers are also up to date for task, but > > > > not authorititive: the current FPSIMD/SVE state may be read from > > > > them, but they must not be written. > > > > > > > > The FPSIMD/SVE backing memory is selected by TIF_SVE: > > > > > > > > * TIF_SVE set: Zn (incorporating Vn in bits[127:0]), Pn and FFR are > > > > stored in task->thread.sve_state, formatted appropriately for vector > > > > length task->thread.sve_vl. task->thread.sve_state must point to a > > > > valid buffer at least sve_state_size(task) bytes in size. > > "Zn [...] stored in task->thread.sve_state" - is this still true with > the changes you proposed? I guess even without these changes, you have > situations where the hardware regs are out of sync with sve_state (see > more below). I guess I need to tweak the wording here. TIF_SVE says where the vector state should be loaded/stored from, but does not say whether the data is up to date in memory, or when it should be loaded/stored. The latter is described by a cocktail of different things including which bit of kernel code we are executing if any, whether the task is running/stopped etc., TIF_FOREIGN_FPSTATE, task->thread.fpsimd_state.cpu and per_cpu(fpsimd_last_state). Does this make any better sense of my code below? > > > > > * TIF_SVE clear: Vn are stored in task->fpsimd_state; Zn[max : 128] are > > > > logically zero[*] but not stored anywhere; Pn, FFR are not stored and > > > > have unspecified values from userspace's point of view. > > > > task->thread.sve_state does not need to be non-null, valid or any > > > > particular size: it must not be dereferenced. > > > > > > > > In practice I don't exploit the "unspecifiedness" much. The Zn high > > > > bits, Pn and FFR are all zeroed when setting TIF_SVE again: > > > > sve_alloc() is the common path for this. > > > > > > > > * FPSR and FPCR are always stored in task->fpsimd_state irrespctive of > > > > whether TIF_SVE is clear or set, since these are not vector length > > > > dependent. > [...] > > > Just wondering, as an optimisation for do_sve_acc() - instead of > > > sve_alloc() and fpsimd_to_sve(), can we not force the loading of the > > > FPSIMD regs on the return to user via TIF_FOREIGN_FPSTATE? This would > > > ensure the zeroing of the top SVE bits and we only need to allocate the > > > SVE state on the saving path. This means enabling SVE for user and > > > setting TIF_SVE without having the backing storage allocated. > > > > Currently the set of places where the "TIF_SVE implies sve_state valid" > > assumption is applied is not very constrained, so while your suggestion > > is reasonable I'd rather not mess with it just now, if possible. > > > > > > But we can do this (which is what my current fixup has): > > > > el0_sve_acc: > > enable_dbg_and_irq > > // ... > > bl do_sve_acc > > b ret_to_user > > > > void do_sve_acc(unsigned int esr, struct pt_regs *regs) > > { > > /* Even if we chose not to use SVE, the hardware could still trap: */ > > if (unlikely(!system_supports_sve()) || WARN_ON(is_compat_task())) { > > force_signal_inject(SIGILL, ILL_ILLOPC, regs, 0); > > return; > > } > > > > sve_alloc(current); > > > > local_bh_disable(); > > if (test_and_clear_thread_flag(TIF_FOREIGN_FPSTATE)) { > > task_fpsimd_load(); /* flushes high Zn bits as a side-effect */ > > sve_flush_pregs(); > > } else { > > sve_flush_all(); /* flush all the SVE bits in-place */ > > } > > > > if (test_and_set_thread_flag(TIF_SVE)) > > WARN_ON(1); /* SVE access shouldn't have trapped */ > > local_bh_enable(); > > } > > > > where sve_flush_all() zeroes all the high Zn bits via a series of > > MOV Vn, Vn instructions, and also zeroes Pn and FFR. sve_fplush_pregs() > > just does the latter. > > This looks fine to me but I added a comment above. IIUC, we can now have > TIF_SVE set while sve_state contains stale data. I don't see an issue > given that every time you enter the kernel from user space you have > TIF_SVE set and the sve_state storage out of sync. Maybe tweak the > TIF_SVE description above slightly. > See my comment above ... any better? If so, I'll paste some of that explanatory text into fpsimd.c (in lieu of a better place to put it). Cheers ---Dave
On Fri, Oct 06, 2017 at 04:15:28PM +0100, Dave P Martin wrote: > On Fri, Oct 06, 2017 at 02:36:40PM +0100, Catalin Marinas wrote: > > On Fri, Oct 06, 2017 at 02:10:09PM +0100, Dave P Martin wrote: > > > On Thu, Oct 05, 2017 at 12:28:35PM +0100, Catalin Marinas wrote: > > > > On Tue, Oct 03, 2017 at 12:33:03PM +0100, Dave P Martin wrote: > > > > > TIF_FOREIGN_FPSTATE's meaning is expanded to cover SVE, but otherwise > > > > > unchanged: > > > > > > > > > > * If a task is running and !TIF_FOREIGN_FPSTATE, then the the CPU > > > > > registers of the CPU the task is running on contain the authoritative > > > > > FPSIMD/SVE state of the task. The backing memory may be stale. > > > > > > > > > > * Otherwise (i.e., task not running, or task running and > > > > > TIF_FOREIGN_FPSTATE), the task's FPSIMD/SVE backing memory is > > > > > authoritative. If additionally per_cpu(fpsimd_last_state, > > > > > task->fpsimd_state.cpu) == &task->fpsimd_state.cpu, then > > > > > task->fpsimd_state.cpu's registers are also up to date for task, but > > > > > not authorititive: the current FPSIMD/SVE state may be read from > > > > > them, but they must not be written. > > > > > > > > > > The FPSIMD/SVE backing memory is selected by TIF_SVE: > > > > > > > > > > * TIF_SVE set: Zn (incorporating Vn in bits[127:0]), Pn and FFR are > > > > > stored in task->thread.sve_state, formatted appropriately for vector > > > > > length task->thread.sve_vl. task->thread.sve_state must point to a > > > > > valid buffer at least sve_state_size(task) bytes in size. > > > > "Zn [...] stored in task->thread.sve_state" - is this still true with > > the changes you proposed? I guess even without these changes, you have > > situations where the hardware regs are out of sync with sve_state (see > > more below). > > I guess I need to tweak the wording here. > > TIF_SVE says where the vector state should be loaded/stored from, > but does not say whether the data is up to date in memory, or when > it should be loaded/stored. > > The latter is described by a cocktail of different things including > which bit of kernel code we are executing if any, whether the task > is running/stopped etc., TIF_FOREIGN_FPSTATE, > task->thread.fpsimd_state.cpu and per_cpu(fpsimd_last_state). > > Does this make any better sense of my code below? Yes, it looks fine.
diff --git a/arch/arm64/include/asm/fpsimd.h b/arch/arm64/include/asm/fpsimd.h index 026a7c7..72090a1 100644 --- a/arch/arm64/include/asm/fpsimd.h +++ b/arch/arm64/include/asm/fpsimd.h @@ -20,6 +20,8 @@ #ifndef __ASSEMBLY__ +#include <linux/stddef.h> + /* * FP/SIMD storage area has: * - FPSR and FPCR @@ -72,6 +74,23 @@ extern void sve_load_state(void const *state, u32 const *pfpsr, unsigned long vq_minus_1); extern unsigned int sve_get_vl(void); +#ifdef CONFIG_ARM64_SVE + +extern size_t sve_state_size(struct task_struct const *task); + +extern void sve_alloc(struct task_struct *task); +extern void fpsimd_release_thread(struct task_struct *task); +extern void fpsimd_dup_sve(struct task_struct *dst, + struct task_struct const *src); + +#else /* ! CONFIG_ARM64_SVE */ + +static void __maybe_unused sve_alloc(struct task_struct *task) { } +static void __maybe_unused fpsimd_release_thread(struct task_struct *task) { } +static void __maybe_unused fpsimd_dup_sve(struct task_struct *dst, + struct task_struct const *src) { } +#endif /* ! CONFIG_ARM64_SVE */ + /* For use by EFI runtime services calls only */ extern void __efi_fpsimd_begin(void); extern void __efi_fpsimd_end(void); diff --git a/arch/arm64/include/asm/processor.h b/arch/arm64/include/asm/processor.h index 29adab8..4831d28 100644 --- a/arch/arm64/include/asm/processor.h +++ b/arch/arm64/include/asm/processor.h @@ -85,6 +85,8 @@ struct thread_struct { unsigned long tp2_value; #endif struct fpsimd_state fpsimd_state; + void *sve_state; /* SVE registers, if any */ + unsigned int sve_vl; /* SVE vector length */ unsigned long fault_address; /* fault info */ unsigned long fault_code; /* ESR_EL1 value */ struct debug_info debug; /* debugging */ diff --git a/arch/arm64/include/asm/thread_info.h b/arch/arm64/include/asm/thread_info.h index 2eca178..f0880fc 100644 --- a/arch/arm64/include/asm/thread_info.h +++ b/arch/arm64/include/asm/thread_info.h @@ -91,6 +91,7 @@ void arch_setup_new_exec(void); #define TIF_RESTORE_SIGMASK 20 #define TIF_SINGLESTEP 21 #define TIF_32BIT 22 /* 32bit process */ +#define TIF_SVE 23 /* Scalable Vector Extension in use */ #define _TIF_SIGPENDING (1 << TIF_SIGPENDING) #define _TIF_NEED_RESCHED (1 << TIF_NEED_RESCHED) diff --git a/arch/arm64/include/asm/traps.h b/arch/arm64/include/asm/traps.h index 4136168..282163f 100644 --- a/arch/arm64/include/asm/traps.h +++ b/arch/arm64/include/asm/traps.h @@ -34,6 +34,8 @@ struct undef_hook { void register_undef_hook(struct undef_hook *hook); void unregister_undef_hook(struct undef_hook *hook); +void force_signal_inject(int signal, int code, struct pt_regs *regs, + unsigned long address); void arm64_notify_segfault(struct pt_regs *regs, unsigned long addr); diff --git a/arch/arm64/kernel/entry.S b/arch/arm64/kernel/entry.S index e1c59d4..e57020f 100644 --- a/arch/arm64/kernel/entry.S +++ b/arch/arm64/kernel/entry.S @@ -607,6 +607,8 @@ el0_sync: b.eq el0_ia cmp x24, #ESR_ELx_EC_FP_ASIMD // FP/ASIMD access b.eq el0_fpsimd_acc + cmp x24, #ESR_ELx_EC_SVE // SVE access + b.eq el0_sve_acc cmp x24, #ESR_ELx_EC_FP_EXC64 // FP/ASIMD exception b.eq el0_fpsimd_exc cmp x24, #ESR_ELx_EC_SYS64 // configurable trap @@ -705,9 +707,19 @@ el0_fpsimd_acc: mov x1, sp bl do_fpsimd_acc b ret_to_user +el0_sve_acc: + /* + * Scalable Vector Extension access + */ + enable_dbg + ct_user_exit + mov x0, x25 + mov x1, sp + bl do_sve_acc + b ret_to_user el0_fpsimd_exc: /* - * Floating Point or Advanced SIMD exception + * Floating Point, Advanced SIMD or SVE exception */ enable_dbg ct_user_exit diff --git a/arch/arm64/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c index 9d762dd..9b1ebd7 100644 --- a/arch/arm64/kernel/fpsimd.c +++ b/arch/arm64/kernel/fpsimd.c @@ -18,18 +18,25 @@ */ #include <linux/bottom_half.h> +#include <linux/bug.h> +#include <linux/compat.h> #include <linux/cpu.h> #include <linux/cpu_pm.h> #include <linux/kernel.h> #include <linux/init.h> #include <linux/percpu.h> #include <linux/preempt.h> +#include <linux/ptrace.h> #include <linux/sched/signal.h> #include <linux/signal.h> +#include <linux/slab.h> #include <asm/fpsimd.h> #include <asm/cputype.h> #include <asm/simd.h> +#include <asm/sigcontext.h> +#include <asm/sysreg.h> +#include <asm/traps.h> #define FPEXC_IOF (1 << 0) #define FPEXC_DZF (1 << 1) @@ -99,6 +106,190 @@ */ static DEFINE_PER_CPU(struct fpsimd_state *, fpsimd_last_state); +static void sve_free(struct task_struct *task) +{ + kfree(task->thread.sve_state); + task->thread.sve_state = NULL; +} + +/* Offset of FFR in the SVE register dump */ +static size_t sve_ffr_offset(int vl) +{ + return SVE_SIG_FFR_OFFSET(sve_vq_from_vl(vl)) - SVE_SIG_REGS_OFFSET; +} + +static void *sve_pffr(struct task_struct *task) +{ + return (char *)task->thread.sve_state + + sve_ffr_offset(task->thread.sve_vl); +} + +static void change_cpacr(u64 val, u64 mask) +{ + u64 cpacr = read_sysreg(CPACR_EL1); + u64 new = (cpacr & ~mask) | val; + + if (new != cpacr) + write_sysreg(new, CPACR_EL1); +} + +static void sve_user_disable(void) +{ + change_cpacr(0, CPACR_EL1_ZEN_EL0EN); +} + +static void sve_user_enable(void) +{ + change_cpacr(CPACR_EL1_ZEN_EL0EN, CPACR_EL1_ZEN_EL0EN); +} + +static void task_fpsimd_load(void) +{ + if (system_supports_sve() && test_thread_flag(TIF_SVE)) + sve_load_state(sve_pffr(current), + ¤t->thread.fpsimd_state.fpsr, + sve_vq_from_vl(current->thread.sve_vl) - 1); + else + fpsimd_load_state(¤t->thread.fpsimd_state); + + if (system_supports_sve()) { + /* Toggle SVE trapping for userspace if needed */ + if (test_thread_flag(TIF_SVE)) + sve_user_enable(); + else + sve_user_disable(); + + /* Serialised by exception return to user */ + } +} + +static void task_fpsimd_save(void) +{ + if (system_supports_sve() && + in_syscall(current_pt_regs()) && + test_thread_flag(TIF_SVE)) { + clear_thread_flag(TIF_SVE); + + /* Trap if the task tries to use SVE again: */ + sve_user_disable(); + } + + if (!test_thread_flag(TIF_FOREIGN_FPSTATE)) { + if (system_supports_sve() && test_thread_flag(TIF_SVE)) + sve_save_state(sve_pffr(current), + ¤t->thread.fpsimd_state.fpsr); + else + fpsimd_save_state(¤t->thread.fpsimd_state); + } +} + +#define ZREG(sve_state, vq, n) ((char *)(sve_state) + \ + (SVE_SIG_ZREG_OFFSET(vq, n) - SVE_SIG_REGS_OFFSET)) + +static void fpsimd_to_sve(struct task_struct *task) +{ + unsigned int vq; + void *sst = task->thread.sve_state; + struct fpsimd_state const *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(ZREG(sst, vq, i), &fst->vregs[i], + sizeof(fst->vregs[i])); +} + +#ifdef CONFIG_ARM64_SVE + +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])); +} + +size_t sve_state_size(struct task_struct const *task) +{ + return SVE_SIG_REGS_SIZE(sve_vq_from_vl(task->thread.sve_vl)); +} + +void sve_alloc(struct task_struct *task) +{ + if (task->thread.sve_state) { + memset(task->thread.sve_state, 0, sve_state_size(current)); + return; + } + + /* This is a small allocation (maximum ~8KB) and Should Not Fail. */ + task->thread.sve_state = + kzalloc(sve_state_size(task), GFP_KERNEL); + + /* + * If future SVE revisions can have larger vectors though, + * this may cease to be true: + */ + BUG_ON(!task->thread.sve_state); +} + +/* + * Handle SVE state across fork(): + * + * dst and src must not end up with aliases of the same sve_state. + * Because a task cannot fork except in a syscall, we can discard SVE + * state for dst here, so long as we take care to retain the FPSIMD + * subset of the state if SVE is in use. Reallocation of the SVE state + * will be deferred until dst tries to use SVE. + */ +void fpsimd_dup_sve(struct task_struct *dst, struct task_struct const *src) +{ + if (test_and_clear_tsk_thread_flag(dst, TIF_SVE)) { + WARN_ON(dst->mm && !in_syscall(task_pt_regs(dst))); + sve_to_fpsimd(dst); + } + + dst->thread.sve_state = NULL; +} + +void fpsimd_release_thread(struct task_struct *dead_task) +{ + sve_free(dead_task); +} + +#endif /* CONFIG_ARM64_SVE */ + +/* + * Trapped SVE access + */ +void do_sve_acc(unsigned int esr, struct pt_regs *regs) +{ + /* Even if we chose not to use SVE, the hardware could still trap: */ + if (unlikely(!system_supports_sve()) || WARN_ON(is_compat_task())) { + force_signal_inject(SIGILL, ILL_ILLOPC, regs, 0); + return; + } + + task_fpsimd_save(); + + sve_alloc(current); + fpsimd_to_sve(current); + if (test_and_set_thread_flag(TIF_SVE)) + WARN_ON(1); /* SVE access shouldn't have trapped */ + + task_fpsimd_load(); +} + /* * Trapped FP/ASIMD access. */ @@ -144,8 +335,8 @@ void fpsimd_thread_switch(struct task_struct *next) * the registers is in fact the most recent userland FPSIMD state of * 'current'. */ - if (current->mm && !test_thread_flag(TIF_FOREIGN_FPSTATE)) - fpsimd_save_state(¤t->thread.fpsimd_state); + if (current->mm) + task_fpsimd_save(); if (next->mm) { /* @@ -167,6 +358,8 @@ void fpsimd_thread_switch(struct task_struct *next) void fpsimd_flush_thread(void) { + int vl; + if (!system_supports_fpsimd()) return; @@ -174,6 +367,30 @@ void fpsimd_flush_thread(void) memset(¤t->thread.fpsimd_state, 0, sizeof(struct fpsimd_state)); fpsimd_flush_task_state(current); + + if (system_supports_sve()) { + clear_thread_flag(TIF_SVE); + sve_free(current); + + /* + * Reset the task vector length as required. + * This is where we ensure that all user tasks have a valid + * vector length configured: no kernel task can become a user + * task without an exec and hence a call to this function. + * If a bug causes this to go wrong, we make some noise and + * try to fudge thread.sve_vl to a safe value here. + */ + vl = current->thread.sve_vl; + + if (vl == 0) + vl = SVE_VL_MIN; + + if (WARN_ON(!sve_vl_valid(vl))) + vl = SVE_VL_MIN; + + current->thread.sve_vl = vl; + } + set_thread_flag(TIF_FOREIGN_FPSTATE); local_bh_enable(); @@ -211,7 +428,7 @@ void fpsimd_restore_current_state(void) if (test_and_clear_thread_flag(TIF_FOREIGN_FPSTATE)) { struct fpsimd_state *st = ¤t->thread.fpsimd_state; - fpsimd_load_state(st); + task_fpsimd_load(); __this_cpu_write(fpsimd_last_state, st); st->cpu = smp_processor_id(); } @@ -376,8 +593,8 @@ static int fpsimd_cpu_pm_notifier(struct notifier_block *self, { switch (cmd) { case CPU_PM_ENTER: - if (current->mm && !test_thread_flag(TIF_FOREIGN_FPSTATE)) - fpsimd_save_state(¤t->thread.fpsimd_state); + if (current->mm) + task_fpsimd_save(); this_cpu_write(fpsimd_last_state, NULL); break; case CPU_PM_EXIT: diff --git a/arch/arm64/kernel/process.c b/arch/arm64/kernel/process.c index e6bf19c..eb1cae7 100644 --- a/arch/arm64/kernel/process.c +++ b/arch/arm64/kernel/process.c @@ -239,6 +239,7 @@ void flush_thread(void) void release_thread(struct task_struct *dead_task) { + fpsimd_release_thread(dead_task); } int arch_dup_task_struct(struct task_struct *dst, struct task_struct *src) @@ -246,6 +247,9 @@ int arch_dup_task_struct(struct task_struct *dst, struct task_struct *src) if (current->mm) fpsimd_preserve_current_state(); *dst = *src; + + fpsimd_dup_sve(dst, src); + return 0; } diff --git a/arch/arm64/kernel/traps.c b/arch/arm64/kernel/traps.c index f202932..7f3b5c6 100644 --- a/arch/arm64/kernel/traps.c +++ b/arch/arm64/kernel/traps.c @@ -358,8 +358,8 @@ static int call_undef_hook(struct pt_regs *regs) return fn ? fn(regs, instr) : 1; } -static void force_signal_inject(int signal, int code, struct pt_regs *regs, - unsigned long address) +void force_signal_inject(int signal, int code, struct pt_regs *regs, + unsigned long address) { siginfo_t info; void __user *pc = (void __user *)instruction_pointer(regs);
This patch adds the core support for switching and managing the SVE architectural state of user tasks. Calls to the existing FPSIMD low-level save/restore functions are factored out as new functions task_fpsimd_{save,load}(), since SVE now dynamically may or may not need to be handled at these points depending on the kernel configuration, hardware features discovered at boot, and the runtime state of the task. To make these decisions as fast as possible, const cpucaps are used where feasible, via the system_supports_sve() helper. The SVE registers are only tracked for threads that have explicitly used SVE, indicated by the new thread flag TIF_SVE. Otherwise, the FPSIMD view of the architectural state is stored in thread.fpsimd_state as usual. When in use, the SVE registers are not stored directly in thread_struct due to their potentially large and variable size. Because the task_struct slab allocator must be configured very early during kernel boot, it is also tricky to configure it correctly to match the maximum vector length provided by the hardware, since this depends on examining secondary CPUs as well as the primary. Instead, a pointer sve_state in thread_struct points to a dynamically allocated buffer containing the SVE register data, and code is added to allocate, duplicate and free this buffer at appropriate times. TIF_SVE is set when taking an SVE access trap from userspace, if suitable hardware support has been detected. This enables SVE for the thread: a subsequent return to userspace will disable the trap accordingly. If such a trap is taken without sufficient hardware support, SIGILL is sent to the thread instead as if an undefined instruction had been executed: this may happen if userspace tries to use SVE in a system where not all CPUs support it for example. The kernel may clear TIF_SVE and disable SVE for the thread whenever an explicit syscall is made by userspace, though this is considered an optimisation opportunity rather than a deterministic guarantee: the kernel may not do this on every syscall, but it is permitted to do so. For backwards compatibility reasons and conformance with the spirit of the base AArch64 procedure call standard, the subset of the SVE register state that aliases the FPSIMD registers is still preserved across a syscall even if this happens. TIF_SVE is also cleared, and SVE disabled, on exec: this is an obvious slow path and a hint that we are running a new binary that may not use SVE. Code is added to sync data between thread.fpsimd_state and thread.sve_state whenever enabling/disabling SVE, in a manner consistent with the SVE architectural programmer's model. Signed-off-by: Dave Martin <Dave.Martin@arm.com> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org> Cc: Alex Bennée <alex.bennee@linaro.org> --- Changes since v1 ---------------- Requested by Ard Biesheuvel: * Fix unbalanced ifelse bracing to match kernel coding style. * Improve abstraction of change_cpacr to be a bit less clunky. * Rearrange task_fpsimd_{load,save}() and friends to eliminate forward declarations. Changes related to Ard Biesheuvel's comments: * Undo suprious replacement of assignment by memset in arch_dup_task_struct(). Requested by Alex Bennée: * Add missing include of <linux/compat.h>. * Make thread_struct.sve_vl an unsigned int: that's the type used virtually everywhere else, and the memory saving is too minor to be interesting. * Thin out BUG_ON()s: Redundant BUG_ON()s and ones that just check invariants are removed. Important sanity-checks are migrated to WARN_ON()s, with some minimal best-effort patch-up code. The fpsimd_dup_sve() case is changed to a WARN_ON, for user threads only, and only if actually clearing TIF_SVE. Other: * [bugfix] Unconditionally drop child task's reference to the parent's SVE state on fork. Otherwise we can end up with it aliased, which is bad. --- arch/arm64/include/asm/fpsimd.h | 19 +++ arch/arm64/include/asm/processor.h | 2 + arch/arm64/include/asm/thread_info.h | 1 + arch/arm64/include/asm/traps.h | 2 + arch/arm64/kernel/entry.S | 14 ++- arch/arm64/kernel/fpsimd.c | 227 ++++++++++++++++++++++++++++++++++- arch/arm64/kernel/process.c | 4 + arch/arm64/kernel/traps.c | 4 +- 8 files changed, 265 insertions(+), 8 deletions(-)