| Message ID | 20221006075542.2658-2-jszhang@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | arm64: constify arguments to satisfy asm constraints | expand |
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 >
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 > >
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 > > >
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 > > >
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 >
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 > > > >
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 > >
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"
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(-)