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

[Dave Martin]

On Mon, Sep 21, 2020 at 07:03:37PM +0100, Mark Brown wrote:
> 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.

Maybe just try to improve the documentation initially, and see where
that gets us?

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

In the original SVE series, I included quite a lot of checks of this
sort in order to minimise any overhead for the FPSIMD-only case.
I don't know how much of a concern that is here.

I guess this simply conforms to the same style.

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

Seems reasonable.  Having checks of this sort has proven quite valuable
in the past for debugging, but we don't have to have them compiled in
unconditionally.

Partly, this serves as a statement that we're making an assumption that
has to be enforced elsewhere.

[...]

Cheers
---Dave