Re: [PATCH 4/6] arm64/signal: Consistently invalidate the in register FP state in restore

[Dave Martin <Dave.Martin@arm.com>]

Hi,

On Tue, Dec 03, 2024 at 04:53:11PM +0000, Mark Brown wrote:
> On Tue, Dec 03, 2024 at 03:34:01PM +0000, Dave Martin wrote:
> > On Tue, Dec 03, 2024 at 12:45:56PM +0000, Mark Brown wrote:
> > > When restoring the SVE and SME specific floating point register states we
> > > flush the task floating point state, marking the hardware state as stale so
> > > that preemption does not result in us saving register state from the signal
> 
> Now I think about it again this should probably be dropped from the
> series, or at least ordered after the reenablement.
> 
> > > +	 * thread floating point state with preemption enabled, so
> > > +	 * protection is needed to prevent a racing context switch
> > > +	 * from writing stale registers back over the new data. Mark
> > > +	 * the register floating point state as invalid and unbind the
> > > +	 * task from the CPU to force a reload before we return to
> > > +	 * userspace. fpsimd_flush_task_state() has a check for FP
> > > +	 * support.
> > > +	 */
> 
> > Maybe add a comment in fpsimd_flush_task_state() about why the
> > system_supports_fpsimd() check is important?  It's not obvious there
> > why we should ever be calling that function on non-FPSIMD systems.
> 
> There already is a comment in there about it?

There's a comment, but it's not clear that calling that function is
considered correct / useful if there is no FPSIMD.

Not a big deal, anyhow.

> > But would it be a good idea to stick a
> > WARN_ON(!test_thread_flag(TIF_FOREIGN_FPSTATE)) at the start of the
> > functions that rely on this?
> 
> As I mentioned in reply to one of your other messages I want to just gut
> the whole API here and replace it with get/put functions for the state
> which would include the get/put functions making sure they're paired
> with each other.

No argument from me on that, but it would be good to have a way to
check that functions that expect to be called with the FP context held,
actually are (similar to lockdep_assert_held() etc.)

If the number of affected functions is low, I guess comments may be
enough, though.

> 
> Please delete unneeded context from mails when replying.  Doing this
> makes it much easier to find your reply in the message, helping ensure
> it won't be missed by people scrolling through the irrelevant quoted
> material.

Ack, but opinions can differ about what context is unneeded.

I'll try to keep the noise down on these threads.

Cheers
---Dave