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

[Pu Lehui <pulehui@huaweicloud.com>]

On 2024/3/9 17:06, Atish Patra wrote:
> 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.
> 

Thanks for your addition, looking forward to the upcoming 
implementation. Will send new version soon.

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


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