linux-kernel - Re: [PATCH v19 11/11] perf: arm_pmuv3: Add support for the Branch Record Buffer Extension (BRBE)

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20250213174530.GH235556@e132581.arm.com>
Date: Thu, 13 Feb 2025 17:45:30 +0000
From: Leo Yan <leo.yan@....com>
To: Rob Herring <robh@...nel.org>
Cc: Will Deacon <will@...nel.org>, Mark Rutland <mark.rutland@....com>,
	Catalin Marinas <catalin.marinas@....com>,
	Jonathan Corbet <corbet@....net>, Marc Zyngier <maz@...nel.org>,
	Oliver Upton <oliver.upton@...ux.dev>,
	Joey Gouly <joey.gouly@....com>,
	Suzuki K Poulose <suzuki.poulose@....com>,
	Zenghui Yu <yuzenghui@...wei.com>,
	James Clark <james.clark@...aro.org>,
	Anshuman Khandual <anshuman.khandual@....com>,
	linux-arm-kernel@...ts.infradead.org,
	linux-perf-users@...r.kernel.org, linux-kernel@...r.kernel.org,
	linux-doc@...r.kernel.org, kvmarm@...ts.linux.dev
Subject: Re: [PATCH v19 11/11] perf: arm_pmuv3: Add support for the Branch
 Record Buffer Extension (BRBE)

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

[...]

> > > +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