linux-kernel - Re: [PATCH 4/7] arm64: add 'runtime constant' support

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 for Android: free password hash cracker in your pocket

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

Date: Tue, 11 Jun 2024 09:56:17 -0700
From: Linus Torvalds <torvalds@...ux-foundation.org>
To: Mark Rutland <mark.rutland@....com>
Cc: Peter Anvin <hpa@...or.com>, Ingo Molnar <mingo@...nel.org>, Borislav Petkov <bp@...en8.de>, 
	Thomas Gleixner <tglx@...utronix.de>, Rasmus Villemoes <linux@...musvillemoes.dk>, 
	Josh Poimboeuf <jpoimboe@...nel.org>, Catalin Marinas <catalin.marinas@....com>, 
	Will Deacon <will@...nel.org>, Linux Kernel Mailing List <linux-kernel@...r.kernel.org>, 
	"the arch/x86 maintainers" <x86@...nel.org>, linux-arm-kernel@...ts.infradead.org, 
	linux-arch <linux-arch@...r.kernel.org>
Subject: Re: [PATCH 4/7] arm64: add 'runtime constant' support

On Tue, 11 Jun 2024 at 07:29, Mark Rutland <mark.rutland@....com> wrote:
>
> Do we expect to use this more widely? If this only really matters for
> d_hash() it might be better to handle this via the alternatives
> framework with callbacks and avoid the need for new infrastructure.

Hmm. The notion of a callback for alternatives is intriguing and would
be very generic, but we don't have anything like that right now.

Is anybody willing to implement something like that? Because while I
like the idea, it sounds like a much bigger change.

> As-is this will break BE kernels [...]

I had forgotten about that horror. BE in this day and age is insane,
but it's easy enough to fix as per your comments. Will do.

> We have some helpers for instruction manipulation, and we can use
> aarch64_insn_encode_immediate() here, e.g.
>
> #include <asm/insn.h>
>
> static inline void __runtime_fixup_16(__le32 *p, unsigned int val)
> {
>         u32 insn = le32_to_cpu(*p);
>         insn = aarch64_insn_encode_immediate(AARCH64_INSN_IMM_16, insn, val);
>         *p = cpu_to_le32(insn);
> }

Ugh. I did that, and then noticed that it makes the generated code
about ten times bigger.

That interface looks positively broken.

There is absolutely nobody who actually wants a dynamic argument, so
it would have made both the callers and the implementation *much*
simpler had the "AARCH64_INSN_IMM_16" been encoded in the function
name the way I did it for my instruction rewriting.

It would have made the use of it simpler, it would have avoided all
the "switch (type)" garbage, and it would have made it all generate
much better code.

So I did that change you suggested, and then undid it again.

Because that whole aarch64_insn_encode_immediate() thing is an
abomination, and should be burned at the stake.  It's misdesigned in
the *worst* possible way.

And no, this code isn't performance-critical, but I have some taste,
and the code I write will not be using that garbage.

> This is missing the necessary cache maintenance and context
> synchronization event.

Will do.

                 Linus