Re: [PATCH] drivers/perf: riscv: Disable PERF_SAMPLE_BRANCH_* while not supported

[Atish Patra <atishp@atishpatra.org>]

On Fri, Mar 8, 2024 at 10:48 PM Pu Lehui <pulehui@huaweicloud.com> wrote:
>
>
>
> On 2024/3/8 1:52, Conor Dooley wrote:
> > On Tue, Mar 05, 2024 at 03:52:23PM +0000, Pu Lehui wrote:
> >> From: Pu Lehui <pulehui@huawei.com>
> >>
> >> RISC-V perf does not yet support branch sampling. Two riscv bpf
> >> testcases get_branch_snapshot and perf_branches/perf_branches_hw failed
> >> due to not disabling such sampling.
> >>
> >> Signed-off-by: Pu Lehui <pulehui@huawei.com>
> >
> > This seems worthy of a fixes tag. For what commit I do not know, but I
> > figure it is a problem currently in mainline and a fix should be
> > backported?
>
> Thanks for review. We need to ban this type of event, otherwise we will
> get unpredictable results. It should be with a fixes tag.
>
> >
> >> ---
> >>   drivers/perf/riscv_pmu.c | 4 ++++
> >>   1 file changed, 4 insertions(+)
> >>
> >> diff --git a/drivers/perf/riscv_pmu.c b/drivers/perf/riscv_pmu.c
> >> index c78a6fd6c57f..bc42cb95a97c 100644
> >> --- a/drivers/perf/riscv_pmu.c
> >> +++ b/drivers/perf/riscv_pmu.c
> >> @@ -313,6 +313,10 @@ static int riscv_pmu_event_init(struct perf_event *event)
> >>      u64 event_config = 0;
> >>      uint64_t cmask;
> >>
> >> +    /* does not support taken branch sampling */
> >
> > Skimming patchwork, I found this comment confusing.
> > Isolated from the commit message, which it will be once committed, it
> > was not immediately clear that you were referring to the driver. I think
> > it is just matter of making this a complete sentence that starts by
> > mentioning /what/ does not support this feature. Is it the driver? Does
> > the spec not allow it? etc
>
> I actually just found out that the current implementation of riscv perf
> driver doesn't support it. After reading the relevant spec, I did not
> find any description related to branch record. Maybe I may have missed
> something or it may be supported in the future, but the current driver
> does not support it, so I think it is more appropriate to limit this

Yes. There was no specification at the time this driver was upstreamed.
The spec for branch record being is on its way for ratification and patches
will be available soon.
https://github.com/riscv/riscv-control-transfer-records

The changes look good to me for now. I will be happy to send a RB tag once you
add the Fixes tag.

> comment to the driver implementation. How about "riscv perf driver does
> not support branch stack sampling"?
>
> >
> > Cheers,
> > Conor.
> >
> >> +    if (has_branch_stack(event))
> >> +            return -EOPNOTSUPP;
> >> +
> >>      hwc->flags = 0;
> >>      mapped_event = rvpmu->event_map(event, &event_config);
> >>      if (mapped_event < 0) {
> >> --
> >> 2.34.1
> >>
>


-- 
Regards,
Atish

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