On Thu, Jul 23, 2026 at 01:47:19PM +0100, Mark Rutland wrote: > FWIW, my view is:
> (1) We shouldn't make speculative optimizations. Removing unnecessary
> code is fine, within reason, but we shouldn't be adding conditional
> logic.
> To that end, I'd be happier with an unconditional ISB here.
> (2) Optimizations should be benchmarked to some extent, such that we can
> decide whether the trade-off is worthwhile.
> (3) In this path specifically, if adding an ISB is a problem, then it
> calls the wider approach into question.
> Is there anyone in particular that you expect feecback from w.r.t.
> barriers in this path?
I think you mainly in this instance, given the sysreg_cond_update()
stuff. TBH I did actually write the unconditional isb() version but it
just looked obviously silly next to the sysreg_cond_update_s() for the
register and I couldn't imagine nobody would raise it as a review
concern, perhaps the best thing here is to expand that into:
if (new_smcr != old_smcr) {
write_sysreg_s(new_smcr, SMCR_EL1);
/* EZT0 and FA64 are not self synchronizing */
isb();
}
instead of using sysreg_cond_update_s(), not suppressing the write would
make using sysreg_cond_update_s() for ZCR_EL1 look strange.
An alternative would be to just drop sysreg_cond_update_s() and it's
usage for both ZCR_EL1 and SMCR_EL1 but given the existing code
suppresses the no change write that seems like something we ought to
benchmark and I'm not sure that's proportionate effort for something so
straightforward and localised. If we didn't have the existing write
suppression or it was very new I'd not worry about it but it's been
there since forever.
signature.asc
Description: PGP signature
