linux-kernel - Re: [RESEND RFC PATCH v1 2/5] arm64: Add BBM Level 2 cpu feature

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20241213164939.GD30314@mazurka.cambridge.arm.com>
Date: Fri, 13 Dec 2024 16:49:39 +0000
From: Mikołaj Lenczewski <miko.lenczewski@....com>
To: Marc Zyngier <maz@...nel.org>
Cc: ryan.roberts@....com, catalin.marinas@....com, will@...nel.org,
	corbet@....net, oliver.upton@...ux.dev, joey.gouly@....com,
	suzuki.poulose@....com, yuzenghui@...wei.com,
	linux-arm-kernel@...ts.infradead.org, linux-doc@...r.kernel.org,
	linux-kernel@...r.kernel.org, kvmarm@...ts.linux.dev
Subject: Re: [RESEND RFC PATCH v1 2/5] arm64: Add BBM Level 2 cpu feature

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@....com> 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