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 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 1996CC433EF for ; Fri, 19 Nov 2021 02:40:13 +0000 (UTC) Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id CAF5A617E4 for ; Fri, 19 Nov 2021 02:40:12 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.4.1 mail.kernel.org CAF5A617E4 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=gmail.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References: Message-ID:Subject:Cc:To:From:Date:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=iHvC6CQ49TFhOKyMz+U7bIVc601+QS8TegblsK2VY4E=; b=DQ8Ct5y1Y9KBXY vC0X36Wu5RpCLsky5TDhmBEPKQbHgoI1G0jox36IW5xdW/tzHI5JqKaZSPPlU2Prt8oT2YO31LFOE wOXyW5fM/Pw30KJupvoGD9SgNqFCOEQ/QitPiBygXIn+pEosy1S+hU+UeshPtgK8gFEyo1fh59hC2 CnpSq+taQ5av605md59JYloXuCraqR2xSQSXpNURCC/KnuN304hP5y4Dy/rCmRmLxjVP40n52paG8 WlRozM6gYla0rdDUm8afV0nFF5lwawdWBvXvpgkxR8hsIwpdlNWIDwiLCWHwkTqRDJ6OkS+v9MFCF etvQd1AGQahcEugOpElA==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1mntnU-009HIX-IN; Fri, 19 Nov 2021 02:38:40 +0000 Received: from mail-pj1-x102f.google.com ([2607:f8b0:4864:20::102f]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1mntnP-009HHq-N1 for linux-arm-kernel@lists.infradead.org; Fri, 19 Nov 2021 02:38:37 +0000 Received: by mail-pj1-x102f.google.com with SMTP id v23so6774813pjr.5 for ; Thu, 18 Nov 2021 18:38:35 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to; bh=be0VjlCfz8v4iknbpHUglEbWLwBU+ul0wNcBljR8czw=; b=osLhPfUZaUrAtbjzclrY943L1HNd0F98wz5ChFWWMPxMnV/5aO/kmsH0IhD+jAnzZq tfDndVEHa7JahCWuxXsu1oneg1QI/FzqPx/4tw/hllAamqk3J8vwV2P1/ZYfBZ6AgeSK K9X1ESmdX3mFjbkC2CuE/IQGbRRpBCgLflpvCdzpQN3o+FnvBmj9b9ZDycBiE4SanMw1 1RG0eZHEs1GBK7X8uHK8HKGYQvx+xIUH/dg5JbLAueSPZcCfXT9ooTzSKZYGqpEaVIaY 7wV0DSkbS/kVxT4sHqKpL7b5bmnC4Awnf+jTfTKBdoJ5ybcMTQP8ugNJojhkf4G2aj6V Uu6w== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=be0VjlCfz8v4iknbpHUglEbWLwBU+ul0wNcBljR8czw=; b=dQnHT6tH1p9qc85D24I6NcnIJgMd4CXd5ANbmiATZnA9yDsRuDIEOR5ofCinIfIix1 i8iUlsLb8pJ/9gfBquFYvc9IH1nLkCASn9MbcGbFrdZvpzigh2BpKdAvtur1Pjf4CubX CXoqwuxQ/2+o4F0YNDtV1uRmZ8ICE9C7hJ0LPQo4hRqp9PIPIqWzlvF9ic6klx7a4l1X mdLPTxlGsvgqeuDb8TZ+Vs+1kM/9C+7GSqZ+fEodOkSpJzHN/tWjXoqpjtqNmjUnujeu S25LcdDDC2cBPoyhWyAi95G84K6N7S1JnmlNjqDvPrfgdAfrZjE9Tk3GNzPOxcDPz/C1 Admg== X-Gm-Message-State: AOAM533xVPQwoESufIiXkbzVa9f5SeNBRvUI0hvAH4SVjm0bKb6XZRM8 8dVXo/zniIGPH72dPphi4w== X-Google-Smtp-Source: ABdhPJyHncGEvA1Ifvztkp4d2Oq8WPMtrNK8euq/nxcFtSICYjpPM84tTCPVDDWfsEUBLgWEXjRifQ== X-Received: by 2002:a17:90b:2389:: with SMTP id mr9mr320418pjb.152.1637289515008; Thu, 18 Nov 2021 18:38:35 -0800 (PST) Received: from piliu.users.ipa.redhat.com ([209.132.188.80]) by smtp.gmail.com with ESMTPSA id i68sm867622pfc.158.2021.11.18.18.38.29 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 18 Nov 2021 18:38:34 -0800 (PST) Date: Fri, 19 Nov 2021 10:38:24 +0800 From: Pingfan Liu To: Marc Zyngier Cc: linux-arm-kernel@lists.infradead.org, Catalin Marinas , Will Deacon , Mark Rutland , Joey Gouly , Sami Tolvanen , Julien Thierry , Yuichi Ito , rcu@vger.kernel.org Subject: Re: [PATCHv3 3/4] irqchip: GICv3: expose pNMI discriminator Message-ID: References: <20211116082450.10357-1-kernelfans@gmail.com> <20211116082450.10357-4-kernelfans@gmail.com> <878rxo8m0a.wl-maz@kernel.org> <87y25n6o70.wl-maz@kernel.org> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <87y25n6o70.wl-maz@kernel.org> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20211118_183835_805340_B38B96A8 X-CRM114-Status: GOOD ( 54.99 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Wed, Nov 17, 2021 at 11:01:23AM +0000, Marc Zyngier wrote: > On Wed, 17 Nov 2021 10:16:53 +0000, > Pingfan Liu wrote: > > > > On Tue, Nov 16, 2021 at 09:53:25AM +0000, Marc Zyngier wrote: > > > Hi Pingfan, > > > > > > On Tue, 16 Nov 2021 08:24:49 +0000, > > > Pingfan Liu wrote: > > > > > > > > Arch level code is ready to take over the nmi_enter()/nmi_exit() > > > > housekeeping. > > > > > > > > GICv3 can expose the pNMI discriminator, then simply remove the > > > > housekeeping. > > > > > > > > Signed-off-by: Pingfan Liu > > > > Cc: Catalin Marinas > > > > Cc: Will Deacon > > > > Cc: Mark Rutland > > > > Cc: Marc Zyngier > > > > Cc: Joey Gouly > > > > Cc: Sami Tolvanen > > > > Cc: Julien Thierry > > > > Cc: Yuichi Ito > > > > Cc: rcu@vger.kernel.org > > > > To: linux-arm-kernel@lists.infradead.org > > > > --- > > > > drivers/irqchip/irq-gic-v3.c | 18 ++++++++++++------ > > > > 1 file changed, 12 insertions(+), 6 deletions(-) > > > > > > > > diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c > > > > index daec3309b014..aa2bcb47b47e 100644 > > > > --- a/drivers/irqchip/irq-gic-v3.c > > > > +++ b/drivers/irqchip/irq-gic-v3.c > > > > @@ -646,12 +646,8 @@ static void gic_deactivate_unhandled(u32 irqnr) > > > > > > > > static inline void gic_handle_nmi(u32 irqnr, struct pt_regs *regs) > > > > { > > > > - bool irqs_enabled = interrupts_enabled(regs); > > > > int err; > > > > > > > > - if (irqs_enabled) > > > > - nmi_enter(); > > > > - > > > > if (static_branch_likely(&supports_deactivate_key)) > > > > gic_write_eoir(irqnr); > > > > /* > > > > @@ -664,8 +660,6 @@ static inline void gic_handle_nmi(u32 irqnr, struct pt_regs *regs) > > > > if (err) > > > > gic_deactivate_unhandled(irqnr); > > > > > > > > - if (irqs_enabled) > > > > - nmi_exit(); > > > > } > > > > > > > > static u32 do_read_iar(struct pt_regs *regs) > > > > @@ -702,6 +696,15 @@ static u32 do_read_iar(struct pt_regs *regs) > > > > return iar; > > > > } > > > > > > > > +static bool gic_is_in_nmi(void) > > > > +{ > > > > + if (gic_supports_nmi() && > > > > + unlikely(gic_read_rpr() == GICD_INT_RPR_PRI(GICD_INT_NMI_PRI))) > > > > + return true; > > > > > > I don't think this fixes anything. > > > > > > RPR stands for 'Running Priority Register', which in GIC speak reports > > > the priority of the most recently Ack'ed interrupt. > > > > > > You cannot use this to find out whether the interrupt that you /will/ > > > ack is a NMI or not. Actually, you cannot find out about *any* > > > priority until you actually ack the interrupt. What you are asking for > > > is the equivalent of a crystal ball, and we're in short supply... ;-) > > > > > > The only case where ICC_RPR_EL1 will return something that is equal to > > > GICD_INT_NMI_PRI is when you are *already* in an NMI context. So > > > unless I have completely misunderstood your approach (which is always > > > possible), I don't see how this can work. > > > > > > > Thank you for the clear explanation. Also I revist this part in "GIC v3 > > and v4 overview" and have a deeper understanding. (Need to spare time to > > go through all later) > > > > You totally got my idea, and I need to find a bail-out. > > > > As all kinds of PIC at least have two parts of functions: active (Ack) and > > deactive(EOI), is it possible to split handle_arch_irq into two parts? > > I.e let irqchip expose two interfaces: > > u32 (*read_irqinfo*)(struct pt_regs *regs, bool *is_nmi) > > void (*handle_arch_irq)(struct pt_regs *regs, u32 irqnr) > > to replace the current interface: > > No. There is no way we will move to such a scheme. We want to isolate > the irqchip stuff in its own corner, and not propagate it into the > arch code. I understood this beautiful design of isolation. But since the x86 and powerpc have integrated the PIC as part of arch at hardware design, they can tell the source of the interrupt at the arch level code. That is why their IPI_RESCHEDULE is cheaper than arm64. But yeah, the benifit may not persuasive enough to breach the design goal. > > If the pseudo-NMI is such a problem, I'm all for removing it *now*, > never to come back again. > It is a pity that pNMI has not fulfilled the roles like other Arches yet. Two benefits come with NMI on x86 and powerpc: -1. hardlockup detector uses NMI to ease the detection and analysis of unpaired irq enable/disable. It can accurately report the stack trace -2. In kdump case, all cpus can be forced into a known code piece. But on arm64, it does not play such a role yet. Once I hit a boot hang issue on a customed kdump kernel, there is no tick irq after adding some printk, and if there is hardlockup detector, it will be easy to debug. > > void (*handle_arch_irq)(struct pt_regs *regs) > > > > I have thought about such stuff for some days. And the benefits include: > > -1. For this bugfix (by the parameter 'is_nmi') > > -2. IPI_RESCHEDULE performance drop issue can be resolved at arch > > code level. (by irqnr - ipi_irq_base == IPI_RESCHEDULE ?) > > -3. The arch level can provide a similar loop as aic_handle_irq() in irq-apple-aic.c, > > which can save cpu by avoiding heavy context sync when irq is > > intensive. > > > > Do you think it is doable? > > I don't think this is even needed, because I don't believe that the > whole thing is a real problem. > > In patch #1, you are claiming that handling a NMI as an IRQ first, and > then upgrading to NMI once we know it really is an NMI is a problem. > How different is this from an IRQ being preempted by a NMI? > It turned out a false alarm as my reply to Mark. In that case, the code should take __el1_pnmi(), but I mistake it as __el1_irq(). Thank again for your time and patient reply. Regards, Pingfan > Your own conclusion is that the this later case isn't a problem. So > why is the former an issue? > > I'm not saying that there is no issue at all, and it could well be > that you have spotted something that I cannot see yet. But if that's > the case, it means that the core code is broken as well. > > M. > > -- > Without deviation from the norm, progress is not possible. > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel