On Fri, Oct 13, 2017 at 05:16:39PM +0100, Dave P Martin wrote:
> On Thu, Oct 12, 2017 at 06:06:32PM +0100, Catalin Marinas wrote:
> > On Tue, Oct 10, 2017 at 07:38:36PM +0100, Dave P Martin wrote:
> > > @@ -702,6 +737,211 @@ static int system_call_set(struct task_struct *target,
> > > return ret;
> > > }
> > >
> > > +#ifdef CONFIG_ARM64_SVE
> > > +
> > > +static void sve_init_header_from_task(struct user_sve_header *header,
> > > + struct task_struct *target)
> > > +{
> > > + unsigned int vq;
> > > +
> > > + memset(header, 0, sizeof(*header));
> > > +
> > > + header->flags = test_tsk_thread_flag(target, TIF_SVE) ?
> > > + SVE_PT_REGS_SVE : SVE_PT_REGS_FPSIMD;
> >
> > For PTRACE_SYSCALL, we may or may not have TIF_SVE depending on what
> > happened with the target. Just a thought: shall we clear TIF_SVE (and
> > sync it to fpsimd) in syscall_trace_enter()?
>
> I'm not so sure: if we were to do that, a syscall that is cancelled by
> writing -1 to REGSET_SYSCALL could still discard the SVE registers as a
> side-effect.
>
> The target committed to discard by executing SVC, but my feeling is
> that cancellation of a syscall in this way shouldn't have avoidable
> side-effects for the target. But the semantics of cancelled syscalls
> are a bit of a grey area, so I can see potential arguments on both
> sides.
We already can't guarantee this - if the task invoking a syscall gets
preempted before syscall_trace_enter(), TIF_SVE will be cleared. Are you
reinstating TIF_SVE if the syscall was cancelled?
So my comment was more about consistency on presenting the SVE state to
the debugger handling PTRACE_SYSCALL.
> > > + /* Registers: FPSIMD-only case */
> > > +
> > > + BUILD_BUG_ON(SVE_PT_FPSIMD_OFFSET != sizeof(header));
> > > + if ((header.flags & SVE_PT_REGS_MASK) == SVE_PT_REGS_FPSIMD) {
> > > + sve_sync_to_fpsimd(target);
> > > +
> > > + ret = __fpr_set(target, regset, pos, count, kbuf, ubuf,
> > > + SVE_PT_FPSIMD_OFFSET);
> > > + clear_tsk_thread_flag(target, TIF_SVE);
> > > + goto out;
> > > + }
> >
> > __fpr_set() already calls sve_sync_to_fpsimd(). Anyway, do you actually
>
> Yes, the call to sve_sync_to_fpsimd() is superfluous here -- I think
> that I realised that all callers of __fpr_set() need this to happen,
> but never deleted the explicit call from sve_set().
>
> I'll delete it.
>
>
> Looking more closely at __fpr_set() though, I think it needs this change
> too, because the sync is unintentionally placed after reading
> thread.fpsimd_state instead of before:
>
> @@ -652,11 +652,12 @@ static int __fpr_set(struct task_struct *target,
> unsigned int start_pos)
> {
> int ret;
> - struct user_fpsimd_state newstate =
> - target->thread.fpsimd_state.user_fpsimd;
> + struct user_fpsimd_state newstate;
>
> sve_sync_to_fpsimd(target);
>
> + newstate = target->thread.fpsimd_state.user_fpsimd;
> +
> ret = user_regset_copyin(&pos, &count, &kbuf, &ubuf, &newstate,
> [...]
>
> (Or were you confident that this was already OK? Maybe I'm confusing
> myself.)
With the sve_sync_to_fpsimd() called before __fpr_set(), it was ok. Once
you removed that you indeed need the change to __fpr_set().
> > need this since we are going to override the FPSIMD state anyway here.
>
> The underlying reason for this is the issue of what should happen
> for short regset writes. Historically, writes through fpr_set() can
> be truncated arbitrarily, and the rest of fpsimd_state will remain
> unchanged.
Ah, I forgot about this.
> The issue is that if TIF_SVE is set, fpsimd_state can be stale for
> target. If the initial sve_sync_to_fpsimd() is removed in sve_set()
> above, then we may resurrect old values for the untouched registers,
> instead of simply leaving them unmodified.
>
> Should I add comments explaining the purpose? I guess it is rather
> non-obvious.
Yes, please.
> > > + set_tsk_thread_flag(target, TIF_SVE);
> >
> > This has the side-effect of enabling TIF_SVE even for PTRACE_SYSCALL
> > which may be cleared in some circumstances. It may not be an issue
> > though.
>
> I would argue that this is correct behaviour: the syscall enter trap and
> exit traps should both behave as if they are outside the syscall,
> allowing the debugger to simulate the effect of inserting any
> instructions the target could have inserted before or after the SVC.
> This may include simulating SVE instructions or modifying SVE regs,
> which would require TIF_SVE to get set.
I'm ok with this approach but I'm not sure we can guarantee it with
preemption enabled.
> If the tracer doesn't cancel the syscall at the enter trap, then a
> real syscall will execute and that may cause the SVE state to be
> discarded in the usual way in the case of preemption or blocking: it
> seems cleaner for the debug illusion that this decision isn't made
> differently just because the target is being traced.
>
> (Spoiler alert though: the syscall exit trap will force the target
> to be scheduled out, which will force discard with the current
> task_fpsimd_save() behaviour ... but that could be changed in the
> future, and I prefer not to document any sort of guarantee here.)
If such state isn't guaranteed, then the de-facto ABI is that the
debugger cannot simulate any SVE instruction via NO_SYSCALL since the
SVE state may be discarded. So it can only rely on what's guaranteed and
changing the behaviour later won't actually help.
--
Catalin
