| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <mhng-516082EA-5A9A-4A76-9448-70828749F95F@palmerdabbelt-mac>
Date: Mon, 23 Jun 2025 15:54:00 -0700 (PDT)
From: Palmer Dabbelt <palmer@...belt.com>
To: rkrcmar@...tanamicro.com
CC: linux-riscv@...ts.infradead.org, linux-kernel@...r.kernel.org,
Paul Walmsley <paul.walmsley@...ive.com>, aou@...s.berkeley.edu, Alexandre Ghiti <alex@...ti.fr>,
Atish Patra <atishp@...osinc.com>, ajones@...tanamicro.com, cleger@...osinc.com, apatel@...tanamicro.com,
thomas.weissschuh@...utronix.de, david.laight.linux@...il.com
Subject: Re: [PATCH v2 3/2] RISC-V: sbi: remove sbi_ecall tracepoints
Having patch 3 of 2 is not normal.
On Thu, 19 Jun 2025 12:03:15 PDT (-0700), rkrcmar@...tanamicro.com wrote:
> 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. :)
So the issue is the extra save/restore on function entry? That's the
sort of think shrink wrapping is supposed to help with. It's been
implemented in GCC for a while, but I'm not sure how well it's been
pushed on (IIRC it was just one of the SPEC workloads).
That said, this is kind of hard to reason about. Can you pull out a
smaller example?
> Signed-off-by: Radim Krčmář <rkrcmar@...tanamicro.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
Powered by blists - more mailing lists