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

[Mark Brown <broonie@kernel.org>]

[-- Attachment #1.1: Type: text/plain, Size: 4347 bytes --]

On Mon, Sep 21, 2020 at 01:36:27PM +0100, Will Deacon wrote:
> On Fri, Aug 28, 2020 at 07:11:54PM +0100, Mark Brown wrote:

> > + * TIF_SVE_NEEDS_FLUSH and TIF_SVE set at the same time should never happen.
> > + * In the unlikely case it happens, the code is able to cope with it. It will
> > + * first restore the SVE registers and then flush them in
> > + * fpsimd_restore_current_state.

> I find this pretty confusing and, if anything, I'd have expected it to be
> the other way around: TIF_SVE_NEEDS_FLUSH should only be checked if TIF_SVE
> is set. Can we leave TIF_SVE set on syscall entry and just check whether
> we need to flush on return?

I'll have a think about this, it'd been a while and I'll need to page
this in again if I ever thought about that bit thoroughly, the
separation already existed when I took over the code and I didn't see it
raised as a concern on the first round of review which I had hoped had
covered the big picture stuff.

> Having said that, one overall concern I have with this patch is that there
> is a lot of ad-hoc flag manipulation which feels like a disaster to
> maintain. Do we really need all 8 states provided by FOREIGN_FPSTATE, SVE
> and SVE_NEEDS_FLUSH?

I think it's clear that we need all three flags, they're all orthogonal 
enough - FOREIGN_FPSTATE says if the hardware state needs syncing and
applies to both FPSIMD and SVE, SVE says if the task should execute SVE
instructions without trapping and SVE_NEEDS_FLUSH says where to restore
the SVE state from.  I'm not really seeing any redundancy that we can
eliminate.

Having spent time looking at this the main issue and the main thing is
to look at allowing TIF_SVE and TIF_SVE_NEEDS_FLUSH to be simultaneously
set as you suggested above - the interaction with TIF_FOREIGN_FPSTATE is
fairly simple so I don't see a big problem there.

> > +	/* Ensure that we only evaluate system_supports_sve() once. */
> > +	if (system_supports_sve()) {

> I don't understand what the comment is getting at here, or how this code
> ensure we only evaluate this once. What's the issue?

There was a performance concern raised about doing the capabilities
check more than once in the same function, this was a refactor to pull
the system_supports_sve() check out which looks a bit more fiddly.
Previously there were two blocks which checked for system_supports_sve()
and tested flags separately.

> > +		} else if (test_and_clear_thread_flag(TIF_SVE_NEEDS_FLUSH)) {
> > +			WARN_ON_ONCE(test_thread_flag(TIF_SVE));

> We already checked TIF_SVE and we know it's false (unless there was a
> concurrent update, but then this would be racy anyway).

Yes, this was just about having a check on use as had been requested
ince we're adding a check rather than the check actually being useful,
it's easier to verify that the checks are all there.

> > +	if (system_supports_sve() &&
> > +	    test_and_clear_thread_flag(TIF_SVE_NEEDS_FLUSH)) {

> Why do we need to check system_supports_sve() here?

I don't think we do, it's been there since the initial version and
looked like a stylistic thing.  I'll just drop it since it's a bit late
to validate at this point anyway and if there were a situation where we
had the flag but not the hardware we probably ought to be screaming
about it rather than silently ignoring it as the code currently does.

> > +	/* The flush should have happened when the thread was stopped */
> > +	if (test_and_clear_tsk_thread_flag(target, TIF_SVE_NEEDS_FLUSH))
> > +		WARN(1, "TIF_SVE_NEEDS_FLUSH was set");

> Given that this adds an atomic operation, I don't think we should be doing
> this unless it's necessary and it looks like a debug check to me.

Yes, one of the previous review suggestions was to add such checks.  I
can either remove it again or put it behind a define/config option that
enables debug checks like this - as you say this code is a bit complex
so I do think there's value in having extra checks of things we know
should be true in there as an option to help with validation.

> > +		/*
> > +		 * If ptrace requested to use FPSIMD, then don't try to
> > +		 * re-enable SVE when the task is running again.
> > +		 */

> I think this comment needs some help. Is "ptrace" the tracer and "the task"
> the tracee?

Yes.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel