Re: [PATCH] irqchip: gic-v2m: Handle multiple MSI base IRQ mis-alignment

[Marc Zyngier <maz@kernel.org>]

On Thu, 07 Aug 2025 12:47:58 +0100,
Christian Bruel <christian.bruel@foss.st.com> 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 <christian.bruel@foss.st.com>
> ---
>  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.