* [PATCH 1/3] riscv: Remove the 'riscv_' prefix of function name
2020-04-21 7:29 [PATCH 0/3] Refactor patch text interfaces and mechanism Zong Li
@ 2020-04-21 7:29 ` Zong Li
2020-04-21 7:30 ` [PATCH 2/3] riscv: Use NOKPROBE_SYMBOL() instead of __krpobes annotation Zong Li
` (2 subsequent siblings)
3 siblings, 0 replies; 5+ messages in thread
From: Zong Li @ 2020-04-21 7:29 UTC (permalink / raw)
To: palmer, paul.walmsley, linux-riscv, linux-kernel
Cc: Zong Li, Masami Hiramatsu, Palmer Dabbelt
Refactor the function name by removing the 'riscv_' prefix, it would be
better unless it could mix up with arch-independent functions.
Signed-off-by: Zong Li <zong.li@sifive.com>
Reviewed-by: Masami Hiramatsu <mhiramat@kernel.org>
Reviewed-by: Palmer Dabbelt <palmerdabbelt@google.com>
---
arch/riscv/include/asm/patch.h | 4 ++--
arch/riscv/kernel/ftrace.c | 2 +-
arch/riscv/kernel/patch.c | 22 +++++++++++-----------
3 files changed, 14 insertions(+), 14 deletions(-)
diff --git a/arch/riscv/include/asm/patch.h b/arch/riscv/include/asm/patch.h
index b5918a6e0615..9a7d7346001e 100644
--- a/arch/riscv/include/asm/patch.h
+++ b/arch/riscv/include/asm/patch.h
@@ -6,7 +6,7 @@
#ifndef _ASM_RISCV_PATCH_H
#define _ASM_RISCV_PATCH_H
-int riscv_patch_text_nosync(void *addr, const void *insns, size_t len);
-int riscv_patch_text(void *addr, u32 insn);
+int patch_text_nosync(void *addr, const void *insns, size_t len);
+int patch_text(void *addr, u32 insn);
#endif /* _ASM_RISCV_PATCH_H */
diff --git a/arch/riscv/kernel/ftrace.c b/arch/riscv/kernel/ftrace.c
index ce69b34ff55d..fb1e2b8fe254 100644
--- a/arch/riscv/kernel/ftrace.c
+++ b/arch/riscv/kernel/ftrace.c
@@ -51,7 +51,7 @@ static int __ftrace_modify_call(unsigned long hook_pos, unsigned long target,
make_call(hook_pos, target, call);
/* Replace the auipc-jalr pair at once. Return -EPERM on write error. */
- if (riscv_patch_text_nosync
+ if (patch_text_nosync
((void *)hook_pos, enable ? call : nops, MCOUNT_INSN_SIZE))
return -EPERM;
diff --git a/arch/riscv/kernel/patch.c b/arch/riscv/kernel/patch.c
index 8a4fc65ee022..de28f23f65cb 100644
--- a/arch/riscv/kernel/patch.c
+++ b/arch/riscv/kernel/patch.c
@@ -11,7 +11,7 @@
#include <asm/cacheflush.h>
#include <asm/fixmap.h>
-struct riscv_insn_patch {
+struct patch_insn {
void *addr;
u32 insn;
atomic_t cpu_count;
@@ -43,7 +43,7 @@ static void __kprobes patch_unmap(int fixmap)
clear_fixmap(fixmap);
}
-static int __kprobes riscv_insn_write(void *addr, const void *insn, size_t len)
+static int __kprobes patch_insn_write(void *addr, const void *insn, size_t len)
{
void *waddr = addr;
bool across_pages = (((uintptr_t) addr & ~PAGE_MASK) + len) > PAGE_SIZE;
@@ -69,18 +69,18 @@ static int __kprobes riscv_insn_write(void *addr, const void *insn, size_t len)
return ret;
}
#else
-static int __kprobes riscv_insn_write(void *addr, const void *insn, size_t len)
+static int __kprobes patch_insn_write(void *addr, const void *insn, size_t len)
{
return probe_kernel_write(addr, insn, len);
}
#endif /* CONFIG_MMU */
-int __kprobes riscv_patch_text_nosync(void *addr, const void *insns, size_t len)
+int __kprobes patch_text_nosync(void *addr, const void *insns, size_t len)
{
u32 *tp = addr;
int ret;
- ret = riscv_insn_write(tp, insns, len);
+ ret = patch_insn_write(tp, insns, len);
if (!ret)
flush_icache_range((uintptr_t) tp, (uintptr_t) tp + len);
@@ -88,14 +88,14 @@ int __kprobes riscv_patch_text_nosync(void *addr, const void *insns, size_t len)
return ret;
}
-static int __kprobes riscv_patch_text_cb(void *data)
+static int __kprobes patch_text_cb(void *data)
{
- struct riscv_insn_patch *patch = data;
+ struct patch_insn *patch = data;
int ret = 0;
if (atomic_inc_return(&patch->cpu_count) == 1) {
ret =
- riscv_patch_text_nosync(patch->addr, &patch->insn,
+ patch_text_nosync(patch->addr, &patch->insn,
GET_INSN_LENGTH(patch->insn));
atomic_inc(&patch->cpu_count);
} else {
@@ -107,14 +107,14 @@ static int __kprobes riscv_patch_text_cb(void *data)
return ret;
}
-int __kprobes riscv_patch_text(void *addr, u32 insn)
+int __kprobes patch_text(void *addr, u32 insn)
{
- struct riscv_insn_patch patch = {
+ struct patch_insn patch = {
.addr = addr,
.insn = insn,
.cpu_count = ATOMIC_INIT(0),
};
- return stop_machine_cpuslocked(riscv_patch_text_cb,
+ return stop_machine_cpuslocked(patch_text_cb,
&patch, cpu_online_mask);
}
--
2.26.1
^ permalink raw reply related [flat|nested] 5+ messages in thread* [PATCH 2/3] riscv: Use NOKPROBE_SYMBOL() instead of __krpobes annotation
2020-04-21 7:29 [PATCH 0/3] Refactor patch text interfaces and mechanism Zong Li
2020-04-21 7:29 ` [PATCH 1/3] riscv: Remove the 'riscv_' prefix of function name Zong Li
@ 2020-04-21 7:30 ` Zong Li
2020-04-21 7:30 ` [PATCH 3/3] riscv: Use text_mutex instead of patch_lock Zong Li
2020-04-27 16:34 ` [PATCH 0/3] Refactor patch text interfaces and mechanism Palmer Dabbelt
3 siblings, 0 replies; 5+ messages in thread
From: Zong Li @ 2020-04-21 7:30 UTC (permalink / raw)
To: palmer, paul.walmsley, linux-riscv, linux-kernel
Cc: Zong Li, Masami Hiramatsu, Palmer Dabbelt
The __kprobes annotation is old style, so change it to NOKPROBE_SYMBOL().
Signed-off-by: Zong Li <zong.li@sifive.com>
Reviewed-by: Masami Hiramatsu <mhiramat@kernel.org>
Reviewed-by: Palmer Dabbelt <palmerdabbelt@google.com>
---
arch/riscv/kernel/patch.c | 21 ++++++++++++++-------
1 file changed, 14 insertions(+), 7 deletions(-)
diff --git a/arch/riscv/kernel/patch.c b/arch/riscv/kernel/patch.c
index de28f23f65cb..8acb9ae2da08 100644
--- a/arch/riscv/kernel/patch.c
+++ b/arch/riscv/kernel/patch.c
@@ -20,7 +20,7 @@ struct patch_insn {
#ifdef CONFIG_MMU
static DEFINE_RAW_SPINLOCK(patch_lock);
-static void __kprobes *patch_map(void *addr, int fixmap)
+static void *patch_map(void *addr, int fixmap)
{
uintptr_t uintaddr = (uintptr_t) addr;
struct page *page;
@@ -37,13 +37,15 @@ static void __kprobes *patch_map(void *addr, int fixmap)
return (void *)set_fixmap_offset(fixmap, page_to_phys(page) +
(uintaddr & ~PAGE_MASK));
}
+NOKPROBE_SYMBOL(patch_map);
-static void __kprobes patch_unmap(int fixmap)
+static void patch_unmap(int fixmap)
{
clear_fixmap(fixmap);
}
+NOKPROBE_SYMBOL(patch_unmap);
-static int __kprobes patch_insn_write(void *addr, const void *insn, size_t len)
+static int patch_insn_write(void *addr, const void *insn, size_t len)
{
void *waddr = addr;
bool across_pages = (((uintptr_t) addr & ~PAGE_MASK) + len) > PAGE_SIZE;
@@ -68,14 +70,16 @@ static int __kprobes patch_insn_write(void *addr, const void *insn, size_t len)
return ret;
}
+NOKPROBE_SYMBOL(patch_insn_write);
#else
-static int __kprobes patch_insn_write(void *addr, const void *insn, size_t len)
+static int patch_insn_write(void *addr, const void *insn, size_t len)
{
return probe_kernel_write(addr, insn, len);
}
+NOKPROBE_SYMBOL(patch_insn_write);
#endif /* CONFIG_MMU */
-int __kprobes patch_text_nosync(void *addr, const void *insns, size_t len)
+int patch_text_nosync(void *addr, const void *insns, size_t len)
{
u32 *tp = addr;
int ret;
@@ -87,8 +91,9 @@ int __kprobes patch_text_nosync(void *addr, const void *insns, size_t len)
return ret;
}
+NOKPROBE_SYMBOL(patch_text_nosync);
-static int __kprobes patch_text_cb(void *data)
+static int patch_text_cb(void *data)
{
struct patch_insn *patch = data;
int ret = 0;
@@ -106,8 +111,9 @@ static int __kprobes patch_text_cb(void *data)
return ret;
}
+NOKPROBE_SYMBOL(patch_text_cb);
-int __kprobes patch_text(void *addr, u32 insn)
+int patch_text(void *addr, u32 insn)
{
struct patch_insn patch = {
.addr = addr,
@@ -118,3 +124,4 @@ int __kprobes patch_text(void *addr, u32 insn)
return stop_machine_cpuslocked(patch_text_cb,
&patch, cpu_online_mask);
}
+NOKPROBE_SYMBOL(patch_text);
--
2.26.1
^ permalink raw reply related [flat|nested] 5+ messages in thread* [PATCH 3/3] riscv: Use text_mutex instead of patch_lock
2020-04-21 7:29 [PATCH 0/3] Refactor patch text interfaces and mechanism Zong Li
2020-04-21 7:29 ` [PATCH 1/3] riscv: Remove the 'riscv_' prefix of function name Zong Li
2020-04-21 7:30 ` [PATCH 2/3] riscv: Use NOKPROBE_SYMBOL() instead of __krpobes annotation Zong Li
@ 2020-04-21 7:30 ` Zong Li
2020-04-27 16:34 ` [PATCH 0/3] Refactor patch text interfaces and mechanism Palmer Dabbelt
3 siblings, 0 replies; 5+ messages in thread
From: Zong Li @ 2020-04-21 7:30 UTC (permalink / raw)
To: palmer, paul.walmsley, linux-riscv, linux-kernel
Cc: Zong Li, Masami Hiramatsu, Palmer Dabbelt
We don't need the additional lock protection when patching the text.
There are two patching interfaces here:
- patch_text: patch code and always synchronize with stop_machine()
- patch_text_nosync: patch code without synchronization, it's caller's
responsibility to synchronize all CPUs if needed.
For the first one, stop_machine() is protected by its own mutex, and
also the irq is already disabled here.
For the second one, in risc-v real case now, it would be used to ftrace
patching the mcount function, since it already running under
kstop_machine(), no other thread will run, so we could use text_mutex
on ftrace side.
Signed-off-by: Zong Li <zong.li@sifive.com>
Reviewed-by: Masami Hiramatsu <mhiramat@kernel.org>
Reviewed-by: Palmer Dabbelt <palmerdabbelt@google.com>
---
arch/riscv/kernel/ftrace.c | 13 +++++++++++++
arch/riscv/kernel/patch.c | 13 +++++++------
2 files changed, 20 insertions(+), 6 deletions(-)
diff --git a/arch/riscv/kernel/ftrace.c b/arch/riscv/kernel/ftrace.c
index fb1e2b8fe254..08396614d6f4 100644
--- a/arch/riscv/kernel/ftrace.c
+++ b/arch/riscv/kernel/ftrace.c
@@ -7,10 +7,23 @@
#include <linux/ftrace.h>
#include <linux/uaccess.h>
+#include <linux/memory.h>
#include <asm/cacheflush.h>
#include <asm/patch.h>
#ifdef CONFIG_DYNAMIC_FTRACE
+int ftrace_arch_code_modify_prepare(void) __acquires(&text_mutex)
+{
+ mutex_lock(&text_mutex);
+ return 0;
+}
+
+int ftrace_arch_code_modify_post_process(void) __releases(&text_mutex)
+{
+ mutex_unlock(&text_mutex);
+ return 0;
+}
+
static int ftrace_check_current_call(unsigned long hook_pos,
unsigned int *expected)
{
diff --git a/arch/riscv/kernel/patch.c b/arch/riscv/kernel/patch.c
index 8acb9ae2da08..5805791cd5b5 100644
--- a/arch/riscv/kernel/patch.c
+++ b/arch/riscv/kernel/patch.c
@@ -5,6 +5,7 @@
#include <linux/spinlock.h>
#include <linux/mm.h>
+#include <linux/memory.h>
#include <linux/uaccess.h>
#include <linux/stop_machine.h>
#include <asm/kprobes.h>
@@ -18,8 +19,6 @@ struct patch_insn {
};
#ifdef CONFIG_MMU
-static DEFINE_RAW_SPINLOCK(patch_lock);
-
static void *patch_map(void *addr, int fixmap)
{
uintptr_t uintaddr = (uintptr_t) addr;
@@ -49,10 +48,14 @@ static int patch_insn_write(void *addr, const void *insn, size_t len)
{
void *waddr = addr;
bool across_pages = (((uintptr_t) addr & ~PAGE_MASK) + len) > PAGE_SIZE;
- unsigned long flags = 0;
int ret;
- raw_spin_lock_irqsave(&patch_lock, flags);
+ /*
+ * Before reaching here, it was expected to lock the text_mutex
+ * already, so we don't need to give another lock here and could
+ * ensure that it was safe between each cores.
+ */
+ lockdep_assert_held(&text_mutex);
if (across_pages)
patch_map(addr + len, FIX_TEXT_POKE1);
@@ -66,8 +69,6 @@ static int patch_insn_write(void *addr, const void *insn, size_t len)
if (across_pages)
patch_unmap(FIX_TEXT_POKE1);
- raw_spin_unlock_irqrestore(&patch_lock, flags);
-
return ret;
}
NOKPROBE_SYMBOL(patch_insn_write);
--
2.26.1
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [PATCH 0/3] Refactor patch text interfaces and mechanism
2020-04-21 7:29 [PATCH 0/3] Refactor patch text interfaces and mechanism Zong Li
` (2 preceding siblings ...)
2020-04-21 7:30 ` [PATCH 3/3] riscv: Use text_mutex instead of patch_lock Zong Li
@ 2020-04-27 16:34 ` Palmer Dabbelt
3 siblings, 0 replies; 5+ messages in thread
From: Palmer Dabbelt @ 2020-04-27 16:34 UTC (permalink / raw)
To: zong.li; +Cc: Paul Walmsley, linux-riscv, linux-kernel, zong.li
On Tue, 21 Apr 2020 00:29:58 PDT (-0700), zong.li@sifive.com wrote:
> This patch set contains the difference from the newest strict memory
> permission. These changes are suggested by Masami Hiramatsu, including
> deprecating old style of kprobe annotation, lock protection and so on.
>
> Zong Li (3):
> riscv: Remove the 'riscv_' prefix of function name
> riscv: Use NOKPROBE_SYMBOL() instead of __krpobes annotation
> riscv: Use text_mutex instead of patch_lock
>
> arch/riscv/include/asm/patch.h | 4 +--
> arch/riscv/kernel/ftrace.c | 15 ++++++++++-
> arch/riscv/kernel/patch.c | 46 ++++++++++++++++++++--------------
> 3 files changed, 43 insertions(+), 22 deletions(-)
Thanks, this is on for-next.
^ permalink raw reply [flat|nested] 5+ messages in thread