| 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, 20 Jan 2015 09:35:54 -0800
From: Joshua Clayton <joshua.clayton@...west.com>
To: linux-arm-kernel@...ts.infradead.org
Cc: Daniel Thompson <daniel.thompson@...aro.org>,
linaro-kernel@...ts.linaro.org,
Russell King <linux@....linux.org.uk>,
Jason Cooper <jason@...edaemon.net>, patches@...aro.org,
Stephen Boyd <sboyd@...eaurora.org>,
linux-kernel@...r.kernel.org, Daniel Drake <drake@...lessm.com>,
Dirk Behme <dirk.behme@...bosch.com>,
John Stultz <john.stultz@...aro.org>,
Tim Sander <tim@...eglstein.org>,
Thomas Gleixner <tglx@...utronix.de>,
Sumit Semwal <sumit.semwal@...aro.org>,
Dmitry Pervushin <dpervushin@...il.com>
Subject: Re: [RFC PATCH 5/5] arm: perf: Use FIQ to handle PMU events.
On Tuesday, January 20, 2015 10:18:10 AM Daniel Thompson wrote:
> On 19/01/15 16:35, Joshua Clayton wrote:
> > On Tuesday, January 13, 2015 04:35:31 PM Daniel Thompson wrote:
> >> Using FIQ (if it is available) gives perf a better insight into the
> >> system by allowing code run with interrupts disabled to be profiled.
> >>
> >> Signed-off-by: Daniel Thompson <daniel.thompson@...aro.org>
> >> ---
> >>
> >> arch/arm/include/asm/pmu.h | 4 ++++
> >> arch/arm/kernel/perf_event.c | 2 +-
> >> arch/arm/kernel/perf_event_cpu.c | 35
> >> ++++++++++++++++++++++++++++++++---
> >> arch/arm/kernel/traps.c | 3 ++-
> >> 4 files changed, 39 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/arch/arm/include/asm/pmu.h b/arch/arm/include/asm/pmu.h
> >> index b1596bd59129..2a7ea97a4a14 100644
> >> --- a/arch/arm/include/asm/pmu.h
> >> +++ b/arch/arm/include/asm/pmu.h
> >> @@ -123,6 +123,8 @@ struct arm_pmu {
> >>
> >> extern const struct dev_pm_ops armpmu_dev_pm_ops;
> >>
> >> +irqreturn_t armpmu_dispatch_irq(int irq, void *dev);
> >> +
> >>
> >> int armpmu_register(struct arm_pmu *armpmu, int type);
> >>
> >> u64 armpmu_event_update(struct perf_event *event);
> >>
> >> @@ -136,6 +138,8 @@ int armpmu_map_event(struct perf_event *event,
> >>
> >> [PERF_COUNT_HW_CACHE_RESULT_MAX],
> >>
> >> u32 raw_event_mask);
> >>
> >> +void cpu_pmu_handle_fiq(int irq);
> >> +
> >>
> >> struct pmu_probe_info {
> >>
> >> unsigned int cpuid;
> >> unsigned int mask;
> >>
> >> diff --git a/arch/arm/kernel/perf_event.c b/arch/arm/kernel/perf_event.c
> >> index f7c65adaa428..5ae9adf7f18e 100644
> >> --- a/arch/arm/kernel/perf_event.c
> >> +++ b/arch/arm/kernel/perf_event.c
> >> @@ -296,7 +296,7 @@ validate_group(struct perf_event *event)
> >>
> >> return 0;
> >>
> >> }
> >>
> >> -static irqreturn_t armpmu_dispatch_irq(int irq, void *dev)
> >> +irqreturn_t armpmu_dispatch_irq(int irq, void *dev)
> >>
> >> {
> >>
> >> struct arm_pmu *armpmu;
> >> struct platform_device *plat_device;
> >>
> >> diff --git a/arch/arm/kernel/perf_event_cpu.c
> >> b/arch/arm/kernel/perf_event_cpu.c index a80309087a7b..5c4e9ce23389
> >> 100644
> >> --- a/arch/arm/kernel/perf_event_cpu.c
> >> +++ b/arch/arm/kernel/perf_event_cpu.c
> >> @@ -36,6 +36,9 @@
> >>
> >> /* Set at runtime when we know what CPU type we are. */
> >> static struct arm_pmu *cpu_pmu;
> >>
> >> +/* Allows us to find out if an IRQ is for us (mostly used from NMI
> >> context) */ +static DEFINE_PER_CPU(int, cpu_pmu_irqs);
> >> +
> >>
> >> /*
> >>
> >> * Despite the names, these two functions are CPU-specific and are used
> >> * by the OProfile/perf code.
> >>
> >> @@ -127,6 +130,24 @@ static void cpu_pmu_free_irq(struct arm_pmu
> >> *cpu_pmu)
> >>
> >> }
> >>
> >> }
> >>
> >> +/*
> >> + * This handler is called *unconditionally* from the default NMI/FIQ
> >> + * handler. The irq may not be anything to do with us so the main
> >> + * job of this function is to figure out if the irq passed in is ours
> >> + * or not.
> >> + */
> >
> > This comment is an indicator that all the code in cpu_pmu_handle_fiq is
> > in the wrong place.
> > It (or something like it) belongs at the level of the default
> > FIQ handler, and not in perf_event_cpu.c
>
> I'm not sure about that.
>
> If we moved this code into the default FIQ handler that means the PMU
> driver would have to explicitly share its irq value with the default FIQ
> handler (which doesn't really care about that).
>
> I'm inclined to view this code as the effect of avoiding indirection in
> the default FIQ handler.
>
> Regular irq code has nothing like this because &armpmu_dispatch_irq, irq
> and get_cpu_ptr(&cpu_pmu->hw_events->percpu_pmu) would all be looked up
> from the irqaction and any unwanted events are naturally filtered by the
> irq dispatch.
>
> After your review I'm very tempted to put together a patch that
> dispatches NMIs indirectly from the default FIQ handler. However I still
> don't have much of an answer to Russell's concerns about code review.
>
Ironically I'm exactly the person RMK wants to protect against.
Perhaps request_nmi_irq (or code called from there) might be the best place to
put a "bad code filter", rather than in the handler/dispatcher as the FIQ is
running.
> >> +void cpu_pmu_handle_fiq(int irq)
> >> +{
> >> + int cpu = smp_processor_id();
> >> +
> >> + if (irq != get_cpu_var(cpu_pmu_irqs))
> >> + return;
> >> +
> >> + (void)armpmu_dispatch_irq(irq,
> >> + get_cpu_ptr(&cpu_pmu->hw_events->percpu_pmu));
> >> +}
> >> +
> >> +
> >>
> >> static int cpu_pmu_request_irq(struct arm_pmu *cpu_pmu, irq_handler_t
> >>
> >> handler) {
> >>
> >> int i, err, irq, irqs;
> >>
> >> @@ -170,9 +191,16 @@ static int cpu_pmu_request_irq(struct arm_pmu
> >> *cpu_pmu, irq_handler_t handler) continue;
> >>
> >> }
> >>
> >> - err = request_irq(irq, handler,
> >> - IRQF_NOBALANCING | IRQF_NO_THREAD, "arm-pmu",
> >> - per_cpu_ptr(&hw_events->percpu_pmu, i));
> >> + err = request_nmi_irq(
> >> + irq, IRQF_NOBALANCING, "arm-pmu",
> >> + per_cpu_ptr(&hw_events->percpu_pmu, i));
> >> + if (err) {
> >> + err = request_irq(
> >> + irq, handler,
> >> + IRQF_NOBALANCING | IRQF_NO_THREAD,
> >> + "arm-pmu",
> >> + per_cpu_ptr(&hw_events->percpu_pmu, i));
> >> + }
> >>
> >> if (err) {
> >>
> >> pr_err("unable to request IRQ%d for ARM PMU counters\n",
> >>
> >> irq);
> >>
> >> @@ -180,6 +208,7 @@ static int cpu_pmu_request_irq(struct arm_pmu
> >> *cpu_pmu,
> >> irq_handler_t handler) }
> >>
> >> cpumask_set_cpu(i, &cpu_pmu->active_irqs);
> >>
> >> + per_cpu(cpu_pmu_irqs, i) = irq;
> >>
> >> }
> >>
> >> }
> >>
> >> diff --git a/arch/arm/kernel/traps.c b/arch/arm/kernel/traps.c
> >> index 74c752b9db68..c581e07517ff 100644
> >> --- a/arch/arm/kernel/traps.c
> >> +++ b/arch/arm/kernel/traps.c
> >> @@ -38,6 +38,7 @@
> >>
> >> #include <asm/tls.h>
> >> #include <asm/system_misc.h>
> >> #include <asm/opcodes.h>
> >>
> >> +#include <asm/pmu.h>
> >>
> >> static const char *handler[]= {
> >>
> >> @@ -485,7 +486,7 @@ asmlinkage void __exception_irq_entry
> >> handle_fiq_as_nmi(struct pt_regs *regs) irq = gic_ack_fiq();
> >>
> >> if (irq) {
> >>
> >> - /* empty - no SPI handlers (yet) */
> >> + cpu_pmu_handle_fiq(irq);
> >>
> >> } else {
> >>
> >> #ifdef CONFIG_SMP
> >>
> >> ipi_cpu_backtrace(regs);
> >>
> >> --
> >> 1.9.3
> >>
> >>
> >> _______________________________________________
> >> linux-arm-kernel mailing list
> >> linux-arm-kernel@...ts.infradead.org
> >> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@...ts.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
--
Joshua Clayton
Software Engineer
UniWest
122 S. 4th Avenue
Pasco, WA 99301
Ph: (509) 544-0720
Fx: (509) 544-0868
--
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/
Powered by blists - more mailing lists