* [PATCH 0/2] arm64: constify arguments to satisfy asm constraints
@ 2022-10-06 7:55 Jisheng Zhang
2022-10-06 7:55 ` [PATCH 1/2] arm64: jump_label: mark arguments as const " Jisheng Zhang
` (2 more replies)
0 siblings, 3 replies; 11+ messages in thread
From: Jisheng Zhang @ 2022-10-06 7:55 UTC (permalink / raw)
To: Catalin Marinas, Will Deacon, Peter Zijlstra, Josh Poimboeuf,
Jason Baron, Steven Rostedt, Ard Biesheuvel, Mark Rutland
Cc: linux-arm-kernel, linux-kernel
Constify arguments of arch_static_branch(), arch_static_branch_jump()
and alternative_has_feature_* to satisfy asm constraints.
Jisheng Zhang (2):
arm64: jump_label: mark arguments as const to satisfy asm constraints
arm64: alternative: constify alternative_has_feature_* argument
arch/arm64/include/asm/alternative-macros.h | 4 ++--
arch/arm64/include/asm/jump_label.h | 8 ++++----
2 files changed, 6 insertions(+), 6 deletions(-)
--
2.37.2
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 1/2] arm64: jump_label: mark arguments as const to satisfy asm constraints
2022-10-06 7:55 [PATCH 0/2] arm64: constify arguments to satisfy asm constraints Jisheng Zhang
@ 2022-10-06 7:55 ` Jisheng Zhang
2022-10-06 8:17 ` Ard Biesheuvel
2022-10-06 10:14 ` Mark Rutland
2022-10-06 7:55 ` [PATCH 2/2] arm64: alternative: constify alternative_has_feature_* argument Jisheng Zhang
2022-11-07 19:08 ` [PATCH 0/2] arm64: constify arguments to satisfy asm constraints Will Deacon
2 siblings, 2 replies; 11+ messages in thread
From: Jisheng Zhang @ 2022-10-06 7:55 UTC (permalink / raw)
To: Catalin Marinas, Will Deacon, Peter Zijlstra, Josh Poimboeuf,
Jason Baron, Steven Rostedt, Ard Biesheuvel, Mark Rutland
Cc: linux-arm-kernel, linux-kernel
Inspired by x86 commit 864b435514b2("x86/jump_label: Mark arguments as
const to satisfy asm constraints"), mark arch_static_branch()'s and
arch_static_branch_jump()'s arguments as const to satisfy asm
constraints. And Steven in [1] also pointed out that "The "i"
constraint needs to be a constant."
Tested with building a simple external kernel module with "O0".
[1]https://lore.kernel.org/all/20210212094059.5f8d05e8@gandalf.local.home/
Signed-off-by: Jisheng Zhang <jszhang@kernel.org>
---
arch/arm64/include/asm/jump_label.h | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/arch/arm64/include/asm/jump_label.h b/arch/arm64/include/asm/jump_label.h
index cea441b6aa5d..48ddc0f45d22 100644
--- a/arch/arm64/include/asm/jump_label.h
+++ b/arch/arm64/include/asm/jump_label.h
@@ -15,8 +15,8 @@
#define JUMP_LABEL_NOP_SIZE AARCH64_INSN_SIZE
-static __always_inline bool arch_static_branch(struct static_key *key,
- bool branch)
+static __always_inline bool arch_static_branch(struct static_key * const key,
+ const bool branch)
{
asm_volatile_goto(
"1: nop \n\t"
@@ -32,8 +32,8 @@ static __always_inline bool arch_static_branch(struct static_key *key,
return true;
}
-static __always_inline bool arch_static_branch_jump(struct static_key *key,
- bool branch)
+static __always_inline bool arch_static_branch_jump(struct static_key * const key,
+ const bool branch)
{
asm_volatile_goto(
"1: b %l[l_yes] \n\t"
--
2.37.2
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 2/2] arm64: alternative: constify alternative_has_feature_* argument
2022-10-06 7:55 [PATCH 0/2] arm64: constify arguments to satisfy asm constraints Jisheng Zhang
2022-10-06 7:55 ` [PATCH 1/2] arm64: jump_label: mark arguments as const " Jisheng Zhang
@ 2022-10-06 7:55 ` Jisheng Zhang
2022-11-07 19:08 ` [PATCH 0/2] arm64: constify arguments to satisfy asm constraints Will Deacon
2 siblings, 0 replies; 11+ messages in thread
From: Jisheng Zhang @ 2022-10-06 7:55 UTC (permalink / raw)
To: Catalin Marinas, Will Deacon, Peter Zijlstra, Josh Poimboeuf,
Jason Baron, Steven Rostedt, Ard Biesheuvel, Mark Rutland
Cc: linux-arm-kernel, linux-kernel
Inspired by x86 commit 864b435514b2("x86/jump_label: Mark arguments as
const to satisfy asm constraints"), constify alternative_has_feature_*
argument to satisfy asm constraints. And Steven in [1] also pointed
out that "The "i" constraint needs to be a constant."
Tested with building a simple external kernel module with "O0".
Before the patch, got similar gcc warnings and errors as below:
In file included from <command-line>:
In function ‘alternative_has_feature_likely’,
inlined from ‘system_capabilities_finalized’ at
arch/arm64/include/asm/cpufeature.h:440:9,
inlined from ‘arm64_preempt_schedule_irq’ at
arch/arm64/kernel/entry-common.c:264:6:
include/linux/compiler_types.h:285:33: warning:
‘asm’ operand 0 probably does not match constraints
285 | #define asm_volatile_goto(x...) asm goto(x)
| ^~~
arch/arm64/include/asm/alternative-macros.h:232:9:
note: in expansion of macro ‘asm_volatile_goto’
232 | asm_volatile_goto(
| ^~~~~~~~~~~~~~~~~
include/linux/compiler_types.h:285:33: error:
impossible constraint in ‘asm’
285 | #define asm_volatile_goto(x...) asm goto(x)
| ^~~
arch/arm64/include/asm/alternative-macros.h:232:9:
note: in expansion of macro ‘asm_volatile_goto’
232 | asm_volatile_goto(
| ^~~~~~~~~~~~~~~~~
After the patch, the simple external test kernel module is built fine
with "-O0".
[1]https://lore.kernel.org/all/20210212094059.5f8d05e8@gandalf.local.home/
Signed-off-by: Jisheng Zhang <jszhang@kernel.org>
---
arch/arm64/include/asm/alternative-macros.h | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/arm64/include/asm/alternative-macros.h b/arch/arm64/include/asm/alternative-macros.h
index 3622e9f4fb44..bdf1f6bcd010 100644
--- a/arch/arm64/include/asm/alternative-macros.h
+++ b/arch/arm64/include/asm/alternative-macros.h
@@ -224,7 +224,7 @@ alternative_endif
#include <linux/types.h>
static __always_inline bool
-alternative_has_feature_likely(unsigned long feature)
+alternative_has_feature_likely(const unsigned long feature)
{
compiletime_assert(feature < ARM64_NCAPS,
"feature must be < ARM64_NCAPS");
@@ -242,7 +242,7 @@ alternative_has_feature_likely(unsigned long feature)
}
static __always_inline bool
-alternative_has_feature_unlikely(unsigned long feature)
+alternative_has_feature_unlikely(const unsigned long feature)
{
compiletime_assert(feature < ARM64_NCAPS,
"feature must be < ARM64_NCAPS");
--
2.37.2
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] arm64: jump_label: mark arguments as const to satisfy asm constraints
2022-10-06 7:55 ` [PATCH 1/2] arm64: jump_label: mark arguments as const " Jisheng Zhang
@ 2022-10-06 8:17 ` Ard Biesheuvel
2022-10-06 8:46 ` Jisheng Zhang
2022-10-06 10:14 ` Mark Rutland
1 sibling, 1 reply; 11+ messages in thread
From: Ard Biesheuvel @ 2022-10-06 8:17 UTC (permalink / raw)
To: Jisheng Zhang
Cc: Catalin Marinas, Will Deacon, Peter Zijlstra, Josh Poimboeuf,
Jason Baron, Steven Rostedt, Mark Rutland, linux-arm-kernel,
linux-kernel
On Thu, 6 Oct 2022 at 10:05, Jisheng Zhang <jszhang@kernel.org> wrote:
>
> Inspired by x86 commit 864b435514b2("x86/jump_label: Mark arguments as
> const to satisfy asm constraints"), mark arch_static_branch()'s and
> arch_static_branch_jump()'s arguments as const to satisfy asm
> constraints. And Steven in [1] also pointed out that "The "i"
> constraint needs to be a constant."
>
> Tested with building a simple external kernel module with "O0".
>
> [1]https://lore.kernel.org/all/20210212094059.5f8d05e8@gandalf.local.home/
>
> Signed-off-by: Jisheng Zhang <jszhang@kernel.org>
> ---
> arch/arm64/include/asm/jump_label.h | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/arch/arm64/include/asm/jump_label.h b/arch/arm64/include/asm/jump_label.h
> index cea441b6aa5d..48ddc0f45d22 100644
> --- a/arch/arm64/include/asm/jump_label.h
> +++ b/arch/arm64/include/asm/jump_label.h
> @@ -15,8 +15,8 @@
>
> #define JUMP_LABEL_NOP_SIZE AARCH64_INSN_SIZE
>
> -static __always_inline bool arch_static_branch(struct static_key *key,
> - bool branch)
> +static __always_inline bool arch_static_branch(struct static_key * const key,
> + const bool branch)
> {
> asm_volatile_goto(
> "1: nop \n\t"
Is this still necessary if we specify the constraints in a more
reasonable manner:
" .quad %c0 - . + %1 \n\t"
: : "i"(key), "i"(branch) : : l_yes);
instead of the horrid hack with the char* cast and using a bool as an
array index?
> @@ -32,8 +32,8 @@ static __always_inline bool arch_static_branch(struct static_key *key,
> return true;
> }
>
> -static __always_inline bool arch_static_branch_jump(struct static_key *key,
> - bool branch)
> +static __always_inline bool arch_static_branch_jump(struct static_key * const key,
> + const bool branch)
> {
> asm_volatile_goto(
> "1: b %l[l_yes] \n\t"
> --
> 2.37.2
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] arm64: jump_label: mark arguments as const to satisfy asm constraints
2022-10-06 8:17 ` Ard Biesheuvel
@ 2022-10-06 8:46 ` Jisheng Zhang
2022-10-06 8:55 ` Jisheng Zhang
2022-10-06 9:08 ` Ard Biesheuvel
0 siblings, 2 replies; 11+ messages in thread
From: Jisheng Zhang @ 2022-10-06 8:46 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: Catalin Marinas, Will Deacon, Peter Zijlstra, Josh Poimboeuf,
Jason Baron, Steven Rostedt, Mark Rutland, linux-arm-kernel,
linux-kernel
On Thu, Oct 06, 2022 at 10:17:44AM +0200, Ard Biesheuvel wrote:
> On Thu, 6 Oct 2022 at 10:05, Jisheng Zhang <jszhang@kernel.org> wrote:
> >
> > Inspired by x86 commit 864b435514b2("x86/jump_label: Mark arguments as
> > const to satisfy asm constraints"), mark arch_static_branch()'s and
> > arch_static_branch_jump()'s arguments as const to satisfy asm
> > constraints. And Steven in [1] also pointed out that "The "i"
> > constraint needs to be a constant."
> >
> > Tested with building a simple external kernel module with "O0".
> >
> > [1]https://lore.kernel.org/all/20210212094059.5f8d05e8@gandalf.local.home/
> >
> > Signed-off-by: Jisheng Zhang <jszhang@kernel.org>
> > ---
> > arch/arm64/include/asm/jump_label.h | 8 ++++----
> > 1 file changed, 4 insertions(+), 4 deletions(-)
> >
> > diff --git a/arch/arm64/include/asm/jump_label.h b/arch/arm64/include/asm/jump_label.h
> > index cea441b6aa5d..48ddc0f45d22 100644
> > --- a/arch/arm64/include/asm/jump_label.h
> > +++ b/arch/arm64/include/asm/jump_label.h
> > @@ -15,8 +15,8 @@
> >
> > #define JUMP_LABEL_NOP_SIZE AARCH64_INSN_SIZE
> >
> > -static __always_inline bool arch_static_branch(struct static_key *key,
> > - bool branch)
> > +static __always_inline bool arch_static_branch(struct static_key * const key,
> > + const bool branch)
> > {
> > asm_volatile_goto(
> > "1: nop \n\t"
>
> Is this still necessary if we specify the constraints in a more
> reasonable manner:
>
> " .quad %c0 - . + %1 \n\t"
> : : "i"(key), "i"(branch) : : l_yes);
Just tried this locally with the simple external kernel module, the
asm operand 0 probably does not match constraints can still be
reproduced with "O0".
Thanks
>
> instead of the horrid hack with the char* cast and using a bool as an
> array index?
>
>
>
>
> > @@ -32,8 +32,8 @@ static __always_inline bool arch_static_branch(struct static_key *key,
> > return true;
> > }
> >
> > -static __always_inline bool arch_static_branch_jump(struct static_key *key,
> > - bool branch)
> > +static __always_inline bool arch_static_branch_jump(struct static_key * const key,
> > + const bool branch)
> > {
> > asm_volatile_goto(
> > "1: b %l[l_yes] \n\t"
> > --
> > 2.37.2
> >
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] arm64: jump_label: mark arguments as const to satisfy asm constraints
2022-10-06 8:46 ` Jisheng Zhang
@ 2022-10-06 8:55 ` Jisheng Zhang
2022-10-06 9:08 ` Ard Biesheuvel
1 sibling, 0 replies; 11+ messages in thread
From: Jisheng Zhang @ 2022-10-06 8:55 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: Catalin Marinas, Will Deacon, Peter Zijlstra, Josh Poimboeuf,
Jason Baron, Steven Rostedt, Mark Rutland, linux-arm-kernel,
linux-kernel
On Thu, Oct 06, 2022 at 04:46:21PM +0800, Jisheng Zhang wrote:
> On Thu, Oct 06, 2022 at 10:17:44AM +0200, Ard Biesheuvel wrote:
> > On Thu, 6 Oct 2022 at 10:05, Jisheng Zhang <jszhang@kernel.org> wrote:
> > >
> > > Inspired by x86 commit 864b435514b2("x86/jump_label: Mark arguments as
> > > const to satisfy asm constraints"), mark arch_static_branch()'s and
> > > arch_static_branch_jump()'s arguments as const to satisfy asm
> > > constraints. And Steven in [1] also pointed out that "The "i"
> > > constraint needs to be a constant."
> > >
> > > Tested with building a simple external kernel module with "O0".
> > >
> > > [1]https://lore.kernel.org/all/20210212094059.5f8d05e8@gandalf.local.home/
> > >
> > > Signed-off-by: Jisheng Zhang <jszhang@kernel.org>
> > > ---
> > > arch/arm64/include/asm/jump_label.h | 8 ++++----
> > > 1 file changed, 4 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/arch/arm64/include/asm/jump_label.h b/arch/arm64/include/asm/jump_label.h
> > > index cea441b6aa5d..48ddc0f45d22 100644
> > > --- a/arch/arm64/include/asm/jump_label.h
> > > +++ b/arch/arm64/include/asm/jump_label.h
> > > @@ -15,8 +15,8 @@
> > >
> > > #define JUMP_LABEL_NOP_SIZE AARCH64_INSN_SIZE
> > >
> > > -static __always_inline bool arch_static_branch(struct static_key *key,
> > > - bool branch)
> > > +static __always_inline bool arch_static_branch(struct static_key * const key,
> > > + const bool branch)
> > > {
> > > asm_volatile_goto(
> > > "1: nop \n\t"
> >
> > Is this still necessary if we specify the constraints in a more
> > reasonable manner:
> >
> > " .quad %c0 - . + %1 \n\t"
> > : : "i"(key), "i"(branch) : : l_yes);
>
> Just tried this locally with the simple external kernel module, the
> asm operand 0 probably does not match constraints can still be
> reproduced with "O0".
I mean the "asm operand 0 probably does not match constraints" warning
and related error can still be reproduced with "-O0" w/o the series.
While after applying the series, I can't reproduce the build warning
any more w/ and w/o the above modifications.
>
> Thanks
>
> >
> > instead of the horrid hack with the char* cast and using a bool as an
> > array index?
> >
> >
> >
> >
> > > @@ -32,8 +32,8 @@ static __always_inline bool arch_static_branch(struct static_key *key,
> > > return true;
> > > }
> > >
> > > -static __always_inline bool arch_static_branch_jump(struct static_key *key,
> > > - bool branch)
> > > +static __always_inline bool arch_static_branch_jump(struct static_key * const key,
> > > + const bool branch)
> > > {
> > > asm_volatile_goto(
> > > "1: b %l[l_yes] \n\t"
> > > --
> > > 2.37.2
> > >
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] arm64: jump_label: mark arguments as const to satisfy asm constraints
2022-10-06 8:46 ` Jisheng Zhang
2022-10-06 8:55 ` Jisheng Zhang
@ 2022-10-06 9:08 ` Ard Biesheuvel
2022-10-06 11:04 ` Jisheng Zhang
1 sibling, 1 reply; 11+ messages in thread
From: Ard Biesheuvel @ 2022-10-06 9:08 UTC (permalink / raw)
To: Jisheng Zhang
Cc: Catalin Marinas, Will Deacon, Peter Zijlstra, Josh Poimboeuf,
Jason Baron, Steven Rostedt, Mark Rutland, linux-arm-kernel,
linux-kernel
On Thu, 6 Oct 2022 at 10:55, Jisheng Zhang <jszhang@kernel.org> wrote:
>
> On Thu, Oct 06, 2022 at 10:17:44AM +0200, Ard Biesheuvel wrote:
> > On Thu, 6 Oct 2022 at 10:05, Jisheng Zhang <jszhang@kernel.org> wrote:
> > >
> > > Inspired by x86 commit 864b435514b2("x86/jump_label: Mark arguments as
> > > const to satisfy asm constraints"), mark arch_static_branch()'s and
> > > arch_static_branch_jump()'s arguments as const to satisfy asm
> > > constraints. And Steven in [1] also pointed out that "The "i"
> > > constraint needs to be a constant."
> > >
> > > Tested with building a simple external kernel module with "O0".
> > >
> > > [1]https://lore.kernel.org/all/20210212094059.5f8d05e8@gandalf.local.home/
> > >
> > > Signed-off-by: Jisheng Zhang <jszhang@kernel.org>
> > > ---
> > > arch/arm64/include/asm/jump_label.h | 8 ++++----
> > > 1 file changed, 4 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/arch/arm64/include/asm/jump_label.h b/arch/arm64/include/asm/jump_label.h
> > > index cea441b6aa5d..48ddc0f45d22 100644
> > > --- a/arch/arm64/include/asm/jump_label.h
> > > +++ b/arch/arm64/include/asm/jump_label.h
> > > @@ -15,8 +15,8 @@
> > >
> > > #define JUMP_LABEL_NOP_SIZE AARCH64_INSN_SIZE
> > >
> > > -static __always_inline bool arch_static_branch(struct static_key *key,
> > > - bool branch)
> > > +static __always_inline bool arch_static_branch(struct static_key * const key,
> > > + const bool branch)
> > > {
> > > asm_volatile_goto(
> > > "1: nop \n\t"
> >
> > Is this still necessary if we specify the constraints in a more
> > reasonable manner:
> >
> > " .quad %c0 - . + %1 \n\t"
> > : : "i"(key), "i"(branch) : : l_yes);
>
> Just tried this locally with the simple external kernel module, the
> asm operand 0 probably does not match constraints can still be
> reproduced with "O0".
>
OK so the key pointer needs to be pointer-to-const, which makes sense.
How about when using "Si" as the constraint for operand 0?
>
> >
> > instead of the horrid hack with the char* cast and using a bool as an
> > array index?
> >
> >
> >
> >
> > > @@ -32,8 +32,8 @@ static __always_inline bool arch_static_branch(struct static_key *key,
> > > return true;
> > > }
> > >
> > > -static __always_inline bool arch_static_branch_jump(struct static_key *key,
> > > - bool branch)
> > > +static __always_inline bool arch_static_branch_jump(struct static_key * const key,
> > > + const bool branch)
> > > {
> > > asm_volatile_goto(
> > > "1: b %l[l_yes] \n\t"
> > > --
> > > 2.37.2
> > >
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] arm64: jump_label: mark arguments as const to satisfy asm constraints
2022-10-06 7:55 ` [PATCH 1/2] arm64: jump_label: mark arguments as const " Jisheng Zhang
2022-10-06 8:17 ` Ard Biesheuvel
@ 2022-10-06 10:14 ` Mark Rutland
2022-10-06 11:53 ` Jisheng Zhang
1 sibling, 1 reply; 11+ messages in thread
From: Mark Rutland @ 2022-10-06 10:14 UTC (permalink / raw)
To: Jisheng Zhang
Cc: Catalin Marinas, Will Deacon, Peter Zijlstra, Josh Poimboeuf,
Jason Baron, Steven Rostedt, Ard Biesheuvel, linux-arm-kernel,
linux-kernel
On Thu, Oct 06, 2022 at 03:55:41PM +0800, Jisheng Zhang wrote:
> Inspired by x86 commit 864b435514b2("x86/jump_label: Mark arguments as
> const to satisfy asm constraints"), mark arch_static_branch()'s and
> arch_static_branch_jump()'s arguments as const to satisfy asm
> constraints. And Steven in [1] also pointed out that "The "i"
> constraint needs to be a constant."
It needs to be a *compile-time constant*, but `const` on a function argument
only ensures that the function can't modify the argument, not that it's a
constant in the caller.
I think this is a quirk of the optimizer rather than anything else.
> Tested with building a simple external kernel module with "O0".
Is building with `-O0` supported? I thought we required using `-O2` or above
for a bunch of code that requires constant propagation, etc.
I don't really have a problem with making this const, but I don't particularly
want to try to "fix" all the other code that depends on constant propagation to
assemble, and I'm worried this is the canary in the coal mine.
Thanks,
Mark.
>
> [1]https://lore.kernel.org/all/20210212094059.5f8d05e8@gandalf.local.home/
>
> Signed-off-by: Jisheng Zhang <jszhang@kernel.org>
> ---
> arch/arm64/include/asm/jump_label.h | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/arch/arm64/include/asm/jump_label.h b/arch/arm64/include/asm/jump_label.h
> index cea441b6aa5d..48ddc0f45d22 100644
> --- a/arch/arm64/include/asm/jump_label.h
> +++ b/arch/arm64/include/asm/jump_label.h
> @@ -15,8 +15,8 @@
>
> #define JUMP_LABEL_NOP_SIZE AARCH64_INSN_SIZE
>
> -static __always_inline bool arch_static_branch(struct static_key *key,
> - bool branch)
> +static __always_inline bool arch_static_branch(struct static_key * const key,
> + const bool branch)
> {
> asm_volatile_goto(
> "1: nop \n\t"
> @@ -32,8 +32,8 @@ static __always_inline bool arch_static_branch(struct static_key *key,
> return true;
> }
>
> -static __always_inline bool arch_static_branch_jump(struct static_key *key,
> - bool branch)
> +static __always_inline bool arch_static_branch_jump(struct static_key * const key,
> + const bool branch)
> {
> asm_volatile_goto(
> "1: b %l[l_yes] \n\t"
> --
> 2.37.2
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] arm64: jump_label: mark arguments as const to satisfy asm constraints
2022-10-06 9:08 ` Ard Biesheuvel
@ 2022-10-06 11:04 ` Jisheng Zhang
0 siblings, 0 replies; 11+ messages in thread
From: Jisheng Zhang @ 2022-10-06 11:04 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: Catalin Marinas, Will Deacon, Peter Zijlstra, Josh Poimboeuf,
Jason Baron, Steven Rostedt, Mark Rutland, linux-arm-kernel,
linux-kernel
On Thu, Oct 06, 2022 at 11:08:32AM +0200, Ard Biesheuvel wrote:
> On Thu, 6 Oct 2022 at 10:55, Jisheng Zhang <jszhang@kernel.org> wrote:
> >
> > On Thu, Oct 06, 2022 at 10:17:44AM +0200, Ard Biesheuvel wrote:
> > > On Thu, 6 Oct 2022 at 10:05, Jisheng Zhang <jszhang@kernel.org> wrote:
> > > >
> > > > Inspired by x86 commit 864b435514b2("x86/jump_label: Mark arguments as
> > > > const to satisfy asm constraints"), mark arch_static_branch()'s and
> > > > arch_static_branch_jump()'s arguments as const to satisfy asm
> > > > constraints. And Steven in [1] also pointed out that "The "i"
> > > > constraint needs to be a constant."
> > > >
> > > > Tested with building a simple external kernel module with "O0".
> > > >
> > > > [1]https://lore.kernel.org/all/20210212094059.5f8d05e8@gandalf.local.home/
> > > >
> > > > Signed-off-by: Jisheng Zhang <jszhang@kernel.org>
> > > > ---
> > > > arch/arm64/include/asm/jump_label.h | 8 ++++----
> > > > 1 file changed, 4 insertions(+), 4 deletions(-)
> > > >
> > > > diff --git a/arch/arm64/include/asm/jump_label.h b/arch/arm64/include/asm/jump_label.h
> > > > index cea441b6aa5d..48ddc0f45d22 100644
> > > > --- a/arch/arm64/include/asm/jump_label.h
> > > > +++ b/arch/arm64/include/asm/jump_label.h
> > > > @@ -15,8 +15,8 @@
> > > >
> > > > #define JUMP_LABEL_NOP_SIZE AARCH64_INSN_SIZE
> > > >
> > > > -static __always_inline bool arch_static_branch(struct static_key *key,
> > > > - bool branch)
> > > > +static __always_inline bool arch_static_branch(struct static_key * const key,
> > > > + const bool branch)
> > > > {
> > > > asm_volatile_goto(
> > > > "1: nop \n\t"
> > >
> > > Is this still necessary if we specify the constraints in a more
> > > reasonable manner:
> > >
> > > " .quad %c0 - . + %1 \n\t"
> > > : : "i"(key), "i"(branch) : : l_yes);
> >
> > Just tried this locally with the simple external kernel module, the
> > asm operand 0 probably does not match constraints can still be
> > reproduced with "O0".
> >
>
> OK so the key pointer needs to be pointer-to-const, which makes sense.
> How about when using "Si" as the constraint for operand 0?
"Si" constraint doesn't help either.
>
>
> >
> > >
> > > instead of the horrid hack with the char* cast and using a bool as an
> > > array index?
> > >
> > >
> > >
> > >
> > > > @@ -32,8 +32,8 @@ static __always_inline bool arch_static_branch(struct static_key *key,
> > > > return true;
> > > > }
> > > >
> > > > -static __always_inline bool arch_static_branch_jump(struct static_key *key,
> > > > - bool branch)
> > > > +static __always_inline bool arch_static_branch_jump(struct static_key * const key,
> > > > + const bool branch)
> > > > {
> > > > asm_volatile_goto(
> > > > "1: b %l[l_yes] \n\t"
> > > > --
> > > > 2.37.2
> > > >
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] arm64: jump_label: mark arguments as const to satisfy asm constraints
2022-10-06 10:14 ` Mark Rutland
@ 2022-10-06 11:53 ` Jisheng Zhang
0 siblings, 0 replies; 11+ messages in thread
From: Jisheng Zhang @ 2022-10-06 11:53 UTC (permalink / raw)
To: Mark Rutland
Cc: Catalin Marinas, Will Deacon, Peter Zijlstra, Josh Poimboeuf,
Jason Baron, Steven Rostedt, Ard Biesheuvel, linux-arm-kernel,
linux-kernel
On Thu, Oct 06, 2022 at 11:14:42AM +0100, Mark Rutland wrote:
> On Thu, Oct 06, 2022 at 03:55:41PM +0800, Jisheng Zhang wrote:
> > Inspired by x86 commit 864b435514b2("x86/jump_label: Mark arguments as
> > const to satisfy asm constraints"), mark arch_static_branch()'s and
> > arch_static_branch_jump()'s arguments as const to satisfy asm
> > constraints. And Steven in [1] also pointed out that "The "i"
> > constraint needs to be a constant."
>
> It needs to be a *compile-time constant*, but `const` on a function argument
> only ensures that the function can't modify the argument, not that it's a
compile-time constant is a subset of `const`.
> constant in the caller.
>
> I think this is a quirk of the optimizer rather than anything else.
I dunno compiler internals, just tried as commit 864b435514b2 suggested
the issue did disappear.
PS: I agree with you about this is a quirk or workaround.
>
> > Tested with building a simple external kernel module with "O0".
>
> Is building with `-O0` supported? I thought we required using `-O2` or above
> for a bunch of code that requires constant propagation, etc.
Per the information of Jason's reply in [1]: the reason tring O0/O1 is "to
play around with GCC's new static analyzer options".
While the reason I constify the arguments is that: in riscv world, even the
"-Os" can also reproduce the warnings and errors[2]. Grepping source found
arm64 also shares the same style, so these two patches.
[2]https://lore.kernel.org/linux-riscv/20220922060958.44203-1-samuel@sholland.org/
>
> I don't really have a problem with making this const, but I don't particularly
> want to try to "fix" all the other code that depends on constant propagation to
> assemble, and I'm worried this is the canary in the coal mine.
IMHO, it's a good idea to constify if the arguments can't be modified.
>
> Thanks,
> Mark.
>
> >
> > [1]https://lore.kernel.org/all/20210212094059.5f8d05e8@gandalf.local.home/
> >
> > Signed-off-by: Jisheng Zhang <jszhang@kernel.org>
> > ---
> > arch/arm64/include/asm/jump_label.h | 8 ++++----
> > 1 file changed, 4 insertions(+), 4 deletions(-)
> >
> > diff --git a/arch/arm64/include/asm/jump_label.h b/arch/arm64/include/asm/jump_label.h
> > index cea441b6aa5d..48ddc0f45d22 100644
> > --- a/arch/arm64/include/asm/jump_label.h
> > +++ b/arch/arm64/include/asm/jump_label.h
> > @@ -15,8 +15,8 @@
> >
> > #define JUMP_LABEL_NOP_SIZE AARCH64_INSN_SIZE
> >
> > -static __always_inline bool arch_static_branch(struct static_key *key,
> > - bool branch)
> > +static __always_inline bool arch_static_branch(struct static_key * const key,
> > + const bool branch)
> > {
> > asm_volatile_goto(
> > "1: nop \n\t"
> > @@ -32,8 +32,8 @@ static __always_inline bool arch_static_branch(struct static_key *key,
> > return true;
> > }
> >
> > -static __always_inline bool arch_static_branch_jump(struct static_key *key,
> > - bool branch)
> > +static __always_inline bool arch_static_branch_jump(struct static_key * const key,
> > + const bool branch)
> > {
> > asm_volatile_goto(
> > "1: b %l[l_yes] \n\t"
> > --
> > 2.37.2
> >
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 0/2] arm64: constify arguments to satisfy asm constraints
2022-10-06 7:55 [PATCH 0/2] arm64: constify arguments to satisfy asm constraints Jisheng Zhang
2022-10-06 7:55 ` [PATCH 1/2] arm64: jump_label: mark arguments as const " Jisheng Zhang
2022-10-06 7:55 ` [PATCH 2/2] arm64: alternative: constify alternative_has_feature_* argument Jisheng Zhang
@ 2022-11-07 19:08 ` Will Deacon
2 siblings, 0 replies; 11+ messages in thread
From: Will Deacon @ 2022-11-07 19:08 UTC (permalink / raw)
To: Josh Poimboeuf, Steven Rostedt, Mark Rutland, Peter Zijlstra,
Catalin Marinas, Jisheng Zhang, Jason Baron, Ard Biesheuvel
Cc: kernel-team, Will Deacon, linux-arm-kernel, linux-kernel
On Thu, 6 Oct 2022 15:55:40 +0800, Jisheng Zhang wrote:
> Constify arguments of arch_static_branch(), arch_static_branch_jump()
> and alternative_has_feature_* to satisfy asm constraints.
>
> Jisheng Zhang (2):
> arm64: jump_label: mark arguments as const to satisfy asm constraints
> arm64: alternative: constify alternative_has_feature_* argument
>
> [...]
Applied to arm64 (for-next/asm-const), thanks!
[1/2] arm64: jump_label: mark arguments as const to satisfy asm constraints
https://git.kernel.org/arm64/c/efbc95787cbe
[2/2] arm64: alternative: constify alternative_has_feature_* argument
https://git.kernel.org/arm64/c/b9024f87d63f
Cheers,
--
Will
https://fixes.arm64.dev
https://next.arm64.dev
https://will.arm64.dev
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2022-11-07 19:10 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-10-06 7:55 [PATCH 0/2] arm64: constify arguments to satisfy asm constraints Jisheng Zhang
2022-10-06 7:55 ` [PATCH 1/2] arm64: jump_label: mark arguments as const " Jisheng Zhang
2022-10-06 8:17 ` Ard Biesheuvel
2022-10-06 8:46 ` Jisheng Zhang
2022-10-06 8:55 ` Jisheng Zhang
2022-10-06 9:08 ` Ard Biesheuvel
2022-10-06 11:04 ` Jisheng Zhang
2022-10-06 10:14 ` Mark Rutland
2022-10-06 11:53 ` Jisheng Zhang
2022-10-06 7:55 ` [PATCH 2/2] arm64: alternative: constify alternative_has_feature_* argument Jisheng Zhang
2022-11-07 19:08 ` [PATCH 0/2] arm64: constify arguments to satisfy asm constraints Will Deacon
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).