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

[James Clark <james.clark@linaro.org>]

On 05/08/2026 16:44, Puranjay Mohan wrote:
> On Wed, Aug 5, 2026 at 4:14 PM James Clark <james.clark@linaro.org> wrote:
>>
>>
>>
>> On 05/08/2026 15:43, Puranjay Mohan wrote:
>>> On Wed, Aug 5, 2026 at 2:58 PM James Clark <james.clark@linaro.org> wrote:
>>>>
>>>>
>>>>
>>>> On 05/08/2026 12:47, Puranjay Mohan wrote:
>>>>> On Wed, Aug 5, 2026 at 11:07 AM James Clark <james.clark@linaro.org> wrote:
>>>>>>
>>>>>>
>>>>>>
>>>>>> On 03/08/2026 7:54 pm, Puranjay Mohan wrote:
>>>>>>> On Mon, Aug 3, 2026 at 12:07 PM James Clark <james.clark@linaro.org> 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 <puranjay@kernel.org>
>>>>>>>>> Reviewed-by: Rob Herring (Arm) <robh@kernel.org>
>>>>>>>>> ---
>>>>>>>>>       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.
>>>>>
>>>>> RBHYTD gives a freeze two effects: PAUSED is set, and BRBTS_EL1 captures a
>>>>> timestamp. Recording has already stopped because we paused, and the driver never
>>>>> reads BRBTS_EL1. On the way out we check PMOVSCLR_EL0 and leave PAUSED set if a
>>>>> counter overflowed, so a pending overflow handler still finds a frozen buffer.
>>>>>
>>>>>> So it takes branches away from PMU events.
>>>>>
>>>>> Yes, but that is brbe_invalidate(), not the pause. Interrupts are masked and
>>>>> nothing else runs on the CPU, so the only branches the pause suppresses are the
>>>>> snapshot's own.
>>>>>
>>>>>>>
>>>>>>> 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. */
>>>>>
>>>>> MRS is not a branch, so that works, but three things would have to change:
>>>>>
>>>>> 1. perf_entry_from_brbe_regset() goes through BRBE_REGN_SWITCH, a 32 case
>>>>>       switch, because the register number has to be an immediate. gcc emits a jump
>>>>>       table and the function has 52 branches in the object file. The read would
>>>>>       need full unrolling.
>>>>>
>>>>
>>>> Yes you would have to manually unroll it. I doubt the compiler would
>>>> emit a branch for BRBE_REGN_SWITCH() because your indexes are static if
>>>> it's unrolled. But if it does you can change it to a sequence of
>>>> read_sysreg_s()s.
>>>>
>>>> If you really want to be sure there are no branches, write it in a
>>>> single asm block.
>>>
>>> I will try that approach in the next version.
>>>
>>>>> 2. RPGDLX needs an ISB before the reads, and Table D19-10 makes it
>>>>>       IMPLEMENTATION DEFINED whether ISB itself generates a record. When we pause,
>>>>>       IZCHRF means the pausing ISB cannot pollute the buffer it is about to read.
>>>>>
>>>>
>>>> I assume one potential extra record from the isb() is acceptible seeing
>>>> as you already have two or more conditions plus a function call before
>>>> the pause? Is the problem that you don't know whether to filter it out
>>>> later because it's IMPDEF and isn't always there? You already don't know
>>>> exactly how many branches there will be before the pause beause it's
>>>> written in C. So I'm not sure what the exact issue here is.
>>>>
>>>>> 3. 64 record parts still need a bank switch, so BRBFCR_EL1 still has to be
>>>>>       written and restored.
>>>>>
>>>>
>>>> Yes that was included in my pseudo code example but it's still
>>>> branchless. I did miss that you might need an isb() after disabling
>>>> interrupts, but you added one for RPGDLX anyway.
>>>>
>>>>> Correctness would then depend on the read staying branchless, which is not
>>>>> checkable at build time and fails silently: a branch mid read shifts the buffer,
>>>>
>>>> Why would the compiler insert a branch between two read_sysreg_s()s,
>>>> which are asm volatile? Is that allowed?
>>>
>>> You are right, I just over complicated it!
>>>
>>>>> giving a duplicate and a gap. 64 * 24 bytes of records also wants a per-CPU
>>>>> scratch buffer rather than the stack.
>>>>>
>>>>
>>>> A per-CPU scratch buffer doesn't sound too bad. But aren't you in
>>>> control of how many entries are available to write to? You can reject
>>>> any calls that have fewer than 64 and always write directly to *entries.
>>>>
>>>> You could also compare with 'cnt' after reading each record and exit the
>>>> read section. Like you say below, branches not taken don't generate
>>>> records, and once the branch is taken you stop reading so after that
>>>> point generating records doesn't matter.
>>>>
>>>>> Happy to prototype it. What I would not do is pause without invalidating:
>>>>> records are from/to pairs, so a consumer walking across the hole reconstructs a
>>>>> call path that never happened. The invalidate was Mark's request after the RFC,
>>>>> "to maintain record contiguity for other consumers", so dropping the pause drops
>>>>> that too.
>>>>
>>>> Well the point was to not have to pause at all, so there's no need to
>>>> invalidate either. Even if you did add a pause, as long as there are no
>>>> branches between the pause and resume you don't need an invalidate
>>>> because you didn't miss any branches.
>>>>
>>>>>
>>>>> I was thinking of this for v6:
>>>>>
>>>>>            flags = raw_local_daif_save();
>>>>>
>>>>>            /* The BRBE sysregs below are UNDEFINED without this. */
>>>>>            if (!valid_brbe_version()) {
>>>>
>>>> You don't need to disable interrupts to call this, it's a constant. Or
>>>> is it to stop migration?
>>>
>>> It was to stop migration.
>>>
>>
>> Ah ok, so V5 wasn't correct then.
> 
> Yes, It was buggy! I missed that one CPU could implement BRBE while
> another doesn't
> 
>>
>> Separately to this, I'm also not sure how this BPF call is invoked. Is
>> it supposed to target a CPU, or an event or a process? How do you know
>> you are getting the branches from the CPU that you want?
> 
> So, A BPF program can attach to a function's entry through ftrace or
> kprobe and from there we call this helper to find out what branches
> were taken to reach that function.
> A BPF program runs on a CPU with migration always disabled, so it

In that case you can check for BRBE support before disabling interrupts.

So I think V5 wasn't buggy and you can carry on doing 
valid_brbe_version() at the same place in V6.

> knows which cpu it is getting the branches from. And It can also
> filter for a process as it knows which process is called the BPF
> program, let's say a process does a syscall and we attach a bpf
> program to it as the example I gave with the retsnoop tool in the
> cover letter's Usage model section. A BPF program can also attach to a
> tracepoint, or even to a perf event and this BPF helper can be called
> from any of those contexts, but the filtering based on cpu or pid or
> anything else is done by the BPF program itself, this helper should
> just return the raw branch records.
> 

Thanks for the explanation.

>>>>
>>>> V5 reads sysregs before disabling interrupts so I assume migration isn't
>>>> an issue here.
>>>>
>>>>>                    raw_local_daif_restore(flags);
>>>>>                    return 0;
>>>>>            }
>>>>>
>>>>>            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();
>>>>>
>>>>>            /* BRBCR_EL1 is zero while the driver has BRBE disabled. */
>>>>>            if (!brbcr) {
>>>>>                    write_sysreg_s(brbfcr, SYS_BRBFCR_EL1);
>>>>>                    isb();
>>>>>                    raw_local_daif_restore(flags);
>>>>>                    return 0;
>>>>>            }
>>>>>
>>>>>            trace_hardirqs_off();
>>>>>
>>>>>            ... read the records ...
>>>>>
>>>>>            if (!(brbfcr & BRBFCR_EL1_PAUSED) &&
>>>>>                !(read_pmovsclr() & (ARMV8_PMU_OVSR_P | ARMV8_PMU_OVSR_F)))
>>>>>                    brbe_invalidate();
>>>>>            else
>>>>>                    brbfcr |= BRBFCR_EL1_PAUSED;
>>>>>
>>>>>            write_sysreg_s(brbfcr, SYS_BRBFCR_EL1);
>>>>>            isb();
>>>>>            local_daif_restore(flags);
>>>>>
>>>>> Exceptions are masked before anything is sampled, BRBCR_EL1 is only read and
>>>>> never written, and BRBFCR_EL1 is written back from the value saved under the
>>>>> mask. cpu_has_brbe() is now valid_brbe_version().
>>>>>
>>>>> Neither conditional costs a record: valid_brbe_version() falls through when BRBE
>>>>> is present (RBBNSZ, only taken branches are recorded), and the BRBCR_EL1 test is
>>>>
>>>> Isn't the compiler free to invert it and make the happy path a taken
>>>> branch. I don't think any of that can be assumed without writing in
>>>> assembly.
>>>>
>>>>> after the pause. It cannot move later because BRBFCR_EL1 is UNDEFINED without
>>>>> FEAT_BRBE.
>>>>>
>>>>> PMOVSCLR_EL0 is masked because PMCCNTR_EL0 is bit 31 and PMCR_EL0.N is at most
>>>>> 31, so the cycle counter is outside every range RNXCWF, RGXGWY, RPKTXQ and
>>>>> RLDMVK name. Unmasked, a cycle counter overflow looked like a freeze.
>>>>
>>>> Sorry I didn't understand this bit. Can you elaborate?
>>>>
>>>>>
>>>>> Do you think this version works?
>>>>>
>>>>> Thanks,
>>>>> Puranjay
>>>>
>>>>
>>>>    From your previous reply:
>>>>
>>>>      "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."
>>>>
>>>> I'm still not 100% convinced this is correct. You read BRBFCR_EL1 and
>>>> write PAUSED to it in separate instructions. The PMU can overflow and
>>>> change the PAUSED state between those two instructions leading to
>>>> restoration of the wrong value.
>>>
>>> I was checking if the overflow happened and leaving it paused in that case.
>>>
>>
>> But couldn't it overflow after that check? I still think it's vulnerable
>> to the same issue unless you disable the PMU completely.
> 
> yeah you are right, I missed that it can overflow after the check.
> 
>>
>>>> Isn't this non pausing version way simpler to understand, and also has
>>>> the benefit of not invalidating someone elses BRBE buffers. The only
>>>> downside seems to be that it might have some assembly to make sure the
>>>> if statements are always not taken branches rather than inverted, but
>>>> personally I don't think that makes it any harder to understand than the
>>>> branchy pausing version in V5:
>>>>
>>>>      #define read_record(i)
>>>>        if (i >= cnt)                                       \
>>>>           goto out;                                        \
>>>>        isb(); /* Ensure our own exit branch isn't read? */ \
>>>>        entries[i].inf = read_brbe_inf(i)                   \
>>>>        if (!inf)                                           \
>>>>           goto out;                                        \
>>>>        entries[i].src = read_brb_src(i)                    \
>>>>        entries[i].dst = read_brb_dst(i)                    \
>>>>
>>>>      raw_local_daif_save();
>>>>
>>>>      /* disable counters to stop BRBFCR_EL1.PAUSE state changing */
>>>>      pmcr = armv8pmu_pmcr_read();
>>>>      armv8pmu_pmcr_write(pmcr & ~ARMV8_PMU_PMCR_E);
>>>>      isb();
>>>>
>>>>      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(1);
>>>>      read_record(32);
>>>>      read_record(33);
>>>>      ...
>>>>
>>>> out:
>>>>      write_sysreg_s(brbfcr, SYS_BRBFCR_EL1);
>>>>      isb();
>>>>      armv8pmu_pmcr_write(pmcr);
>>>>      raw_local_daif_restore();
>>>>
>>>>      /* Post process entries[n].inf etc into correct format */
>>>
>>> I will try out this approach and get back.
>>>
>>> Thanks,
>>> Puranjay
>>
>> Nice, hope it can work.
>>