linux-kernel - Re: [PATCH V5 2/7] arm64/perf: Update struct arm

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 18 Nov 2022 17:47:05 +0000
From:   Mark Rutland <mark.rutland@....com>
To:     Anshuman Khandual <anshuman.khandual@....com>
Cc:     Suzuki K Poulose <suzuki.poulose@....com>,
        linux-kernel@...r.kernel.org, linux-perf-users@...r.kernel.org,
        linux-arm-kernel@...ts.infradead.org, peterz@...radead.org,
        acme@...nel.org, will@...nel.org, catalin.marinas@....com,
        Mark Brown <broonie@...nel.org>,
        James Clark <james.clark@....com>,
        Rob Herring <robh@...nel.org>, Marc Zyngier <maz@...nel.org>,
        Ingo Molnar <mingo@...hat.com>
Subject: Re: [PATCH V5 2/7] arm64/perf: Update struct arm_pmu for BRBE


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.