jump_label: get rid of NOP patching where possible

PATCH Superseded Ard Biesheuvel 4 participants 9 messages Jun 8, 2022 – Jun 15, 2022

Subsystems (11)

Documentation Arm (32-bit) x86 MIPS +7 more subsystems

lore

Share & tools

lore

Ard Biesheuvel

author reviewer

2022-06-08 10:45 UTC #0 lore raw

cover jump_label: get rid of NOP patching where possible

The only architecture that actually needs to convert compiler generated jump label NOP encodings into something else at runtime is MIPS, because the assembler cannot be trusted to emit a sequence that can be safely patched into a branch instruction.

All other architectures either do nothing with jump label NOPs at load time, or patch a perfectly good NOP into a different one, or into the same one - none of this seems very useful, so let's get rid of it where we can.

Cc Peter Zijlstra <[email protected]>

Cc Thomas Bogendoerfer <[email protected]>

Cc Heiko Carstens <[email protected]>

Cc Vasily Gorbik <[email protected]>

Cc Alexander Gordeev <[email protected]>

Cc Christian Borntraeger <[email protected]>

Cc Sven Schnelle <[email protected]>

Cc [email protected]

Ard Biesheuvel (3):
jump_label: s390: avoid pointless initial NOP patching
jump_label: mips: move module NOP patching into arch code
jump_label: make initial NOP patching the special case

 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/mips/kernel/jump_label.c         | 17 ++++++++
 arch/mips/kernel/module.c             |  5 ++-
 arch/parisc/kernel/jump_label.c       | 11 ------
 arch/riscv/kernel/jump_label.c        | 12 ------
 arch/s390/include/asm/jump_label.h    |  5 +--
 arch/s390/kernel/jump_label.c         | 28 +++----------
 arch/s390/kernel/module.c             |  1 -
 arch/sparc/kernel/module.c            |  3 --
 arch/x86/kernel/jump_label.c          | 13 -------
 arch/x86/kernel/module.c              |  3 --
 include/linux/jump_label.h            |  7 +---
 kernel/jump_label.c                   | 41 ++------------------
 17 files changed, 34 insertions(+), 147 deletions(-)

Signature · 1 line

2.30.2

Ard Biesheuvel

author reviewer

2022-06-08 10:45 UTC #1 lore raw

PATCH 1/3 jump_label: s390: avoid pointless initial NOP patching

Patching NOPs into other NOPs at boot time serves no purpose, so let's use the same NOP encodings at compile time and runtime.

Signed-off-by Ard Biesheuvel <[email protected]>

---
 arch/s390/include/asm/jump_label.h |  5 ++---
 arch/s390/kernel/jump_label.c      | 23 +++++---------------
 2 files changed, 7 insertions(+), 21 deletions(-)

arch/s390/include/asm/jump_label.h +2 -3

index 916cfcb36d8a..895f774bbcc5 100644
@@ -10,7 +10,6 @@
 #include <linux/stringify.h>
 
 #define JUMP_LABEL_NOP_SIZE 6
-#define JUMP_LABEL_NOP_OFFSET 2
 
 #ifdef CONFIG_CC_IS_CLANG
 #define JUMP_LABEL_STATIC_KEY_CONSTRAINT "i"

@@ -21,12 +20,12 @@
 #endif
 
 /*
- * We use a brcl 0,2 instruction for jump labels at compile time so it
+ * We use a brcl 0,<offset> instruction for jump labels so it
  * can be easily distinguished from a hotpatch generated instruction.
  */
 static __always_inline bool arch_static_branch(struct static_key *key, bool branch)
 {
-	asm_volatile_goto("0:	brcl	0,"__stringify(JUMP_LABEL_NOP_OFFSET)"\n"
+	asm_volatile_goto("0:	brcl 0,%l[label]\n"
 			  ".pushsection __jump_table,\"aw\"\n"
 			  ".balign	8\n"
 			  ".long	0b-.,%l[label]-.\n"

arch/s390/kernel/jump_label.c +5 ~~-18~~

index 6bec000c6c1c..d764f0d229ab 100644
@@ -44,14 +44,8 @@ static void jump_label_bug(struct jump_entry *entry, struct insn *expected,
 	panic("Corrupted kernel text");
 }
 
-static struct insn orignop = {
-	.opcode = 0xc004,
-	.offset = JUMP_LABEL_NOP_OFFSET >> 1,
-};
-
 static void jump_label_transform(struct jump_entry *entry,
-				 enum jump_label_type type,
-				 int init)
+				 enum jump_label_type type)
 {
 	void *code = (void *)jump_entry_code(entry);
 	struct insn old, new;

@@ -63,27 +57,22 @@ static void jump_label_transform(struct jump_entry *entry,
 		jump_label_make_branch(entry, &old);
 		jump_label_make_nop(entry, &new);
 	}
-	if (init) {
-		if (memcmp(code, &orignop, sizeof(orignop)))
-			jump_label_bug(entry, &orignop, &new);
-	} else {
-		if (memcmp(code, &old, sizeof(old)))
-			jump_label_bug(entry, &old, &new);
-	}
+	if (memcmp(code, &old, sizeof(old)))
+		jump_label_bug(entry, &old, &new);
 	s390_kernel_write(code, &new, sizeof(new));
 }
 
 void arch_jump_label_transform(struct jump_entry *entry,
 			       enum jump_label_type type)
 {
-	jump_label_transform(entry, type, 0);
+	jump_label_transform(entry, type);
 	text_poke_sync();
 }
 
 bool arch_jump_label_transform_queue(struct jump_entry *entry,
 				     enum jump_label_type type)
 {
-	jump_label_transform(entry, type, 0);
+	jump_label_transform(entry, type);
 	return true;
 }

@@ -95,6 +84,4 @@ void arch_jump_label_transform_apply(void)
 void __init_or_module arch_jump_label_transform_static(struct jump_entry *entry,
 						       enum jump_label_type type)
 {
-	jump_label_transform(entry, type, 1);
-	text_poke_sync();
 }

Signature · 1 line

2.30.2

Ard Biesheuvel

author reviewer

2022-06-08 10:45 UTC #2 lore raw

PATCH 2/3 jump_label: mips: move module NOP patching into arch code

In reply to Ard Biesheuvel

MIPS is the only remaining architecture that needs to patch jump label NOP encodings to initialize them at load time. So let's move the module patching part of that from generic code into arch/mips, and drop it from the others.

Signed-off-by Ard Biesheuvel <[email protected]>

---
 arch/mips/kernel/jump_label.c | 17 ++++++++++++
 arch/mips/kernel/module.c     |  5 ++--
 arch/s390/kernel/module.c     |  1 -
 arch/sparc/kernel/module.c    |  3 ---
 arch/x86/kernel/module.c      |  3 ---
 include/linux/jump_label.h    |  7 +----
 kernel/jump_label.c           | 27 +-------------------
 7 files changed, 22 insertions(+), 41 deletions(-)

arch/mips/kernel/jump_label.c +17 -0

index 662c8db9f45b..0f98e7f1461f 100644
@@ -88,3 +88,20 @@ void arch_jump_label_transform(struct jump_entry *e,
 
 	mutex_unlock(&text_mutex);
 }
+
+void jump_label_apply_nops(struct module *mod)
+{
+	struct jump_entry *iter_start = mod->jump_entries;
+	struct jump_entry *iter_stop = iter_start + mod->num_jump_entries;
+	struct jump_entry *iter;
+
+	/* if the module doesn't have jump label entries, just return */
+	if (iter_start == iter_stop)
+		return;
+
+	for (iter = iter_start; iter < iter_stop; iter++) {
+		/* Only write NOPs for arch_branch_static(). */
+		if (jump_label_init_type(iter) == JUMP_LABEL_NOP)
+			arch_jump_label_transform(iter, JUMP_LABEL_NOP);
+	}
+}

arch/mips/kernel/module.c +3 -2

index 14f46d17500a..0c936cbf20c5 100644
@@ -21,6 +21,7 @@
 #include <linux/spinlock.h>
 #include <linux/jump_label.h>
 
+extern void jump_label_apply_nops(struct module *mod);
 
 struct mips_hi16 {
 	struct mips_hi16 *next;

@@ -428,8 +429,8 @@ int module_finalize(const Elf_Ehdr *hdr,
 	const Elf_Shdr *s;
 	char *secstrings = (void *)hdr + sechdrs[hdr->e_shstrndx].sh_offset;
 
-	/* Make jump label nops. */
-	jump_label_apply_nops(me);
+	if (IS_ENABLED(CONFIG_JUMP_LABEL))
+		jump_label_apply_nops(me);
 
 	INIT_LIST_HEAD(&me->arch.dbe_list);
 	for (s = sechdrs; s < sechdrs + hdr->e_shnum; s++) {

arch/s390/kernel/module.c +0 -1

index 26125a9c436d..2d159b32885b 100644
@@ -548,6 +548,5 @@ int module_finalize(const Elf_Ehdr *hdr,
 #endif /* CONFIG_FUNCTION_TRACER */
 	}
 
-	jump_label_apply_nops(me);
 	return 0;
 }

arch/sparc/kernel/module.c +0 -3

index df39580f398d..66c45a2764bc 100644
@@ -208,9 +208,6 @@ int module_finalize(const Elf_Ehdr *hdr,
 		    const Elf_Shdr *sechdrs,
 		    struct module *me)
 {
-	/* make jump label nops */
-	jump_label_apply_nops(me);
-
 	do_patch_sections(hdr, sechdrs);
 
 	/* Cheetah's I-cache is fully coherent.  */

arch/x86/kernel/module.c +0 -3

index b98ffcf4d250..95b9cf25d4bd 100644
@@ -304,9 +304,6 @@ int module_finalize(const Elf_Ehdr *hdr,
 					    tseg, tseg + text->sh_size);
 	}
 
-	/* make jump label nops */
-	jump_label_apply_nops(me);
-
 	if (orc && orc_ip)
 		unwind_module_init(me, (void *)orc_ip->sh_addr, orc_ip->sh_size,
 				   (void *)orc->sh_addr, orc->sh_size);

include/linux/jump_label.h +1 -6

index bf1eef337a07..2003a0935478 100644
@@ -230,12 +230,12 @@ extern void static_key_slow_inc(struct static_key *key);
 extern void static_key_slow_dec(struct static_key *key);
 extern void static_key_slow_inc_cpuslocked(struct static_key *key);
 extern void static_key_slow_dec_cpuslocked(struct static_key *key);
-extern void jump_label_apply_nops(struct module *mod);
 extern int static_key_count(struct static_key *key);
 extern void static_key_enable(struct static_key *key);
 extern void static_key_disable(struct static_key *key);
 extern void static_key_enable_cpuslocked(struct static_key *key);
 extern void static_key_disable_cpuslocked(struct static_key *key);
+extern enum jump_label_type jump_label_init_type(struct jump_entry *entry);
 
 /*
  * We should be using ATOMIC_INIT() for initializing .enabled, but

@@ -303,11 +303,6 @@ static inline int jump_label_text_reserved(void *start, void *end)
 static inline void jump_label_lock(void) {}
 static inline void jump_label_unlock(void) {}
 
-static inline int jump_label_apply_nops(struct module *mod)
-{
-	return 0;
-}
-
 static inline void static_key_enable(struct static_key *key)
 {
 	STATIC_KEY_CHECK_USE(key);

kernel/jump_label.c +1 ~~-26~~

index b156e152d6b4..b1ac2948be79 100644
@@ -508,7 +508,7 @@ void __init jump_label_init(void)
 
 #ifdef CONFIG_MODULES
 
-static enum jump_label_type jump_label_init_type(struct jump_entry *entry)
+enum jump_label_type jump_label_init_type(struct jump_entry *entry)
 {
 	struct static_key *key = jump_entry_key(entry);
 	bool type = static_key_type(key);

@@ -596,31 +596,6 @@ static void __jump_label_mod_update(struct static_key *key)
 	}
 }
 
-/***
- * apply_jump_label_nops - patch module jump labels with arch_get_jump_label_nop()
- * @mod: module to patch
- *
- * Allow for run-time selection of the optimal nops. Before the module
- * loads patch these with arch_get_jump_label_nop(), which is specified by
- * the arch specific jump label code.
- */
-void jump_label_apply_nops(struct module *mod)
-{
-	struct jump_entry *iter_start = mod->jump_entries;
-	struct jump_entry *iter_stop = iter_start + mod->num_jump_entries;
-	struct jump_entry *iter;
-
-	/* if the module doesn't have jump label entries, just return */
-	if (iter_start == iter_stop)
-		return;
-
-	for (iter = iter_start; iter < iter_stop; iter++) {
-		/* Only write NOPs for arch_branch_static(). */
-		if (jump_label_init_type(iter) == JUMP_LABEL_NOP)
-			arch_jump_label_transform_static(iter, JUMP_LABEL_NOP);
-	}
-}
-
 static int jump_label_add_module(struct module *mod)
 {
 	struct jump_entry *iter_start = mod->jump_entries;

Signature · 1 line

2.30.2

kernel test robot

build bot

2022-06-10 00:42 UTC #3 lore raw

Re: [PATCH 2/3] jump_label: mips: move module NOP patching into arch code

I love your patch! Yet something to improve:

Hi Ard,

I love your patch! Yet something to improve:

[auto build test ERROR on s390/features]
[also build test ERROR on tip/x86/core vgupta-arc/for-next arm64/for-next/core deller-parisc/for-next linus/master v5.19-rc1 next-20220609] [If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/intel-lab-lkp/linux/commits/Ard-Biesheuvel/jump_label-get-rid-of-NOP-patching-where-possible/20220608-184857
base:   https://git.kernel.org/pub/scm/linux/kernel/git/s390/linux.git features
config: mips-rs90_defconfig (https://download.01.org/0day-ci/archive/20220610/[email protected]/config)
compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project 971e13d69e3e7b687213fef22952be6a328c426c)

reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross
# install mips cross compiling tool for clang build
# apt-get install binutils-mipsel-linux-gnu
# https://github.com/intel-lab-lkp/linux/commit/0f76bf7e2cda1943571bcfa08d52505950425ef8
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Ard-Biesheuvel/jump_label-get-rid-of-NOP-patching-where-possible/20220608-184857 git checkout 0f76bf7e2cda1943571bcfa08d52505950425ef8
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=mips SHELL=/bin/bash arch/mips/kernel/

If you fix the issue, kindly add following tag where applicable

Reported-by kernel test robot <[email protected]>

All error/warnings (new ones prefixed by >>):

>> arch/mips/kernel/jump_label.c:94:37: error: incomplete definition of type 'struct module'
           struct jump_entry *iter_start = mod->jump_entries;
                                           ~~~^
   include/linux/jump_label.h:196:8: note: forward declaration of 'struct module'
   struct module;
          ^
   arch/mips/kernel/jump_label.c:95:49: error: incomplete definition of type 'struct module'
           struct jump_entry *iter_stop = iter_start + mod->num_jump_entries;
                                                       ~~~^
   include/linux/jump_label.h:196:8: note: forward declaration of 'struct module'
   struct module;
          ^
>> arch/mips/kernel/jump_label.c:92:6: warning: no previous prototype for function 'jump_label_apply_nops' [-Wmissing-prototypes]
   void jump_label_apply_nops(struct module *mod)
        ^

arch/mips/kernel/jump_label.c:92:1: note: declare 'static' if the function is not intended to be used outside of this translation unit void jump_label_apply_nops(struct module *mod)
^
static
1 warning and 2 errors generated.

vim +94 arch/mips/kernel/jump_label.c

    91	
  > 92	void jump_label_apply_nops(struct module *mod)
    93	{
  > 94		struct jump_entry *iter_start = mod->jump_entries;

Signature · 2 lines

0-DAY CI Kernel Test Service
https://01.org/lkp

Ard Biesheuvel

author reviewer

2022-06-08 10:45 UTC #4 lore raw

PATCH 3/3 jump_label: make initial NOP patching the special case

In reply to Ard Biesheuvel

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 <[email protected]>

---
 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(-)

Documentation/staging/static-keys.rst +0 -3

index 38290b9f25eb..b0a519f456cf 100644
@@ -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

arch/arc/kernel/jump_label.c +0 ~~-13~~

index b8600dc325b5..70b74a5d047b 100644
@@ -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: "

arch/arm/kernel/jump_label.c +0 -6

index 303b3ab87f7e..eb9c24b6e8e2 100644
@@ -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);
-}

arch/arm64/kernel/jump_label.c +0 ~~-11~~

index fc98037e1220..faf88ec9c48e 100644
@@ -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.
-	 */
-}

arch/mips/include/asm/jump_label.h +2 -0

index 3185fd3220ec..c5c6864e64bc 100644
@@ -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>

arch/parisc/kernel/jump_label.c +0 ~~-11~~

index d2f3cb12e282..e253b134500d 100644
@@ -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.
-	 */
-}

arch/riscv/kernel/jump_label.c +0 ~~-12~~

index 20e09056d141..e6694759dbd0 100644
@@ -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.
-	 */
-}

arch/s390/kernel/jump_label.c +0 -5

index d764f0d229ab..e808bb8bc0da 100644
@@ -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)
-{
-}

arch/x86/kernel/jump_label.c +0 ~~-13~~

index 68f091ba8443..f5b8ef02d172 100644
@@ -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);
-}

kernel/jump_label.c +3 ~~-11~~

index b1ac2948be79..ff8576c00893 100644
@@ -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)
 {

Signature · 1 line

2.30.2

Mark Rutland

reviewer Acked-by on v1 3/3

2022-06-15 09:52 UTC #5 lore raw

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

Quoted 8 lines · Ard Biesheuvel

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 <[email protected]>

Quoted 12 lines

---
 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 <[email protected]>

[...]

Quoted 12 lines

kernel/jump_label.c +0 -4

index b1ac2948be79..ff8576c00893 100644
@@ -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

kernel/jump_label.c static int __jump_label_text_reserved(struct jump_entry *iter_start, · line 337

Quoted 10 lines

kernel/jump_label.c +3 -7

- * 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.

Ard Biesheuvel

author reviewer

2022-06-15 09:59 UTC #6 lore raw

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

Quoted 56 lines · Mark Rutland

Quoted 8 lines · Ard Biesheuvel
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 <[email protected]>
Quoted 12 lines
---
 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 <[email protected]>
[...]
Quoted 12 lines
kernel/jump_label.c +0 -4
index b1ac2948be79..ff8576c00893 100644
@@ -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
kernel/jump_label.c static int __jump_label_text_reserved(struct jump_entry *iter_start, · line 337
Quoted 10 lines
kernel/jump_label.c +3 -7
- * 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.

Quoted 9 lines

| #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)

Peter Zijlstra

maintainer Acked-by

2022-06-15 10:06 UTC #7 lore raw

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

In reply to Mark Rutland

Quoted 22 lines · Mark Rutland

Quoted 8 lines · Ard Biesheuvel
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 <[email protected]>
Quoted 12 lines
---
 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(-)

Quoted 2 lines

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).

Quoted 2 lines

Acked-by Mark Rutland <[email protected]>

With the thing Mark pointed out fixed:

Acked-by Peter Zijlstra (Intel) <[email protected]>

(although, I'll probably be the one to eventually apply these I suppose, unless they're needed in a different tree?)

Ard Biesheuvel

author reviewer

2022-06-15 10:21 UTC #8 lore raw

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

Quoted 31 lines · Peter Zijlstra

Quoted 22 lines · Mark Rutland
Quoted 8 lines · Ard Biesheuvel
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 <[email protected]>
Quoted 12 lines
---
 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(-)
Quoted 2 lines
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).
Quoted 1 line

Quoted 8 lines

Quoted 1 line
Acked-by Mark Rutland <[email protected]>
With the thing Mark pointed out fixed:
Acked-by Peter Zijlstra (Intel) <[email protected]>
(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.