Received-SPF: pass (google.com: domain of linux-kernel+bounces-101214-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Date: Wed, 13 Mar 2024 09:26:10 +0100
From: Andrew Jones <ajones@ventanamicro.com>
To: Samuel Holland <samuel.holland@sifive.com>
Cc: Palmer Dabbelt <palmer@dabbelt.com>, linux-riscv@lists.infradead.org, 
	Albert Ou <aou@eecs.berkeley.edu>, Alexandre Ghiti <alexghiti@rivosinc.com>, 
	Anup Patel <apatel@ventanamicro.com>, Paul Walmsley <paul.walmsley@sifive.com>, 
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH] riscv: Add tracepoints for SBI calls and returns
Message-ID: <20240313-d0cf274ace35c0f54c66f7fa@orel>
References: <20240312212441.2322765-1-samuel.holland@sifive.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240312212441.2322765-1-samuel.holland@sifive.com>

On Tue, Mar 12, 2024 at 02:23:34PM -0700, Samuel Holland wrote:
> These are useful for measuring the latency of SBI calls. The SBI HSM
> extension is excluded because those functions are called from contexts
> such as cpuidle where instrumentation is not allowed.

Thanks for this. These will be nice to have.

> 
> Signed-off-by: Samuel Holland <samuel.holland@sifive.com>
> ---
> 
>  arch/riscv/include/asm/trace.h | 60 ++++++++++++++++++++++++++++++++++
>  arch/riscv/kernel/sbi.c        |  7 ++++
>  2 files changed, 67 insertions(+)
>  create mode 100644 arch/riscv/include/asm/trace.h
> 
> diff --git a/arch/riscv/include/asm/trace.h b/arch/riscv/include/asm/trace.h
> new file mode 100644
> index 000000000000..f96091f83c25
> --- /dev/null
> +++ b/arch/riscv/include/asm/trace.h
> @@ -0,0 +1,60 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#undef TRACE_SYSTEM
> +#define TRACE_SYSTEM riscv
> +
> +#if !defined(_TRACE_RISCV_H) || defined(TRACE_HEADER_MULTI_READ)
> +#define _TRACE_RISCV_H
> +
> +#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),

nit: Can we remove the three blank lines above?

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),

Same nit as above.

> +
> +	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
> +#undef TRACE_INCLUDE_FILE
> +
> +#define TRACE_INCLUDE_PATH asm
> +#define TRACE_INCLUDE_FILE trace
> +
> +#include <trace/define_trace.h>
> diff --git a/arch/riscv/kernel/sbi.c b/arch/riscv/kernel/sbi.c
> index e66e0999a800..a1d21d8f5293 100644
> --- a/arch/riscv/kernel/sbi.c
> +++ b/arch/riscv/kernel/sbi.c
> @@ -14,6 +14,9 @@
>  #include <asm/smp.h>
>  #include <asm/tlbflush.h>
>  
> +#define CREATE_TRACE_POINTS
> +#include <asm/trace.h>
> +
>  /* default SBI version is 0.1 */
>  unsigned long sbi_spec_version __ro_after_init = SBI_SPEC_VERSION_DEFAULT;
>  EXPORT_SYMBOL(sbi_spec_version);
> @@ -31,6 +34,8 @@ struct sbiret sbi_ecall(int ext, int fid, unsigned long arg0,
>  {
>  	struct sbiret ret;
>  
> +	trace_sbi_call(ext, fid);
> +
>  	register uintptr_t a0 asm ("a0") = (uintptr_t)(arg0);
>  	register uintptr_t a1 asm ("a1") = (uintptr_t)(arg1);
>  	register uintptr_t a2 asm ("a2") = (uintptr_t)(arg2);
> @@ -46,6 +51,8 @@ struct sbiret sbi_ecall(int ext, int fid, unsigned long arg0,
>  	ret.error = a0;
>  	ret.value = a1;
>  
> +	trace_sbi_return(ext, ret.error, ret.value);
> +
>  	return ret;
>  }
>  EXPORT_SYMBOL(sbi_ecall);
> -- 
> 2.43.1
>

Other than the blank line nits,

Reviewed-by: Andrew Jones <ajones@ventanamicro.com>

Thanks,
drew