From patchwork Tue Jan 28 16:02:36 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Stefano Stabellini X-Patchwork-Id: 23800 Return-Path: X-Original-To: linaro@patches.linaro.org Delivered-To: linaro@patches.linaro.org Received: from mail-ob0-f198.google.com (mail-ob0-f198.google.com [209.85.214.198]) by ip-10-151-82-157.ec2.internal (Postfix) with ESMTPS id 7EE6B202FA for ; Tue, 28 Jan 2014 16:04:36 +0000 (UTC) Received: by mail-ob0-f198.google.com with SMTP id wp4sf1746794obc.5 for ; Tue, 28 Jan 2014 08:04:35 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:delivered-to:date:from:to:in-reply-to:message-id :references:user-agent:mime-version:cc:subject:precedence:list-id :list-unsubscribe:list-post:list-help:list-subscribe:sender :errors-to:x-original-sender:x-original-authentication-results :mailing-list:list-archive:content-type:content-transfer-encoding; bh=TZmOvSP8Hwuwz2yAqdjnWSaECO4Mja9HrKACYYvzonQ=; b=PwK4wgzSjQqAiUhr/BHqFnYRu+mwVrdV13I5VbRNXtVwyqpVHxwJ/ZFeB4s5fz3+n6 P9srfGNqFofQEflwVRHjrJtRE6wQoZ7bQKBOzS98eqfDKTUTVXAgyoixd+qkLJhcbSr8 sX6mexxGiivnBSLX1bMaW9XOHb0OOzhYSj27xg4HX3UuY5u7bRE/o9N/m2PIbF9EvCGp /AcaCHK3XZNEcKF89epqr5dVMkaHqqgODtfxxxXqb1cXqB4WSWlRtpJDEhVfY8oRB94X U/tkugHflIl8oMspY85c9v0z5Db63jbFML6/BKUUDmd5eeYNtGEAnD6sPLAkHgkQCUV+ xGIQ== X-Gm-Message-State: ALoCoQkb7dkAO+tBSIlu1L7l/3H0+mO7yBZJdYa0ewbpNA+IQAVaOHZ+SjFNJYQK7b1+/rwRFpj+ X-Received: by 10.182.66.137 with SMTP id f9mr807415obt.3.1390925075563; Tue, 28 Jan 2014 08:04:35 -0800 (PST) X-BeenThere: patchwork-forward@linaro.org Received: by 10.140.17.75 with SMTP id 69ls2012542qgc.34.gmail; Tue, 28 Jan 2014 08:04:35 -0800 (PST) X-Received: by 10.220.193.70 with SMTP id dt6mr1651478vcb.17.1390925075469; Tue, 28 Jan 2014 08:04:35 -0800 (PST) Received: from mail-ve0-f169.google.com (mail-ve0-f169.google.com [209.85.128.169]) by mx.google.com with ESMTPS id sg4si747304vdc.131.2014.01.28.08.04.35 for (version=TLSv1 cipher=ECDHE-RSA-RC4-SHA bits=128/128); Tue, 28 Jan 2014 08:04:35 -0800 (PST) Received-SPF: neutral (google.com: 209.85.128.169 is neither permitted nor denied by best guess record for domain of patch+caf_=patchwork-forward=linaro.org@linaro.org) client-ip=209.85.128.169; Received: by mail-ve0-f169.google.com with SMTP id oy12so392988veb.28 for ; Tue, 28 Jan 2014 08:04:35 -0800 (PST) X-Received: by 10.220.58.202 with SMTP id i10mr1653198vch.23.1390925075356; Tue, 28 Jan 2014 08:04:35 -0800 (PST) X-Forwarded-To: patchwork-forward@linaro.org X-Forwarded-For: patch@linaro.org patchwork-forward@linaro.org Delivered-To: patch@linaro.org Received: by 10.220.174.196 with SMTP id u4csp41718vcz; Tue, 28 Jan 2014 08:04:34 -0800 (PST) X-Received: by 10.229.71.69 with SMTP id g5mr3754871qcj.6.1390925074467; Tue, 28 Jan 2014 08:04:34 -0800 (PST) Received: from lists.xen.org (lists.xen.org. [50.57.142.19]) by mx.google.com with ESMTPS id 7si11596364qal.173.2014.01.28.08.04.33 for (version=TLSv1 cipher=RC4-SHA bits=128/128); Tue, 28 Jan 2014 08:04:34 -0800 (PST) Received-SPF: pass (google.com: best guess record for domain of xen-devel-bounces@lists.xen.org designates 50.57.142.19 as permitted sender) client-ip=50.57.142.19; Received: from localhost ([127.0.0.1] helo=lists.xen.org) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1W8B7e-0000UP-RC; Tue, 28 Jan 2014 16:02:46 +0000 Received: from mail6.bemta14.messagelabs.com ([193.109.254.103]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1W8B7d-0000UG-6J for xen-devel@lists.xen.org; Tue, 28 Jan 2014 16:02:45 +0000 Received: from [193.109.254.147:19121] by server-15.bemta-14.messagelabs.com id FD/00-22186-4A4D7E25; Tue, 28 Jan 2014 16:02:44 +0000 X-Env-Sender: Stefano.Stabellini@citrix.com X-Msg-Ref: server-13.tower-27.messagelabs.com!1390924962!390188!1 X-Originating-IP: [66.165.176.89] X-SpamReason: No, hits=0.0 required=7.0 tests=sa_preprocessor: VHJ1c3RlZCBJUDogNjYuMTY1LjE3Ni44OSA9PiAyMDMwMDc=\n X-StarScan-Received: X-StarScan-Version: 6.9.16; banners=-,-,- X-VirusChecked: Checked Received: (qmail 11715 invoked from network); 28 Jan 2014 16:02:43 -0000 Received: from smtp.citrix.com (HELO SMTP.CITRIX.COM) (66.165.176.89) by server-13.tower-27.messagelabs.com with RC4-SHA encrypted SMTP; 28 Jan 2014 16:02:43 -0000 X-IronPort-AV: E=Sophos;i="4.95,736,1384300800"; d="scan'208";a="97299607" Received: from accessns.citrite.net (HELO FTLPEX01CL03.citrite.net) ([10.9.154.239]) by FTLPIPO01.CITRIX.COM with ESMTP; 28 Jan 2014 16:02:42 +0000 Received: from ukmail1.uk.xensource.com (10.80.16.128) by smtprelay.citrix.com (10.13.107.80) with Microsoft SMTP Server id 14.2.342.4; Tue, 28 Jan 2014 11:02:41 -0500 Received: from kaball.uk.xensource.com ([10.80.2.59]) by ukmail1.uk.xensource.com with esmtp (Exim 4.69) (envelope-from ) id 1W8B7Y-0008CR-Qs; Tue, 28 Jan 2014 16:02:40 +0000 Date: Tue, 28 Jan 2014 16:02:36 +0000 From: Stefano Stabellini X-X-Sender: sstabellini@kaball.uk.xensource.com To: Ian Campbell In-Reply-To: <1390921544.7753.108.camel@kazak.uk.xensource.com> Message-ID: References: <1390844023-23123-1-git-send-email-oleksandr.tyshchenko@globallogic.com> <1390844023-23123-3-git-send-email-oleksandr.tyshchenko@globallogic.com> <1390903423.7753.23.camel@kazak.uk.xensource.com> <1390921544.7753.108.camel@kazak.uk.xensource.com> User-Agent: Alpine 2.02 (DEB 1266 2009-07-14) MIME-Version: 1.0 X-DLP: MIA2 Cc: Oleksandr Tyshchenko , xen-devel@lists.xen.org, Stefano Stabellini Subject: Re: [Xen-devel] [PATCH v1 2/2] xen/arm: Fix deadlock in on_selected_cpus function X-BeenThere: xen-devel@lists.xen.org X-Mailman-Version: 2.1.13 Precedence: list List-Id: List-Unsubscribe: , List-Post: , List-Help: , List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org X-Removed-Original-Auth: Dkim didn't pass. X-Original-Sender: stefano.stabellini@eu.citrix.com X-Original-Authentication-Results: mx.google.com; spf=neutral (google.com: 209.85.128.169 is neither permitted nor denied by best guess record for domain of patch+caf_=patchwork-forward=linaro.org@linaro.org) smtp.mail=patch+caf_=patchwork-forward=linaro.org@linaro.org Mailing-list: list patchwork-forward@linaro.org; contact patchwork-forward+owners@linaro.org X-Google-Group-Id: 836684582541 List-Archive: On Tue, 28 Jan 2014, Ian Campbell wrote: > On Tue, 2014-01-28 at 14:00 +0000, Stefano Stabellini wrote: > > On Tue, 28 Jan 2014, Ian Campbell wrote: > > > On Mon, 2014-01-27 at 19:00 +0000, Stefano Stabellini wrote: > > > > Alternatively, as Ian suggested, we could increase the priotiry of SGIs > > > > but I am a bit wary of making that change at RC2. > > > > > > I'm leaning the other way -- I'm wary of open coding magic locking > > > primitives to work around this issue on a case by case basis. It's just > > > too subtle IMHO. > > > > > > The IPI and cross CPU calling primitives are basically predicated on > > > those IPIs interrupting normal interrupt handlers. > > > > The problem is that we don't know if we can context switch properly > > nested interrupts. > > What do you mean? We don't have to context switch an IPI. Sorry, I meant save/restore registers, stack pointer, processor mode, etc, for nested interrupts. > > Also I would need to think harder whether everything > > would work correctly without hitches with multiple SGIs happening > > simultaneously (with more than 2 cpus involved). > > Since all IPIs would be at the same higher priority only one will be > active on each CPU at a time. If you are worried about multiple CPUs > then that is already an issue today, just at a lower priority. That is correct, but if we moved the on_selected_cpus call out of the interrupt handler I don't think the problem could occur. > I have hacked the IPI priority to be higher in the past and it worked > fine, I just never got round to cleaning it up for submission (I hadn't > thought of the locking thing and my use case was low priority). > > The interrupt entry and exit paths were written with nested interrupt in > mind and they have to be so in order to handle interrupts which occur > from both guest and hypervisor context. > > > On the other hand we know that both Oleksandr's and my solution should > > work OK with no surprises if implemented correctly. > > That's a big if in my mind, any use of trylock is very subtle IMOH. I wouldn't accept that patch for 4.4 either (or for 4.5 given that we can certainly come up with something better by that time). > AIUI this issue only occurs with "proto device assignment" patches added > to 4.4, n which case I think the solution can wait until 4.5 and can be > done properly via the IPI priority fix. I think this is a pretty significant problem, even if we don't commit a fix, we should post a proper patch that we deem acceptable and link it to the wiki so that anybody that needs it can find it and be sure that it works correctly. In my opinion if we go to this length then we might as well commit it (if it doesn't touch common code of course), but I'll leave the decision up to you and George. Given the constraints, the solution I would feel more comfortable with at this time is something like the following patch (lightly tested): diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c index e6257a7..b00ca73 100644 --- a/xen/arch/arm/gic.c +++ b/xen/arch/arm/gic.c @@ -58,6 +58,25 @@ static DEFINE_PER_CPU(uint64_t, lr_mask); static unsigned nr_lrs; +static void gic_irq_eoi(void *info) +{ + int virq = (uintptr_t) info; + GICC[GICC_DIR] = virq; +} + +static DEFINE_PER_CPU(struct pending_irq*, eoi_irq); +static void eoi_action(unsigned long unused) +{ + struct pending_irq *p = this_cpu(eoi_irq); + ASSERT(p != NULL); + + on_selected_cpus(cpumask_of(p->desc->arch.eoi_cpu), + gic_irq_eoi, (void*)(uintptr_t)p->desc->irq, 0); + + this_cpu(eoi_irq) = NULL; +} +static DECLARE_TASKLET(eoi_tasklet, eoi_action, 0); + /* The GIC mapping of CPU interfaces does not necessarily match the * logical CPU numbering. Let's use mapping as returned by the GIC * itself @@ -897,12 +916,6 @@ int gicv_setup(struct domain *d) } -static void gic_irq_eoi(void *info) -{ - int virq = (uintptr_t) info; - GICC[GICC_DIR] = virq; -} - static void maintenance_interrupt(int irq, void *dev_id, struct cpu_user_regs *regs) { int i = 0, virq, pirq = -1; @@ -962,8 +975,11 @@ static void maintenance_interrupt(int irq, void *dev_id, struct cpu_user_regs *r if ( cpu == smp_processor_id() ) gic_irq_eoi((void*)(uintptr_t)pirq); else - on_selected_cpus(cpumask_of(cpu), - gic_irq_eoi, (void*)(uintptr_t)pirq, 0); + { + ASSERT(this_cpu(eoi_irq) == NULL); + this_cpu(eoi_irq) = p; + tasklet_schedule(&eoi_tasklet); + } } i++;