2024-03-27 16:11:28

by Samuel Holland

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

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

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

2024-03-27 16:11:41

by Samuel Holland

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

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 <[email protected]>
Signed-off-by: Samuel Holland <[email protected]>
---

(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

2024-03-27 16:11:57

by Samuel Holland

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

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

Reviewed-by: Björn Töpel <[email protected]>
Signed-off-by: Samuel Holland <[email protected]>
---

(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

2024-03-27 16:12:06

by Samuel Holland

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

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 <[email protected]>
Signed-off-by: Samuel Holland <[email protected]>
---

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

2024-03-27 16:12:13

by Samuel Holland

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

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

Signed-off-by: Samuel Holland <[email protected]>
---

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

2024-03-27 16:12:36

by Samuel Holland

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

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 <[email protected]>
Signed-off-by: Samuel Holland <[email protected]>
---

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

2024-03-27 16:12:57

by Samuel Holland

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

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

Reviewed-by: Björn Töpel <[email protected]>
Signed-off-by: Samuel Holland <[email protected]>
---

(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

2024-03-27 16:35:26

by Samuel Holland

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

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

Reviewed-by: Björn Töpel <[email protected]>
Signed-off-by: Samuel Holland <[email protected]>
---

(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

2024-04-02 11:14:10

by Björn Töpel

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

Samuel Holland <[email protected]> writes:

> This reduces the number of variables and makes the code easier to parse.
>
> Signed-off-by: Samuel Holland <[email protected]>
> ---
>
> 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 <[email protected]>

2024-04-24 10:44:27

by Conor Dooley

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

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

Thanks,
Conor.

Attachments:

2024-04-24 10:47:55

by Conor Dooley

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

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

Attachments:

2024-04-24 10:49:56

by Conor Dooley

Subject: Re: [PATCH v2 5/7] riscv: Pass patch_text() the length in 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 <[email protected]>
> Signed-off-by: Samuel Holland <[email protected]>

Reviewed-by: Conor Dooley <[email protected]>

Attachments:

2024-04-24 10:51:31

by Conor Dooley

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

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

Attachments:

2024-04-24 10:54:48

by Conor Dooley

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

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 <[email protected]>
> Signed-off-by: Samuel Holland <[email protected]>

Reviewed-by: Conor Dooley <[email protected]>

Attachments:

2024-05-22 16:36:02

by Palmer Dabbelt

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

On Wed, 27 Mar 2024 09:04:39 PDT (-0700), [email protected] 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/[email protected]/
>
> 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.