Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <4efc0ae1-564e-dd05-842a-46fb1aeb4ad8@arm.com>
Date:   Fri, 18 Nov 2022 12:09:07 +0530
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.2.2
Subject: Re: [PATCH V5 2/7] arm64/perf: Update struct arm_pmu for BRBE
Content-Language: en-US
To:     Suzuki K Poulose <suzuki.poulose@arm.com>,
        linux-kernel@vger.kernel.org, linux-perf-users@vger.kernel.org,
        linux-arm-kernel@lists.infradead.org, peterz@infradead.org,
        acme@kernel.org, mark.rutland@arm.com, will@kernel.org,
        catalin.marinas@arm.com
Cc:     Mark Brown <broonie@kernel.org>, James Clark <james.clark@arm.com>,
        Rob Herring <robh@kernel.org>, Marc Zyngier <maz@kernel.org>,
        Ingo Molnar <mingo@redhat.com>
References: <20221107062514.2851047-1-anshuman.khandual@arm.com>
 <20221107062514.2851047-3-anshuman.khandual@arm.com>
 <8f6d3424-2650-8e8b-61f7-1431aec4633b@arm.com>
From:   Anshuman Khandual <anshuman.khandual@arm.com>
In-Reply-To: <8f6d3424-2650-8e8b-61f7-1431aec4633b@arm.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Precedence: bulk


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