linux-kernel - Re: [PATCH v2 5/7] ARM: perf_event: Fully support Krait CPU PMU events

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: <20140121180711.GN30706@mudshark.cambridge.arm.com>
Date:	Tue, 21 Jan 2014 18:07:11 +0000
From:	Will Deacon <will.deacon@....com>
To:	Stephen Boyd <sboyd@...eaurora.org>
Cc:	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"linux-arm-msm@...r.kernel.org" <linux-arm-msm@...r.kernel.org>,
	"linux-arm-kernel@...ts.infradead.org" 
	<linux-arm-kernel@...ts.infradead.org>,
	Neil Leeder <nleeder@...eaurora.org>,
	Ashwin Chaugule <ashwinc@...eaurora.org>
Subject: Re: [PATCH v2 5/7] ARM: perf_event: Fully support Krait CPU PMU
 events

Hi Stephen,

Thanks for the updates. A few more comments inline.

On Wed, Jan 15, 2014 at 05:55:33PM +0000, Stephen Boyd wrote:
> Krait supports a set of performance monitor region event
> selection registers (PMRESR) sitting behind a cp15 based
> interface that extend the architected PMU events to include Krait
> CPU and Venum VFP specific events. To use these events the user
> is expected to program the region register (PMRESRn) with the
> event code shifted into the group they care about and then point
> the PMNx event at that region+group combo by writing a
> PMRESRn_GROUPx event. Add support for this hardware.
> 
> Note: the raw event number is a pure software construct that
> allows us to map the multi-dimensional number space of regions,
> groups, and event codes into a flat event number space suitable
> for use by the perf framework.

[...]

> +static u32 krait_read_pmresrn(int n)
> +{
> +       u32 val;
> +
> +       switch (n) {
> +       case 0:
> +               asm volatile("mrc p15, 1, %0, c9, c15, 0" : "=r" (val));
> +               break;
> +       case 1:
> +               asm volatile("mrc p15, 1, %0, c9, c15, 1" : "=r" (val));
> +               break;
> +       case 2:
> +               asm volatile("mrc p15, 1, %0, c9, c15, 2" : "=r" (val));
> +               break;
> +       default:
> +               BUG(); /* Should be validated in krait_pmu_get_event_idx() */
> +       }
> +
> +       return val;
> +}
> +
> +static void krait_write_pmresrn(int n, u32 val)
> +{
> +       switch (n) {
> +       case 0:
> +               asm volatile("mcr p15, 1, %0, c9, c15, 0" : : "r" (val));
> +               break;
> +       case 1:
> +               asm volatile("mcr p15, 1, %0, c9, c15, 1" : : "r" (val));
> +               break;
> +       case 2:
> +               asm volatile("mcr p15, 1, %0, c9, c15, 2" : : "r" (val));
> +               break;
> +       default:
> +               BUG(); /* Should be validated in krait_pmu_get_event_idx() */
> +       }
> +}

Do you need isbs to ensure the pmresrn side-effects have happened, or are
the registers self-synchronising? Similarly for your other IMP DEF
registers.

> +static void krait_pre_vpmresr0(u32 *venum_orig_val, u32 *fp_orig_val)
> +{
> +       u32 venum_new_val;
> +       u32 fp_new_val;
> +
> +       /* CPACR Enable CP10 and CP11 access */
> +       *venum_orig_val = get_copro_access();
> +       venum_new_val = *venum_orig_val | CPACC_SVC(10) | CPACC_SVC(11);
> +       set_copro_access(venum_new_val);
> +
> +       /* Enable FPEXC */
> +       *fp_orig_val = fmrx(FPEXC);
> +       fp_new_val = *fp_orig_val | FPEXC_EN;
> +       fmxr(FPEXC, fp_new_val);

Messing around with the lot (especially with kernel-mode neon now in
mainline) does scare me. I'd like some BUG_ON(preemptible()) and you could
consider using kernel_neon_{begin,end} but they're a lot heavier than you
need (due to non-lazy switching)

Finally, I'd really like to see this get some test coverage, but I don't
want to try running mainline on my phone :) Could you give your patches a
spin with Vince's perf fuzzer please?

  https://github.com/deater/perf_event_tests.git

(then build the contents of the fuzzer directory and run it for as long as
you can).

Cheers,

Will
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/