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 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 smtp.lore.kernel.org (Postfix) with ESMTPS id 06B2FC52D7C for ; Tue, 13 Aug 2024 10:34:48 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender:List-Subscribe:List-Help :List-Post:List-Archive:List-Unsubscribe:List-Id:In-Reply-To:Content-Type: MIME-Version:References:Message-ID:Subject:Cc:To:From:Date:Reply-To: Content-Transfer-Encoding:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=/cDKal9J73iL+7s1mHmp6H+VKXEoc+H1zTCcXXsc6s4=; b=4Gdk1krL1HNOjPkqY4YN0WHCWR 7EVASwIr3+Fi9SKxLCmNU7zWdPXDRwai9ibduYwkawKqXKjbemQGAv13qGoibfAkuoWjwuETqqyH/ Ockh6CZ7nbV55YZ6p9FqA3ppyfGn06JeKmULBSEpeI468RWjppkGZHaMxZZsphGaQJ25dYL7/CPlb 0CB2OYMTB8neXX81ruwAnwht3DN8dBrgJzBU/e5fpBYVL8pbex4Rnct2+BWJYok247Lo+CGKRcXlh FnSgJCmSdBKY8ghBdGam8W72BPQUBB0eiX28X+V32+1Rhlu7BJ97Ay77aSoh/yuP/YmZwxnfRSI3H RZsD+NNQ==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.97.1 #2 (Red Hat Linux)) id 1sdorL-00000003LvE-2GlY; Tue, 13 Aug 2024 10:34:35 +0000 Received: from foss.arm.com ([217.140.110.172]) by bombadil.infradead.org with esmtp (Exim 4.97.1 #2 (Red Hat Linux)) id 1sdoqi-00000003LmA-0SNL for linux-arm-kernel@lists.infradead.org; Tue, 13 Aug 2024 10:33:57 +0000 Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 3410312FC; Tue, 13 Aug 2024 03:34:18 -0700 (PDT) Received: from bogus (e103737-lin.cambridge.arm.com [10.1.197.49]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id D0F073F587; Tue, 13 Aug 2024 03:33:50 -0700 (PDT) Date: Tue, 13 Aug 2024 11:33:47 +0100 From: Sudeep Holla To: Marc Zyngier Cc: Shanker Donthineni , Thomas Gleixner , Catalin Marinas , Will Deacon , Jonathan Corbet , Sudeep Holla , , Subject: Re: [PATCH] irqchip/gic-v3: Allow unused SGIs for drivers/modules Message-ID: References: <20240813033925.925947-1-sdonthineni@nvidia.com> <86zfpgztmt.wl-maz@kernel.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <86zfpgztmt.wl-maz@kernel.org> X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20240813_033356_276985_F8A3DEBB X-CRM114-Status: GOOD ( 36.57 ) 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: , Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Tue, Aug 13, 2024 at 09:58:34AM +0100, Marc Zyngier wrote: > On Tue, 13 Aug 2024 04:39:25 +0100, > Shanker Donthineni wrote: > > > > The commit 897e9e60c016 ("firmware: arm_ffa: Initial support for scheduler > > receiver interrupt") adds support for SGI interrupts in the FFA driver. > > However, the validation for SGIs in the GICv3 is too strict, causing the > > driver probe to fail. > > It probably is a good thing that I wasn't on Cc for this patch, > because I would have immediately NAK'd it. Sudeep, please consider > this a retrospective NAK! > Sure, I am happy to work on any suggestions to replace it with better/cleaner solution. > > > > This patch relaxes the SGI validation check, allowing callers to use SGIs > > if the requested SGI number is greater than or equal to MAX_IPI, which > > fixes the TFA driver probe failure. > > > > This issue is observed on NVIDIA server platform with FFA-v1.1. > > [ 7.918099] PTP clock support registered > > [ 7.922110] EDAC MC: Ver: 3.0.0 > > [ 7.945063] ARM FF-A: Driver version 1.1 > > [ 7.949068] ARM FF-A: Firmware version 1.1 found > > [ 7.977832] GICv3: [Firmware Bug]: Illegal GSI8 translation request > > [ 7.984237] ARM FF-A: Failed to create IRQ mapping! > > [ 7.989220] ARM FF-A: Notification setup failed -61, not enabled > > [ 8.000198] ARM FF-A: Failed to register driver sched callback -95 > > [ 8.011322] scmi_core: SCMI protocol bus registered > > > > Signed-off-by: Shanker Donthineni > > --- > > arch/arm64/include/asm/arch_gicv3.h | 17 +++++++++++++++++ > > arch/arm64/kernel/smp.c | 17 ----------------- > > drivers/irqchip/irq-gic-v3.c | 2 +- > > 3 files changed, 18 insertions(+), 18 deletions(-) > > > > diff --git a/arch/arm64/include/asm/arch_gicv3.h b/arch/arm64/include/asm/arch_gicv3.h > > index 9e96f024b2f19..ecf81df2915c7 100644 > > --- a/arch/arm64/include/asm/arch_gicv3.h > > +++ b/arch/arm64/include/asm/arch_gicv3.h > > @@ -188,5 +188,22 @@ static inline bool gic_has_relaxed_pmr_sync(void) > > return cpus_have_cap(ARM64_HAS_GIC_PRIO_RELAXED_SYNC); > > } > > > > +enum ipi_msg_type { > > + IPI_RESCHEDULE, > > + IPI_CALL_FUNC, > > + IPI_CPU_STOP, > > + IPI_CPU_CRASH_STOP, > > + IPI_TIMER, > > + IPI_IRQ_WORK, > > + NR_IPI, > > + /* > > + * Any enum >= NR_IPI and < MAX_IPI is special and not tracable > > + * with trace_ipi_* > > + */ > > + IPI_CPU_BACKTRACE = NR_IPI, > > + IPI_KGDB_ROUNDUP, > > + MAX_IPI > > +}; > > + > > #endif /* __ASSEMBLY__ */ > > #endif /* __ASM_ARCH_GICV3_H */ > > diff --git a/arch/arm64/kernel/smp.c b/arch/arm64/kernel/smp.c > > index 5e18fbcee9a20..373cd815d9a43 100644 > > --- a/arch/arm64/kernel/smp.c > > +++ b/arch/arm64/kernel/smp.c > > @@ -64,23 +64,6 @@ struct secondary_data secondary_data; > > /* Number of CPUs which aren't online, but looping in kernel text. */ > > static int cpus_stuck_in_kernel; > > > > -enum ipi_msg_type { > > - IPI_RESCHEDULE, > > - IPI_CALL_FUNC, > > - IPI_CPU_STOP, > > - IPI_CPU_CRASH_STOP, > > - IPI_TIMER, > > - IPI_IRQ_WORK, > > - NR_IPI, > > - /* > > - * Any enum >= NR_IPI and < MAX_IPI is special and not tracable > > - * with trace_ipi_* > > - */ > > - IPI_CPU_BACKTRACE = NR_IPI, > > - IPI_KGDB_ROUNDUP, > > - MAX_IPI > > -}; > > - > > static int ipi_irq_base __ro_after_init; > > static int nr_ipi __ro_after_init = NR_IPI; > > static struct irq_desc *ipi_desc[MAX_IPI] __ro_after_init; > > diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c > > index c19083bfb9432..0d2038d8cd311 100644 > > --- a/drivers/irqchip/irq-gic-v3.c > > +++ b/drivers/irqchip/irq-gic-v3.c > > @@ -1655,7 +1655,7 @@ static int gic_irq_domain_translate(struct irq_domain *d, > > if(fwspec->param_count != 2) > > return -EINVAL; > > > > - if (fwspec->param[0] < 16) { > > + if (fwspec->param[0] < MAX_IPI) { > > pr_err(FW_BUG "Illegal GSI%d translation request\n", > > fwspec->param[0]); > > return -EINVAL; > > No. This is the wrong approach, and leads to inconsistent behaviour if > we ever change this MAX_IPI value. It also breaks 32 bit builds, and > makes things completely inconsistent between ACPI and DT. > > I don't know how the FFA code was tested, because I cannot see how it > can work. > > *IF* we are going to allow random SGIs being requested by random > drivers, we need to be able to do it properly. Not as a side hack like > this. I am open for any ideas as FF-A spec authors/architects decided to allow secure world to donate one of its SGI to the normal world for FF-A notifications. -- Regards, Sudeep