[V5,2/7] arm64/perf: Update struct arm_pmu for BRBE

  Although BRBE is an armv8 speciifc HW feature, abstracting out its various
function callbacks at the struct arm_pmu level is preferred, as it cleaner
, easier to follow and maintain.

Besides some helpers i.e brbe_supported(), brbe_probe() and brbe_reset()
might not fit seamlessly, when tried to be embedded via existing arm_pmu
helpers in the armv8 implementation.

Updates the struct arm_pmu to include all required helpers that will drive
BRBE functionality for a given PMU implementation. These are the following.

- brbe_filter	: Convert perf event filters into BRBE HW filters
- brbe_probe	: Probe BRBE HW and capture its attributes
- brbe_enable	: Enable BRBE HW with a given config
- brbe_disable	: Disable BRBE HW
- brbe_read	: Read BRBE buffer for captured branch records
- brbe_reset	: Reset BRBE buffer
- brbe_supported: Whether BRBE is supported or not

A BRBE driver implementation needs to provide these functionalities.

Cc: Will Deacon <will@kernel.org>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-perf-users@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
---
 arch/arm64/kernel/perf_event.c | 36 ++++++++++++++++++++++++++++++++++
 include/linux/perf/arm_pmu.h   | 21 ++++++++++++++++++++
 2 files changed, 57 insertions(+)
  

On 07/11/2022 06:25, Anshuman Khandual wrote:
> Although BRBE is an armv8 speciifc HW feature, abstracting out its various
> function callbacks at the struct arm_pmu level is preferred, as it cleaner
> , easier to follow and maintain.
> 
> Besides some helpers i.e brbe_supported(), brbe_probe() and brbe_reset()
> might not fit seamlessly, when tried to be embedded via existing arm_pmu
> helpers in the armv8 implementation.
> 
> Updates the struct arm_pmu to include all required helpers that will drive
> BRBE functionality for a given PMU implementation. These are the following.
> 
> - brbe_filter	: Convert perf event filters into BRBE HW filters
> - brbe_probe	: Probe BRBE HW and capture its attributes
> - brbe_enable	: Enable BRBE HW with a given config
> - brbe_disable	: Disable BRBE HW
> - brbe_read	: Read BRBE buffer for captured branch records
> - brbe_reset	: Reset BRBE buffer
> - brbe_supported: Whether BRBE is supported or not
> 
> A BRBE driver implementation needs to provide these functionalities.

Could these not be hidden from the generic arm_pmu and kept in the
arm64 pmu backend  ? It looks like they are quite easy to simply
move these to the corresponding hooks in arm64 pmu.

Suzuki
  

On 11/9/22 17:00, Suzuki K Poulose wrote:
> On 07/11/2022 06:25, Anshuman Khandual wrote:
>> Although BRBE is an armv8 speciifc HW feature, abstracting out its various
>> function callbacks at the struct arm_pmu level is preferred, as it cleaner
>> , easier to follow and maintain.
>>
>> Besides some helpers i.e brbe_supported(), brbe_probe() and brbe_reset()
>> might not fit seamlessly, when tried to be embedded via existing arm_pmu
>> helpers in the armv8 implementation.
>>
>> Updates the struct arm_pmu to include all required helpers that will drive
>> BRBE functionality for a given PMU implementation. These are the following.
>>
>> - brbe_filter    : Convert perf event filters into BRBE HW filters
>> - brbe_probe    : Probe BRBE HW and capture its attributes
>> - brbe_enable    : Enable BRBE HW with a given config
>> - brbe_disable    : Disable BRBE HW
>> - brbe_read    : Read BRBE buffer for captured branch records
>> - brbe_reset    : Reset BRBE buffer
>> - brbe_supported: Whether BRBE is supported or not
>>
>> A BRBE driver implementation needs to provide these functionalities.
> 
> Could these not be hidden from the generic arm_pmu and kept in the
> arm64 pmu backend  ? It looks like they are quite easy to simply
> move these to the corresponding hooks in arm64 pmu.

We have had this discussion multiple times in the past [1], but I still
believe, keeping BRBE implementation hooks at the PMU level rather than
embedding them with other PMU events handling, is a much better logical
abstraction.

[1] https://lore.kernel.org/all/c3804290-bdb1-d1eb-3526-9b0ce4c8e8b1@arm.com/

--------------------------------------------------------------------------
> 
> One thing to answer in the commit msg is why we need the hooks here.  
> Have we concluded that adding BRBE hooks to struct arm_pmu for what is 
> an armv8 specific feature is the right approach? I don't recall 
> reaching that conclusion.

Although it might be possible to have this implementation embedded in
the existing armv8 PMU implementation, I still believe that the BRBE
functionalities abstracted out at the arm_pmu level with a separate
config option is cleaner, easier to follow and to maintain as well.

Besides some helpers i.e brbe_supported(), brbe_probe() and brbe_reset()
might not fit seamlessly, when tried to be embedded via existing arm_pmu
helpers in the armv8 implementation.

Nonetheless if arm_pmu based additional BRBE helpers is absolutely a no
go for folks here in general, will explore arm64 based implementation.
----------------------------------------------------------------------------

I am still waiting for maintainer's take on this issue. I will be happy to
rework this series to move all these implementation inside arm64 callbacks
instead, if that is required or preferred by the maintainers. But according
to me, this current abstraction layout is much better.

- Anshuman
  

Hi Anshuman,

Apologies for the delayi n reviewing this.

On Fri, Nov 18, 2022 at 12:09:07PM +0530, Anshuman Khandual wrote:
> On 11/9/22 17:00, Suzuki K Poulose wrote:
> > On 07/11/2022 06:25, Anshuman Khandual wrote:
> >> Although BRBE is an armv8 speciifc HW feature, abstracting out its various
> >> function callbacks at the struct arm_pmu level is preferred, as it cleaner
> >> , easier to follow and maintain.
> >>
> >> Besides some helpers i.e brbe_supported(), brbe_probe() and brbe_reset()
> >> might not fit seamlessly, when tried to be embedded via existing arm_pmu
> >> helpers in the armv8 implementation.
> >>
> >> Updates the struct arm_pmu to include all required helpers that will drive
> >> BRBE functionality for a given PMU implementation. These are the following.
> >>
> >> - brbe_filter    : Convert perf event filters into BRBE HW filters
> >> - brbe_probe    : Probe BRBE HW and capture its attributes
> >> - brbe_enable    : Enable BRBE HW with a given config
> >> - brbe_disable    : Disable BRBE HW
> >> - brbe_read    : Read BRBE buffer for captured branch records
> >> - brbe_reset    : Reset BRBE buffer
> >> - brbe_supported: Whether BRBE is supported or not
> >>
> >> A BRBE driver implementation needs to provide these functionalities.
> > 
> > Could these not be hidden from the generic arm_pmu and kept in the
> > arm64 pmu backend  ? It looks like they are quite easy to simply
> > move these to the corresponding hooks in arm64 pmu.
> 
> We have had this discussion multiple times in the past [1], but I still
> believe, keeping BRBE implementation hooks at the PMU level rather than
> embedding them with other PMU events handling, is a much better logical
> abstraction.
> 
> [1] https://lore.kernel.org/all/c3804290-bdb1-d1eb-3526-9b0ce4c8e8b1@arm.com/
> 
> --------------------------------------------------------------------------
> > 
> > One thing to answer in the commit msg is why we need the hooks here.  
> > Have we concluded that adding BRBE hooks to struct arm_pmu for what is 
> > an armv8 specific feature is the right approach? I don't recall 
> > reaching that conclusion.
> 
> Although it might be possible to have this implementation embedded in
> the existing armv8 PMU implementation, I still believe that the BRBE
> functionalities abstracted out at the arm_pmu level with a separate
> config option is cleaner, easier to follow and to maintain as well.
> 
> Besides some helpers i.e brbe_supported(), brbe_probe() and brbe_reset()
> might not fit seamlessly, when tried to be embedded via existing arm_pmu
> helpers in the armv8 implementation.
> 
> Nonetheless if arm_pmu based additional BRBE helpers is absolutely a no
> go for folks here in general, will explore arm64 based implementation.
> ----------------------------------------------------------------------------
> 
> I am still waiting for maintainer's take on this issue. I will be happy to
> rework this series to move all these implementation inside arm64 callbacks
> instead, if that is required or preferred by the maintainers. But according
> to me, this current abstraction layout is much better.

To be honest, I'm not sure what's best right now; but at the moment it's not
clear to me why this couldn't fit within the existing hooks.

Above you say brbe_supported() / brbe_probe() / brbe_reset() didn't fit
seamlessly; can you give an example of problem? I think I'm missing something
obvious.

Thanks,
Mark.
  

On 11/18/22 23:17, Mark Rutland wrote:
> 
> Hi Anshuman,
> 
> Apologies for the delayi n reviewing this.
> 
> On Fri, Nov 18, 2022 at 12:09:07PM +0530, Anshuman Khandual wrote:
>> On 11/9/22 17:00, Suzuki K Poulose wrote:
>>> On 07/11/2022 06:25, Anshuman Khandual wrote:
>>>> Although BRBE is an armv8 speciifc HW feature, abstracting out its various
>>>> function callbacks at the struct arm_pmu level is preferred, as it cleaner
>>>> , easier to follow and maintain.
>>>>
>>>> Besides some helpers i.e brbe_supported(), brbe_probe() and brbe_reset()
>>>> might not fit seamlessly, when tried to be embedded via existing arm_pmu
>>>> helpers in the armv8 implementation.
>>>>
>>>> Updates the struct arm_pmu to include all required helpers that will drive
>>>> BRBE functionality for a given PMU implementation. These are the following.
>>>>
>>>> - brbe_filter    : Convert perf event filters into BRBE HW filters
>>>> - brbe_probe    : Probe BRBE HW and capture its attributes
>>>> - brbe_enable    : Enable BRBE HW with a given config
>>>> - brbe_disable    : Disable BRBE HW
>>>> - brbe_read    : Read BRBE buffer for captured branch records
>>>> - brbe_reset    : Reset BRBE buffer
>>>> - brbe_supported: Whether BRBE is supported or not
>>>>
>>>> A BRBE driver implementation needs to provide these functionalities.
>>>
>>> Could these not be hidden from the generic arm_pmu and kept in the
>>> arm64 pmu backend  ? It looks like they are quite easy to simply
>>> move these to the corresponding hooks in arm64 pmu.
>>
>> We have had this discussion multiple times in the past [1], but I still
>> believe, keeping BRBE implementation hooks at the PMU level rather than
>> embedding them with other PMU events handling, is a much better logical
>> abstraction.
>>
>> [1] https://lore.kernel.org/all/c3804290-bdb1-d1eb-3526-9b0ce4c8e8b1@arm.com/
>>
>> --------------------------------------------------------------------------
>>>
>>> One thing to answer in the commit msg is why we need the hooks here.  
>>> Have we concluded that adding BRBE hooks to struct arm_pmu for what is 
>>> an armv8 specific feature is the right approach? I don't recall 
>>> reaching that conclusion.
>>
>> Although it might be possible to have this implementation embedded in
>> the existing armv8 PMU implementation, I still believe that the BRBE
>> functionalities abstracted out at the arm_pmu level with a separate
>> config option is cleaner, easier to follow and to maintain as well.
>>
>> Besides some helpers i.e brbe_supported(), brbe_probe() and brbe_reset()
>> might not fit seamlessly, when tried to be embedded via existing arm_pmu
>> helpers in the armv8 implementation.
>>
>> Nonetheless if arm_pmu based additional BRBE helpers is absolutely a no
>> go for folks here in general, will explore arm64 based implementation.
>> ----------------------------------------------------------------------------
>>
>> I am still waiting for maintainer's take on this issue. I will be happy to
>> rework this series to move all these implementation inside arm64 callbacks
>> instead, if that is required or preferred by the maintainers. But according
>> to me, this current abstraction layout is much better.
> 
> To be honest, I'm not sure what's best right now; but at the moment it's not
> clear to me why this couldn't fit within the existing hooks.
> 
> Above you say brbe_supported() / brbe_probe() / brbe_reset() didn't fit
> seamlessly; can you give an example of problem? I think I'm missing something
> obvious.

I tried to move them inside armv8 implementation callbacks.

arm64_pmu_brbe_supported() can be moved inside __armv8_pmuv3_map_event(), so that
event viability can be validated during armpmu_event_init(). arm64_pmu_brbe_probe()
can be moved inside __armv8pmu_probe_pmu() as you have suggested earlier on another
thread. arm64_pmu_brbe_reset() can also be moved inside armv8pmu_enable_event(),
and also armv8pmu_reset().

The only problem being armpmu_sched_task() where earlier we had BRBE reset, but I
guess it can be replaced with entire PMU reset which does the BRBE reset as well ?

static void armpmu_sched_task(struct perf_event_context *ctx, bool sched_in)
{
        struct arm_pmu *armpmu = to_arm_pmu(ctx->pmu);

        if (sched_in)
                armpmu->reset(armpmu);
}