[PATCH V11 06/10] arm64/perf: Enable branch stack events via FEAT_BRBE

[Anshuman Khandual]

On 6/2/23 07:15, Namhyung Kim wrote:
> Hello,
> 
> On Tue, May 30, 2023 at 9:21 PM Anshuman Khandual
> <anshuman.khandual at arm.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 at arm.com>
>> Cc: Will Deacon <will at kernel.org>
>> Cc: Mark Rutland <mark.rutland at arm.com>
>> Cc: linux-arm-kernel at lists.infradead.org
>> Cc: linux-kernel at vger.kernel.org
>> Tested-by: James Clark <james.clark at arm.com>
>> Signed-off-by: Anshuman Khandual <anshuman.khandual at arm.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;
        }