| 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
| ||
|
Date: Tue, 21 Nov 2023 16:17:08 +0530
From: Anshuman Khandual <anshuman.khandual@....com>
To: James Clark <james.clark@....com>
Cc: Mark Brown <broonie@...nel.org>, Rob Herring <robh@...nel.org>,
Marc Zyngier <maz@...nel.org>,
Suzuki Poulose <suzuki.poulose@....com>,
Peter Zijlstra <peterz@...radead.org>,
Ingo Molnar <mingo@...hat.com>,
Arnaldo Carvalho de Melo <acme@...nel.org>,
linux-perf-users@...r.kernel.org,
linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
will@...nel.org, catalin.marinas@....com, mark.rutland@....com
Subject: Re: [V14 4/8] drivers: perf: arm_pmuv3: Enable branch stack sampling
via FEAT_BRBE
On 11/14/23 17:41, James Clark wrote:
>
>
> On 14/11/2023 05:13, Anshuman Khandual wrote:
> [...]
>
>> +/*
>> + * BRBE supports the following functional branch type filters while
>> + * generating branch records. These branch filters can be enabled,
>> + * either individually or as a group i.e ORing multiple filters
>> + * with each other.
>> + *
>> + * BRBFCR_EL1_CONDDIR - Conditional direct branch
>> + * BRBFCR_EL1_DIRCALL - Direct call
>> + * BRBFCR_EL1_INDCALL - Indirect call
>> + * BRBFCR_EL1_INDIRECT - Indirect branch
>> + * BRBFCR_EL1_DIRECT - Direct branch
>> + * BRBFCR_EL1_RTN - Subroutine return
>> + */
>> +static u64 branch_type_to_brbfcr(int branch_type)
>> +{
>> + u64 brbfcr = 0;
>> +
>> + if (branch_type & PERF_SAMPLE_BRANCH_ANY) {
>> + brbfcr |= BRBFCR_EL1_BRANCH_FILTERS;
>> + return brbfcr;
>> + }
>> +
>> + if (branch_type & PERF_SAMPLE_BRANCH_ANY_CALL) {
>> + brbfcr |= BRBFCR_EL1_INDCALL;
>> + brbfcr |= BRBFCR_EL1_DIRCALL;
>> + }
>> +
>> + if (branch_type & PERF_SAMPLE_BRANCH_ANY_RETURN)
>> + brbfcr |= BRBFCR_EL1_RTN;
>> +
>> + if (branch_type & PERF_SAMPLE_BRANCH_IND_CALL)
>> + brbfcr |= BRBFCR_EL1_INDCALL;
>> +
>> + if (branch_type & PERF_SAMPLE_BRANCH_COND)
>> + brbfcr |= BRBFCR_EL1_CONDDIR;
>> +
>> + if (branch_type & PERF_SAMPLE_BRANCH_IND_JUMP)
>> + brbfcr |= BRBFCR_EL1_INDIRECT;
>> +
>> + if (branch_type & PERF_SAMPLE_BRANCH_CALL)
>> + brbfcr |= BRBFCR_EL1_DIRCALL;
>> +
>> + return brbfcr;
>> +}
>> +
>> +/*
>> + * BRBE supports the following privilege mode filters while generating
>> + * branch records.
>> + *
>> + * BRBCR_ELx_E0BRE - EL0 branch records
>> + * BRBCR_ELx_ExBRE - EL1/EL2 branch records
>> + *
>> + * BRBE also supports the following additional functional branch type
>> + * filters while generating branch records.
>> + *
>> + * BRBCR_ELx_EXCEPTION - Exception
>> + * BRBCR_ELx_ERTN - Exception return
>> + */
>> +static u64 branch_type_to_brbcr(int branch_type)
>> +{
>> + u64 brbcr = BRBCR_ELx_DEFAULT_TS;
>> +
>> + /*
>> + * BRBE should be paused on PMU interrupt while tracing kernel
>> + * space to stop capturing further branch records. Otherwise
>> + * interrupt handler branch records might get into the samples
>> + * which is not desired.
>> + *
>> + * BRBE need not be paused on PMU interrupt while tracing only
>> + * the user space, because it will automatically be inside the
>> + * prohibited region. But even after PMU overflow occurs, the
>> + * interrupt could still take much more cycles, before it can
>> + * be taken and by that time BRBE will have been overwritten.
>> + * Hence enable pause on PMU interrupt mechanism even for user
>> + * only traces as well.
>> + */
>> + brbcr |= BRBCR_ELx_FZP;
>> +
>> + if (branch_type & PERF_SAMPLE_BRANCH_USER)
>> + brbcr |= BRBCR_ELx_E0BRE;
>> +
>> + /*
>> + * When running in the hyp mode, writing into BRBCR_EL1
>> + * actually writes into BRBCR_EL2 instead. Field E2BRE
>> + * is also at the same position as E1BRE.
>> + */
>> + if (branch_type & PERF_SAMPLE_BRANCH_KERNEL)
>> + brbcr |= BRBCR_ELx_ExBRE;
>> +
>> + if (branch_type & PERF_SAMPLE_BRANCH_HV) {
>> + if (is_kernel_in_hyp_mode())
>> + brbcr |= BRBCR_ELx_ExBRE;
>> + }
>> +
>> + if (!(branch_type & PERF_SAMPLE_BRANCH_NO_CYCLES))
>> + brbcr |= BRBCR_ELx_CC;
>> +
>> + if (!(branch_type & PERF_SAMPLE_BRANCH_NO_FLAGS))
>> + brbcr |= BRBCR_ELx_MPRED;
>> +
>> + /*
>> + * The exception and exception return branches could be
>> + * captured, irrespective of the perf event's privilege.
>> + * If the perf event does not have enough privilege for
>> + * a given exception level, then addresses which falls
>> + * under that exception level will be reported as zero
>> + * for the captured branch record, creating source only
>> + * or target only records.
>> + */
>> + if (branch_type & PERF_SAMPLE_BRANCH_ANY) {
>> + brbcr |= BRBCR_ELx_EXCEPTION;
>> + brbcr |= BRBCR_ELx_ERTN;
>> + }
>> +
>> + if (branch_type & PERF_SAMPLE_BRANCH_ANY_CALL)
>> + brbcr |= BRBCR_ELx_EXCEPTION;
>> +
>> + if (branch_type & PERF_SAMPLE_BRANCH_ANY_RETURN)
>> + brbcr |= BRBCR_ELx_ERTN;
>> +
>> + return brbcr & BRBCR_ELx_CONFIG_MASK;
>> +}
>> +
>> +void armv8pmu_branch_enable(struct arm_pmu *arm_pmu)
>> +{
>> + struct pmu_hw_events *cpuc = this_cpu_ptr(arm_pmu->hw_events);
>> + u64 brbfcr, brbcr;
>> +
>> + if (!(cpuc->brbe_sample_type && cpuc->brbe_users))
>> + return;
>> +
>> + /*
>> + * BRBE gets configured with a new mismatched branch sample
>> + * type request, overriding any previous branch filters.
>> + */
>> + brbfcr = read_sysreg_s(SYS_BRBFCR_EL1);
>> + brbfcr &= ~BRBFCR_EL1_DEFAULT_CONFIG;
>
> This is called default_config, but is being used semantically the same
> way as BRBCR_ELx_CONFIG_MASK below to clear out the fields. Doesn't that
> mean that it's a mask rather than a default config? It's only ever used
> in this way. default_config implies it's written or used as an
> initialiser at some point.
Sure, will rename BRBFCR_EL1_DEFAULT_CONFIG as BRBFCR_EL1_CONFIG_MASK
making it similar to BRBCR_ELx_CONFIG_MASK.
>
>> + brbfcr |= branch_type_to_brbfcr(cpuc->brbe_sample_type);
>> + write_sysreg_s(brbfcr, SYS_BRBFCR_EL1);
>> + isb();
>> +
>> + brbcr = read_sysreg_s(SYS_BRBCR_EL1);
>> + brbcr &= ~BRBCR_ELx_CONFIG_MASK;
>> + brbcr |= branch_type_to_brbcr(cpuc->brbe_sample_type);
>
> BRBCR_ELx_CONFIG_MASK is already &'d at the end of
> branch_type_to_brbcr(), so isn't it easier and equivalent to just do the
> following instead of the read(), &= and then |= ?
>
> write_sysreg_s(branch_type_to_brbcr(...), SYS_BRBCR_EL1);
>
> Or at least make branch_type_to_brbfcr() consistent and &
> BRBFCR_EL1_DEFAULT_CONFIG at the end of that function too.
This sounds better I guess, will '&' BRBFCR_EL1_CONFIG_MASK at the end
of branch_type_to_brbfcr().
>
>> + write_sysreg_s(brbcr, SYS_BRBCR_EL1);
>> + isb();
>> +}
>> +
Powered by blists - more mailing lists