Hi Dave,
On Mon, Apr 27, 2020 at 05:57:37PM +0100, Dave P Martin wrote:
> 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@xxxxxxx>
> > Cc: Will Deacon <will@xxxxxxxxxx>
> > ---
> > 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).
That I agree.
The reason I didn't leave the alternative in place here is that if gas
doesn't support MTE, it will fail to compile. I wanted to avoid the
several #ifdef's.
> I'm struggling to find non-trivial users of this that actually want the
> whole block to be deleted dependent on the config.
Some of the errata stuff like CONFIG_ARM64_REPEAT_TLBI ends up with
unnecessary nops. Similarly for CONFIG_ARM64_UAO/PAN and maybe a few
others (it's all additional nops). We also have a few errata workaround
where we didn't bother with the config enable option at all.
While this is C code + inline asm, I'd like to have a consistent
behaviour of ALTERNATIVE between C and .S files. Now, given that some of
them (like UAO/PAN) are on by default, it probably doesn't make any
difference if we always keep the first block (non-alternative).
We could add a new macro ALTERNATIVE_OR_NOP.
> 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...
I'll give it a go in v4 and see how it looks.
Another option would be an alternative_else which takes an enable
argument.
Thanks.
--
Catalin
