[PATCH v2 3/2] RISC-V: sbi: remove sbi_ecall tracepoints

["Radim Krčmář" <rkrcmar@ventanamicro.com>]

Tracepoits generate bad code in the non-trace path.

The acceptable tracepoint overhead in the non-tracing path is a nop, and
possibly a second 2 byte nop for alignment, but the actual overhead is
way higher.  For example, the sbi_fwft_set with tracepoints:
   0xffffffff80022ee8 <+0>:	auipc	a5,0x2cec
   0xffffffff80022eec <+4>:	lbu	a5,1704(a5) # 0xffffffff82d0f590 <sbi_fwft_supported>
   0xffffffff80022ef0 <+8>:	beqz	a5,0xffffffff80022fa0 <sbi_fwft_set+184>
   0xffffffff80022ef2 <+10>:	addi	sp,sp,-48
   0xffffffff80022ef4 <+12>:	sd	s0,32(sp)
   0xffffffff80022ef6 <+14>:	sd	s1,24(sp)
   0xffffffff80022ef8 <+16>:	sd	s2,16(sp)
   0xffffffff80022efa <+18>:	sd	ra,40(sp)
   0xffffffff80022efc <+20>:	addi	s0,sp,48
   0xffffffff80022efe <+22>:	slli	s1,a0,0x20
   0xffffffff80022f02 <+26>:	mv	s2,a1
   0xffffffff80022f04 <+28>:	srli	s1,s1,0x20
   0xffffffff80022f06 <+30>:	nop
   0xffffffff80022f08 <+32>:	nop
   0xffffffff80022f0c <+36>:	lui	a7,0x46574
   0xffffffff80022f10 <+40>:	mv	a0,s1
   0xffffffff80022f12 <+42>:	mv	a1,s2
   0xffffffff80022f14 <+44>:	addi	a7,a7,1620 # 0x46574654
   0xffffffff80022f18 <+48>:	li	a6,0
   0xffffffff80022f1a <+50>:	ecall
   0xffffffff80022f1e <+54>:	mv	s1,a0
   0xffffffff80022f20 <+56>:	nop
   0xffffffff80022f24 <+60>:	addiw	a0,s1,14
   0xffffffff80022f28 <+64>:	li	a5,14
   0xffffffff80022f2a <+66>:	bltu	a5,a0,0xffffffff80022f9a <sbi_fwft_set+178>
   0xffffffff80022f2e <+70>:	slli	a5,a0,0x20
   0xffffffff80022f32 <+74>:	srli	a0,a5,0x1e
   0xffffffff80022f36 <+78>:	auipc	a5,0x1c75
   0xffffffff80022f3a <+82>:	addi	a5,a5,-886 # 0xffffffff81c97bc0 <CSWTCH.177>
   0xffffffff80022f3e <+86>:	add	a5,a5,a0
   0xffffffff80022f40 <+88>:	lw	a0,0(a5)
   0xffffffff80022f42 <+90>:	ld	ra,40(sp)
   0xffffffff80022f44 <+92>:	ld	s0,32(sp)
   0xffffffff80022f46 <+94>:	ld	s1,24(sp)
   0xffffffff80022f48 <+96>:	ld	s2,16(sp)
   0xffffffff80022f4a <+98>:	addi	sp,sp,48
   0xffffffff80022f4c <+100>:	ret
   [tracepoint slowpaths]
   0xffffffff80022f9a <+178>:	li	a0,-524
   0xffffffff80022f9e <+182>:	j	0xffffffff80022f42 <sbi_fwft_set+90>
   0xffffffff80022fa0 <+184>:	li	a0,-95
   0xffffffff80022fa4 <+188>:	ret

Without tracepoints:
   0xffffffff80022b40 <+0>:	addi	sp,sp,-16
   0xffffffff80022b42 <+2>:	sd	s0,0(sp)
   0xffffffff80022b44 <+4>:	sd	ra,8(sp)
   0xffffffff80022b46 <+6>:	addi	s0,sp,16
   0xffffffff80022b48 <+8>:	auipc	a5,0x2ced
   0xffffffff80022b4c <+12>:	lbu	a5,-1464(a5) # 0xffffffff82d0f590 <sbi_fwft_supported>
   0xffffffff80022b50 <+16>:	beqz	a5,0xffffffff80022b8e <sbi_fwft_set+78>
   0xffffffff80022b52 <+18>:	lui	a7,0x46574
   0xffffffff80022b56 <+22>:	slli	a0,a0,0x20
   0xffffffff80022b58 <+24>:	srli	a0,a0,0x20
   0xffffffff80022b5a <+26>:	addi	a7,a7,1620 # 0x46574654
   0xffffffff80022b5e <+30>:	li	a6,0
   0xffffffff80022b60 <+32>:	ecall
   0xffffffff80022b64 <+36>:	li	a5,14
   0xffffffff80022b66 <+38>:	addiw	a0,a0,14
   0xffffffff80022b68 <+40>:	bltu	a5,a0,0xffffffff80022b88 <sbi_fwft_set+72>
   0xffffffff80022b6c <+44>:	slli	a5,a0,0x20
   0xffffffff80022b70 <+48>:	srli	a0,a5,0x1e
   0xffffffff80022b74 <+52>:	auipc	a5,0x1c75
   0xffffffff80022b78 <+56>:	addi	a5,a5,-300 # 0xffffffff81c97a48 <CSWTCH.176>
   0xffffffff80022b7c <+60>:	add	a5,a5,a0
   0xffffffff80022b7e <+62>:	lw	a0,0(a5)
   0xffffffff80022b80 <+64>:	ld	ra,8(sp)
   0xffffffff80022b82 <+66>:	ld	s0,0(sp)
   0xffffffff80022b84 <+68>:	addi	sp,sp,16
   0xffffffff80022b86 <+70>:	ret

   0xffffffff80022b88 <+72>:	li	a0,-524
   0xffffffff80022b8c <+76>:	j	0xffffffff80022b80 <sbi_fwft_set+64>
   0xffffffff80022b8e <+78>:	li	a0,-95
   0xffffffff80022b92 <+82>:	j	0xffffffff80022b80 <sbi_fwft_set+64>

It would be nice if RISC-V had a way to tell compilers to generate the
desired code, because if this issue isn't limited to ecall tracepoints,
then disabling CONFIG_TRACEPOINTS is starting to look good. :)

Signed-off-by: Radim Krčmář <rkrcmar@ventanamicro.com>
---
 arch/riscv/include/asm/sbi.h   | 30 ++--------------------------
 arch/riscv/include/asm/trace.h | 36 ----------------------------------
 arch/riscv/kernel/sbi_ecall.c  | 18 -----------------
 3 files changed, 2 insertions(+), 82 deletions(-)

diff --git a/arch/riscv/include/asm/sbi.h b/arch/riscv/include/asm/sbi.h
index 7aff31583a3d..ffab0614d24a 100644
--- a/arch/riscv/include/asm/sbi.h
+++ b/arch/riscv/include/asm/sbi.h
@@ -11,7 +11,6 @@
 #include <linux/types.h>
 #include <linux/cpumask.h>
 #include <linux/jump_label.h>
-#include <linux/tracepoint-defs.h>
 
 #ifdef CONFIG_RISCV_SBI
 enum sbi_ext_id {
@@ -461,16 +460,6 @@ struct sbiret {
 	long value;
 };
 
-#ifdef CONFIG_TRACEPOINTS
-DECLARE_TRACEPOINT(sbi_call);
-DECLARE_TRACEPOINT(sbi_return);
-extern void do_trace_sbi_call(int ext, int fid);
-extern void do_trace_sbi_return(int ext, long error, long value);
-#else
-static inline void do_trace_sbi_call(int ext, int fid) {};
-static inline void do_trace_sbi_return(int ext, long error, long value) {};
-#endif
-
 void sbi_init(void);
 long __sbi_base_ecall(int fid);
 
@@ -509,32 +498,17 @@ long __sbi_base_ecall(int fid);
 #define __sbi_ecall_constraints7  __sbi_ecall_constraints6, "r" (__a4)
 #define __sbi_ecall_constraints8  __sbi_ecall_constraints7, "r" (__a5)
 
-#define __sbi_ecall_trace_call() \
-	if (tracepoint_enabled(sbi_call)) \
-		do_trace_sbi_call(__ta7, __ta6)
-
-#define __sbi_ecall_trace_return() \
-	if (tracepoint_enabled(sbi_return)) \
-		do_trace_sbi_return(__ta7, __ret.error, __ret.value)
-
-/*
- * Clear a1 to avoid leaking unrelated kernel state through tracepoints in case
- * the register doesn't get overwritten by the ecall nor the arguments.
- */
 #define sbi_ecall(A...) \
 ({ \
 	CONCATENATE(__sbi_ecall_args, COUNT_ARGS(A))(A); \
-	__sbi_ecall_trace_call(); \
 	register uintptr_t __r0 asm ("a0"); \
-	register uintptr_t __r1 asm ("a1") = 0; \
+	register uintptr_t __r1 asm ("a1"); \
 	CONCATENATE(__sbi_ecall_regs, COUNT_ARGS(A)); \
 	asm volatile ("ecall" \
 			: "=r" (__r0), "=r" (__r1) \
 			: CONCATENATE(__sbi_ecall_constraints, COUNT_ARGS(A)) \
 			: "memory"); \
-	struct sbiret __ret = {.error = __r0, .value = __r1}; \
-	__sbi_ecall_trace_return(); \
-	__ret; \
+	(struct sbiret){.error = __r0, .value = __r1}; \
 })
 
 #ifdef CONFIG_RISCV_SBI_V01
diff --git a/arch/riscv/include/asm/trace.h b/arch/riscv/include/asm/trace.h
index 6151cee5450c..7c99d91fcce3 100644
--- a/arch/riscv/include/asm/trace.h
+++ b/arch/riscv/include/asm/trace.h
@@ -7,42 +7,6 @@
 
 #include <linux/tracepoint.h>
 
-TRACE_EVENT_CONDITION(sbi_call,
-	TP_PROTO(int ext, int fid),
-	TP_ARGS(ext, fid),
-	TP_CONDITION(ext != SBI_EXT_HSM),
-
-	TP_STRUCT__entry(
-		__field(int, ext)
-		__field(int, fid)
-	),
-
-	TP_fast_assign(
-		__entry->ext = ext;
-		__entry->fid = fid;
-	),
-
-	TP_printk("ext=0x%x fid=%d", __entry->ext, __entry->fid)
-);
-
-TRACE_EVENT_CONDITION(sbi_return,
-	TP_PROTO(int ext, long error, long value),
-	TP_ARGS(ext, error, value),
-	TP_CONDITION(ext != SBI_EXT_HSM),
-
-	TP_STRUCT__entry(
-		__field(long, error)
-		__field(long, value)
-	),
-
-	TP_fast_assign(
-		__entry->error = error;
-		__entry->value = value;
-	),
-
-	TP_printk("error=%ld value=0x%lx", __entry->error, __entry->value)
-);
-
 #endif /* _TRACE_RISCV_H */
 
 #undef TRACE_INCLUDE_PATH
diff --git a/arch/riscv/kernel/sbi_ecall.c b/arch/riscv/kernel/sbi_ecall.c
index b783a46fff1c..62489ffeae2c 100644
--- a/arch/riscv/kernel/sbi_ecall.c
+++ b/arch/riscv/kernel/sbi_ecall.c
@@ -2,8 +2,6 @@
 /* Copyright (c) 2024 Rivos Inc. */
 
 #include <asm/sbi.h>
-#define CREATE_TRACE_POINTS
-#include <asm/trace.h>
 
 long __sbi_base_ecall(int fid)
 {
@@ -16,19 +14,3 @@ long __sbi_base_ecall(int fid)
 		return sbi_err_map_linux_errno(ret.error);
 }
 EXPORT_SYMBOL(__sbi_base_ecall);
-
-#ifdef CONFIG_TRACEPOINTS
-void do_trace_sbi_call(int ext, int fid)
-{
-	trace_sbi_call(ext, fid);
-}
-EXPORT_SYMBOL(do_trace_sbi_call);
-EXPORT_TRACEPOINT_SYMBOL(sbi_call);
-
-void do_trace_sbi_return(int ext, long error, long value)
-{
-	trace_sbi_return(ext, error, value);
-}
-EXPORT_SYMBOL(do_trace_sbi_return);
-EXPORT_TRACEPOINT_SYMBOL(sbi_return);
-#endif
-- 
2.49.0


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv