Re: [PATCH v5 3/4] perf/arm64: Add BRBE support for bpf_get_branch_snapshot()

On 03/08/2026 7:54 pm, Puranjay Mohan wrote:
> On Mon, Aug 3, 2026 at 12:07 PM James Clark <[email protected]> wrote:
>>
>>
>>
>> On 16/06/2026 16:57, Puranjay Mohan wrote:
>>> Enable bpf_get_branch_snapshot() on ARM64 by implementing the
>>> perf_snapshot_branch_stack static call for BRBE.
>>>
>>> BRBE is paused before masking exceptions to avoid branch buffer
>>> pollution from trace_hardirqs_off(). Exceptions are then masked with
>>> local_daif_save() to prevent PMU overflow pseudo-NMIs from interfering.
>>> If an overflow between pause and DAIF save re-enables BRBE, the snapshot
>>> detects this via BRBFCR_EL1.PAUSED and bails out.
>>>
>>> Branch records are read using perf_entry_from_brbe_regset() with a NULL
>>> event pointer to bypass event-specific filtering. The buffer is
>>> invalidated after reading.
>>>
>>> Introduce a for_each_brbe_entry() iterator to deduplicate bank
>>> iteration between brbe_read_filtered_entries() and the snapshot.
>>>
>>> Signed-off-by: Puranjay Mohan <[email protected]>
>>> Reviewed-by: Rob Herring (Arm) <[email protected]>
>>> ---
>>>    drivers/perf/arm_brbe.c  | 128 ++++++++++++++++++++++++++++++++-------
>>>    drivers/perf/arm_brbe.h  |   9 +++
>>>    drivers/perf/arm_pmuv3.c |   5 +-
>>>    3 files changed, 120 insertions(+), 22 deletions(-)
>>>
>>> diff --git a/drivers/perf/arm_brbe.c b/drivers/perf/arm_brbe.c
>>> index effbdeacfcbb..a141ad7abcf2 100644
>>> --- a/drivers/perf/arm_brbe.c
>>> +++ b/drivers/perf/arm_brbe.c
>>> @@ -9,6 +9,7 @@
>>>    #include <linux/types.h>
>>>    #include <linux/bitmap.h>
>>>    #include <linux/perf/arm_pmu.h>
>>> +#include <asm/daifflags.h>
>>>    #include "arm_brbe.h"
>>>
>>>    #define BRBFCR_EL1_BRANCH_FILTERS (BRBFCR_EL1_DIRECT   | \
>>> @@ -256,6 +257,14 @@ static bool valid_brbe_version(int brbe_version)
>>>               brbe_version == ID_AA64DFR0_EL1_BRBE_BRBE_V1P1;
>>>    }
>>>
>>> +static __always_inline bool cpu_has_brbe(void)
>>
>> This should be more like cpu_valid_brbe_version(). has_brbe() only
>> implies that the CPU has BRBE, not that it's a version that the driver
>> supports. And it's actually just a wrapper around valid_brbe_version()
>> that accesses the ID reg on that CPU, not a functionally different check.
>>
>> But it also looks like valid_brbe_version() isn't called from anywhere
>> else, so why not delete that function and use its name for the new one?
> 
> I will do that in next version
> 
>>
>>> +{
>>> +     u64 aa64dfr0 = read_sysreg_s(SYS_ID_AA64DFR0_EL1);
>>> +     int brbe = cpuid_feature_extract_unsigned_field(aa64dfr0, ID_AA64DFR0_EL1_BRBE_SHIFT);
>>> +
>>> +     return valid_brbe_version(brbe);
>>> +}
>>> +
>>>    static void select_brbe_bank(int bank)
>>>    {
>>>        u64 brbfcr;
>>> @@ -271,6 +280,20 @@ static void select_brbe_bank(int bank)
>>>        isb();
>>>    }
>>>
>>> +static inline void __brbe_advance(int *bank, int *idx, int nr_hw)
>>> +{
>>> +     if (++(*idx) >= BRBE_BANK_MAX_ENTRIES &&
>>> +         *bank * BRBE_BANK_MAX_ENTRIES + *idx < nr_hw) {
>>> +             *idx = 0;
>>> +             select_brbe_bank(++(*bank));
>>> +     }
>>> +}
>>> +
>>> +#define for_each_brbe_entry(idx, nr_hw)                                      \
>>> +     for (int __bank = (select_brbe_bank(0), 0), idx = 0;            \
>>> +          __bank * BRBE_BANK_MAX_ENTRIES + idx < (nr_hw);            \
>>> +          __brbe_advance(&__bank, &idx, (nr_hw)))
>>> +
>>>    static bool __read_brbe_regset(struct brbe_regset *entry, int idx)
>>>    {
>>>        entry->brbinf = get_brbinf_reg(idx);
>>> @@ -474,11 +497,9 @@ unsigned int brbe_num_branch_records(const struct arm_pmu *armpmu)
>>>
>>>    void brbe_probe(struct arm_pmu *armpmu)
>>>    {
>>> -     u64 brbidr, aa64dfr0 = read_sysreg_s(SYS_ID_AA64DFR0_EL1);
>>> -     u32 brbe;
>>> +     u64 brbidr;
>>>
>>> -     brbe = cpuid_feature_extract_unsigned_field(aa64dfr0, ID_AA64DFR0_EL1_BRBE_SHIFT);
>>> -     if (!valid_brbe_version(brbe))
>>> +     if (!cpu_has_brbe())
>>>                return;
>>>
>>>        brbidr = read_sysreg_s(SYS_BRBIDR0_EL1);
>>> @@ -618,10 +639,10 @@ static bool perf_entry_from_brbe_regset(int index, struct perf_branch_entry *ent
>>>
>>>        brbe_set_perf_entry_type(entry, brbinf);
>>>
>>> -     if (!branch_sample_no_cycles(event))
>>> +     if (!event || !branch_sample_no_cycles(event))
>>>                entry->cycles = brbinf_get_cycles(brbinf);
>>>
>>> -     if (!branch_sample_no_flags(event)) {
>>> +     if (!event || !branch_sample_no_flags(event)) {
>>>                /* Mispredict info is available for source only and complete branch records. */
>>>                if (!brbe_record_is_target_only(brbinf)) {
>>>                        entry->mispred = brbinf_get_mispredict(brbinf);
>>> @@ -774,32 +795,97 @@ void brbe_read_filtered_entries(struct perf_branch_stack *branch_stack,
>>>    {
>>>        struct arm_pmu *cpu_pmu = to_arm_pmu(event->pmu);
>>>        int nr_hw = brbe_num_branch_records(cpu_pmu);
>>> -     int nr_banks = DIV_ROUND_UP(nr_hw, BRBE_BANK_MAX_ENTRIES);
>>>        int nr_filtered = 0;
>>>        u64 branch_sample_type = event->attr.branch_sample_type;
>>>        DECLARE_BITMAP(event_type_mask, PERF_BR_ARM64_MAX);
>>>
>>>        prepare_event_branch_type_mask(branch_sample_type, event_type_mask);
>>>
>>> -     for (int bank = 0; bank < nr_banks; bank++) {
>>> -             int nr_remaining = nr_hw - (bank * BRBE_BANK_MAX_ENTRIES);
>>> -             int nr_this_bank = min(nr_remaining, BRBE_BANK_MAX_ENTRIES);
>>> +     for_each_brbe_entry(i, nr_hw) {
>>> +             struct perf_branch_entry *pbe = &branch_stack->entries[nr_filtered];
>>>
>>> -             select_brbe_bank(bank);
>>> +             if (!perf_entry_from_brbe_regset(i, pbe, event))
>>> +                     break;
>>>
>>> -             for (int i = 0; i < nr_this_bank; i++) {
>>> -                     struct perf_branch_entry *pbe = &branch_stack->entries[nr_filtered];
>>> +             if (!filter_branch_record(pbe, branch_sample_type, event_type_mask))
>>> +                     continue;
>>>
>>> -                     if (!perf_entry_from_brbe_regset(i, pbe, event))
>>> -                             goto done;
>>> +             nr_filtered++;
>>> +     }
>>>
>>> -                     if (!filter_branch_record(pbe, branch_sample_type, event_type_mask))
>>> -                             continue;
>>> +     branch_stack->nr = nr_filtered;
>>> +}
>>>
>>> -                     nr_filtered++;
>>> -             }
>>> +/*
>>> + * Best-effort BRBE snapshot for BPF tracing. Pause BRBE to avoid
>>> + * self-recording and return 0 if the snapshot state appears disturbed.
>>> + */
>>> +int arm_brbe_snapshot_branch_stack(struct perf_branch_entry *entries, unsigned int cnt)
>>> +{
>>> +     unsigned long flags;
>>> +     int nr_hw, nr_copied = 0;
>>> +     u64 brbfcr, brbcr;
>>> +
>>> +     if (!cnt)
>>> +             return 0;
>>
>> If you're trying to avoid branches before pausing BRBE, can't you check
>> this after the pause?
> 
> will drop this in the next version and this is just an optimization.
> 
>>
>>> +
>>> +     /* Guard against running on a CPU without BRBE (e.g. big.LITTLE). */
>>> +     if (!cpu_has_brbe())
>>> +             return 0;
>>> +
>>> +     /*
>>> +      * Pause BRBE first to avoid recording our own branches. The
>>> +      * sysreg read/write and ISB are branchless, so pausing before
>>> +      * checking BRBCR avoids polluting the buffer with our own
>>> +      * conditional branches.
>>> +      */
>>> +     brbfcr = read_sysreg_s(SYS_BRBFCR_EL1);
>>> +     brbcr = read_sysreg_s(SYS_BRBCR_EL1);
>>> +     write_sysreg_s(brbfcr | BRBFCR_EL1_PAUSED, SYS_BRBFCR_EL1);
>>
>> Can this work without first disabling interrupts? Sashiko pointed it
>> out, but I think it's correct. If you read an active state into brbfcr,
>> then the PMU event fires and disables BRBE, then you disable interrupts,
>> then you would restore an active state when it should be inactive.
>> Surely the only way to do it properly is to disable interrupts before
>> touching anything at all, if the PMU handler is also touching the same
>> registers?
>>
>> If you want to avoid trace_hardirqs_off() can you make a new
>> raw_local_daif_save() that disables interrupts and then call
>> trace_hardirqs_off() yourself after pausing BRBE? Or not call
>> trace_hardirqs_off() at all? There is a comment mentioning something
>> like that in arch/arm64/kernel/suspend.c.
> 
> Yes. I'll add raw_local_daif_save()/raw_local_daif_restore() and mask before
> touching any BRBE register, which fixes the stale restore. trace_hardirqs_off()
> then moves below the pause so lockdep still sees a balanced off/on pair while
> its branches land in an already paused buffer.
> 
>>
>> Also, disabling interrupts doesn't stop the PMU event from overflowing
>> and changing the state of PAUSED either. I think this is another path
>> that leads to you restoring the wrong state, so don't you also need to
>> disable the PMU?
> 
> Hardware only sets PAUSED on a BRBE freeze event (RBHYTD), and a freeze needs
> BRBE to not already be paused (RNXCWF). So once we have paused, nothing changes
> underneath us and the PMU does not need disabling. It would not help anyway:
> armv8pmu_stop() calls brbe_disable(), which zeroes BRBCR_EL1 and discards the
> records we came to read.

That does mean you throw away real freeze events while paused though, in 
addition to the brbe_invalidate() you need to avoid non contiguous 
buffers. So it takes branches away from PMU events.

> 
> You are right that a freeze can still land in the window between reading BRBFCR
> and setting PAUSED, and restoring the value we read would then clear a PAUSED
> bit the hardware set. So v6 checks PMOVSCLR_EL0 and leaves BRBE paused if a
> counter has overflowed. Reads of PMOVSCLR are non-destructive and it stays set
> until the overflow handler clears it, so it is still visible after we have set
> PAUSED ourselves:
> 
>          if (!valid_brbe_version())
>                  return 0;
> 
>          flags = raw_local_daif_save();
> 
>          brbfcr = read_sysreg_s(SYS_BRBFCR_EL1);
>          brbcr = read_sysreg_s(SYS_BRBCR_EL1);
> 
>          write_sysreg_s(brbfcr | BRBFCR_EL1_PAUSED, SYS_BRBFCR_EL1);
>          isb();
> 
>          trace_hardirqs_off();
> 
>          /* BRBCR_EL1 is zero while the driver has BRBE disabled. */
>          if (!brbcr)
>                  goto restore;
> 
>          ... read the records ...
> 

Up to the point where you read the records you technically don't need 
any branches (if you don't do trace_hardirqs_off()) so you could do the 
whole thing without pausing. Just disable interrupts, read every branch 
entry unconditionally, re-enable interrupts and then find the last valid 
entry and do the branchy stuff after reading.

I'm thinking out loud, but doesn't that make it a lot easier? And it 
avoids the brbe_invalidate() which takes the branches away from the PMU 
event. It also avoids having to think too hard about racing with PMU 
events causing a freeze even after interrupts are disabled and after 
reading the freeze value:

   raw_local_daif_save();
   brbfcr = read_sysreg_s(SYS_BRBFCR_EL1);
   brbcr = read_sysreg_s(SYS_BRBCR_EL1);
   select_bank(0);
   read_record(0);
   read_record(1);
   ...
   select_bank(0);
   read_record(0);
   read_record(1);
   ...
   write_sysreg_s(brbfcr, SYS_BRBFCR_EL1);
   isb();
   local_daif_restore();

   /* Now do post processing, find last valid record, check if it was 
enabled by looking at brbcr etc. */

>          if ((brbfcr & BRBFCR_EL1_PAUSED) || read_pmovsclr())
>                  brbfcr |= BRBFCR_EL1_PAUSED;
>          else
>                  brbe_invalidate();
> restore:
>          write_sysreg_s(brbfcr, SYS_BRBFCR_EL1);
>          isb();
>          local_daif_restore(flags);
> 
> If BRBE stays paused the records are left alone, they belong to the pending
> overflow handler. Otherwise recording resumes and the records either side of
> the pause are not contiguous (RPKZCF), so they are discarded.
> 
> What do you think about this?
> 
> Thanks,
> Puranjay