[PATCH v3 1/3] perf: RISC-V: fix access beyond allocated array

[Conor.Dooley at microchip.com]

On 28/08/2022 22:31, Sergey Matyukevich wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
>>> [PATCH v3 1/3] perf: RISC-V: fix access beyond allocated array
>>                                ^^^ (see below)
>>>
>>> From: Sergey Matyukevich <sergey.matyukevich at syntacore.com>
>>>
>>> The root cause could be related to the interpretation of the number of
>>> counters reported by SBI firmware. For instance, if we assume that unused
>>> timer counter with index 1 is not reported, then the range is correct
>>> and larger array needs to be allocated.
>>
>> I found this to be confusingly worded, had to read it a few times before
>> I understood what you meant. Maybe it's just late on a Friday, I think the
>> theorycrafting about why the code looks how it does got me lol
> 
> Well, it was not exactly theorycrafting, see accompanying opensbi changes:
> http://lists.infradead.org/pipermail/opensbi/2022-June/002926.html
> I tried to provide some context for this one-liner, but failed to do so.

It may not have been theorycrafting, but it read like it was. Your proposed
commit message below I think does a much better job of explaining why this
needs to be fixed.

> 
>>> This is not the case though since SBI firmware is supposed to report the
>>> total number of firmware and hardware counters including special or
>>> unused ones like the timer counter. So just fix the range in for-loop.
>>                                               ^^^
>> I see "fix" mentioned twice here, what commmit does it fix?
> 
> That is the initial commit adding the whole SBI PMU driver, so it is a
> bit confusing to add 'Fixes' in this case. I think the following commit
> message should be sufficient:

Fixes tags are good so we know how far back a fix should be backported.
The driver was added in 5.18 AFAICT, but that's EOL so this fix would
only need to be backported to 5.19. The appropriate fixes line would be:

Fixes: e9991434596f ("RISC-V: Add perf platform driver based on SBI PMU extension")

> 
> SBI firmware should report total number of firmware and hardware counters
> including unused ones or special ones. In this case the kernel doesn't need
> to make any assumptions about gaps in reported counters, e.g. excluded timer
> counter. That was fixed in OpenSBI v1.1 by commit 3f66465fb6bf ("lib: pmu:
> allow to use the highest available counter"). This kernel patch has no effect
> if SBI firmware behaves correctly. However it eliminates access beyond the
> allocated pmu_ctr_list if the kernel is used with OpenSBI older than v1.1.

As I said above, I think this commit message does a far better job of
explaining. For v4, could you please use this as the commit message and
add the fixes line that I pasted above?

Thanks,
Conor.