Messages in this thread | | | Date | Thu, 23 Jun 2016 18:17:33 +0100 | From | Catalin Marinas <> | 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@arm.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
| |