On Thu, Feb 13, 2025 at 11:13:49AM -0600, Rob Herring wrote:

[...]

quoted

quoted

+void brbe_enable(const struct arm_pmu *arm_pmu)
+{
+       struct pmu_hw_events *cpuc = this_cpu_ptr(arm_pmu->hw_events);
+       u64 brbfcr = 0, brbcr = 0;
+
+       /*
+        * Merge the permitted branch filters of all events.
+        */
+       for (int i = 0; i < ARMPMU_MAX_HWEVENTS; i++) {
+               struct perf_event *event = cpuc->events[i];
+
+               if (event && has_branch_stack(event)) {
+                       brbfcr |= event->hw.branch_reg.config;
+                       brbcr |= event->hw.extra_reg.config;
+               }
+       }
+
+       /*
+        * If the record buffer contains any branches, we've already read them
+        * out and don't want to read them again.
+        * No need to sync as we're already stopped.
+        */
+       brbe_invalidate_nosync();
+       isb(); // Make sure invalidate takes effect before enabling
+
+       /*
+        * In VHE mode with MDCR_EL2.HPMN set to PMCR_EL0.N, the counters are
+        * controlled by BRBCR_EL1 rather than BRBCR_EL2 (which writes to
+        * BRBCR_EL1 are redirected to). Use the same value for both register
+        * except keep EL1 and EL0 recording disabled in guests.
+        */
+       if (is_kernel_in_hyp_mode())
+               write_sysreg_s(brbcr & ~(BRBCR_ELx_ExBRE | BRBCR_ELx_E0BRE), SYS_BRBCR_EL12);
+       write_sysreg_s(brbcr, SYS_BRBCR_EL1);
+       isb(); // Ensure BRBCR_ELx settings take effect before unpausing
+
+       write_sysreg_s(brbfcr, SYS_BRBFCR_EL1);

Seems to me, it is weird that first enable recording (BRBCR), then set
control register BRBFCR.  And the writing SYS_BRBFCR_EL1 not guarded
by a barrier is also a bit concerned.

We are always disabled (paused) when we enter brbe_enable(). So the
last thing we do is unpause. The only ordering we care about after
writing SYS_BRBFCR_EL1 is writing PMCR which has an isb before it is
written.

Maybe it is good to add a comment to record the info.

Thanks,
Leo