From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from smtp.kernel.org (aws-us-west-2-korg-mail-1.web.codeaurora.org [10.30.226.201]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id CD44B3208 for ; Thu, 7 Aug 2025 17:20:47 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=10.30.226.201 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1754587247; cv=none; b=Vkjm4OtaOZAN8rQBLn4eZ/kMRvrc3SNY/Ru3VgBKQFKxdsjIbTw1R8sqJFp4l69msI8dxIBY6k8KVSgjw07mQAcy+JMhea3JPXHuPlIu+tRn37/IFwA1KI5//qU/KNHjx5SEP5yUX/cZHnnwiMSdg5rt20rLEndTQa05cgyWW+M= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1754587247; c=relaxed/simple; bh=e2RyNcZjMIJHuZUIhTCHbHcsfF9RgB8S/NxnIlLipao=; h=Date:Message-ID:From:To:Cc:Subject:In-Reply-To:References: MIME-Version:Content-Type; b=mku/IT36/+VAd+W0EQJ3UcbVg6GLkMIQKbiiLhfHKfkW1iXey+3O2nMOGeKDncXLex4AGRcZArkycMWyi60X9ZJK1kTgUSWFGvJdM0bpF/Ns8nHu+QgSPr0Z0NTU/VnolqWc8cJGNFxn6ZhWQKhP/j5XwcFOdoD+pTmqkbzWewQ= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b=WA0tPp+8; arc=none smtp.client-ip=10.30.226.201 Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b="WA0tPp+8" Received: by smtp.kernel.org (Postfix) with ESMTPSA id 4135AC4CEED; Thu, 7 Aug 2025 17:20:47 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1754587247; bh=e2RyNcZjMIJHuZUIhTCHbHcsfF9RgB8S/NxnIlLipao=; h=Date:From:To:Cc:Subject:In-Reply-To:References:From; b=WA0tPp+8mfbmRWW0wF6J067wl2sj+05XIDGHPMpm3Qa1UJcHkJXqUDcLOSvgXU3t2 nJZm3Sb2OUQOX33P2Uv1D5/+vk1gZSuDpeI28o0lYIUFoIqOpRMXfo5Rynp2C5z+Je HtGDymzlBvUFok61JP3tL6EZgkPtvsypLWSbcvfw4wsFyDfJEpM14Bu2M3OvYYUEgg RS2/LDVDQ/NgnBZ4P5gbstkonqBTZ0bHplDiKKDX6WyJ3B8pI+UyUOcBAm5CQf1U4w jeD0st2RKwwWXLhf5qGi1ePTyk2B5PM/eR5IK3e2RFO51VQO5tgcDsvj6JNYSClDy+ udRSp/gtDBkhg== Received: from sofa.misterjones.org ([185.219.108.64] helo=goblin-girl.misterjones.org) by disco-boy.misterjones.org with esmtpsa (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.95) (envelope-from ) id 1uk4IH-0050gp-0r; Thu, 07 Aug 2025 18:20:45 +0100 Date: Thu, 07 Aug 2025 18:20:44 +0100 Message-ID: <8634a39hg3.wl-maz@kernel.org> From: Marc Zyngier To: Christian Bruel Cc: , , , , Subject: Re: [PATCH] irqchip: gic-v2m: Handle multiple MSI base IRQ mis-alignment In-Reply-To: <20250807114759.3966195-1-christian.bruel@foss.st.com> References: <20250807114759.3966195-1-christian.bruel@foss.st.com> User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI-EPG/1.14.7 (Harue) FLIM-LB/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL-LB/10.8 EasyPG/1.0.0 Emacs/30.1 (aarch64-unknown-linux-gnu) MULE/6.0 (HANACHIRUSATO) Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue") Content-Type: text/plain; charset=US-ASCII X-SA-Exim-Connect-IP: 185.219.108.64 X-SA-Exim-Rcpt-To: christian.bruel@foss.st.com, tglx@linutronix.de, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, fabrice.gasnier@foss.st.com, mani@kernel.org X-SA-Exim-Mail-From: maz@kernel.org X-SA-Exim-Scanned: No (on disco-boy.misterjones.org); SAEximRunCond expanded to false On Thu, 07 Aug 2025 12:47:58 +0100, Christian Bruel wrote: > > The PCI Local Bus Specification (section 6.8.3.4 in Rev 3) allows modifying > the low-order bits of the MSI Message Data to encode the interrupt number. > However, the base SPI used for allocation may not be aligned to the > requested number of irqs. > > For instance, on STM32MP25 with an initial MSI_TYPER base SPI of 0x16A, > allocating a multiple MSI of size 8 with the first two slots reserved, the > offset returned is 8, resulting in an MSI DATA base of 0x172. > This causes the endpoint device to send a wrong interrupt number: > > 1st MSI = 0x172 | 0x0 = 0x172 > 2nd MSI = 0x172 | 0x1 = 0x173 > 3rd MSI = 0x172 | 0x2 = 0x172 wrongly triggers the 1st MSI > > The alignment offset can be computed in the gicv2m driver: > replacing bitmap_find_free_region() with bitmap_find_next_zero_area_off() > to accommodate the required alignment. Well, that's only telling half of the story. We get there because the SPI range has been allocated in a pretty dumb manner (the starting SPI is not a multiple of 32). Pretty damning if you're considering PCI. > > Without this fix, the workaround is to force the alignment in the DT > within the MSI range (if 32 MSIs are mapped from 362 to 393): > arm,msi-base-spi = <368> > arm,msi-num-spis = <26> > with the effect of reducing the number of available MSIs. This doesn't really belong here. But the example is pretty wrong anyway, because what you need for PCI is the low 5 bits to be clear, so that you can allocate 32 MSIs. So your range would start at 384, and... oh wait... > > Change-Id: I316a580755cd1b1684929d2540295f4a45f0532d Neither does this. > Signed-off-by: Christian Bruel > --- > drivers/irqchip/irq-gic-v2m.c | 12 ++++++++---- > 1 file changed, 8 insertions(+), 4 deletions(-) > > diff --git a/drivers/irqchip/irq-gic-v2m.c b/drivers/irqchip/irq-gic-v2m.c > index 24ef5af569fe..21a14d15e7a9 100644 > --- a/drivers/irqchip/irq-gic-v2m.c > +++ b/drivers/irqchip/irq-gic-v2m.c > @@ -153,14 +153,18 @@ static int gicv2m_irq_domain_alloc(struct irq_domain *domain, unsigned int virq, > { > msi_alloc_info_t *info = args; > struct v2m_data *v2m = NULL, *tmp; > - int hwirq, offset, i, err = 0; > + int hwirq, i, err = 0; > + unsigned long align_mask = (1 << get_count_order(nr_irqs)) - 1; Use BIT(). > + unsigned long align_off, offset; > > spin_lock(&v2m_lock); > list_for_each_entry(tmp, &v2m_nodes, entry) { > - offset = bitmap_find_free_region(tmp->bm, tmp->nr_spis, > - get_count_order(nr_irqs)); > - if (offset >= 0) { > + align_off = tmp->spi_start & info->desc->pci.msi_attrib.multiple; Err, no. MSIs are not just PCI only, and there is no reason why you should go and rummage into these data structures. That's none of an irqchip's business. The correct way to go about this is to consider nr_irqs, because that's what you are trying to align for. But I really don't get how you compute that offset. 'multiple' is the number of bits required to encode the number of vectors. How does this work? > + offset = bitmap_find_next_zero_area_off(tmp->bm, tmp->nr_spis, 0, > + nr_irqs, align_mask, align_off); > + if (offset < tmp->nr_spis) { > v2m = tmp; > + bitmap_set(v2m->bm, offset, nr_irqs); > break; > } > } M. -- Without deviation from the norm, progress is not possible.