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.
