Re: [PATCH v10 02/30] arm64/fpsimd: Update FA64 and ZT0 enables when loading SME state

[Mark Rutland <mark.rutland@arm.com>]

On Tue, May 26, 2026 at 02:25:56PM +0100, Mark Brown wrote:
> On Tue, May 26, 2026 at 01:48:41PM +0100, Mark Rutland wrote:
> > On Fri, Mar 06, 2026 at 05:00:54PM +0000, Mark Brown wrote:
> 
> > > We provide a helper which does the configuration as part of a
> > > read/modify/write operation along with the configuration of the task VL,
> > > then update the floating point state load and SME access trap to use it.
> 
> > > +		if (fa64)					\
> > > +			__new |= SMCR_ELx_FA64;			\
> > > +		if (zt0)					\
> > > +			__new |= SMCR_ELx_EZT0;			\
> 
> > I'd strongly prefer that we make it the caller's responsiblity to track
> > all the bits within SMCR, rather than requiring each caller to pass a
> > bag of booleans.
> 
> I was explicitly going for the opposite of that in order to make it
> harder for someone implementing a future extension to miss a place where
> an update is required, having the callers independently constructing the
> register values feels like it's asking for trouble.

I didn't say callers should *construct* the value independently, and I
showed how to centralize the construction in a __task_smcr() function.

I think callers should pass the entire value around rather than a
collection of discrete booleans: constructing a collection of discrete
booleans is functionally equivalent to construction the entire value,
and we can more easily manage the construction and passing of the entire
value.

> > 	unsigned long __task_smcr(const struct task_struct *tsk)
> > 	{
> > 		unsigned long vq = sve_vq_from_vl(task_get_sme_vl(tsk));
> > 		unsigned long smcr = vq - 1;
> 
> I agree that's a better pattern for the main kernel - we could also do
> something similar with a task_set_smcr() which wraps the explicitly
> specifed version.

I don't think a task_set_smcr() function would gain much vs
using sysreg_cond_update_s(). I expect that we'd use the SMCR value as
the source of truth (and hence that would need to be generated outside
of task_set_smcr()), at which point either task_set_smcr() would be a
thin wrapper that just hides the register name, or it would generate the
value independently and obscure the connection.

I expect that what we should have eventually is something like:

	unsigned long smcr = __task_smcr(task);

	sysreg_cond_update_s(SYS_SMCR_EL1, smcr);

	if (za) {
		__sme load_za(sme_state);
		if (smcr & SMCR_ELx_EZT0)
			__sme_load_zt(sme_state);
	}

... or:

	const unsigned long smcr = __task_smcr(task);
	const bool zt0 = smcr & SMCR_ELx_EZT0;

	sysreg_cond_update_s(SYS_SMCR_EL1, smcr);

	if (za) {
		__sme load_za(sme_state);
		if (zt0)
			__sme_load_zt(sme_state);
	}

... where in either case it's clear at the function level that the value
programmed into SMCR matches what we're using for boolean decisions, and
it's clear at a higher level that functions are consistent given
consistent usage of __task_smcr().

> That would I think avoid most of the issue you're seeing?

I'm not sure what you mean here.

I don't think we need task_set_smcr(), and I'd prefer what I suggested.

Mark.