Re: [PATCH v2 3/3] jump_label: make initial NOP patching the special case

From: Alexander Gordeev
Date: Sun Jun 26 2022 - 04:05:24 EST

On Wed, Jun 15, 2022 at 05:41:42PM +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@xxxxxxxxxx>
> Acked-by: Mark Rutland <mark.rutland@xxxxxxx>
> Acked-by: Peter Zijlstra (Intel) <peterz@xxxxxxxxxxxxx>
> ---
> 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, 7 insertions(+), 83 deletions(-)
>
> 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..714ac4c3b556 100644
> --- a/kernel/jump_label.c
> +++ b/kernel/jump_label.c
> @@ -332,17 +332,13 @@ 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)
> +#ifndef arch_jump_label_transform_static
> +static void arch_jump_label_transform_static(struct jump_entry *entry,
> + enum jump_label_type type)
> {
> - arch_jump_label_transform(entry, type);
> + /* nothing to do on most architectures */
> }
> +#endif
>
> static inline struct jump_entry *static_key_entries(struct static_key *key)
> {

With the follow-up fixup to the common and s390 parts:

Acked-by: Alexander Gordeev <agordeev@xxxxxxxxxxxxx>

> --
> 2.35.1
>