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

[Mark Rutland]

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@xxxxxxx/
> 
> --------------------------------------------------------------------------
> > 
> > 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.