[v3,19/28] arm64/sve: ptrace and ELF coredump support

This patch defines and implements a new regset NT_ARM_SVE, which
describes a thread's SVE register state.  This allows a debugger to
manipulate the SVE state, as well as being included in ELF
coredumps for post-mortem debugging.

Because the regset size and layout are dependent on the thread's
current vector length, it is not possible to define a C struct to
describe the regset contents as is done for existing regsets.
Instead, and for the same reasons, NT_ARM_SVE is based on the
freeform variable-layout approach used for the SVE signal frame.

Additionally, to reduce debug overhead when debugging threads that
might or might not have live SVE register state, NT_ARM_SVE may be
presented in one of two different formats: the old struct
user_fpsimd_state format is embedded for describing the state of a
thread with no live SVE state, whereas a new variable-layout
structure is embedded for describing live SVE state.  This avoids a
debugger needing to poll NT_PRFPREG in addition to NT_ARM_SVE, and
allows existing userspace code to handle the non-SVE case without
too much modification.

For this to work, NT_ARM_SVE is defined with a fixed-format header
of type struct user_sve_header, which the recipient can use to
figure out the content, size and layout of the reset of the regset.
Accessor macros are defined to allow the vector-length-dependent
parts of the regset to be manipulated.

Signed-off-by: Alan Hayward <alan.hayward@arm.com>
Signed-off-by: Dave Martin <Dave.Martin@arm.com>
Cc: Alex Bennée <alex.bennee@linaro.org>
Cc: Okamoto Takayuki <tokamoto@jp.fujitsu.com>

---

Dropped Alex Bennée's reviewed-by, since bug fixes have been applied.

Changes since v2
----------------

Bug fixes:

 * Initialised user_sve_header.sve_max_vl properly in
   sve_init_header_from_task().  (Reported by Okamoto Takayuki.)

   This bug was introduced by the refactoring since v1 to remove
   BUG_ON()s.  A WARN_ON() was introduced to fix up the error case
   here, but the code for the non-error case was lost.

   This resulted in userspace seeing max_size == 0 when reading
   NT_ARM_SVE with PTRACE_GETREGSET.  NT_ARM_SVE would also get
   truncated in coredmups (though I've not tested that).

   This fix ensures that max_size is initialised, and only overrides
   the value written if it would be garbage.

 * In sve_set(), the flags for sve_set_vector_length() are shifted
   into the correct position.  Without this, a PTRACE_SETREGSET for
   NT_ARM_SVE may reject valid flags (including flags read via
   PTRACE_GETREGSET) with -EINVAL: thus, legitimate uses including
   save/restore may not work.

Miscellaneous:

 * Clarified comment in ptrace.h about keeping flags in sync with
   prctl.h so that it won't be misinterpreted as applying to
   SVE_PT_REGS_{MASK, FPSIMD,SVE} (which deliberately have no prctl
   equivalent).

 * Added comments explaining the intent, purpose and basic constraints
   for fpsimd.c helpers.
---
 arch/arm64/include/asm/fpsimd.h      |  13 +-
 arch/arm64/include/uapi/asm/ptrace.h | 138 ++++++++++++++++++
 arch/arm64/kernel/fpsimd.c           |  60 ++++++++
 arch/arm64/kernel/ptrace.c           | 271 +++++++++++++++++++++++++++++++++--
 include/uapi/linux/elf.h             |   1 +
 5 files changed, 474 insertions(+), 9 deletions(-)

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()?

> +	if (test_tsk_thread_flag(target, TIF_SVE_VL_INHERIT))
> +		header->flags |= SVE_PT_VL_INHERIT;
> +
> +	header->vl = target->thread.sve_vl;
> +	vq = sve_vq_from_vl(header->vl);
> +
> +	header->max_vl = sve_max_vl;
> +	if (WARN_ON(!sve_vl_valid(sve_max_vl)))
> +		header->max_vl = header->vl;
> +
> +	header->size = SVE_PT_SIZE(vq, header->flags);
> +	header->max_size = SVE_PT_SIZE(sve_vq_from_vl(header->max_vl),
> +				      SVE_PT_REGS_SVE);
> +}
[...]
> +static int sve_set(struct task_struct *target,
> +		   const struct user_regset *regset,
> +		   unsigned int pos, unsigned int count,
> +		   const void *kbuf, const void __user *ubuf)
> +{
> +	int ret;
> +	struct user_sve_header header;
> +	unsigned int vq;
> +	unsigned long start, end;
> +
> +	if (!system_supports_sve())
> +		return -EINVAL;
> +
> +	/* Header */
> +	if (count < sizeof(header))
> +		return -EINVAL;
> +	ret = user_regset_copyin(&pos, &count, &kbuf, &ubuf, &header,
> +				 0, sizeof(header));
> +	if (ret)
> +		goto out;
> +
> +	/*
> +	 * Apart from PT_SVE_REGS_MASK, all PT_SVE_* flags are consumed by
> +	 * sve_set_vector_length(), which will also validate them for us:
> +	 */
> +	ret = sve_set_vector_length(target, header.vl,
> +		((unsigned long)header.flags & ~SVE_PT_REGS_MASK) << 16);
> +	if (ret)
> +		goto out;
> +
> +	/* Actual VL set may be less than the user asked for: */
> +	vq = sve_vq_from_vl(target->thread.sve_vl);
> +
> +	/* 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
need this since we are going to override the FPSIMD state anyway here.

> +
> +	/* Otherwise: full SVE case */
> +
> +	/*
> +	 * If setting a different VL from the requested VL and there is
> +	 * register data, the data layout will be wrong: don't even
> +	 * try to set the registers in this case.
> +	 */
> +	if (count && vq != sve_vq_from_vl(header.vl)) {
> +		ret = -EIO;
> +		goto out;
> +	}
> +
> +	sve_alloc(target);
> +	fpsimd_sync_to_sve(target);

Similarly here, it's a full SVE case, so we are going to override it
anyway.

> +	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.

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.

The current approach at least saves a bit of code.  What do you think?

> > +	if (test_tsk_thread_flag(target, TIF_SVE_VL_INHERIT))
> > +		header->flags |= SVE_PT_VL_INHERIT;
> > +
> > +	header->vl = target->thread.sve_vl;
> > +	vq = sve_vq_from_vl(header->vl);
> > +
> > +	header->max_vl = sve_max_vl;
> > +	if (WARN_ON(!sve_vl_valid(sve_max_vl)))
> > +		header->max_vl = header->vl;
> > +
> > +	header->size = SVE_PT_SIZE(vq, header->flags);
> > +	header->max_size = SVE_PT_SIZE(sve_vq_from_vl(header->max_vl),
> > +				      SVE_PT_REGS_SVE);
> > +}
> [...]
> > +static int sve_set(struct task_struct *target,
> > +		   const struct user_regset *regset,
> > +		   unsigned int pos, unsigned int count,
> > +		   const void *kbuf, const void __user *ubuf)
> > +{
> > +	int ret;
> > +	struct user_sve_header header;
> > +	unsigned int vq;
> > +	unsigned long start, end;
> > +
> > +	if (!system_supports_sve())
> > +		return -EINVAL;
> > +
> > +	/* Header */
> > +	if (count < sizeof(header))
> > +		return -EINVAL;
> > +	ret = user_regset_copyin(&pos, &count, &kbuf, &ubuf, &header,
> > +				 0, sizeof(header));
> > +	if (ret)
> > +		goto out;
> > +
> > +	/*
> > +	 * Apart from PT_SVE_REGS_MASK, all PT_SVE_* flags are consumed by
> > +	 * sve_set_vector_length(), which will also validate them for us:
> > +	 */
> > +	ret = sve_set_vector_length(target, header.vl,
> > +		((unsigned long)header.flags & ~SVE_PT_REGS_MASK) << 16);
> > +	if (ret)
> > +		goto out;
> > +
> > +	/* Actual VL set may be less than the user asked for: */
> > +	vq = sve_vq_from_vl(target->thread.sve_vl);
> > +
> > +	/* 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.)

> 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.

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.


Of course, I don't know whether userspace should really rely on partial
regset writes doing anything sane, but I figured the implemented
behaviour is at least less surprising with respect to the fpr_set()
behavior.

No legacy software can be relying on NT_ARM_SVE at all, so the behaviour
here may not matter that much.  My idea was to reduce the invasiveness
of porting ptrace clients to use NT_ARM_SVE.

> 
> > +
> > +	/* Otherwise: full SVE case */
> > +
> > +	/*
> > +	 * If setting a different VL from the requested VL and there is
> > +	 * register data, the data layout will be wrong: don't even
> > +	 * try to set the registers in this case.
> > +	 */
> > +	if (count && vq != sve_vq_from_vl(header.vl)) {
> > +		ret = -EIO;
> > +		goto out;
> > +	}
> > +
> > +	sve_alloc(target);
> > +	fpsimd_sync_to_sve(target);
> 
> Similarly here, it's a full SVE case, so we are going to override it
> anyway.

The argument here is similar: target->sve_state might already be
allocated and if so it could contain data that doesn't match the user
task's idea of what's in the V-regs.  (In fact, sve_alloc() currently
zeroes sve_state, but that's still different from the current V-regs.)

There is no possibility of legacy software relying on this code path,
so we could say that a short write to NT_ARM_SVE with PT_SVE_REGS_SVE
in flags zeroes any trailing data instead of preserving it.


Either way, I don't intend to document the behaviour of partial
writes to NT_ARM_SVE.  From a userspace point of view, I leave it
unspecified.

> > +	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.

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.)


Does this make sense?  There may be issues or corner cases here
that I didn't spot...

Cheers
---Dave

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.

On Wed, Oct 18, 2017 at 11:32:55AM +0100, Catalin Marinas wrote:
> 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.

See the end of this reply.

> > > > +	/* 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().

Hmmm, yes.  Anyway, I've applied the above fix, so I think this should
be fine now.

> > > 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.

Will do.  It's pretty yucky.

> 
> > > > +	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.

Hmmm, I think I'm guilty of inventing a spurious argument here.


Apparently gdb does not do syscall cancelling.

Strace does though.
(See http://man7.org/linux/man-pages/man1/strace.1.html, search for
"syscall tampering" ;)

However, it never tries to skip a syscall entirelity AFAICT:
instead, it only attempts to fake what occurs during the syscall,
such as bailing out with a predetermined return value.

> > 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.

I think you're right.

Simulating the content of the syscall does work though, and doesn't
depend on whether SVE discard occurs or not.

If the tracer sets the SVE regs at the syscall enter/exit traps,
they could still be discarded again before the tracee reenters
userspace, but that's no different from what happens in a real
syscall.


So I think my overall argument would be:

ptrace should be as orthogonal as possible to SVE discard.


Currently ptrace neither assumes that SVE discard will happen or that it
won't: it just does what the tracer asks for and tries to be safe with
either and doesn't try to hide the effects from the tracer or tracee.

I worry that introducing more interdependencies between ptrace and the
fpsimd.c code will complicate maintenance rather than making it easier.

Does that may my position clearer?

If we go with this, I should add a note to the documentation explaining
how NT_ARM_SVE writes interact with ptrace syscall traps.


The alternative would be to forcibly discard SVE in
syscall_trace_enter(), but this doesn't seem to simplify the NT_ARM_SVE
implementation at all -- that code needs to work for all types of
ptrace-stop, not just syscall traps: other that syscall traps, SVE
is potentially live for the tracee and must not be discarded.
So I'm still not clear on what the gain is.

Cheers
---Dave