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

[Alexey Kardashevskiy <aik@ozlabs.ru>]

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,


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

-- 
Alexey