| Message ID | 20220608104512.1176209-4-ardb@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | jump_label: get rid of NOP patching where possible | expand |
On Wed, Jun 08, 2022 at 12:45:12PM +0200, Ard Biesheuvel wrote: > Instead of defaulting to patching NOP opcodes at init time, and leaving > it to the architectures to override this if this is not needed, switch > to a model where doing nothing is the default. This is the common case > by far, as only MIPS requires NOP patching at init time. On all other > architectures, the correct encodings are emitted by the compiler and so > no initial patching is needed. > > Signed-off-by: Ard Biesheuvel <ardb@kernel.org> > --- > Documentation/staging/static-keys.rst | 3 --- > arch/arc/kernel/jump_label.c | 13 ------------- > arch/arm/kernel/jump_label.c | 6 ------ > arch/arm64/kernel/jump_label.c | 11 ----------- > arch/mips/include/asm/jump_label.h | 2 ++ > arch/parisc/kernel/jump_label.c | 11 ----------- > arch/riscv/kernel/jump_label.c | 12 ------------ > arch/s390/kernel/jump_label.c | 5 ----- > arch/x86/kernel/jump_label.c | 13 ------------- > kernel/jump_label.c | 14 +++----------- > 10 files changed, 5 insertions(+), 85 deletions(-) I have one minor comment below, but either way this is a nice cleanup (and I'm always happy to see __weak functions disappear), so FWIW: Acked-by: Mark Rutland <mark.rutland@arm.com> [...] > diff --git a/kernel/jump_label.c b/kernel/jump_label.c > index b1ac2948be79..ff8576c00893 100644 > --- a/kernel/jump_label.c > +++ b/kernel/jump_label.c > @@ -332,17 +332,9 @@ static int __jump_label_text_reserved(struct jump_entry *iter_start, > return 0; > } > > -/* > - * Update code which is definitely not currently executing. > - * Architectures which need heavyweight synchronization to modify > - * running code can override this to make the non-live update case > - * cheaper. > - */ > -void __weak __init_or_module arch_jump_label_transform_static(struct jump_entry *entry, > - enum jump_label_type type) > -{ > - arch_jump_label_transform(entry, type); > -} > +#ifndef arch_jump_label_transform_static > +#define arch_jump_label_transform_static(entry, type) > +#endif It might be slightly better to make this a static inline stub so that we always get the compiler to type-check it, e.g. | #ifndef arch_jump_label_transform_static | static inline void arch_jump_label_transform_static(struct jump_entry *entry, | enum jump_label_type type) | { | /* nothing to do on most architectures */ | } | #define arch_jump_label_transform_static arch_jump_label_transform_static | #endif Mark.
On Wed, 15 Jun 2022 at 11:52, Mark Rutland <mark.rutland@arm.com> wrote: > > On Wed, Jun 08, 2022 at 12:45:12PM +0200, Ard Biesheuvel wrote: > > Instead of defaulting to patching NOP opcodes at init time, and leaving > > it to the architectures to override this if this is not needed, switch > > to a model where doing nothing is the default. This is the common case > > by far, as only MIPS requires NOP patching at init time. On all other > > architectures, the correct encodings are emitted by the compiler and so > > no initial patching is needed. > > > > Signed-off-by: Ard Biesheuvel <ardb@kernel.org> > > --- > > Documentation/staging/static-keys.rst | 3 --- > > arch/arc/kernel/jump_label.c | 13 ------------- > > arch/arm/kernel/jump_label.c | 6 ------ > > arch/arm64/kernel/jump_label.c | 11 ----------- > > arch/mips/include/asm/jump_label.h | 2 ++ > > arch/parisc/kernel/jump_label.c | 11 ----------- > > arch/riscv/kernel/jump_label.c | 12 ------------ > > arch/s390/kernel/jump_label.c | 5 ----- > > arch/x86/kernel/jump_label.c | 13 ------------- > > kernel/jump_label.c | 14 +++----------- > > 10 files changed, 5 insertions(+), 85 deletions(-) > > I have one minor comment below, but either way this is a nice cleanup (and I'm > always happy to see __weak functions disappear), so FWIW: > > Acked-by: Mark Rutland <mark.rutland@arm.com> > > [...] > > > diff --git a/kernel/jump_label.c b/kernel/jump_label.c > > index b1ac2948be79..ff8576c00893 100644 > > --- a/kernel/jump_label.c > > +++ b/kernel/jump_label.c > > @@ -332,17 +332,9 @@ static int __jump_label_text_reserved(struct jump_entry *iter_start, > > return 0; > > } > > > > -/* > > - * Update code which is definitely not currently executing. > > - * Architectures which need heavyweight synchronization to modify > > - * running code can override this to make the non-live update case > > - * cheaper. > > - */ > > -void __weak __init_or_module arch_jump_label_transform_static(struct jump_entry *entry, > > - enum jump_label_type type) > > -{ > > - arch_jump_label_transform(entry, type); > > -} > > +#ifndef arch_jump_label_transform_static > > +#define arch_jump_label_transform_static(entry, type) > > +#endif > > It might be slightly better to make this a static inline stub so that we always > get the compiler to type-check it, e.g. > > | #ifndef arch_jump_label_transform_static > | static inline void arch_jump_label_transform_static(struct jump_entry *entry, > | enum jump_label_type type) > | { > | /* nothing to do on most architectures */ > | } > | #define arch_jump_label_transform_static arch_jump_label_transform_static > | #endif > Yeah, good point. Note that the current patch is broken because of this, as the empty string is not a statement, and so the if binds to the following line (I got a couple of bot warnings about this)
On Wed, Jun 15, 2022 at 10:52:41AM +0100, Mark Rutland wrote: > On Wed, Jun 08, 2022 at 12:45:12PM +0200, Ard Biesheuvel wrote: > > Instead of defaulting to patching NOP opcodes at init time, and leaving > > it to the architectures to override this if this is not needed, switch > > to a model where doing nothing is the default. This is the common case > > by far, as only MIPS requires NOP patching at init time. On all other > > architectures, the correct encodings are emitted by the compiler and so > > no initial patching is needed. > > > > Signed-off-by: Ard Biesheuvel <ardb@kernel.org> > > --- > > Documentation/staging/static-keys.rst | 3 --- > > arch/arc/kernel/jump_label.c | 13 ------------- > > arch/arm/kernel/jump_label.c | 6 ------ > > arch/arm64/kernel/jump_label.c | 11 ----------- > > arch/mips/include/asm/jump_label.h | 2 ++ > > arch/parisc/kernel/jump_label.c | 11 ----------- > > arch/riscv/kernel/jump_label.c | 12 ------------ > > arch/s390/kernel/jump_label.c | 5 ----- > > arch/x86/kernel/jump_label.c | 13 ------------- > > kernel/jump_label.c | 14 +++----------- > > 10 files changed, 5 insertions(+), 85 deletions(-) > > I have one minor comment below, but either way this is a nice cleanup (and I'm > always happy to see __weak functions disappear), so FWIW: (I've got a new found hatred for __weak after having had to fix so many objtool issues with it, so yeah, that). > > Acked-by: Mark Rutland <mark.rutland@arm.com> With the thing Mark pointed out fixed: Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org> (although, I'll probably be the one to eventually apply these I suppose, unless they're needed in a different tree?)
On Wed, 15 Jun 2022 at 12:06, Peter Zijlstra <peterz@infradead.org> wrote: > > On Wed, Jun 15, 2022 at 10:52:41AM +0100, Mark Rutland wrote: > > On Wed, Jun 08, 2022 at 12:45:12PM +0200, Ard Biesheuvel wrote: > > > Instead of defaulting to patching NOP opcodes at init time, and leaving > > > it to the architectures to override this if this is not needed, switch > > > to a model where doing nothing is the default. This is the common case > > > by far, as only MIPS requires NOP patching at init time. On all other > > > architectures, the correct encodings are emitted by the compiler and so > > > no initial patching is needed. > > > > > > Signed-off-by: Ard Biesheuvel <ardb@kernel.org> > > > --- > > > Documentation/staging/static-keys.rst | 3 --- > > > arch/arc/kernel/jump_label.c | 13 ------------- > > > arch/arm/kernel/jump_label.c | 6 ------ > > > arch/arm64/kernel/jump_label.c | 11 ----------- > > > arch/mips/include/asm/jump_label.h | 2 ++ > > > arch/parisc/kernel/jump_label.c | 11 ----------- > > > arch/riscv/kernel/jump_label.c | 12 ------------ > > > arch/s390/kernel/jump_label.c | 5 ----- > > > arch/x86/kernel/jump_label.c | 13 ------------- > > > kernel/jump_label.c | 14 +++----------- > > > 10 files changed, 5 insertions(+), 85 deletions(-) > > > > I have one minor comment below, but either way this is a nice cleanup (and I'm > > always happy to see __weak functions disappear), so FWIW: > > (I've got a new found hatred for __weak after having had to fix so many > objtool issues with it, so yeah, that). > > > > > Acked-by: Mark Rutland <mark.rutland@arm.com> > > With the thing Mark pointed out fixed: > > Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org> > > (although, I'll probably be the one to eventually apply these I suppose, > unless they're needed in a different tree?) Not really - this just came up when Jason was looking into how to enable jump labels extremely early on every single architecture, but fortunately, that issue got fixed in a different way. I'll respin and resend and leave it to you to apply them whenever convenient.
diff --git a/Documentation/staging/static-keys.rst b/Documentation/staging/static-keys.rst index 38290b9f25eb..b0a519f456cf 100644 --- a/Documentation/staging/static-keys.rst +++ b/Documentation/staging/static-keys.rst @@ -201,9 +201,6 @@ static_key->entry field makes use of the two least significant bits. * ``void arch_jump_label_transform(struct jump_entry *entry, enum jump_label_type type)``, see: arch/x86/kernel/jump_label.c -* ``__init_or_module void arch_jump_label_transform_static(struct jump_entry *entry, enum jump_label_type type)``, - see: arch/x86/kernel/jump_label.c - * ``struct jump_entry``, see: arch/x86/include/asm/jump_label.h diff --git a/arch/arc/kernel/jump_label.c b/arch/arc/kernel/jump_label.c index b8600dc325b5..70b74a5d047b 100644 --- a/arch/arc/kernel/jump_label.c +++ b/arch/arc/kernel/jump_label.c @@ -96,19 +96,6 @@ void arch_jump_label_transform(struct jump_entry *entry, flush_icache_range(entry->code, entry->code + JUMP_LABEL_NOP_SIZE); } -void arch_jump_label_transform_static(struct jump_entry *entry, - enum jump_label_type type) -{ - /* - * We use only one NOP type (1x, 4 byte) in arch_static_branch, so - * there's no need to patch an identical NOP over the top of it here. - * The generic code calls 'arch_jump_label_transform' if the NOP needs - * to be replaced by a branch, so 'arch_jump_label_transform_static' is - * never called with type other than JUMP_LABEL_NOP. - */ - BUG_ON(type != JUMP_LABEL_NOP); -} - #ifdef CONFIG_ARC_DBG_JUMP_LABEL #define SELFTEST_MSG "ARC: instruction generation self-test: " diff --git a/arch/arm/kernel/jump_label.c b/arch/arm/kernel/jump_label.c index 303b3ab87f7e..eb9c24b6e8e2 100644 --- a/arch/arm/kernel/jump_label.c +++ b/arch/arm/kernel/jump_label.c @@ -27,9 +27,3 @@ void arch_jump_label_transform(struct jump_entry *entry, { __arch_jump_label_transform(entry, type, false); } - -void arch_jump_label_transform_static(struct jump_entry *entry, - enum jump_label_type type) -{ - __arch_jump_label_transform(entry, type, true); -} diff --git a/arch/arm64/kernel/jump_label.c b/arch/arm64/kernel/jump_label.c index fc98037e1220..faf88ec9c48e 100644 --- a/arch/arm64/kernel/jump_label.c +++ b/arch/arm64/kernel/jump_label.c @@ -26,14 +26,3 @@ void arch_jump_label_transform(struct jump_entry *entry, aarch64_insn_patch_text_nosync(addr, insn); } - -void arch_jump_label_transform_static(struct jump_entry *entry, - enum jump_label_type type) -{ - /* - * We use the architected A64 NOP in arch_static_branch, so there's no - * need to patch an identical A64 NOP over the top of it here. The core - * will call arch_jump_label_transform from a module notifier if the - * NOP needs to be replaced by a branch. - */ -} diff --git a/arch/mips/include/asm/jump_label.h b/arch/mips/include/asm/jump_label.h index 3185fd3220ec..c5c6864e64bc 100644 --- a/arch/mips/include/asm/jump_label.h +++ b/arch/mips/include/asm/jump_label.h @@ -8,6 +8,8 @@ #ifndef _ASM_MIPS_JUMP_LABEL_H #define _ASM_MIPS_JUMP_LABEL_H +#define arch_jump_label_transform_static arch_jump_label_transform + #ifndef __ASSEMBLY__ #include <linux/types.h> diff --git a/arch/parisc/kernel/jump_label.c b/arch/parisc/kernel/jump_label.c index d2f3cb12e282..e253b134500d 100644 --- a/arch/parisc/kernel/jump_label.c +++ b/arch/parisc/kernel/jump_label.c @@ -42,14 +42,3 @@ void arch_jump_label_transform(struct jump_entry *entry, patch_text(addr, insn); } - -void arch_jump_label_transform_static(struct jump_entry *entry, - enum jump_label_type type) -{ - /* - * We use the architected NOP in arch_static_branch, so there's no - * need to patch an identical NOP over the top of it here. The core - * will call arch_jump_label_transform from a module notifier if the - * NOP needs to be replaced by a branch. - */ -} diff --git a/arch/riscv/kernel/jump_label.c b/arch/riscv/kernel/jump_label.c index 20e09056d141..e6694759dbd0 100644 --- a/arch/riscv/kernel/jump_label.c +++ b/arch/riscv/kernel/jump_label.c @@ -39,15 +39,3 @@ void arch_jump_label_transform(struct jump_entry *entry, patch_text_nosync(addr, &insn, sizeof(insn)); mutex_unlock(&text_mutex); } - -void arch_jump_label_transform_static(struct jump_entry *entry, - enum jump_label_type type) -{ - /* - * We use the same instructions in the arch_static_branch and - * arch_static_branch_jump inline functions, so there's no - * need to patch them up here. - * The core will call arch_jump_label_transform when those - * instructions need to be replaced. - */ -} diff --git a/arch/s390/kernel/jump_label.c b/arch/s390/kernel/jump_label.c index d764f0d229ab..e808bb8bc0da 100644 --- a/arch/s390/kernel/jump_label.c +++ b/arch/s390/kernel/jump_label.c @@ -80,8 +80,3 @@ void arch_jump_label_transform_apply(void) { text_poke_sync(); } - -void __init_or_module arch_jump_label_transform_static(struct jump_entry *entry, - enum jump_label_type type) -{ -} diff --git a/arch/x86/kernel/jump_label.c b/arch/x86/kernel/jump_label.c index 68f091ba8443..f5b8ef02d172 100644 --- a/arch/x86/kernel/jump_label.c +++ b/arch/x86/kernel/jump_label.c @@ -146,16 +146,3 @@ void arch_jump_label_transform_apply(void) text_poke_finish(); mutex_unlock(&text_mutex); } - -static enum { - JL_STATE_START, - JL_STATE_NO_UPDATE, - JL_STATE_UPDATE, -} jlstate __initdata_or_module = JL_STATE_START; - -__init_or_module void arch_jump_label_transform_static(struct jump_entry *entry, - enum jump_label_type type) -{ - if (jlstate == JL_STATE_UPDATE) - jump_label_transform(entry, type, 1); -} diff --git a/kernel/jump_label.c b/kernel/jump_label.c index b1ac2948be79..ff8576c00893 100644 --- a/kernel/jump_label.c +++ b/kernel/jump_label.c @@ -332,17 +332,9 @@ static int __jump_label_text_reserved(struct jump_entry *iter_start, return 0; } -/* - * Update code which is definitely not currently executing. - * Architectures which need heavyweight synchronization to modify - * running code can override this to make the non-live update case - * cheaper. - */ -void __weak __init_or_module arch_jump_label_transform_static(struct jump_entry *entry, - enum jump_label_type type) -{ - arch_jump_label_transform(entry, type); -} +#ifndef arch_jump_label_transform_static +#define arch_jump_label_transform_static(entry, type) +#endif static inline struct jump_entry *static_key_entries(struct static_key *key) {
Instead of defaulting to patching NOP opcodes at init time, and leaving it to the architectures to override this if this is not needed, switch to a model where doing nothing is the default. This is the common case by far, as only MIPS requires NOP patching at init time. On all other architectures, the correct encodings are emitted by the compiler and so no initial patching is needed. Signed-off-by: Ard Biesheuvel <ardb@kernel.org> --- Documentation/staging/static-keys.rst | 3 --- arch/arc/kernel/jump_label.c | 13 ------------- arch/arm/kernel/jump_label.c | 6 ------ arch/arm64/kernel/jump_label.c | 11 ----------- arch/mips/include/asm/jump_label.h | 2 ++ arch/parisc/kernel/jump_label.c | 11 ----------- arch/riscv/kernel/jump_label.c | 12 ------------ arch/s390/kernel/jump_label.c | 5 ----- arch/x86/kernel/jump_label.c | 13 ------------- kernel/jump_label.c | 14 +++----------- 10 files changed, 5 insertions(+), 85 deletions(-)