Re: [PATCH kernel] powerpc/perf: Stop crashing with generic_compat_pmu

[Madhavan Srinivasan <maddy@linux.ibm.com>]

On 12/2/20 8:31 AM, Alexey Kardashevskiy wrote:
> Hi Maddy,
>
> I just noticed that I still have "powerpc/perf: Add checks for 
> reserved values" in my pile (pushed here 
> https://github.com/aik/linux/commit/61e1bc3f2e19d450e2e2d39174d422160b21957b 
> ), do we still need it? The lockups I saw were fixed by 
> https://github.com/aik/linux/commit/17899eaf88d689 but it is hardly a 
> replacement. Thanks,

sorry missed this. Will look at this again. Since we will need 
generation specific checks for the reserve field.

Maddy

>
>
> On 04/06/2020 02:34, Madhavan Srinivasan wrote:
>>
>>
>> On 6/2/20 8:26 AM, Alexey Kardashevskiy wrote:
>>> The bhrb_filter_map ("The  Branch History  Rolling  Buffer") 
>>> callback is
>>> only defined in raw CPUs' power_pmu structs. The "architected" CPUs use
>>> generic_compat_pmu which does not have this callback and crashed occur.
>>>
>>> This add a NULL pointer check for bhrb_filter_map() which behaves as if
>>> the callback returned an error.
>>>
>>> This does not add the same check for config_bhrb() as the only caller
>>> checks for cpuhw->bhrb_users which remains zero if bhrb_filter_map==0.
>>
>> Changes looks fine.
>> Reviewed-by: Madhavan Srinivasan <maddy@linux.ibm.com>
>>
>> The commit be80e758d0c2e ('powerpc/perf: Add generic compat mode pmu 
>> driver')
>> which introduced generic_compat_pmu was merged in v5.2.  So we need to
>> CC stable starting from 5.2 :( .  My bad,  sorry.
>>
>> Maddy
>>
>>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>>> ---
>>>   arch/powerpc/perf/core-book3s.c | 19 ++++++++++++++-----
>>>   1 file changed, 14 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/arch/powerpc/perf/core-book3s.c 
>>> b/arch/powerpc/perf/core-book3s.c
>>> index 3dcfecf858f3..36870569bf9c 100644
>>> --- a/arch/powerpc/perf/core-book3s.c
>>> +++ b/arch/powerpc/perf/core-book3s.c
>>> @@ -1515,9 +1515,16 @@ static int power_pmu_add(struct perf_event 
>>> *event, int ef_flags)
>>>       ret = 0;
>>>    out:
>>>       if (has_branch_stack(event)) {
>>> -        power_pmu_bhrb_enable(event);
>>> -        cpuhw->bhrb_filter = ppmu->bhrb_filter_map(
>>> -                    event->attr.branch_sample_type);
>>> +        u64 bhrb_filter = -1;
>>> +
>>> +        if (ppmu->bhrb_filter_map)
>>> +            bhrb_filter = ppmu->bhrb_filter_map(
>>> +                event->attr.branch_sample_type);
>>> +
>>> +        if (bhrb_filter != -1) {
>>> +            cpuhw->bhrb_filter = bhrb_filter;
>>> +            power_pmu_bhrb_enable(event); /* Does bhrb_users++ */
>>> +        }
>>>       }
>>>
>>>       perf_pmu_enable(event->pmu);
>>> @@ -1839,7 +1846,6 @@ static int power_pmu_event_init(struct 
>>> perf_event *event)
>>>       int n;
>>>       int err;
>>>       struct cpu_hw_events *cpuhw;
>>> -    u64 bhrb_filter;
>>>
>>>       if (!ppmu)
>>>           return -ENOENT;
>>> @@ -1945,7 +1951,10 @@ static int power_pmu_event_init(struct 
>>> perf_event *event)
>>>       err = power_check_constraints(cpuhw, events, cflags, n + 1);
>>>
>>>       if (has_branch_stack(event)) {
>>> -        bhrb_filter = ppmu->bhrb_filter_map(
>>> +        u64 bhrb_filter = -1;
>>> +
>>> +        if (ppmu->bhrb_filter_map)
>>> +            bhrb_filter = ppmu->bhrb_filter_map(
>>>                       event->attr.branch_sample_type);
>>>
>>>           if (bhrb_filter == -1) {
>>
>