From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751634AbcFWRRj (ORCPT ); Thu, 23 Jun 2016 13:17:39 -0400 Received: from foss.arm.com ([217.140.101.70]:54851 "EHLO foss.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751483AbcFWRRh (ORCPT ); Thu, 23 Jun 2016 13:17:37 -0400 Date: Thu, 23 Jun 2016 18:17:33 +0100 From: Catalin Marinas To: Andre Przywara Cc: Will Deacon , linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org Subject: Re: [PATCH 1/6] arm64: alternatives: drop enable parameter from _else and _endif macro Message-ID: <20160623171733.GR6521@e104818-lin.cambridge.arm.com> References: <1462812590-4494-1-git-send-email-andre.przywara@arm.com> <1462812590-4494-2-git-send-email-andre.przywara@arm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1462812590-4494-2-git-send-email-andre.przywara@arm.com> User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 > --- > 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