Re: [PATCH 1/6] arm64: alternatives: drop enable parameter from _else and _endif macro

[Catalin Marinas]

On Mon, May 09, 2016 at 05:49:45PM +0100, Andre Przywara wrote:
> Commit 77ee306c0aea9 ("arm64: alternatives: add enable parameter to
> conditional asm macros") extended the alternative assembly macros.
> Unfortunately this does not really work as one would expect, as the
> enable parameter in fact correctly protects the alternative section
> magic, but not the actual code sequences.

I don't remember how we ended up with this code because it's not used
anywhere.

> So if enable is false, we will have the original instruction(s) _and_
> the alternative ones in the file, which is just wrong.
> To make this work one would need to additionally protect the
> alternative sequence with extra .if directives, which makes
> the intention of the enable parameter rather pointless.
> Instead users should directly guard the whole "_else; insn; _endif"
> sequence with .if directives.
> Add a comment describing this usage and drop the enable parameter from
> the alternative_else and alternative_endif macros.
> 
> This reverts parts of commit 77ee306c0aea ("arm64: alternatives: add
> enable parameter to conditional asm macros").
> 
> Signed-off-by: Andre Przywara <andre.przywara@xxxxxxx>
> ---
>  arch/arm64/include/asm/alternative.h | 24 ++++++++++++++++++------
>  1 file changed, 18 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/alternative.h b/arch/arm64/include/asm/alternative.h
> index beccbde..502c9ef 100644
> --- a/arch/arm64/include/asm/alternative.h
> +++ b/arch/arm64/include/asm/alternative.h
> @@ -94,6 +94,8 @@ void apply_alternatives(void *start, size_t length);
>   *
>   * The code that follows this macro will be assembled and linked as
>   * normal. There are no restrictions on this code.
> + * If you use the enable parameter, see the comments below for _else
> + * and _endif.
>   */
>  .macro alternative_if_not cap, enable = 1
>  	.if \enable
> @@ -117,23 +119,33 @@ void apply_alternatives(void *start, size_t length);
>   * 2. Not contain a branch target that is used outside of the
>   *    alternative sequence it is defined in (branches into an
>   *    alternative sequence are not fixed up).
> + *
> + * If you used the optional enable parameter in the opening
> + * alternative_if_not macro above, please protect the whole _else
> + * branch with an .if directive:
> + *	alternative_if_not CAP_SOMETHING, condition
> + *		orig_insn
> + *	.if condition
> + *	alternative_else
> + *		repl_insn
> + *	alternative_endif
> + *	.endif

I think the intention was to that in the !condition case, both
alternatives are dropped. That includes the original code.

I propose we revert the original commit, I don't think it is strictly
needed for your patches (I'll try to fix them up and see how it goes).

-- 
Catalin