[PATCH 0/2] arm64: constify arguments to satisfy asm constraints

[PATCH 0/2] arm64: constify arguments to satisfy asm constraints

[Jisheng Zhang]

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

[PATCH 1/2] arm64: jump_label: mark arguments as const to satisfy asm constraints

[Jisheng Zhang]

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

[PATCH 2/2] arm64: alternative: constify alternative_has_feature_* argument

[Jisheng Zhang]

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

Re: [PATCH 1/2] arm64: jump_label: mark arguments as const to satisfy asm constraints

[Ard Biesheuvel]

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

Re: [PATCH 1/2] arm64: jump_label: mark arguments as const to satisfy asm constraints

[Jisheng Zhang]

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

Re: [PATCH 1/2] arm64: jump_label: mark arguments as const to satisfy asm constraints

[Jisheng Zhang]

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

Re: [PATCH 1/2] arm64: jump_label: mark arguments as const to satisfy asm constraints

[Ard Biesheuvel]

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

Re: [PATCH 1/2] arm64: jump_label: mark arguments as const to satisfy asm constraints

[Mark Rutland]

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

Re: [PATCH 1/2] arm64: jump_label: mark arguments as const to satisfy asm constraints

[Jisheng Zhang]

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

Re: [PATCH 1/2] arm64: jump_label: mark arguments as const to satisfy asm constraints

[Jisheng Zhang]

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

Re: [PATCH 0/2] arm64: constify arguments to satisfy asm constraints

[Will Deacon]

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