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 B6ABEC7115B for ; Mon, 23 Jun 2025 08:01:35 +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=dodbzlqA9pg+Xv3rARuhyF8DlYA03JGA7gzYMEsEDP8=; b=hezjXhPEGsbK7ivgfK1skRluR2 KBLGJ4zW0AsVGuSPe9KD060kyX6Qi8GB/Dp5hlzGTFYqqZpHCjpERK3/8HYB+tX3geVxuPPbxwwcd Mbi3tOglcecVKFrTs/2cpnumog30fuAkIkduxGO9QBbxeRmSL0ruQ1mUkmSNrJya9oUgNtxx247fE 2zVmjAO3WgKAnTwCTByE+PNYDj1A8txPbjVFgVGmqJsIWBV05OPzH/dK+3d1llnu9OPOKSksPTkVk GQ8gW0Entc1sjU5IYGEq/3FPzFJ4bmXaL9b/LnNYTqSj2dM52sI7fBmAl6UaC/XghMIj3Y3jTqgac EyY517ig==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.98.2 #2 (Red Hat Linux)) id 1uTc7O-00000001wiP-0vR7; Mon, 23 Jun 2025 08:01:30 +0000 Received: from tor.source.kernel.org ([172.105.4.254]) by bombadil.infradead.org with esmtps (Exim 4.98.2 #2 (Red Hat Linux)) id 1uTbqV-00000001trc-43wJ for linux-arm-kernel@lists.infradead.org; Mon, 23 Jun 2025 07:44:04 +0000 Received: from smtp.kernel.org (transwarp.subspace.kernel.org [100.75.92.58]) by tor.source.kernel.org (Postfix) with ESMTP id 3D90161137; Mon, 23 Jun 2025 07:44:03 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 177E5C4CEED; Mon, 23 Jun 2025 07:43:57 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1750664642; bh=FvVZWSxbxCHHvArP/ksDKZe8srWR5WNbNBaWxp4Q1Aw=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=dHl251/wXN40lIxPpKuiJLq7g+cRz0OPuZjvkBC/xMaROyDdQ8w4ZrZ/Jm0YFW3B0 mOLWWQvceF45KymGEVE7huhiVsitvfvHZoqe9cLRWhFQa1h8Xq7gyb/5t7qcKADrEK XPNOJcoTrNQyyyS6RTkFIj4cWsqzhujE3Bf3xMCGOBxu1Mein/CGPhzl0rxrcglnu3 HiEzJCBfhvIS9si++V+jD7MCmeGmvszcG84dSFsEWCX0GVnJFRY6h9VRHWLjXjGroh n/d+u4osjwOnWk+14GQaPkcuA7kpeSpmiNNkZzW+gVhdMoNGbIv3g0V1iM6jMIEOe8 hsFP1L+eIkm6w== Date: Mon, 23 Jun 2025 09:43:55 +0200 From: Lorenzo Pieralisi To: Thomas Gleixner Cc: Marc Zyngier , Rob Herring , Krzysztof Kozlowski , Conor Dooley , Catalin Marinas , Will Deacon , Arnd Bergmann , Sascha Bischoff , Jonathan Cameron , Timothy Hayes , Bjorn Helgaas , "Liam R. Howlett" , Peter Maydell , Mark Rutland , Jiri Slaby , linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, devicetree@vger.kernel.org, linux-pci@vger.kernel.org Subject: Re: [PATCH v5 24/27] irqchip/gic-v5: Add GICv5 ITS support Message-ID: References: <20250618-gicv5-host-v5-0-d9e622ac5539@kernel.org> <20250618-gicv5-host-v5-24-d9e622ac5539@kernel.org> <87y0tmp6gn.ffs@tglx> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <87y0tmp6gn.ffs@tglx> 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 Fri, Jun 20, 2025 at 09:18:32PM +0200, Thomas Gleixner wrote: > On Wed, Jun 18 2025 at 12:17, Lorenzo Pieralisi wrote: > > drivers/of/irq.c | 21 + > > drivers/pci/msi/irqdomain.c | 19 + > > include/linux/msi.h | 5 + > > include/linux/of_irq.h | 7 + > > This are preparatory changes. Please split them out into a seperate patch. Done. I did not do it because some maintainers require changes to be added in the patch(es) that uses them. > > ...3-its-msi-parent.c => irq-gic-its-msi-parent.c} | 187 ++- > > drivers/irqchip/irq-gic-its-msi-parent.h | 14 + > > drivers/irqchip/irq-gic-v3-its.c | 3 +- > > Ditto, i.e. the rename and code move, not the v5 add ons. Done. > > +static bool its_v5_init_dev_msi_info(struct device *dev, struct irq_domain *domain, > > + struct irq_domain *real_parent, struct msi_domain_info *info) > > +{ > > + if (!msi_lib_init_dev_msi_info(dev, domain, real_parent, info)) > > + return false; > > + > > + switch (info->bus_token) { > > + case DOMAIN_BUS_PCI_DEVICE_MSI: > > + case DOMAIN_BUS_PCI_DEVICE_MSIX: > > + /* > > + * FIXME: This probably should be done after a (not yet > > + * existing) post domain creation callback once to make > > + * support for dynamic post-enable MSI-X allocations > > + * work without having to reevaluate the domain size > > + * over and over. It is known already at allocation > > + * time via info->hwsize. > > + * > > + * That should work perfectly fine for MSI/MSI-X but needs > > + * some thoughts for purely software managed MSI domains > > + * where the index space is only limited artificially via > > + * %MSI_MAX_INDEX. > > > This comment is stale after Marc moved the prepare callback into > the domain creation, where the prepare callback gets hwsize for scaling. > > The only valid caveat are software managed domains, where hwsize is > unspecified, but that's a different problem (and not used as of today). Right, I should have removed it. Done. > > + */ > > + info->ops->msi_prepare = its_v5_pci_msi_prepare; > > + info->ops->msi_teardown = its_msi_teardown; > > + break; > > + case DOMAIN_BUS_DEVICE_MSI: > > + case DOMAIN_BUS_WIRED_TO_MSI: > > + /* > > + * FIXME: See the above PCI prepare comment. The domain > > + * size is also known at domain creation time. > > + */ > > See above. > > > +void gicv5_irs_syncr(void) > > +{ > > + struct gicv5_irs_chip_data *irs_data; > > + u32 syncr; > > + > > + irs_data = list_first_entry_or_null(&irs_nodes, > > + struct gicv5_irs_chip_data, entry); > > Let it stick out. You have 100 characters. Done. > > + if (WARN_ON(!irs_data)) > > WARN_ON_ONCE() ? Done. > > > +static unsigned int gicv5_its_l2sz_to_l2_bits(unsigned int sz) > > +{ > > + switch (sz) { > > + case GICV5_ITS_DT_ITT_CFGR_L2SZ_64k: > > + return 13; > > + case GICV5_ITS_DT_ITT_CFGR_L2SZ_16k: > > + return 11; > > + case GICV5_ITS_DT_ITT_CFGR_L2SZ_4k: > > + default: > > + return 9; > > Magic numbers pulled out of thin air? I will add defines but I thought the function name would be enough to tag the value with a meaning. > > +static __le64 *gicv5_its_device_get_itte_ref(struct gicv5_its_dev *its_dev, > > + u16 event_id) > > +{ > > + unsigned int l1_idx, l2_idx, l2_bits; > > + __le64 *l2_itt, *l1_itt; > > + > > + if (!its_dev->itt_cfg.l2itt) { > > + __le64 *itt = its_dev->itt_cfg.linear.itt; > > + > > + return &itt[event_id]; > > + } > > + > > + l1_itt = its_dev->itt_cfg.l2.l1itt; > > + > > + l2_bits = gicv5_its_l2sz_to_l2_bits(its_dev->itt_cfg.l2.l2sz); > > + > > + l1_idx = event_id >> l2_bits; > > + > > + BUG_ON(!FIELD_GET(GICV5_ITTL1E_VALID, le64_to_cpu(l1_itt[l1_idx]))); > > I assume this is truly unrecoverable At this stage, we mapped an IRQ and we are activating it, this must not happen (like any bug out there, granted). I can remove it, I understand it bothers you adding BUG_ON(), if something goes wrong there we will notice. > > + l2_idx = event_id & GENMASK(l2_bits - 1, 0); > > + > > + l2_itt = its_dev->itt_cfg.l2.l2ptrs[l1_idx]; > > + > > + return &l2_itt[l2_idx]; > > +} > > + > .... > > + > > + return 0; > > +out_free_lpi: > > It's amazing. All the code has a gazillion of empty newlines, which just > take up space and have no delimiting value. But on these error labels, > you glue them right at the return statement (not only here, noticed that > before). You have a point here, will change it (and I will check other error labels sites). > > + gicv5_free_lpi(lpi); > > +out_eventid: > > + gicv5_its_free_eventid(its_dev, event_id_base, nr_irqs); > > + > > + return ret; > > +} > > > + * Taken from msi_lib_irq_domain_select(). The only difference is that > > + * we have to match the fwspec->fwnode parent against the domain->fwnode > > + * in that in GICv5 the ITS domain is represented by the ITS fwnode but > > + * the MSI controller (ie the ITS frames) are ITS child nodes. > > + */ > > +static int gicv5_its_irq_domain_select(struct irq_domain *d, struct irq_fwspec *fwspec, > > + enum irq_domain_bus_token bus_token) > > +{ > > + const struct msi_parent_ops *ops = d->msi_parent_ops; > > + u32 busmask = BIT(bus_token); > > + > > + if (!ops) > > + return 0; > > + > > + if (fwnode_get_parent(fwspec->fwnode) != d->fwnode || > > + fwspec->param_count != 0) > > + return 0; > > Just add a MSI flag and set it in parent_ops::required_flags and extend > the lib with > > struct fwnode_handle *fwh; > > fwh = d->flags & MAGIC ? fwnode_get_parent(fwspec->fwnode) : fwspec->fwnode; > > No? I did not know if this use case granted a new flag. You cleared my concern. > > diff --git a/drivers/irqchip/irq-gic-v5.c b/drivers/irqchip/irq-gic-v5.c > > index c2e7ba7e38f7..4a0990f46358 100644 > > --- a/drivers/irqchip/irq-gic-v5.c > > +++ b/drivers/irqchip/irq-gic-v5.c > > @@ -57,12 +57,12 @@ static void release_lpi(u32 lpi) > > ida_free(&lpi_ida, lpi); > > } > > > > -static int gicv5_alloc_lpi(void) > > +int gicv5_alloc_lpi(void) > > { > > return alloc_lpi(); > > } > > > > -static void gicv5_free_lpi(u32 lpi) > > +void gicv5_free_lpi(u32 lpi) > > { > > release_lpi(lpi); > > } > > Just make them global right away when you implement them. No point for > this kind of churn. Yes, hopefully someone will not point out that in the respective patch the function could be static :). Thanks a lot for your review. Lorenzo