linux-kernel - Re: [PATCH V11 06/10] arm64/perf: Enable branch stack events via FEAT

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: <a04b4e03-39cf-ab47-e2fc-48feebbd1635@arm.com>
Date:   Mon, 5 Jun 2023 08:30:55 +0530
From:   Anshuman Khandual <anshuman.khandual@....com>
To:     Namhyung Kim <namhyung@...nel.org>
Cc:     linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
        will@...nel.org, catalin.marinas@....com, mark.rutland@....com,
        Mark Brown <broonie@...nel.org>,
        James Clark <james.clark@....com>,
        Rob Herring <robh@...nel.org>, Marc Zyngier <maz@...nel.org>,
        Suzuki Poulose <suzuki.poulose@....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: [PATCH V11 06/10] arm64/perf: Enable branch stack events via
 FEAT_BRBE



On 6/2/23 07:15, Namhyung Kim wrote:
> Hello,
> 
> On Tue, May 30, 2023 at 9:21 PM Anshuman Khandual
> <anshuman.khandual@....com> wrote:
>>
>> This enables branch stack sampling events in ARMV8 PMU, via an architecture
>> feature FEAT_BRBE aka branch record buffer extension. This defines required
>> branch helper functions pmuv8pmu_branch_XXXXX() and the implementation here
>> is wrapped with a new config option CONFIG_ARM64_BRBE.
>>
>> Cc: Catalin Marinas <catalin.marinas@....com>
>> Cc: Will Deacon <will@...nel.org>
>> Cc: Mark Rutland <mark.rutland@....com>
>> Cc: linux-arm-kernel@...ts.infradead.org
>> Cc: linux-kernel@...r.kernel.org
>> Tested-by: James Clark <james.clark@....com>
>> Signed-off-by: Anshuman Khandual <anshuman.khandual@....com>
>> ---
> 
> [SNIP]
>> +void armv8pmu_branch_read(struct pmu_hw_events *cpuc, struct perf_event *event)
>> +{
>> +       struct brbe_hw_attr *brbe_attr = (struct brbe_hw_attr *)cpuc->percpu_pmu->private;
>> +       u64 brbfcr, brbcr;
>> +       int idx, loop1_idx1, loop1_idx2, loop2_idx1, loop2_idx2, count;
>> +
>> +       brbcr = read_sysreg_s(SYS_BRBCR_EL1);
>> +       brbfcr = read_sysreg_s(SYS_BRBFCR_EL1);
>> +
>> +       /* Ensure pause on PMU interrupt is enabled */
>> +       WARN_ON_ONCE(!(brbcr & BRBCR_EL1_FZP));
>> +
>> +       /* Pause the buffer */
>> +       write_sysreg_s(brbfcr | BRBFCR_EL1_PAUSED, SYS_BRBFCR_EL1);
>> +       isb();
>> +
>> +       /* Determine the indices for each loop */
>> +       loop1_idx1 = BRBE_BANK0_IDX_MIN;
>> +       if (brbe_attr->brbe_nr <= BRBE_BANK_MAX_ENTRIES) {
>> +               loop1_idx2 = brbe_attr->brbe_nr - 1;
>> +               loop2_idx1 = BRBE_BANK1_IDX_MIN;
>> +               loop2_idx2 = BRBE_BANK0_IDX_MAX;
> 
> Is this to disable the bank1?  Maybe need a comment.

Sure, will add a comment.

> 
> 
>> +       } else {
>> +               loop1_idx2 = BRBE_BANK0_IDX_MAX;
>> +               loop2_idx1 = BRBE_BANK1_IDX_MIN;
>> +               loop2_idx2 = brbe_attr->brbe_nr - 1;
>> +       }
> 
> The loop2_idx1 is the same for both cases.  Maybe better
> to move it out of the if statement.

Sure, will do the following change as suggested but wondering whether should
the change be implemented from this patch onwards or in the later patch that
adds capture_brbe_regset().
 
--- a/drivers/perf/arm_brbe.c
+++ b/drivers/perf/arm_brbe.c
@@ -56,13 +56,14 @@ static int capture_brbe_regset(struct brbe_hw_attr *brbe_attr, struct brbe_regse
        int idx, count;
 
        loop1_idx1 = BRBE_BANK0_IDX_MIN;
+       loop2_idx1 = BRBE_BANK1_IDX_MIN;
        if (brbe_attr->brbe_nr <= BRBE_BANK_MAX_ENTRIES) {
                loop1_idx2 = brbe_attr->brbe_nr - 1;
-               loop2_idx1 = BRBE_BANK1_IDX_MIN;
+
+               /* Disable capturing the bank 1 */
                loop2_idx2 = BRBE_BANK0_IDX_MAX;
        } else {
                loop1_idx2 = BRBE_BANK0_IDX_MAX;
-               loop2_idx1 = BRBE_BANK1_IDX_MIN;
                loop2_idx2 = brbe_attr->brbe_nr - 1;
        }