Re: [PATCH v2 11/28] arm64/sve: Core task context handling
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) == >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. -- Catalin ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH v2 11/28] arm64/sve: Core task context handling
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) == >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
>
Re: [PATCH v2 11/28] arm64/sve: Core task context handling
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) == >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.
--
Catalin
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH v2 11/28] arm64/sve: Core task context handling
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) == >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
Re: [PATCH v2 11/28] arm64/sve: Core task context handling
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) == >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.
--
Catalin
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
Re: [PATCH v2 11/28] arm64/sve: Core task context handling
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
Re: [PATCH v2 11/28] arm64/sve: Core task context handling
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) == >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
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH v2 11/28] arm64/sve: Core task context handling
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
Re: [PATCH v2 11/28] arm64/sve: Core task context handling
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).
--
Catalin
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH v2 11/28] arm64/sve: Core task context handling
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.
--
Catalin
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH v2 11/28] arm64/sve: Core task context handling
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
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH v2 11/28] arm64/sve: Core task context handling
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
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH v2 11/28] arm64/sve: Core task context handling
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);
--
Catalin
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH v2 11/28] arm64/sve: Core task context handling
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 ;)).
--
Catalin
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH v2 11/28] arm64/sve: Core task context handling
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;
> }
--
Catalin
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
