From: Catalin Marinas <catalin.marinas@arm.com> To: Andre Przywara <andre.przywara@arm.com> Cc: Will Deacon <will.deacon@arm.com>, 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 Date: Thu, 23 Jun 2016 18:17:33 +0100 [thread overview] Message-ID: <20160623171733.GR6521@e104818-lin.cambridge.arm.com> (raw) In-Reply-To: <1462812590-4494-2-git-send-email-andre.przywara@arm.com> 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
WARNING: multiple messages have this Message-ID (diff)
From: catalin.marinas@arm.com (Catalin Marinas) To: linux-arm-kernel@lists.infradead.org Subject: [PATCH 1/6] arm64: alternatives: drop enable parameter from _else and _endif macro Date: Thu, 23 Jun 2016 18:17:33 +0100 [thread overview] Message-ID: <20160623171733.GR6521@e104818-lin.cambridge.arm.com> (raw) In-Reply-To: <1462812590-4494-2-git-send-email-andre.przywara@arm.com> 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
next prev parent reply other threads:[~2016-06-23 17:17 UTC|newest] Thread overview: 30+ messages / expand[flat|nested] mbox.gz Atom feed top 2016-05-09 16:49 [PATCH 0/6] arm64: Extend Cortex-A53 errata workaround Andre Przywara 2016-05-09 16:49 ` Andre Przywara 2016-05-09 16:49 ` [PATCH 1/6] arm64: alternatives: drop enable parameter from _else and _endif macro Andre Przywara 2016-05-09 16:49 ` Andre Przywara 2016-06-23 17:17 ` Catalin Marinas [this message] 2016-06-23 17:17 ` Catalin Marinas 2016-05-09 16:49 ` [PATCH 2/6] arm64: fix "dc cvau" cache operation on errata-affected core Andre Przywara 2016-05-09 16:49 ` Andre Przywara 2016-05-09 16:49 ` [PATCH 3/6] arm64: include alternative handling in dcache_by_line_op Andre Przywara 2016-05-09 16:49 ` Andre Przywara 2016-06-24 15:32 ` Catalin Marinas 2016-06-24 15:32 ` Catalin Marinas 2016-05-09 16:49 ` [PATCH 4/6] arm64: errata: Calling enable functions for CPU errata too Andre Przywara 2016-05-09 16:49 ` Andre Przywara 2016-06-10 15:31 ` Suzuki K Poulose 2016-06-10 15:31 ` Suzuki K Poulose 2016-06-24 15:34 ` Catalin Marinas 2016-06-24 15:34 ` Catalin Marinas 2016-05-09 16:49 ` [PATCH 5/6] arm64: consolidate signal injection on emulation errors Andre Przywara 2016-05-09 16:49 ` Andre Przywara 2016-05-09 16:49 ` [PATCH 6/6] arm64: trap userspace "dc cvau" cache operation on errata-affected core Andre Przywara 2016-05-09 16:49 ` Andre Przywara 2016-06-14 16:16 ` Suzuki K Poulose 2016-06-14 16:16 ` Suzuki K Poulose 2016-06-17 17:20 ` Andre Przywara 2016-06-17 17:20 ` Andre Przywara 2016-06-17 17:25 ` Suzuki K Poulose 2016-06-17 17:25 ` Suzuki K Poulose 2016-06-24 16:25 ` Catalin Marinas 2016-06-24 16:25 ` Catalin Marinas
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20160623171733.GR6521@e104818-lin.cambridge.arm.com \ --to=catalin.marinas@arm.com \ --cc=andre.przywara@arm.com \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-kernel@vger.kernel.org \ --cc=will.deacon@arm.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.