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
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/[email protected]/
Signed-off-by: Jisheng Zhang <[email protected]>
---
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
On Thu, 6 Oct 2022 at 10:05, Jisheng Zhang <[email protected]> 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/[email protected]/
>
> Signed-off-by: Jisheng Zhang <[email protected]>
> ---
> 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
>
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 <[email protected]> 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/[email protected]/
> > >
> > > Signed-off-by: Jisheng Zhang <[email protected]>
> > > ---
> > > 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
> > >
On Thu, 6 Oct 2022 at 10:55, Jisheng Zhang <[email protected]> wrote:
>
> On Thu, Oct 06, 2022 at 10:17:44AM +0200, Ard Biesheuvel wrote:
> > On Thu, 6 Oct 2022 at 10:05, Jisheng Zhang <[email protected]> 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/[email protected]/
> > >
> > > Signed-off-by: Jisheng Zhang <[email protected]>
> > > ---
> > > 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
> > >
On Thu, Oct 06, 2022 at 10:17:44AM +0200, Ard Biesheuvel wrote:
> On Thu, 6 Oct 2022 at 10:05, Jisheng Zhang <[email protected]> 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/[email protected]/
> >
> > Signed-off-by: Jisheng Zhang <[email protected]>
> > ---
> > 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
> >
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/[email protected]/
>
> Signed-off-by: Jisheng Zhang <[email protected]>
> ---
> 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
>
On Thu, Oct 06, 2022 at 11:08:32AM +0200, Ard Biesheuvel wrote:
> On Thu, 6 Oct 2022 at 10:55, Jisheng Zhang <[email protected]> wrote:
> >
> > On Thu, Oct 06, 2022 at 10:17:44AM +0200, Ard Biesheuvel wrote:
> > > On Thu, 6 Oct 2022 at 10:05, Jisheng Zhang <[email protected]> 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/[email protected]/
> > > >
> > > > Signed-off-by: Jisheng Zhang <[email protected]>
> > > > ---
> > > > 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
> > > >
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/[email protected]/
>
> 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/[email protected]/
> >
> > Signed-off-by: Jisheng Zhang <[email protected]>
> > ---
> > 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
> >
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