From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-10.1 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,INCLUDES_PATCH,MAILING_LIST_MULTI, SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 802A6C433E3 for ; Fri, 10 Jul 2020 16:30:00 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 57E53206F4 for ; Fri, 10 Jul 2020 16:30:00 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=default; t=1594398600; bh=q0yxkqPHIM1TwXDC5jIPZtPqmDMrfw+O3OukvvwRv1I=; h=Date:From:To:Cc:Subject:In-Reply-To:References:List-ID:From; b=PTGWKHVWlIRJ2xvI0LpSpBktHbanQBSLOSvQY062VY4ObJyf4BYo9l97pmWxBzeOp 7Q1hqeNllxGKvbzdLkFBDhvH/ykf/b73per4bzBXiDPvT5ftYdUJp5Mu0o4bh5CZr9 4wfiEt1sHC/Hfi+Xik5EeUO2YwMmi0zc4jMule9Q= Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1728112AbgGJQ37 (ORCPT ); Fri, 10 Jul 2020 12:29:59 -0400 Received: from mail.kernel.org ([198.145.29.99]:49358 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1727771AbgGJQ36 (ORCPT ); Fri, 10 Jul 2020 12:29:58 -0400 Received: from disco-boy.misterjones.org (disco-boy.misterjones.org [51.254.78.96]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPSA id 79C2A20657; Fri, 10 Jul 2020 16:29:57 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=default; t=1594398597; bh=q0yxkqPHIM1TwXDC5jIPZtPqmDMrfw+O3OukvvwRv1I=; h=Date:From:To:Cc:Subject:In-Reply-To:References:From; b=D5/wRzppwjD6vaSdfn0jyLEgZfujHQYHXtVcM/HEP/UO1H4Iuom2ExmTFGaYj0e0r Ckbh12jD34eOVeo7Xx7hWD7fy+H5rHjTwJAFlHnPxYnK24x0OpOJFc/DPq+txFys/o 1pNjC/XdHnvJdTIvujntztJJkg/vHqLoXdcCsTbM= Received: from host109-149-250-171.range109-149.btcentralplus.com ([109.149.250.171] helo=wait-a-minute.misterjones.org) by disco-boy.misterjones.org with esmtpsa (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1jtvuO-00Ajkq-0c; Fri, 10 Jul 2020 17:29:56 +0100 Date: Fri, 10 Jul 2020 17:29:49 +0100 Message-ID: <87tuyfxria.wl-maz@kernel.org> From: Marc Zyngier To: Valentin Schneider Cc: linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, Thomas Gleixner , Jason Cooper , Lorenzo Pieralisi Subject: Re: [RFC PATCH] irqchip/gic: Implement irq_chip->irq_retrigger() In-Reply-To: <20200710145642.28978-1-valentin.schneider@arm.com> References: <20200710145642.28978-1-valentin.schneider@arm.com> User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI-EPG/1.14.7 (Harue) FLIM/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL/10.8 EasyPG/1.0.0 Emacs/26.3 (x86_64-pc-linux-gnu) MULE/6.0 (HANACHIRUSATO) MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue") Content-Type: text/plain; charset=US-ASCII X-SA-Exim-Connect-IP: 109.149.250.171 X-SA-Exim-Rcpt-To: valentin.schneider@arm.com, linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, tglx@linutronix.de, jason@lakedaemon.net, Lorenzo.Pieralisi@arm.com X-SA-Exim-Mail-From: maz@kernel.org X-SA-Exim-Scanned: No (on disco-boy.misterjones.org); SAEximRunCond expanded to false Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Fri, 10 Jul 2020 15:56:42 +0100, Valentin Schneider wrote: > > While digging around IRQCHIP_EOI_IF_HANDLED and irq/resend.c, it has come > to my attention that the IRQ resend situation seems a bit precarious for > the GIC(s). Below is a (somewhat structured) dump of my notes/thoughts > about that. > > IRQCHIP_EOI_IF_HANDLED > ====================== > > If the irqchip doesn't have this flag set, we may issue an irq_eoi() > despite not having handled the IRQ in the following cases: > > o !irq_may_run() > - IRQ may be in progress (handle_irq_event() ongoing) > - IRQ is an armed wakeup source (also sets it pending) > > o !action or IRQ disabled; in this case it is set as pending. > > irq_resend() > ============ > > For the above cases where the IRQ is marked as pending, it means that when > we'll go through check_irq_resend() (e.g. when we re-enable the IRQ) we > will end up in resend_irqs() which goes through the flow handler again, IOW > will issue *another* EOI on the same IRQ. > > Now, this is all done via a tasklet, so AFAICT cannot happen in irq > context (as defined by in_interrupt() - it can happen at the tail of > handling an IRQ if it wasn't nesting). > > GIC woes > ======== > > The TL;DR for IRQ handling on the GIC is that we have 3 steps: > o Acknowledgement (Ack) > o priority drop (PD) > o deactivation (D) > > The GIC also has an "eoimode" knob that says whether PD and D are split, IOW: > o eoimode=0: irq_eoi() does PD + D > o eoimode=1: irq_eoi() does D > > Regardless of the mode, it is paramount that any PD is > o issued on the same CPU that Ack'd the IRQ > o issued in reverse order of the Acks > > IHI0069D, 4.1.1 Physical CPU interface, Priority drop > """ > A priority drop must be performed by the same PE that activated the > interrupt. > > [...] > > For each CPU interface, the GIC architecture requires the order of the > valid writes to ICC_EOIR0_EL1 and ICC_EOIR1_EL1 to be the exact reverse of > the order of the reads from ICC_IAR0_EL1 and ICC_IAR1_EL1 > """ > > IHI0069D, 8.2.9 ICC_EOIR0_EL1, Interrupt Controller End Of Interrupt Register 0 > """ > A write to this register must correspond to the most recent valid read by > this PE from an Interrupt Acknowledge Register, and must correspond to the > INTID that was read from ICC_IAR0_EL1, otherwise the system behavior is > UNPREDICTABLE. > """ > > For eoimode=1, the PD is hidden from genirq and is contained within the GIC > driver. This means that issuing another irq_eoi() will only be re-issuing a > D, which I think the GIC can live with (even if issued from a different CPU). > > For eoimode=0, it is more troubling, as we break the aforementioned > restrictions. That said, IIUC this cannot cause e.g. a bogus running > priority reduction due to the tasklet context mentioned above (running > priority ought to be idle priority). > > Linux hosts will almost always use eoimode=1, so that leaves us with > guests running with eoimode=0, and I don't know how common it is (if at all > possible) for those to use pm / wakeup IRQs. I suppose that is a reason > why this hasn't cropped up before, that or I miserably misunderstood the > whole thing. > > In any case, the virtual interface follows the same restrictions wrt > PD ordering: > > IHI0069D 8.3.7 ICV_EOIR0_EL1, Interrupt Controller Virtual End Of Interrupt Register 0 > """ > A write to this register must correspond to the most recent valid read by > this vPE from a Virtual Interrupt Acknowledge Register, and must correspond > to the INTID that was read from ICV_IAR0_EL1, otherwise the system behavior > is UNPREDICTABLE. > """ > > Change > ====== > > One way to ensure we only get one PD per interrupt activation and maintain > the expected ordering is to add the IRQCHIP_EOI_IF_HANDLED flag to all > irq-gic chips, but that only really works for eoimode=1; for eoimode=0 that > would mean leaving the flow handler without having issued a PD at all. > > At the same time, this whole IRQS_PENDING & resend thing feels like > something we can handle in hardware: we can leverage > irq_chip.irq_retrigger() and use this to mark the interrupt as pending in > the GIC itself, in which case we *don't* want to have > IRQCHIP_EOI_IF_HANDLED (as the retrigger will lead to another ack+eoi > pair). > > Implement irq_chip.irq_retrigger() for both GICs. Although I am very grateful for the whole documentation, I'd rather have a slightly more condensed changelog that documents the implementation of the retrigger callback! ;-) > > Signed-off-by: Valentin Schneider > --- > drivers/irqchip/irq-gic-v3.c | 7 +++++++ > drivers/irqchip/irq-gic.c | 6 ++++++ > 2 files changed, 13 insertions(+) > > diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c > index cc46bc2d634b..c025e8b51464 100644 > --- a/drivers/irqchip/irq-gic-v3.c > +++ b/drivers/irqchip/irq-gic-v3.c > @@ -1207,6 +1207,11 @@ static int gic_set_affinity(struct irq_data *d, const struct cpumask *mask_val, > #define gic_smp_init() do { } while(0) > #endif > > +static int gic_retrigger(struct irq_data *data) > +{ > + return gic_irq_set_irqchip_state(data, IRQCHIP_STATE_PENDING, true); If I'm not mistaken, check_irq_resend() requires a non-zero return value if the retrigger has succeeded. So something like return !gic_irq_set_irqchip_state(data, IRQCHIP_STATE_PENDING, true); would be more appropriate. Otherwise, looks good. Thanks, M. -- Without deviation from the norm, progress is not possible.