Re: [PATCH v1] irqchip: gic-v2m: Handle Multiple MSI base IRQ Alignment

[Marc Zyngier <maz@kernel.org>]

On Mon, 11 Aug 2025 11:39:42 +0100,
Christian Bruel <christian.bruel@foss.st.com> wrote:
> 
> The PCI Local Bus Specification (section 6.8.3.4 in Rev 3) permits
> modifying the low-order bits of the DATA register to encode the interrupt
> number. These bits must be reserved, but the base SPI may not be aligned to
> the requested number of SPIs.

The PCI spec knows nothing of SPIs.

> For example, with an initial MSI_TYPER base SPI of 0x16A and allocating a
> multiple MSI of size 8, the offset returned is 8, resulting in an MSI DATA
> base of 0x172.
> This causes the endpoint device to send interrupt 3 wrong interrupt number:
>

Please use the correct terminology: an interrupt is a signal delivered
to the CPU. A *message* is what the device sends, which the GIC turns
into an interrupt. Here, you are using the same word for two different
things.

The problem is that the device encodes a delta from a base message,
that the delta is encoded with log2(nr_vectors) bits, OR'ing the
vector number with the base message. If the base message is not
correctly aligned, shit happens.

> 1st MSI = 0x172 | 0x0 = 0x172
> 2nd MSI = 0x172 | 0x1 = 0x173
> 3rd MSI = 0x172 | 0x2 = 0x172 wrongly triggers the 1st MSI
> ...
> 
> To fix this, use bitmap_find_next_zero_area_off() instead of
> bitmap_find_free_region() applying an initial offset of
> base_spi - rounded(base_spi, nr_irqs) to accommodate the required alignment
> for the first MSI.
> 
> With the above case, the returned bitmap offset is 6 which results in the
> correct interrupts number encoding:
> 
> 1st MSI = 0x170 | 0x0 = 0x170
> 2nd MSI = 0x170 | 0x1 = 0x171
> 3rd MSI = 0x170 | 0x2 = 0x172
> ...

Please rephrase this commit message so that it actually makes sense.
We shouldn't need examples if the commit message was correctly
written.

> 
> Signed-off-by: Christian Bruel <christian.bruel@foss.st.com>
> ---
> Changes in v1:
>    (Marc Zyngier)
>  - Replace the incorrect usage of msi_attrib.multiple with nr_irqs
>  - Reworked changelog
> ---
>  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..2d5cf36340b1 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_off, offset;

Move the definition of align_off inside the loop.

> +	unsigned long align_mask = nr_irqs - 1;
>  
>  	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 - (tmp->spi_start & ~align_mask);
> +		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;
>  		}
>  	}

Isn't the GICv3 MBI driver affected by the same issue?

	M.

-- 
Jazz isn't dead. It just smells funny.