[PATCH 00/14] arm64: Preparatory FPSIMD/SVE/SME fixes

[Mark Brown]

On Tue, Apr 08, 2025 at 05:46:31PM +0100, Mark Rutland wrote:
> On Fri, Apr 04, 2025 at 07:06:50PM +0100, Mark Brown wrote:
> > On Fri, Apr 04, 2025 at 06:44:21PM +0100, Mark Rutland wrote:

> > > * The ptrace ABI for SME is written around the assumption that
> > >   SVE_PT_REGS_FPSIMD and SVE_PT_REGS_SVE are separate bit flags, whereas
> > >   in reality these are different values (0 and 1 respectively) for
> > >   bit 0 of the user_sve_header flags.

> > The documentation definitely doesn't reflect what actually got
> > implemented, IIRC the errors are more around returning the header only
> > rather than the assumption that they're separate flags but at this point
> > it doesn't really matter given that the documentation is clearly wrong.

....

> This means portions of the code are wildly misleading, and this caused
> secondary problems such as providing register data when we didn't intend
> to, which I assume is what you mean by "the errors are more around
> returning the header only".

Yes, exactly.

> > >   I haven't yet worked through all of the implications of this, but
> > >   AFAICT we can't reliably indicate the value of PSTATE.SM, and
> > >   userspace cannot reliably save/restore a task's state in all cases.

> > Userspace should be able to read by looking the SSVE registers, if you
> > get SVE format register state back you're in streaming mode. Streaming
> > mode state will always be saved in SVE format so a read of streaming SVE
> > vector state will come out with the SVE bit set, if we're not in
> > streaming mode it won't be set.

> Unfortunately, that's not always true today.

> While fpsimd_save_user_state() will save streaming mode state into the
> FP_STATE_SVE format, other code can manipulate the task into a state
> where thread_sm_enabled() is true, but the task's registers are saved in
> the FPSIMD format. That combination is broken for a number of reasons,
> but you seemed to permit this deliberately in commit:

>   bbc6172eefdb276b ("arm64/fpsimd: SME no longer requires SVE register state")

> ... which says:

> | Now that we track the type of the stored register state separately
> | to what is active in the task, it is valid to have the FPSIMD
> | register state stored while in streaming mode. 

> I will assume that commit message is misleading, and that we do not want
> to permit that case. It's possible to get a task into a bad state
> regardless of that commit.

Ah, yes - that case is indeed going to break.

> We can fix that in a number of ways, but we need to think of the shape
> of ABI we actually want, e.g. whether writes to NT_ARM_SSVE with
> SVE_PT_REGS_FPSIMD should be rejected outright.

It's hard to see a use case for it so it does seem reasonable to
disallow that, it would really be nice if SVE_PT_REGS_FPSIMD weren't
part of the ABI at all.  OTOH it will be implementable with any read
side ABI and it *is* there.

I think the intended ABI where returning SVE register state from
NT_ARM_SSVE indicates streaming mode is probably a good choice given
that it is compatible with the existing userspace code you've surveyed,
will be the behaviour seen in most actual usage with the current code
and is fairly close to the documentation.

If we do that then when streaming mode is disabled the obvious options
are that we can either keep the current behaviour and return FPSIMD
register state, or change to only return the header.  The former is less
change and therefore safer, the latter is probably a nicer ABI.  GDB
appears to do the right thing if it only gets the header back.  LLDB
appears to only attempt to read the header when determining if it's in
streaming mode so shouldn't see any difference at all, though that code
is a bit less clear to me.
-------------- 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/20250408/fb9d305c/attachment.sig>