Re: [PATCH v3 01/23] arm64: alternative: Allow alternative_insn to always issue the first instruction

From: Dave Martin <dave.martin@arm.com>
To: Catalin Marinas <catalin.marinas@arm.com>
Cc: linux-arm-kernel@lists.infradead.org, linux-arch@vger.kernel.org,
	Richard Earnshaw <Richard.Earnshaw@arm.com>,
	Szabolcs Nagy <szabolcs.nagy@arm.com>,
	Andrey Konovalov <andreyknvl@google.com>,
	Kevin Brodsky <kevin.brodsky@arm.com>,
	Peter Collingbourne <pcc@google.com>,
	linux-mm@kvack.org, Vincenzo Frascino <vincenzo.frascino@arm.com>,
	Will Deacon <will@kernel.org>
Subject: Re: [PATCH v3 01/23] arm64: alternative: Allow alternative_insn to always issue the first instruction
Date: Mon, 27 Apr 2020 17:57:37 +0100	[thread overview]
Message-ID: <20200427165737.GD15808@arm.com> (raw)
In-Reply-To: <20200421142603.3894-2-catalin.marinas@arm.com>

On Tue, Apr 21, 2020 at 03:25:41PM +0100, Catalin Marinas wrote:
> There are situations where we do not want to disable the whole block
> based on a config option, only the alternative part while keeping the
> first instruction. Improve the alternative_insn assembler macro to take
> a 'first_insn' argument, default 0, to preserve the current behaviour.
> 
> Signed-off-by: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Will Deacon <will@kernel.org>
> ---
>  arch/arm64/include/asm/alternative.h | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/include/asm/alternative.h b/arch/arm64/include/asm/alternative.h
> index 5e5dc05d63a0..67d7cc608336 100644
> --- a/arch/arm64/include/asm/alternative.h
> +++ b/arch/arm64/include/asm/alternative.h
> @@ -111,7 +111,11 @@ static inline void apply_alternatives_module(void *start, size_t length) { }
>  	.byte \alt_len
>  .endm
>  
> -.macro alternative_insn insn1, insn2, cap, enable = 1
> +/*
> + * Disable the whole block if enable == 0, unless first_insn == 1 in which
> + * case insn1 will always be issued but without an alternative insn2.
> + */
> +.macro alternative_insn insn1, insn2, cap, enable = 1, first_insn = 0
>  	.if \enable
>  661:	\insn1
>  662:	.pushsection .altinstructions, "a"
> @@ -122,6 +126,8 @@ static inline void apply_alternatives_module(void *start, size_t length) { }
>  664:	.popsection
>  	.org	. - (664b-663b) + (662b-661b)
>  	.org	. - (662b-661b) + (664b-663b)
> +	.elseif \first_insn
> +	\insn1

This becomes quite unreadable at the invocation site, especially when
invoked as "alternative_insn ..., 1".  "... first_insn=1" is not much
better either).

I'm struggling to find non-trivial users of this that actually want the
whole block to be deleted dependent on the config.

Can we instead just always behave as if first_insn=1 instead?  This this
works intuitively as an alternative, not the current weird 3-way choice
between insn1, insn2 and nothing at all.  The only time that makes sense
is when one of the insns is a branch that skips the block, but that's
handled via the alternative_if macros instead.

Behaving always like first_insn=1 provides an if-else that is statically
optimised if the relevant feature is configured out, which I think is
the only thing people are ever going to want.

Maybe something depends on the current behaviour, but I can't see it so
far...

[...]

Cheers
---Dave