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

On Mon, Aug 3, 2026 at 12:07=E2=80=AFPM 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 snapsho=
t
> > 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 =3D=3D 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 =3D read_sysreg_s(SYS_ID_AA64DFR0_EL1);
> > +     int brbe =3D cpuid_feature_extract_unsigned_field(aa64dfr0, ID_AA=
64DFR0_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) >=3D BRBE_BANK_MAX_ENTRIES &&
> > +         *bank * BRBE_BANK_MAX_ENTRIES + *idx < nr_hw) {
> > +             *idx =3D 0;
> > +             select_brbe_bank(++(*bank));
> > +     }
> > +}
> > +
> > +#define for_each_brbe_entry(idx, nr_hw)                               =
       \
> > +     for (int __bank =3D (select_brbe_bank(0), 0), idx =3D 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 =3D 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 =3D read_sysreg_s(SYS_ID_AA64DFR0_EL1);
> > -     u32 brbe;
> > +     u64 brbidr;
> >
> > -     brbe =3D cpuid_feature_extract_unsigned_field(aa64dfr0, ID_AA64DF=
R0_EL1_BRBE_SHIFT);
> > -     if (!valid_brbe_version(brbe))
> > +     if (!cpu_has_brbe())
> >               return;
> >
> >       brbidr =3D 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 =3D 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 compl=
ete branch records. */
> >               if (!brbe_record_is_target_only(brbinf)) {
> >                       entry->mispred =3D brbinf_get_mispredict(brbinf);
> > @@ -774,32 +795,97 @@ void brbe_read_filtered_entries(struct perf_branc=
h_stack *branch_stack,
> >   {
> >       struct arm_pmu *cpu_pmu =3D to_arm_pmu(event->pmu);
> >       int nr_hw =3D brbe_num_branch_records(cpu_pmu);
> > -     int nr_banks =3D DIV_ROUND_UP(nr_hw, BRBE_BANK_MAX_ENTRIES);
> >       int nr_filtered =3D 0;
> >       u64 branch_sample_type =3D event->attr.branch_sample_type;
> >       DECLARE_BITMAP(event_type_mask, PERF_BR_ARM64_MAX);
> >
> >       prepare_event_branch_type_mask(branch_sample_type, event_type_mas=
k);
> >
> > -     for (int bank =3D 0; bank < nr_banks; bank++) {
> > -             int nr_remaining =3D nr_hw - (bank * BRBE_BANK_MAX_ENTRIE=
S);
> > -             int nr_this_bank =3D min(nr_remaining, BRBE_BANK_MAX_ENTR=
IES);
> > +     for_each_brbe_entry(i, nr_hw) {
> > +             struct perf_branch_entry *pbe =3D &branch_stack->entries[=
nr_filtered];
> >
> > -             select_brbe_bank(bank);
> > +             if (!perf_entry_from_brbe_regset(i, pbe, event))
> > +                     break;
> >
> > -             for (int i =3D 0; i < nr_this_bank; i++) {
> > -                     struct perf_branch_entry *pbe =3D &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 =3D 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 =3D 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 =3D read_sysreg_s(SYS_BRBFCR_EL1);
> > +     brbcr =3D 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 befor=
e
touching any BRBE register, which fixes the stale restore. trace_hardirqs_o=
ff()
then moves below the pause so lockdep still sees a balanced off/on pair whi=
le
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 nee=
ds
BRBE to not already be paused (RNXCWF). So once we have paused, nothing cha=
nges
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 t=
he
records we came to read.

You are right that a freeze can still land in the window between reading BR=
BFCR
and setting PAUSED, and restoring the value we read would then clear a PAUS=
ED
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 =3D raw_local_daif_save();

        brbfcr =3D read_sysreg_s(SYS_BRBFCR_EL1);
        brbcr =3D 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 ...

        if ((brbfcr & BRBFCR_EL1_PAUSED) || read_pmovsclr())
                brbfcr |=3D 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 o=
f
the pause are not contiguous (RPKZCF), so they are discarded.

What do you think about this?

Thanks,
Puranjay