From patchwork Thu Jul 2 13:31:40 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Daniel Thompson X-Patchwork-Id: 6709801 Return-Path: X-Original-To: patchwork-linux-arm@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork1.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.29.136]) by patchwork1.web.kernel.org (Postfix) with ESMTP id C9F529F54C for ; Thu, 2 Jul 2015 13:34:14 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id BF0A32061D for ; Thu, 2 Jul 2015 13:34:13 +0000 (UTC) Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.9]) (using TLSv1.2 with cipher AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id B381F20618 for ; Thu, 2 Jul 2015 13:34:12 +0000 (UTC) Received: from localhost ([127.0.0.1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.80.1 #2 (Red Hat Linux)) id 1ZAeb2-0008OU-GS; Thu, 02 Jul 2015 13:32:08 +0000 Received: from mail-wg0-f48.google.com ([74.125.82.48]) by bombadil.infradead.org with esmtps (Exim 4.80.1 #2 (Red Hat Linux)) id 1ZAeaz-0008Kf-1w for linux-arm-kernel@lists.infradead.org; Thu, 02 Jul 2015 13:32:05 +0000 Received: by wguu7 with SMTP id u7so63398662wgu.3 for ; Thu, 02 Jul 2015 06:31:42 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:subject:to:references:cc:from:message-id:date :user-agent:mime-version:in-reply-to:content-type :content-transfer-encoding; bh=bA8ehaQydtmS6CDizEsu1IPP3g1bJyD/NG5z87lfxUg=; b=e9cFBmbTyEZlKqgU1Xh5y52TEfKmqdZpfBMiI5xZTKIn3x3DG/CiOcqCIIQQr9Zf42 EA/UeuYbdUWBAqEMF5CxkQFlzfweRjAbq8VAJdBzZA9rCRlMiPpBYgQ61ZyLyCH4hy8o QryaKAb2LXh9iSyj/vtOepuSTvw8jvaLedQFvjIAqHZZ3brKv4Te/5hWP45GHw/X/k4a HqSETspclxtXYFhYWOh9wjBaMgJNRJmX8O92QfKjrXkf12y2jCRtXJ5KjRF+FFp/NRaw 87SueugUqhwAzhI35NVV/b8Si4y8Iye12aIqIapbxnR9F0Jhf7CCrvHViuyIaH/hcAtM LRBQ== X-Gm-Message-State: ALoCoQlWWPTtNsenB4rGTiKVbo4hmdKe05gnROugfZJKg66UBGWY12uGug0WBkG5nK1Dxexx0MHp X-Received: by 10.194.81.67 with SMTP id y3mr57360260wjx.7.1435843902456; Thu, 02 Jul 2015 06:31:42 -0700 (PDT) Received: from [192.168.1.125] (cpc4-aztw19-0-0-cust71.18-1.cable.virginm.net. [82.33.25.72]) by mx.google.com with ESMTPSA id z9sm27346268wiv.9.2015.07.02.06.31.40 (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Thu, 02 Jul 2015 06:31:41 -0700 (PDT) Subject: Re: [RESEND PATCH 4.0-rc7 v20 3/6] irqchip: gic: Introduce plumbing for IPI FIQ To: Mark Rutland 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> <5536BDE8.2030403@linaro.org> <20150422103841.GC27345@leverpostej> From: Daniel Thompson Message-ID: <55953D3C.9080800@linaro.org> Date: Thu, 2 Jul 2015 14:31:40 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.0.1 MIME-Version: 1.0 In-Reply-To: <20150422103841.GC27345@leverpostej> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20150702_063205_255673_C7E2A094 X-CRM114-Status: GOOD ( 30.81 ) X-Spam-Score: -2.6 (--) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.20 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: "linaro-kernel@lists.linaro.org" , Russell King , Jason Cooper , "patches@linaro.org" , Marc Zyngier , Will Deacon , Stephen Boyd , "linux-kernel@vger.kernel.org" , Steven Rostedt , Daniel Drake , Dirk Behme , John Stultz , Tim Sander , Catalin Marinas , Thomas Gleixner , Sumit Semwal , Dmitry Pervushin , "linux-arm-kernel@lists.infradead.org" Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+patchwork-linux-arm=patchwork.kernel.org@lists.infradead.org X-Spam-Status: No, score=-4.8 required=5.0 tests=BAYES_00, RCVD_IN_DNSWL_MED, RP_MATCHES_RCVD, UNPARSEABLE_RELAY autolearn=unavailable version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on mail.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP On 22/04/15 11:38, Mark Rutland wrote: >>> 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. > > Patch 3 appears to be to blame. I see the issue with patches 1-3 alone > applied atop of v4.0. With patch 3 reverted secondaries come up as > expected. So I'm back looking at this after a bit of a break. The problem is almost certainly due to mismanaging the NSATT bit within GICD_SGIR. Specifically we must use a different value for NSATT before a CPU is booted for the first time because that CPU will not have setup its banked copy of IGROUP[0] yet. I have played with a couple of fixes but I think the simplest is to detect if we are running from secure mode and, if we are, to write to GICD_SGIR twice (once without NSATT, once with). Note that we do have to detect ourselves to be running from secure mode before trying the double-write approach. If we were running from non-secure mode then the double write could risk two IPIs being generated. Anyhow the main benefit of this approach is that it is stateless so we don't have to do any state tracking (which I think would require using rwlocks). I plan to react to the outstanding review comments and roll the fix into the existing patches but, for clarity, here are the fixes that I think are needed to solve the TC2 boot problems. I have tested both from secure and non-secure modes but have not been able to test on TC2. --- Daniel. diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c index 4f9e4296438c..a7d721e43db6 100644 --- a/drivers/irqchip/irq-gic.c +++ b/drivers/irqchip/irq-gic.c @@ -73,6 +73,7 @@ struct gic_chip_data { struct irq_domain *domain; unsigned int gic_irqs; u32 igroup0_shadow; + bool sgi_with_nsatt; #ifdef CONFIG_GIC_NON_BANKED void __iomem *(*get_base)(union gic_base *); #endif @@ -512,16 +513,27 @@ static void __init gic_dist_init(struct gic_chip_data writel_relaxed(GICD_ENABLE_GRP1 | GICD_ENABLE, base + GIC_DIST_CTRL); /* - * Set all global interrupts to be group 1 if (and only if) it - * is possible to enable group 1 interrupts. This register is RAZ/WI - * if not accessible or not implemented, however some GICv1 devices - * do not implement the EnableGrp1 bit making it unsafe to set - * this register unconditionally. + * Some GICv1 devices (even those with security extensions) do not + * implement EnableGrp1 meaning some parts of the above write might + * be ignored. We will only enable FIQ support if the bit can be set. */ - if (GICD_ENABLE_GRP1 & readl_relaxed(base + GIC_DIST_CTRL)) + if (GICD_ENABLE_GRP1 & readl_relaxed(base + GIC_DIST_CTRL)) { + /* + * Set all global interrupts to be group 1 (signalled with + * IRQ). + */ for (i = 32; i < gic_irqs; i += 32) writel_relaxed(0xffffffff, base + GIC_DIST_IGROUP + i * 4 / 32); + + /* + * If the GIC supports the security extension then SGIs + * will be filtered based on the value of NSATT. If the + * GIC has this support then enable NSATT support. + */ + if (GICD_SECURITY_EXTN & readl_relaxed(base + GIC_DIST_CTR)) + gic->sgi_with_nsatt = true; + } } static void gic_cpu_init(struct gic_chip_data *gic) @@ -782,6 +794,7 @@ static void gic_raise_softirq(const struct cpumask *mask, int cpu; unsigned long map = 0; unsigned long softint; + void __iomem *dist_base; gic_migration_lock(); @@ -789,20 +802,20 @@ static void gic_raise_softirq(const struct cpumask *mask, for_each_cpu(cpu, mask) map |= gic_cpu_map[cpu]; + /* This always happens on GIC0 */ + dist_base = gic_data_dist_base(&gic_data[0]); + /* * Ensure that stores to Normal memory are visible to the * other CPUs before they observe us issuing the IPI. */ dmb(ishst); - /* We avoid a readl here by using the shadow copy of IGROUP[0] */ softint = map << 16 | irq; - if (gic_data[0].igroup0_shadow & BIT(irq)) - softint |= 0x8000; - /* This always happens on GIC0 */ - writel_relaxed(softint, - gic_data_dist_base(&gic_data[0]) + GIC_DIST_SOFTINT); + writel_relaxed(softint, dist_base + GIC_DIST_SOFTINT); + if (gic_data[0].sgi_with_nsatt) + writel_relaxed(softint | 0x8000, dist_base + GIC_DIST_SOFTINT); gic_migration_unlock(); } diff --git a/include/linux/irqchip/arm-gic.h b/include/linux/irqchip/arm-gic.h index 361dddfe205a..22cf475e1deb 100644 --- a/include/linux/irqchip/arm-gic.h +++ b/include/linux/irqchip/arm-gic.h @@ -50,6 +50,7 @@ #define GICD_ENABLE 0x1 #define GICD_ENABLE_GRP1 0x2 #define GICD_DISABLE 0x0 +#define GICD_SECURITY_EXTN 0x400 #define GICD_INT_ACTLOW_LVLTRIG 0x0 #define GICD_INT_EN_CLR_X32 0xffffffff #define GICD_INT_EN_SET_SGI 0x0000ffff