Re: [PATCH v2] perf parse-events: default data breakpoints to any size on RISC-V

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Low] perf parse-events: default_data_breakpoint_len() uses unmerged
  bp_len=0 ABI
--

commit cd4f828c59020f263743338e4d9c3465f6fbb3f6
Author: Rui Qi <qirui.001@bytedance.com>

perf parse-events: default data breakpoints to any size on RISC-V

This commit introduces default_data_breakpoint_len() so that data
breakpoints without an explicit length default to 0 on RISC-V and
HW_BREAKPOINT_LEN_4 on other architectures, and updates the parse-events
tests accordingly.

> diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
> index f3ba70d4f76b2..e752f41f2b369 100644
> --- a/tools/perf/util/parse-events.c
> +++ b/tools/perf/util/parse-events.c
> @@ -598,6 +598,27 @@ size_t default_breakpoint_len(void)
>  #endif
>  }
>  
> +/*
> + * Default length for data breakpoints when the user did not specify one.
> + *
> + * Most architectures treat the length as a byte range that is compared
> + * against the address of the access, so 4 bytes is a reasonable default
> + * that matches most accesses.  RISC-V debug triggers (Sdtrig mcontrol6)
> + * encode the width of the access in the trigger instead, and a trigger
> + * only fires when the access has exactly that width.  This holds for
> + * both RV32 and RV64.  With a fixed default, an access of a different
> + * width silently never matches, so pass 0 on RISC-V, which the kernel
> + * translates to "match any size".
> + */
> +size_t default_data_breakpoint_len(void)
> +{
> +#if defined(__riscv)
> +	return 0;
> +#else
> +	return HW_BREAKPOINT_LEN_4;
> +#endif
> +}
> +
>  static int
>  parse_breakpoint_type(const char *type, struct perf_event_attr *attr)
>  {
> @@ -658,7 +679,7 @@ int parse_events_add_breakpoint(struct parse_events_state *parse_state,
>  		if (attr.bp_type == HW_BREAKPOINT_X)
>  			len = default_breakpoint_len();
>  		else
> -			len = HW_BREAKPOINT_LEN_4;
> +			len = default_data_breakpoint_len();
>  	}
>  
>  	attr.bp_len = len;

[Severity: Low]
Does setting attr.bp_len to 0 in default_data_breakpoint_len() rely on an
unmerged kernel ABI for RISC-V hardware breakpoints?

Mainline arch/riscv does not yet select HAVE_HW_BREAKPOINT or provide
hw_breakpoint_arch_parse(), and include/uapi/linux/hw_breakpoint.h only
defines HW_BREAKPOINT_LEN_1 through HW_BREAKPOINT_LEN_8 to describe the
byte length of the watched address range [bp_addr, bp_addr + bp_len)
rather than the exact memory access operand width.  Baking attr.bp_len = 0
into tools/perf before the RISC-V kernel hw_breakpoint support and UAPI
semantics are merged upstream risks breaking default mem:<addr> watchpoints
with -EINVAL if the upstream RISC-V implementation follows the standard
HW_BREAKPOINT_LEN_* byte-range contract expected by tests such as
test__wp_modify() in tools/perf/tests/wp.c.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261111022648.71649-2-qirui.001@bytedance.com?part=1