[PATCH v3 7/8] arm64/sve: Don't disable SVE on syscalls return

[Mark Brown]

On Wed, Jul 15, 2020 at 05:52:31PM +0100, Dave Martin wrote:
> On Mon, Jun 29, 2020 at 02:35:55PM +0100, Mark Brown wrote:

> > @@ -1163,6 +1184,17 @@ void fpsimd_restore_current_state(void)
> >  		fpsimd_bind_task_to_cpu();
> >  	}
> >  
> > +	if (system_supports_sve() &&
> > +	    test_and_clear_thread_flag(TIF_SVE_NEEDS_FLUSH)) {

> This if() and the previous one should be mutually exclusive, right?

No, TIF_FOREIGN_FPSTATE is set without looking at the SVE state in
several places and we can get both set simultaneously (with fun
concurrency issues which would be hard to unpick) - it basically just
means a restore is needed, not that it's plain FPSIMD state in
particular that needs to be restored.

> It looks a bit weird to load the regs and then flush, but this won't
> happen if TIF_FOREIGN_FPSTATE and TIF_SVE_NEEDS_FLUSH are never set at
> the same time.

The code is currently relying on task_fpsimd_load() to get the vector
length and the first 128 bits of each value configured if they don't
correspond to the current state.  I'll add a comment explaining what's
going on here and also a note in the comment documenting the new flag
saying that it's valid with TIF_FOREIGN_FPSTATE and what should happen
there.

> > +		/*
> > +		 * The userspace had SVE enabled on entry to the kernel
> > +		 * and requires the state to be flushed.
> > +		 */
> > +		sve_flush_live();
> > +		sve_user_enable();
> 
> Can we actually get here with SVE still disabled via CPACR_EL1 for EL0?

> This might become true if future patches allow for a dynamic decision
> about whether to disable SVE or flush the regs in place, but for now I
> wonder whether it's possible: this patch removes the sve_user_disable()
> call from sve_user_discard() after all.

> Could be worth a comment, if nothing else.

We will be able to after patch 8 which uses TIF_SVE_NEEDS_FLUSH in
the SVE access trap.

> > diff --git a/arch/arm64/kernel/ptrace.c b/arch/arm64/kernel/ptrace.c
> > index 68b7f34a08f5..aed7230b1fb8 100644
> > --- a/arch/arm64/kernel/ptrace.c
> > +++ b/arch/arm64/kernel/ptrace.c
> > @@ -900,6 +900,11 @@ static int sve_set(struct task_struct *target,
> >  		ret = __fpr_set(target, regset, pos, count, kbuf, ubuf,
> >  				SVE_PT_FPSIMD_OFFSET);
> >  		clear_tsk_thread_flag(target, TIF_SVE);
> > +		/*
> > +		 * If ptrace requested to use FPSIMD, then don't try to
> > +		 * re-enable SVE when the task is running again.
> > +		 */
> > +		clear_tsk_thread_flag(target, TIF_SVE_NEEDS_FLUSH);

> I wonder whether we also want to do this when loading the SVE regs.

We do clear TIF_SVE_NEEDS_FLUSH when loading the SVE regs already as far
as I can see unless I'm misunderstanding you?  It should be redundant
ATM but it seems as well to have the code handle this ATM since it's
simple enough.

> What happens if the task is at the syscall exit trap (so
> TIF_SVE_NEEDS_FLUSH is set), and we try to update the SVE regs?  Do they
> get flushed again before target re-enters userspace?

Setting the regs will clear TIF_SVE_NEEDS_FLUSH with the current code.

> Similarly, what should we do if the tracer wants to read the regs and
> TIF_SVE_NEEDS_FLUSH is set?  Should we give the tracer flushed versions
> of the regs?

> Or is TIF_SVE_NEEDS_FLUSH always clear, due fpsimd_save() being called
> when target stopped?  In that case, we might reduce this to

>         if (test_and_clear_tsk_thread_flag(target, TIF_SVE_NEEDS_FLUSH))
>                 WARN();

It should be clear AFAICT.  I'll add the WARN() to make sure this is
being handled, it seems like should it need to be handled we should be
returning flushed registers.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20200821/3214996a/attachment.sig>