linux-kernel - Re: [RFC V1 06/11] arm64/perf: Drive BRBE from perf event states

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]

Message-ID: <9e56509a-2772-a333-ccc3-ad49a2028616@arm.com>
Date:   Thu, 27 Jan 2022 17:50:54 +0530
From:   Anshuman Khandual <anshuman.khandual@....com>
To:     Rob Herring <robh@...nel.org>
Cc:     linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
        Catalin Marinas <catalin.marinas@....com>,
        Will Deacon <will@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        Peter Zijlstra <peterz@...radead.org>,
        Ingo Molnar <mingo@...hat.com>,
        Arnaldo Carvalho de Melo <acme@...nel.org>,
        linux-perf-users@...r.kernel.org
Subject: Re: [RFC V1 06/11] arm64/perf: Drive BRBE from perf event states


On 1/26/22 10:37 PM, Rob Herring wrote:
> On Mon, Jan 24, 2022 at 10:00:48AM +0530, Anshuman Khandual wrote:
>> Branch stack sampling rides along the normal perf event and all the branch
>> records get captured during the PMU interrupt. This just changes perf event
>> handling on the arm64 platform to accommodate required BRBE operations that
>> will enable branch stack sampling support.
>>
>> Cc: Peter Zijlstra <peterz@...radead.org>
>> Cc: Ingo Molnar <mingo@...hat.com>
>> Cc: Arnaldo Carvalho de Melo <acme@...nel.org>
>> Cc: Mark Rutland <mark.rutland@....com>
>> Cc: Will Deacon <will@...nel.org>
>> Cc: Catalin Marinas <catalin.marinas@....com>
>> Cc: linux-perf-users@...r.kernel.org
>> Cc: linux-kernel@...r.kernel.org
>> Cc: linux-arm-kernel@...ts.infradead.org
>> Signed-off-by: Anshuman Khandual <anshuman.khandual@....com>
>> ---
>>  arch/arm64/kernel/perf_event.c |  6 +++++
>>  drivers/perf/arm_pmu.c         | 40 ++++++++++++++++++++++++++++++++++
>>  2 files changed, 46 insertions(+)
>>
>> diff --git a/arch/arm64/kernel/perf_event.c b/arch/arm64/kernel/perf_event.c
>> index f6a47036b0b4..11c82c8f2eec 100644
>> --- a/arch/arm64/kernel/perf_event.c
>> +++ b/arch/arm64/kernel/perf_event.c
>> @@ -864,6 +864,12 @@ static irqreturn_t armv8pmu_handle_irq(struct arm_pmu *cpu_pmu)
>>  		if (!armpmu_event_set_period(event))
>>  			continue;
>>  
>> +		if (has_branch_stack(event)) {
>> +			cpu_pmu->brbe_read(cpuc, event);
> 
> Is has_branch_stack() guaranteed to be false on arm32? If not, this will 
> be a NULL function ptr.

armpmu_event_init() blocks a perf event from being created with branch
stack sampling request without CONFIG_ARM_BRBE_PMU option being enabled
first, which has dependency on CONFIG_ARM64. So has_branch_stack() is
guaranteed to be false on arm32.

static int armpmu_event_init(struct perf_event *event)
{
	....
        if (has_branch_stack(event)) {
                /*
                 * BRBE support is absent. Select CONFIG_ARM_BRBE_PMU
                 * in the config, before branch stack sampling events
                 * can be requested.
                 */
                if (!IS_ENABLED(CONFIG_ARM_BRBE_PMU)) {
                        pr_warn_once("BRBE is disabled, select CONFIG_ARM_BRBE_PMU\n");
                        return -EOPNOTSUPP;
                }


config ARM_BRBE_PMU
        tristate "Enable support for Branch Record Buffer Extension (BRBE)"
        depends on ARM64 && ARM_PMU
        default y
        help
          Enable perf support for Branch Record Buffer Extension (BRBE) which
          records all branches taken in an execution path. This supports some
          branch types and privilege based filtering. It captured additional
          relevant information such as cycle count, misprediction and branch
          type, branch privilege level etc.

> 
> To add to my other comments, this patch is where I would add 
> brbe_read(), etc. to arm_pmu.

Because all new arm_pmu helpers get added and get used in the perf driver
in the same patch, although the actual helper implementation would still
come by bit later via the driver. This also uses updates to pmu_hw_events
struct as well, then that patch needs to be folded here as well.

There is no problem as such, kind of bit subjective. I just feel inclined
to keep the independent infrastructure changes separate making it easy to
review while also creating a flow.