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: 50592 Return-Path: X-Original-To: linaro@patches.linaro.org Delivered-To: linaro@patches.linaro.org Received: from mail-la0-f71.google.com (mail-la0-f71.google.com [209.85.215.71]) by ip-10-151-82-157.ec2.internal (Postfix) with ESMTPS id F11DA214B3 for ; Thu, 2 Jul 2015 13:31:44 +0000 (UTC) Received: by lagx9 with SMTP id x9sf20136611lag.2 for ; Thu, 02 Jul 2015 06:31:43 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:delivered-to:subject:to:references:cc:from :message-id:date:user-agent:mime-version:in-reply-to:content-type :content-transfer-encoding:x-original-sender :x-original-authentication-results:precedence:mailing-list:list-id :list-post:list-help:list-archive:list-unsubscribe; bh=bA8ehaQydtmS6CDizEsu1IPP3g1bJyD/NG5z87lfxUg=; b=JvVhPbssAfOSbKxAmGzB+k30OiFAJ00njJQIZqiBzHwHEt/CtzsQBDJoEV4iU/kBv1 tzABeILFlHuug+oWprJciLDscdfJ/LdcN2vfQghkaNC0ff7MttLXdkwYmCC4EdtSnuEF QLQN39x15UuxhhKSPb2x031D3lL/6ep+1KIu9Gr5ONIbk/3Lpdie9YD0KNigzpyNAMoj +YUoyCOYk6wDSs2HH/hbhdO5YdPd6JU9vQXnWCL+usjvOy28bZBgPySPNnEpUSS+8Z+o BI9QlhrEpTNDbchJykJ5/VsW7lpRpekHVHv/DOHsmymmP53rA9xDZIFQfQD5KvPTrFEj lmRA== X-Gm-Message-State: ALoCoQmMsssNVGF+k5o3h0U1jyeAqgSXKrrXPF7jWhE0VJI6vXwvl5VUqmRLG9ryZ6CWFnS3emoW X-Received: by 10.180.106.10 with SMTP id gq10mr17783388wib.0.1435843903894; Thu, 02 Jul 2015 06:31:43 -0700 (PDT) X-BeenThere: patchwork-forward@linaro.org Received: by 10.152.115.161 with SMTP id jp1ls318706lab.96.gmail; Thu, 02 Jul 2015 06:31:43 -0700 (PDT) X-Received: by 10.152.1.227 with SMTP id 3mr30383584lap.61.1435843903733; Thu, 02 Jul 2015 06:31:43 -0700 (PDT) Received: from mail-la0-f43.google.com (mail-la0-f43.google.com. [209.85.215.43]) by mx.google.com with ESMTPS id a4si4458729lak.156.2015.07.02.06.31.43 for (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Thu, 02 Jul 2015 06:31:43 -0700 (PDT) Received-SPF: pass (google.com: domain of patch+caf_=patchwork-forward=linaro.org@linaro.org designates 209.85.215.43 as permitted sender) client-ip=209.85.215.43; Received: by lagh6 with SMTP id h6so61148382lag.2 for ; Thu, 02 Jul 2015 06:31:43 -0700 (PDT) X-Received: by 10.152.4.163 with SMTP id l3mr31117327lal.35.1435843903585; Thu, 02 Jul 2015 06:31:43 -0700 (PDT) X-Forwarded-To: patchwork-forward@linaro.org X-Forwarded-For: patch@linaro.org patchwork-forward@linaro.org Delivered-To: patches@linaro.org Received: by 10.112.108.230 with SMTP id hn6csp484444lbb; Thu, 2 Jul 2015 06:31:42 -0700 (PDT) X-Received: by 10.194.93.226 with SMTP id cx2mr11047491wjb.28.1435843902811; Thu, 02 Jul 2015 06:31:42 -0700 (PDT) Received: from mail-wg0-f51.google.com (mail-wg0-f51.google.com. [74.125.82.51]) by mx.google.com with ESMTPS id lk20si9480602wic.101.2015.07.02.06.31.42 for (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Thu, 02 Jul 2015 06:31:42 -0700 (PDT) Received-SPF: pass (google.com: domain of daniel.thompson@linaro.org designates 74.125.82.51 as permitted sender) client-ip=74.125.82.51; Received: by wguu7 with SMTP id u7so63398661wgu.3 for ; Thu, 02 Jul 2015 06:31:42 -0700 (PDT) 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> 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" 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-Removed-Original-Auth: Dkim didn't pass. X-Original-Sender: daniel.thompson@linaro.org X-Original-Authentication-Results: mx.google.com; spf=pass (google.com: domain of patch+caf_=patchwork-forward=linaro.org@linaro.org designates 209.85.215.43 as permitted sender) smtp.mail=patch+caf_=patchwork-forward=linaro.org@linaro.org Precedence: list Mailing-list: list patchwork-forward@linaro.org; contact patchwork-forward+owners@linaro.org List-ID: X-Google-Group-Id: 836684582541 List-Post: , List-Help: , List-Archive: List-Unsubscribe: , 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