Re: [PATCH] arm64: asmlinkage: Enable use of BTI_C macro in SYM_CODE

[Mark Brown <broonie@kernel.org>]

[-- Attachment #1.1: Type: text/plain, Size: 2810 bytes --]

On Mon, Dec 06, 2021 at 04:30:41PM +0000, Mark Rutland wrote:

> Looking around, there are other places that open-code `hint 34`, e.g.
> arch/arm64/crypto/aes-modes.S. Those are unconditional, so we should probably
> figure out whether we want those to be conditional (or if we're happy to make
> the other cases similarly unconditional).

It seems to *just* be aes-modes.S AFAICT and that does rely on having
the BTIs actually emitted (well, and kselftest but that can't use any
kernel internal helpers so it's not relevant here).

> I'd argue we should probably place BTIs in assembly unconditionally, on the
> assumption that they shouldn't have an measureable performance impact in
> practice (as we're already assuming that when CONFIG_ARM64_BTI_KERNEL is
> selected anyhow). Thoughts?

I'm not sure what you mean here?  We do already add BTIs unconditionally
to SYM_FUNC when BTI is enabled for the kernel, it's only SYM_CODE where
we don't.  For SYM_CODE it seems unwise to add the BTIs since there's
things like the vectors that get covered and it's not really idiomatic
for SYM_CODE's whole "I know exactly what I'm doing" thing.  I also
don't know if we end up with SYM_CODEs for anything particularly space
constrained other than the vectors, I can't think of anything.

If you mean that the macro should unconditionally emit BTI when the
macro is used then the main reason I didn't do that was for consistency
with C code - that will only have BTIs added if we're building with BTI
enabled.  There is at least one implementation out there which implements
unknown HINTs with a performance overhead compared to NOPs so that is
one of the reasons why the kernel might be built with BTI disabled.

> Regardless, I reckon we should probably define a `bti` macro centrally in
> arch/arm64/include/asm/assembler.h, something like:

> That seems to do the right thing:

Yes, even seems to do the right thing without complaint if the assembler
is configured v8.5 and so understands v8.5.  My main worry would be the
potential confusion caused when people see what looks like it should be
a v8.5 instruction in code which is supposed to build configured for
v8.0, it looks wrong to see something that looks like a more modern
instruction in code that should be buildable with a toolchain that
doesn't understand it.

> Can we simplify the ifdeffery so that we only conditionally define BTI_C, and
> unconditionally define the sites using it? That way if there's a problem with
> any of the users that will consistently show up in testing, regardless of
> whether CONFIG_ARM64_BTI_KERNEL is selected.
> 
> e.g. we can make this:
> 
> | #ifdef CONFIG_ARM64_BTI_KERNEL
> | #define BTI_C	hint #34 ;
> | #else
> | #define BTI_C	
> | #endif

Should work.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel