[PATCH v2 0/7] riscv: Various text patching improvements

[PATCH v2 0/7] riscv: Various text patching improvements

[Samuel Holland]

Here are a few changes to minimize calls to stop_machine() and
flush_icache_*() in the various text patching functions, as well as
to simplify the code.

This series is based on "[PATCH v3 0/2] riscv: fix patching with IPI"[1].

[1]: https://lore.kernel.org/linux-riscv/20240229121056.203419-1-alexghiti@rivosinc.com/

Changes in v2:
 - Remove unnecessary line wrapping
 - Further simplify patch_insn_set()/patch_insn_write() loop conditions
 - Use min() instead of min_t() since both sides are unsigned long

Samuel Holland (7):
  riscv: jump_label: Batch icache maintenance
  riscv: jump_label: Simplify assembly syntax
  riscv: kprobes: Use patch_text_nosync() for insn slots
  riscv: Simplify text patching loops
  riscv: Pass patch_text() the length in bytes
  riscv: Use offset_in_page() in text patching functions
  riscv: Remove extra variable in patch_text_nosync()

 arch/riscv/include/asm/jump_label.h |  4 +-
 arch/riscv/include/asm/patch.h      |  2 +-
 arch/riscv/kernel/jump_label.c      | 16 +++++--
 arch/riscv/kernel/patch.c           | 69 ++++++++++++++---------------
 arch/riscv/kernel/probes/kprobes.c  | 19 ++++----
 arch/riscv/net/bpf_jit_comp64.c     |  7 +--
 6 files changed, 63 insertions(+), 54 deletions(-)

-- 
2.43.1

[PATCH v2 1/7] riscv: jump_label: Batch icache maintenance

[Samuel Holland]

Switch to the batched version of the jump label update functions so
instruction cache maintenance is deferred until the end of the update.

Reviewed-by: Björn Töpel <bjorn@rivosinc.com>
Signed-off-by: Samuel Holland <samuel.holland@sifive.com>
---

(no changes since v1)

 arch/riscv/include/asm/jump_label.h |  2 ++
 arch/riscv/kernel/jump_label.c      | 16 ++++++++++++----
 2 files changed, 14 insertions(+), 4 deletions(-)

diff --git a/arch/riscv/include/asm/jump_label.h b/arch/riscv/include/asm/jump_label.h
index 4a35d787c019..6290b26f4a14 100644
--- a/arch/riscv/include/asm/jump_label.h
+++ b/arch/riscv/include/asm/jump_label.h
@@ -12,6 +12,8 @@
 #include <linux/types.h>
 #include <asm/asm.h>
 
+#define HAVE_JUMP_LABEL_BATCH
+
 #define JUMP_LABEL_NOP_SIZE 4
 
 static __always_inline bool arch_static_branch(struct static_key * const key,
diff --git a/arch/riscv/kernel/jump_label.c b/arch/riscv/kernel/jump_label.c
index e6694759dbd0..11ad789c60c6 100644
--- a/arch/riscv/kernel/jump_label.c
+++ b/arch/riscv/kernel/jump_label.c
@@ -9,13 +9,14 @@
 #include <linux/memory.h>
 #include <linux/mutex.h>
 #include <asm/bug.h>
+#include <asm/cacheflush.h>
 #include <asm/patch.h>
 
 #define RISCV_INSN_NOP 0x00000013U
 #define RISCV_INSN_JAL 0x0000006fU
 
-void arch_jump_label_transform(struct jump_entry *entry,
-			       enum jump_label_type type)
+bool arch_jump_label_transform_queue(struct jump_entry *entry,
+				     enum jump_label_type type)
 {
 	void *addr = (void *)jump_entry_code(entry);
 	u32 insn;
@@ -24,7 +25,7 @@ void arch_jump_label_transform(struct jump_entry *entry,
 		long offset = jump_entry_target(entry) - jump_entry_code(entry);
 
 		if (WARN_ON(offset & 1 || offset < -524288 || offset >= 524288))
-			return;
+			return true;
 
 		insn = RISCV_INSN_JAL |
 			(((u32)offset & GENMASK(19, 12)) << (12 - 12)) |
@@ -36,6 +37,13 @@ void arch_jump_label_transform(struct jump_entry *entry,
 	}
 
 	mutex_lock(&text_mutex);
-	patch_text_nosync(addr, &insn, sizeof(insn));
+	patch_insn_write(addr, &insn, sizeof(insn));
 	mutex_unlock(&text_mutex);
+
+	return true;
+}
+
+void arch_jump_label_transform_apply(void)
+{
+	flush_icache_all();
 }
-- 
2.43.1

[PATCH v2 2/7] riscv: jump_label: Simplify assembly syntax

[Samuel Holland]

The idiomatic way to write "jal zero" is "j".

Reviewed-by: Björn Töpel <bjorn@rivosinc.com>
Signed-off-by: Samuel Holland <samuel.holland@sifive.com>
---

(no changes since v1)

 arch/riscv/include/asm/jump_label.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/riscv/include/asm/jump_label.h b/arch/riscv/include/asm/jump_label.h
index 6290b26f4a14..1c768d02bd0c 100644
--- a/arch/riscv/include/asm/jump_label.h
+++ b/arch/riscv/include/asm/jump_label.h
@@ -46,7 +46,7 @@ static __always_inline bool arch_static_branch_jump(struct static_key * const ke
 		"	.option push				\n\t"
 		"	.option norelax				\n\t"
 		"	.option norvc				\n\t"
-		"1:	jal		zero, %l[label]		\n\t"
+		"1:	j		%l[label]		\n\t"
 		"	.option pop				\n\t"
 		"	.pushsection	__jump_table, \"aw\"	\n\t"
 		"	.align		" RISCV_LGPTR "		\n\t"
-- 
2.43.1

[PATCH v2 3/7] riscv: kprobes: Use patch_text_nosync() for insn slots

[Samuel Holland]

These instructions are not yet visible to the rest of the system,
so there is no need to do the whole stop_machine() dance.

Reviewed-by: Björn Töpel <bjorn@rivosinc.com>
Signed-off-by: Samuel Holland <samuel.holland@sifive.com>
---

Changes in v2:
 - Remove unnecessary line wrapping

 arch/riscv/kernel/probes/kprobes.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/arch/riscv/kernel/probes/kprobes.c b/arch/riscv/kernel/probes/kprobes.c
index 2f08c14a933d..fecbbcf40ac3 100644
--- a/arch/riscv/kernel/probes/kprobes.c
+++ b/arch/riscv/kernel/probes/kprobes.c
@@ -28,9 +28,8 @@ static void __kprobes arch_prepare_ss_slot(struct kprobe *p)
 
 	p->ainsn.api.restore = (unsigned long)p->addr + offset;
 
-	patch_text(p->ainsn.api.insn, &p->opcode, 1);
-	patch_text((void *)((unsigned long)(p->ainsn.api.insn) + offset),
-		   &insn, 1);
+	patch_text_nosync(p->ainsn.api.insn, &p->opcode, 1);
+	patch_text_nosync(p->ainsn.api.insn + offset, &insn, 1);
 }
 
 static void __kprobes arch_prepare_simulate(struct kprobe *p)
-- 
2.43.1

[PATCH v2 4/7] riscv: Simplify text patching loops

[Samuel Holland]

This reduces the number of variables and makes the code easier to parse.

Signed-off-by: Samuel Holland <samuel.holland@sifive.com>
---

Changes in v2:
 - Further simplify patch_insn_set()/patch_insn_write() loop conditions
 - Use min() instead of min_t() since both sides are unsigned long

 arch/riscv/kernel/patch.c | 37 +++++++++++++++++++++----------------
 1 file changed, 21 insertions(+), 16 deletions(-)

diff --git a/arch/riscv/kernel/patch.c b/arch/riscv/kernel/patch.c
index 9a1bce1adf5a..0d1700d1934c 100644
--- a/arch/riscv/kernel/patch.c
+++ b/arch/riscv/kernel/patch.c
@@ -155,22 +155,24 @@ NOKPROBE_SYMBOL(__patch_insn_write);
 
 static int patch_insn_set(void *addr, u8 c, size_t len)
 {
-	size_t patched = 0;
 	size_t size;
-	int ret = 0;
+	int ret;
 
 	/*
 	 * __patch_insn_set() can only work on 2 pages at a time so call it in a
 	 * loop with len <= 2 * PAGE_SIZE.
 	 */
-	while (patched < len && !ret) {
-		size = min_t(size_t, PAGE_SIZE * 2 - offset_in_page(addr + patched), len - patched);
-		ret = __patch_insn_set(addr + patched, c, size);
-
-		patched += size;
+	while (len) {
+		size = min(len, PAGE_SIZE * 2 - offset_in_page(addr));
+		ret = __patch_insn_set(addr, c, size);
+		if (ret)
+			return ret;
+
+		addr += size;
+		len -= size;
 	}
 
-	return ret;
+	return 0;
 }
 NOKPROBE_SYMBOL(patch_insn_set);
 
@@ -190,22 +192,25 @@ NOKPROBE_SYMBOL(patch_text_set_nosync);
 
 int patch_insn_write(void *addr, const void *insn, size_t len)
 {
-	size_t patched = 0;
 	size_t size;
-	int ret = 0;
+	int ret;
 
 	/*
 	 * Copy the instructions to the destination address, two pages at a time
 	 * because __patch_insn_write() can only handle len <= 2 * PAGE_SIZE.
 	 */
-	while (patched < len && !ret) {
-		size = min_t(size_t, PAGE_SIZE * 2 - offset_in_page(addr + patched), len - patched);
-		ret = __patch_insn_write(addr + patched, insn + patched, size);
-
-		patched += size;
+	while (len) {
+		size = min(len, PAGE_SIZE * 2 - offset_in_page(addr));
+		ret = __patch_insn_write(addr, insn, size);
+		if (ret)
+			return ret;
+
+		addr += size;
+		insn += size;
+		len -= size;
 	}
 
-	return ret;
+	return 0;
 }
 NOKPROBE_SYMBOL(patch_insn_write);
 
-- 
2.43.1

Re: [PATCH v2 4/7] riscv: Simplify text patching loops

[Björn Töpel]

Samuel Holland <samuel.holland@sifive.com> writes:

> This reduces the number of variables and makes the code easier to parse.
>
> Signed-off-by: Samuel Holland <samuel.holland@sifive.com>
> ---
>
> Changes in v2:
>  - Further simplify patch_insn_set()/patch_insn_write() loop conditions
>  - Use min() instead of min_t() since both sides are unsigned long

Thanks for addressing the changes! Nice cleanup!

Reviewed-by: Björn Töpel <bjorn@rivosinc.com>

Re: [PATCH v2 4/7] riscv: Simplify text patching loops

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 1468 bytes --]

On Wed, Mar 27, 2024 at 09:04:43AM -0700, Samuel Holland wrote:
> This reduces the number of variables and makes the code easier to parse.
> 
> Signed-off-by: Samuel Holland <samuel.holland@sifive.com>
> ---
> 
> Changes in v2:
>  - Further simplify patch_insn_set()/patch_insn_write() loop conditions
>  - Use min() instead of min_t() since both sides are unsigned long
> 
>  arch/riscv/kernel/patch.c | 37 +++++++++++++++++++++----------------
>  1 file changed, 21 insertions(+), 16 deletions(-)
> 
> diff --git a/arch/riscv/kernel/patch.c b/arch/riscv/kernel/patch.c
> index 9a1bce1adf5a..0d1700d1934c 100644
> --- a/arch/riscv/kernel/patch.c
> +++ b/arch/riscv/kernel/patch.c
> @@ -155,22 +155,24 @@ NOKPROBE_SYMBOL(__patch_insn_write);
>  
>  static int patch_insn_set(void *addr, u8 c, size_t len)
>  {
> -	size_t patched = 0;
>  	size_t size;
> -	int ret = 0;
>  
>  	/*
>  	 * __patch_insn_set() can only work on 2 pages at a time so call it in a
>  	 * loop with len <= 2 * PAGE_SIZE.
>  	 */
> -	while (patched < len && !ret) {
> -		size = min_t(size_t, PAGE_SIZE * 2 - offset_in_page(addr + patched), len - patched);
> -		ret = __patch_insn_set(addr + patched, c, size);
> -
> -		patched += size;
>  	}
>  
> -	return ret;
>  }

Weren't these actively wrong before, if a non-ultimate
__patch_insn_set() failed we'd just ignore the error?

Reviewed-by: Conor Dooley <conor.dooley@microchip.com>

Thanks,
Conor.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v2 4/7] riscv: Simplify text patching loops

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 1573 bytes --]

On Wed, Apr 24, 2024 at 11:44:17AM +0100, Conor Dooley wrote:
> On Wed, Mar 27, 2024 at 09:04:43AM -0700, Samuel Holland wrote:
> > This reduces the number of variables and makes the code easier to parse.
> > 
> > Signed-off-by: Samuel Holland <samuel.holland@sifive.com>
> > ---
> > 
> > Changes in v2:
> >  - Further simplify patch_insn_set()/patch_insn_write() loop conditions
> >  - Use min() instead of min_t() since both sides are unsigned long
> > 
> >  arch/riscv/kernel/patch.c | 37 +++++++++++++++++++++----------------
> >  1 file changed, 21 insertions(+), 16 deletions(-)
> > 
> > diff --git a/arch/riscv/kernel/patch.c b/arch/riscv/kernel/patch.c
> > index 9a1bce1adf5a..0d1700d1934c 100644
> > --- a/arch/riscv/kernel/patch.c
> > +++ b/arch/riscv/kernel/patch.c
> > @@ -155,22 +155,24 @@ NOKPROBE_SYMBOL(__patch_insn_write);
> >  
> >  static int patch_insn_set(void *addr, u8 c, size_t len)
> >  {
> > -	size_t patched = 0;
> >  	size_t size;
> > -	int ret = 0;
> >  
> >  	/*
> >  	 * __patch_insn_set() can only work on 2 pages at a time so call it in a
> >  	 * loop with len <= 2 * PAGE_SIZE.
> >  	 */
> > -	while (patched < len && !ret) {
> > -		size = min_t(size_t, PAGE_SIZE * 2 - offset_in_page(addr + patched), len - patched);
> > -		ret = __patch_insn_set(addr + patched, c, size);
> > -
> > -		patched += size;
> >  	}
> >  
> > -	return ret;
> >  }
> 
> Weren't these actively wrong before, if a non-ultimate
> __patch_insn_set() failed we'd just ignore the error?

nvm, failure to read on my part..

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[PATCH v2 5/7] riscv: Pass patch_text() the length in bytes

[Samuel Holland]

patch_text_nosync() already handles an arbitrary length of code, so this
removes a superfluous loop and reduces the number of icache flushes.

Reviewed-by: Björn Töpel <bjorn@rivosinc.com>
Signed-off-by: Samuel Holland <samuel.holland@sifive.com>
---

Changes in v2:
 - Remove unnecessary line wrapping

 arch/riscv/include/asm/patch.h     |  2 +-
 arch/riscv/kernel/patch.c          | 14 +++++---------
 arch/riscv/kernel/probes/kprobes.c | 18 ++++++++++--------
 arch/riscv/net/bpf_jit_comp64.c    |  7 ++++---
 4 files changed, 20 insertions(+), 21 deletions(-)

diff --git a/arch/riscv/include/asm/patch.h b/arch/riscv/include/asm/patch.h
index 9f5d6e14c405..7228e266b9a1 100644
--- a/arch/riscv/include/asm/patch.h
+++ b/arch/riscv/include/asm/patch.h
@@ -9,7 +9,7 @@
 int patch_insn_write(void *addr, const void *insn, size_t len);
 int patch_text_nosync(void *addr, const void *insns, size_t len);
 int patch_text_set_nosync(void *addr, u8 c, size_t len);
-int patch_text(void *addr, u32 *insns, int ninsns);
+int patch_text(void *addr, u32 *insns, size_t len);
 
 extern int riscv_patch_in_stop_machine;
 
diff --git a/arch/riscv/kernel/patch.c b/arch/riscv/kernel/patch.c
index 0d1700d1934c..243e1573410b 100644
--- a/arch/riscv/kernel/patch.c
+++ b/arch/riscv/kernel/patch.c
@@ -19,7 +19,7 @@
 struct patch_insn {
 	void *addr;
 	u32 *insns;
-	int ninsns;
+	size_t len;
 	atomic_t cpu_count;
 };
 
@@ -231,14 +231,10 @@ NOKPROBE_SYMBOL(patch_text_nosync);
 static int patch_text_cb(void *data)
 {
 	struct patch_insn *patch = data;
-	unsigned long len;
-	int i, ret = 0;
+	int ret = 0;
 
 	if (atomic_inc_return(&patch->cpu_count) == num_online_cpus()) {
-		for (i = 0; ret == 0 && i < patch->ninsns; i++) {
-			len = GET_INSN_LENGTH(patch->insns[i]);
-			ret = patch_insn_write(patch->addr + i * len, &patch->insns[i], len);
-		}
+		ret = patch_insn_write(patch->addr, patch->insns, patch->len);
 		/*
 		 * Make sure the patching store is effective *before* we
 		 * increment the counter which releases all waiting CPUs
@@ -258,13 +254,13 @@ static int patch_text_cb(void *data)
 }
 NOKPROBE_SYMBOL(patch_text_cb);
 
-int patch_text(void *addr, u32 *insns, int ninsns)
+int patch_text(void *addr, u32 *insns, size_t len)
 {
 	int ret;
 	struct patch_insn patch = {
 		.addr = addr,
 		.insns = insns,
-		.ninsns = ninsns,
+		.len = len,
 		.cpu_count = ATOMIC_INIT(0),
 	};
 
diff --git a/arch/riscv/kernel/probes/kprobes.c b/arch/riscv/kernel/probes/kprobes.c
index fecbbcf40ac3..f3cbc3ec9e6a 100644
--- a/arch/riscv/kernel/probes/kprobes.c
+++ b/arch/riscv/kernel/probes/kprobes.c
@@ -23,13 +23,13 @@ post_kprobe_handler(struct kprobe *, struct kprobe_ctlblk *, struct pt_regs *);
 
 static void __kprobes arch_prepare_ss_slot(struct kprobe *p)
 {
+	size_t len = GET_INSN_LENGTH(p->opcode);
 	u32 insn = __BUG_INSN_32;
-	unsigned long offset = GET_INSN_LENGTH(p->opcode);
 
-	p->ainsn.api.restore = (unsigned long)p->addr + offset;
+	p->ainsn.api.restore = (unsigned long)p->addr + len;
 
-	patch_text_nosync(p->ainsn.api.insn, &p->opcode, 1);
-	patch_text_nosync(p->ainsn.api.insn + offset, &insn, 1);
+	patch_text_nosync(p->ainsn.api.insn, &p->opcode, len);
+	patch_text_nosync(p->ainsn.api.insn + len, &insn, GET_INSN_LENGTH(insn));
 }
 
 static void __kprobes arch_prepare_simulate(struct kprobe *p)
@@ -116,16 +116,18 @@ void *alloc_insn_page(void)
 /* install breakpoint in text */
 void __kprobes arch_arm_kprobe(struct kprobe *p)
 {
-	u32 insn = (p->opcode & __INSN_LENGTH_MASK) == __INSN_LENGTH_32 ?
-		   __BUG_INSN_32 : __BUG_INSN_16;
+	size_t len = GET_INSN_LENGTH(p->opcode);
+	u32 insn = len == 4 ? __BUG_INSN_32 : __BUG_INSN_16;
 
-	patch_text(p->addr, &insn, 1);
+	patch_text(p->addr, &insn, len);
 }
 
 /* remove breakpoint from text */
 void __kprobes arch_disarm_kprobe(struct kprobe *p)
 {
-	patch_text(p->addr, &p->opcode, 1);
+	size_t len = GET_INSN_LENGTH(p->opcode);
+
+	patch_text(p->addr, &p->opcode, len);
 }
 
 void __kprobes arch_remove_kprobe(struct kprobe *p)
diff --git a/arch/riscv/net/bpf_jit_comp64.c b/arch/riscv/net/bpf_jit_comp64.c
index aac190085472..6ed4035a8136 100644
--- a/arch/riscv/net/bpf_jit_comp64.c
+++ b/arch/riscv/net/bpf_jit_comp64.c
@@ -15,6 +15,7 @@
 #include "bpf_jit.h"
 
 #define RV_FENTRY_NINSNS 2
+#define RV_FENTRY_NBYTES (RV_FENTRY_NINSNS * 4)
 
 #define RV_REG_TCC RV_REG_A6
 #define RV_REG_TCC_SAVED RV_REG_S6 /* Store A6 in S6 if program do calls */
@@ -663,7 +664,7 @@ int bpf_arch_text_poke(void *ip, enum bpf_text_poke_type poke_type,
 	if (ret)
 		return ret;
 
-	if (memcmp(ip, old_insns, RV_FENTRY_NINSNS * 4))
+	if (memcmp(ip, old_insns, RV_FENTRY_NBYTES))
 		return -EFAULT;
 
 	ret = gen_jump_or_nops(new_addr, ip, new_insns, is_call);
@@ -672,8 +673,8 @@ int bpf_arch_text_poke(void *ip, enum bpf_text_poke_type poke_type,
 
 	cpus_read_lock();
 	mutex_lock(&text_mutex);
-	if (memcmp(ip, new_insns, RV_FENTRY_NINSNS * 4))
-		ret = patch_text(ip, new_insns, RV_FENTRY_NINSNS);
+	if (memcmp(ip, new_insns, RV_FENTRY_NBYTES))
+		ret = patch_text(ip, new_insns, RV_FENTRY_NBYTES);
 	mutex_unlock(&text_mutex);
 	cpus_read_unlock();
 
-- 
2.43.1

Re: [PATCH v2 5/7] riscv: Pass patch_text() the length in bytes

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 385 bytes --]

On Wed, Mar 27, 2024 at 09:04:44AM -0700, Samuel Holland wrote:
> patch_text_nosync() already handles an arbitrary length of code, so this
> removes a superfluous loop and reduces the number of icache flushes.
> 
> Reviewed-by: Björn Töpel <bjorn@rivosinc.com>
> Signed-off-by: Samuel Holland <samuel.holland@sifive.com>

Reviewed-by: Conor Dooley <conor.dooley@microchip.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[PATCH v2 6/7] riscv: Use offset_in_page() in text patching functions

[Samuel Holland]

This is a bit easier to parse than the equivalent bit manipulation.

Reviewed-by: Björn Töpel <bjorn@rivosinc.com>
Signed-off-by: Samuel Holland <samuel.holland@sifive.com>
---

(no changes since v1)

 arch/riscv/kernel/patch.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/riscv/kernel/patch.c b/arch/riscv/kernel/patch.c
index 243e1573410b..cfcb9926e722 100644
--- a/arch/riscv/kernel/patch.c
+++ b/arch/riscv/kernel/patch.c
@@ -54,7 +54,7 @@ static __always_inline void *patch_map(void *addr, const unsigned int fixmap)
 	BUG_ON(!page);
 
 	return (void *)set_fixmap_offset(fixmap, page_to_phys(page) +
-					 (uintaddr & ~PAGE_MASK));
+					 offset_in_page(addr));
 }
 
 static void patch_unmap(int fixmap)
@@ -65,8 +65,8 @@ NOKPROBE_SYMBOL(patch_unmap);
 
 static int __patch_insn_set(void *addr, u8 c, size_t len)
 {
+	bool across_pages = (offset_in_page(addr) + len) > PAGE_SIZE;
 	void *waddr = addr;
-	bool across_pages = (((uintptr_t)addr & ~PAGE_MASK) + len) > PAGE_SIZE;
 
 	/*
 	 * Only two pages can be mapped at a time for writing.
@@ -98,8 +98,8 @@ NOKPROBE_SYMBOL(__patch_insn_set);
 
 static int __patch_insn_write(void *addr, const void *insn, size_t len)
 {
+	bool across_pages = (offset_in_page(addr) + len) > PAGE_SIZE;
 	void *waddr = addr;
-	bool across_pages = (((uintptr_t) addr & ~PAGE_MASK) + len) > PAGE_SIZE;
 	int ret;
 
 	/*
-- 
2.43.1

Re: [PATCH v2 6/7] riscv: Use offset_in_page() in text patching functions

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 272 bytes --]

On Wed, Mar 27, 2024 at 09:04:45AM -0700, Samuel Holland wrote:
> This is a bit easier to parse than the equivalent bit manipulation.

And it is used elsewhere in patch.c too, so makes things more consistent
to boot.
Reviewed-by: Conor Dooley <conor.dooley@microchip.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[PATCH v2 7/7] riscv: Remove extra variable in patch_text_nosync()

[Samuel Holland]

This cast is superfluous, and is incorrect anyway if compressed
instructions may be present.

Reviewed-by: Björn Töpel <bjorn@rivosinc.com>
Signed-off-by: Samuel Holland <samuel.holland@sifive.com>
---

(no changes since v1)

 arch/riscv/kernel/patch.c | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/arch/riscv/kernel/patch.c b/arch/riscv/kernel/patch.c
index cfcb9926e722..9b8d633f6536 100644
--- a/arch/riscv/kernel/patch.c
+++ b/arch/riscv/kernel/patch.c
@@ -178,13 +178,11 @@ NOKPROBE_SYMBOL(patch_insn_set);
 
 int patch_text_set_nosync(void *addr, u8 c, size_t len)
 {
-	u32 *tp = addr;
 	int ret;
 
-	ret = patch_insn_set(tp, c, len);
-
+	ret = patch_insn_set(addr, c, len);
 	if (!ret)
-		flush_icache_range((uintptr_t)tp, (uintptr_t)tp + len);
+		flush_icache_range((uintptr_t)addr, (uintptr_t)addr + len);
 
 	return ret;
 }
@@ -216,13 +214,11 @@ NOKPROBE_SYMBOL(patch_insn_write);
 
 int patch_text_nosync(void *addr, const void *insns, size_t len)
 {
-	u32 *tp = addr;
 	int ret;
 
-	ret = patch_insn_write(tp, insns, len);
-
+	ret = patch_insn_write(addr, insns, len);
 	if (!ret)
-		flush_icache_range((uintptr_t) tp, (uintptr_t) tp + len);
+		flush_icache_range((uintptr_t)addr, (uintptr_t)addr + len);
 
 	return ret;
 }
-- 
2.43.1

Re: [PATCH v2 7/7] riscv: Remove extra variable in patch_text_nosync()

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 336 bytes --]

On Wed, Mar 27, 2024 at 09:04:46AM -0700, Samuel Holland wrote:
> This cast is superfluous, and is incorrect anyway if compressed
> instructions may be present.
> 
> Reviewed-by: Björn Töpel <bjorn@rivosinc.com>
> Signed-off-by: Samuel Holland <samuel.holland@sifive.com>

Reviewed-by: Conor Dooley <conor.dooley@microchip.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v2 0/7] riscv: Various text patching improvements

[Palmer Dabbelt]

On Wed, 27 Mar 2024 09:04:39 PDT (-0700), samuel.holland@sifive.com wrote:
> Here are a few changes to minimize calls to stop_machine() and
> flush_icache_*() in the various text patching functions, as well as
> to simplify the code.
>
> This series is based on "[PATCH v3 0/2] riscv: fix patching with IPI"[1].
>
> [1]: https://lore.kernel.org/linux-riscv/20240229121056.203419-1-alexghiti@rivosinc.com/
>
> Changes in v2:
>  - Remove unnecessary line wrapping
>  - Further simplify patch_insn_set()/patch_insn_write() loop conditions
>  - Use min() instead of min_t() since both sides are unsigned long
>
> Samuel Holland (7):
>   riscv: jump_label: Batch icache maintenance
>   riscv: jump_label: Simplify assembly syntax
>   riscv: kprobes: Use patch_text_nosync() for insn slots
>   riscv: Simplify text patching loops
>   riscv: Pass patch_text() the length in bytes
>   riscv: Use offset_in_page() in text patching functions
>   riscv: Remove extra variable in patch_text_nosync()
>
>  arch/riscv/include/asm/jump_label.h |  4 +-
>  arch/riscv/include/asm/patch.h      |  2 +-
>  arch/riscv/kernel/jump_label.c      | 16 +++++--
>  arch/riscv/kernel/patch.c           | 69 ++++++++++++++---------------
>  arch/riscv/kernel/probes/kprobes.c  | 19 ++++----
>  arch/riscv/net/bpf_jit_comp64.c     |  7 +--
>  6 files changed, 63 insertions(+), 54 deletions(-)

I don't have any issues with this, but given that we've run into some 
possible text patching bug with this ftrace thing I'm just going to hold 
off until 6.11 for these.  Maybe that's a bit too conservative, but with 
the bug only manifesting on HW it might be tough to sort out.

Re: [PATCH v2 0/7] riscv: Various text patching improvements

[patchwork-bot+linux-riscv]

Hello:

This series was applied to riscv/linux.git (for-next)
by Palmer Dabbelt <palmer@rivosinc.com>:

On Wed, 27 Mar 2024 09:04:39 -0700 you wrote:
> Here are a few changes to minimize calls to stop_machine() and
> flush_icache_*() in the various text patching functions, as well as
> to simplify the code.
> 
> This series is based on "[PATCH v3 0/2] riscv: fix patching with IPI"[1].
> 
> [1]: https://lore.kernel.org/linux-riscv/20240229121056.203419-1-alexghiti@rivosinc.com/
> 
> [...]

Here is the summary with links:
  - [v2,1/7] riscv: jump_label: Batch icache maintenance
    https://git.kernel.org/riscv/c/652b56b18439
  - [v2,2/7] riscv: jump_label: Simplify assembly syntax
    https://git.kernel.org/riscv/c/2aa30d19cfbb
  - [v2,3/7] riscv: kprobes: Use patch_text_nosync() for insn slots
    https://git.kernel.org/riscv/c/b1756750a397
  - [v2,4/7] riscv: Simplify text patching loops
    https://git.kernel.org/riscv/c/5080ca0fe9b5
  - [v2,5/7] riscv: Pass patch_text() the length in bytes
    https://git.kernel.org/riscv/c/51781ce8f448
  - [v2,6/7] riscv: Use offset_in_page() in text patching functions
    https://git.kernel.org/riscv/c/eaee54875630
  - [v2,7/7] riscv: Remove extra variable in patch_text_nosync()
    https://git.kernel.org/riscv/c/47742484ee16

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html