[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20160623171733.GR6521@e104818-lin.cambridge.arm.com>
Date: Thu, 23 Jun 2016 18:17:33 +0100
From: Catalin Marinas <catalin.marinas@....com>
To: Andre Przywara <andre.przywara@....com>
Cc: Will Deacon <will.deacon@....com>, linux-kernel@...r.kernel.org,
linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH 1/6] arm64: alternatives: drop enable parameter from
_else and _endif macro
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@....com>
> ---
> 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
Powered by blists - more mailing lists