From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S964887AbbDUVPb (ORCPT ); Tue, 21 Apr 2015 17:15:31 -0400 Received: from mail-wi0-f173.google.com ([209.85.212.173]:36265 "EHLO mail-wi0-f173.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753466AbbDUVPX (ORCPT ); Tue, 21 Apr 2015 17:15:23 -0400 Message-ID: <5536BDE8.2030403@linaro.org> Date: Tue, 21 Apr 2015 22:15:20 +0100 From: Daniel Thompson User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.6.0 MIME-Version: 1.0 To: Mark Rutland CC: Thomas Gleixner , Jason Cooper , "linaro-kernel@lists.linaro.org" , Russell King , "patches@linaro.org" , Marc Zyngier , Stephen Boyd , Will Deacon , "linux-kernel@vger.kernel.org" , Steven Rostedt , Daniel Drake , Dmitry Pervushin , Dirk Behme , John Stultz , Tim Sander , Catalin Marinas , Sumit Semwal , "linux-arm-kernel@lists.infradead.org" Subject: Re: [RESEND PATCH 4.0-rc7 v20 3/6] irqchip: gic: Introduce plumbing for IPI FIQ References: <1427216014-5324-1-git-send-email-daniel.thompson@linaro.org> <1428659511-9590-1-git-send-email-daniel.thompson@linaro.org> <1428659511-9590-4-git-send-email-daniel.thompson@linaro.org> <20150421145007.GB10164@leverpostej> In-Reply-To: <20150421145007.GB10164@leverpostej> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 21/04/15 15:50, Mark Rutland wrote: > Hi, > > On Fri, Apr 10, 2015 at 10:51:48AM +0100, Daniel Thompson wrote: >> Currently it is not possible to exploit FIQ for systems with a GIC, even if >> the systems are otherwise capable of it. This patch makes it possible >> for IPIs to be delivered using FIQ. >> >> To do so it modifies the register state so that normal interrupts are >> placed in group 1 and specific IPIs are placed into group 0. It also >> configures the controller to raise group 0 interrupts using the FIQ >> signal. It provides a means for architecture code to define which IPIs >> shall use FIQ and to acknowledge any IPIs that are raised. >> >> All GIC hardware except GICv1-without-TrustZone support provides a means >> to group exceptions into group 0 and group 1 but the hardware >> functionality is unavailable to the kernel when a secure monitor is >> present because access to the grouping registers are prohibited outside >> "secure world". However when grouping is not available (or in the case >> of early GICv1 implementations is very hard to configure) the code to >> change groups does not deploy and all IPIs will be raised via IRQ. >> >> It has been tested and shown working on two systems capable of >> supporting grouping (Freescale i.MX6 and STiH416). It has also been >> tested for boot regressions on two systems that do not support grouping >> (vexpress-a9 and Qualcomm Snapdragon 600). > > I just gave this a spin on my (non-MCPM) TC2, and secondaries don't come > up: > > CPU1: failed to boot: -38 > CPU2: failed to boot: -38 > CPU3: failed to boot: -38 > CPU4: failed to boot: -38 > Brought up 1 CPUs > SMP: Total of 1 processors activated (48.00 BogoMIPS). > > I tried investigating with a debugger. The unbooted CPUs look to be > stuck at the FW's spin loop, but the text doesn't look right (I see a > load of ADDEQ r0, r0, r0, #LSL 1 where there was previously a WFI loop). > That could be a bug with my debugger though. > > If I pause the CPUs at the right point, they sometimes enter the kernel > successfully. I don't have a good explanation for that. > > [...] Rats! I presume it is patch 3 that causes the regression? Patch 3 is the one that causes the GIC to adopt a different configuration if it find the kernel running in secure world (it sets all interrupts to group 1 and routes group 0 to FIQ). I only ask because it isn't until patch 6 that we actually place any interrupt sources into group 0. >> @@ -427,6 +535,7 @@ static void gic_cpu_init(struct gic_chip_data *gic) >> void __iomem *base = gic_data_cpu_base(gic); >> unsigned int cpu_mask, cpu = smp_processor_id(); >> int i; >> + unsigned long secure_irqs, secure_irq; > > I think secure_irq(s) is a misnomer here. It's just a mask of FIQ bits. I guess so, on GICv2 without security extentions these are not secure irqs. This is one of the places were IRQ, FIQ, irq and hwirq meet together and naming things is hard. What sort of name do you like: fiq(s), fiq_hwirq(s)? >> >> /* >> * Get what the GIC says our CPU mask is. >> @@ -445,6 +554,20 @@ static void gic_cpu_init(struct gic_chip_data *gic) >> >> gic_cpu_config(dist_base, NULL); >> >> + /* >> + * If the distributor is configured to support interrupt grouping >> + * then set any PPI and SGI interrupts not set in SMP_IPI_FIQ_MASK >> + * to be group1 and ensure any remaining group 0 interrupts have >> + * the right priority. >> + */ >> + if (GICD_ENABLE_GRP1 & readl_relaxed(dist_base + GIC_DIST_CTRL)) { >> + secure_irqs = SMP_IPI_FIQ_MASK; >> + writel_relaxed(~secure_irqs, dist_base + GIC_DIST_IGROUP + 0); >> + gic->igroup0_shadow = ~secure_irqs; >> + for_each_set_bit(secure_irq, &secure_irqs, 16) >> + gic_set_group_irq(gic, secure_irq, 0); >> + } > > This only pokes GICD registers. Why isn't this in gic_dist_init? GIC_DIST_IGROUP[0] (which controls grouping for SGIs and PPIs) is banked per-cpu and form part of the per-cpu configuration. Daniel.