Re: [PATCH] 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 2 potential
issues to consider:
- [Medium] perf parse-events: RISC-V bp_len=0 default breaks
  parse-events unit tests
- [Low] perf parse-events: default_data_breakpoint_len() uses unmerged
  bp_len=0 ABI
--

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

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

This commit adds default_data_breakpoint_len() to default unspecified data
breakpoint lengths to 0 on RISC-V while keeping HW_BREAKPOINT_LEN_4 on
other architectures.

> diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
> index f3ba70d4f76b2..b72e4b0da8a15 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".
> + */
> +static size_t default_data_breakpoint_len(void)
> +{
> +#if defined(__riscv)
> +	return 0;
> +#else
> +	return HW_BREAKPOINT_LEN_4;
> +#endif
> +}

[Severity: Low]
Does default_data_breakpoint_len() rely on a kernel ABI that is not yet in
mainline?

Mainline arch/riscv does not select HAVE_HW_BREAKPOINT or implement
hw_breakpoint_arch_parse(), and include/uapi/linux/hw_breakpoint.h only
defines HW_BREAKPOINT_LEN_1 through HW_BREAKPOINT_LEN_8 to describe a
watched byte range (as relied upon by test__wp_modify() in
tools/perf/tests/wp.c).  All in-tree hw_breakpoint_arch_parse()
implementations reject attr->bp_len == 0 with -EINVAL, so baking bp_len = 0
into tools/perf before the RISC-V kernel hw_breakpoint support and UAPI are
merged upstream risks breaking default data breakpoints if the upstream
implementation follows standard byte-range semantics.

>  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();
>  	}

[Severity: Medium]
Does this regression break the "Parse event definition strings" unit test
on RISC-V?

When commit 70b27c756f95 ("perf parse-events: Add default_breakpoint_len
helper") introduced default_breakpoint_len() for instruction breakpoints,
it exported the helper in tools/perf/util/parse-events.h and updated
test__checkevent_breakpoint_x() in tools/perf/tests/parse-events.c.  Here,
test__checkevent_breakpoint(), test__checkevent_breakpoint_r(),
test__checkevent_breakpoint_w(), and test__checkevent_breakpoint_rw() still
assert that evsel->core.attr.bp_len equals HW_BREAKPOINT_LEN_4:

tools/perf/tests/parse-events.c:test__checkevent_breakpoint() {
    ...
	TEST_ASSERT_EVSEL("wrong bp_len", HW_BREAKPOINT_LEN_4 == evsel->core.attr.bp_len, evsel);
    ...
}

On RISC-V, parsing data breakpoints without an explicit length (such as
mem:0, mem:0:r, mem:0:w, and mem:0:rw) now sets attr.bp_len to 0, causing
all 16 un-sized data breakpoint test cases to fail.  Exporting
default_data_breakpoint_len() for those checks (while keeping the explicit
HW_BREAKPOINT_LEN_4 check for test__checkevent_breakpoint_len_rw_modifier(),
which tests mem:0/4:rw:u and delegates to test__checkevent_breakpoint_rw())
would keep the test suite passing.

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