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 12:09:07 +0530
From:   Anshuman Khandual <anshuman.khandual@....com>
To:     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, mark.rutland@....com, will@...nel.org,
        catalin.marinas@....com
Cc:     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



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