On Thu, Dec 12, 2024 at 08:25:53AM +0000, Marc Zyngier wrote:
> Ah, so this is where this is hiding. I missed it in my review of patch
> #1 yesterday.
>
> On Wed, 11 Dec 2024 16:01:38 +0000,
> Mikołaj Lenczewski <miko.lenczewski@xxxxxxx> wrote:
> >
> > The Break-Before-Make cpu feature supports multiple levels (levels 0-2),
> > and this commit adds a dedicated BBML2 cpufeature to test against
> > support for.
> >
> > In supporting BBM level 2, we open ourselves up to potential TLB
> > Conflict Abort Exceptions during expected execution, instead of only
> > in exceptional circumstances. In the case of an abort, it is
> > implementation defined at what stage the abort is generated, and
>
> *IF* stage-2 is enabled. Also, in the case of the EL2&0 translation
> regime, no stage-2 applies, so it can only be a stage-1 abort.
>
> > the minimal set of required invalidations is also implementation
> > defined. The maximal set of invalidations is to do a `tlbi vmalle1`
> > or `tlbi vmalls12e1`, depending on the stage.
> >
> > Such aborts should not occur on Arm hardware, and were not seen in
> > benchmarked systems, so unless performance concerns arise, implementing
>
> Which systems? Given that you have deny-listed *all* half recent ARM
> Ltd implementations, I'm a bit puzzled.
>
I had tested on an earlier series of the patchset that didn't introduce
the MIDR checks (has_bbml2() only read the claimed level of support),
but otherwise had the same implementation.
> >
> > +static inline bool system_supports_bbml2(void)
> > +{
> > + /* currently, BBM is only relied on by code touching the userspace page
> > + * tables, and as such we are guaranteed that caps have been finalised.
> > + *
> > + * if later we want to use BBM for kernel mappings, particularly early
> > + * in the kernel, this may return 0 even if BBML2 is actually supported,
> > + * which means unnecessary break-before-make sequences, but is still
> > + * correct
>
> Comment style, capitalisation, punctuation.
>
> > + if (!system_supports_bbml2())
> > + return do_bad(far, esr, regs);
> > +
> > + /* if we receive a TLB conflict abort, we know that there are multiple
> > + * TLB entries that translate the same address range. the minimum set
> > + * of invalidations to clear these entries is implementation defined.
> > + * the maximum set is defined as either tlbi(vmalls12e1) or tlbi(alle1).
> > + *
> > + * if el2 is enabled and stage 2 translation enabled, this may be
> > + * raised as a stage 2 abort. if el2 is enabled but stage 2 translation
> > + * disabled, or if el2 is disabled, it will be raised as a stage 1
> > + * abort.
> > + *
> > + * local_flush_tlb_all() does a tlbi(vmalle1), which is enough to
> > + * handle a stage 1 abort.
>
> Same comment about comments.
>
Will fix.
Kind regard,
Mikołaj
