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

[Christian Bruel]

On 8/7/25 19:20, Marc Zyngier wrote:
> On Thu, 07 Aug 2025 12:47:58 +0100,
> Christian Bruel <christian.bruel@xxxxxxxxxxx> 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.

Not ideal, agree, but, afaik, alignment is not required to be aligned:

In ARM Base System Architecture Platform Design Document 1.2
"Base SPI number, bits [25:16]
Returns the IMPLEMENTATION DEFINED ID of the lowest SPI assigned to the 
frame. SPI ID values must be in the range 32 to 1020"

> > 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...

For this platform, we never allocate 32 Multiple MSI as the Multiple 
Message CAP is hw defined with 4 (16 Vectors)

OK, I remove the w/a from the log.

> > Change-Id: I316a580755cd1b1684929d2540295f4a45f0532d
>
> Neither does this.

Urgh

> > Signed-off-by: Christian Bruel <christian.bruel@xxxxxxxxxxx>
> > ---
> >   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().

Dropped, we can use nr_irq -1. nr_irqs is a powerof2.

> > +	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.

OK. Alignment on the max size is too restrictive

> 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?

Damned, I used multiple as a mask. lets rewrite and use nr_irqs instead, 
as you commented

align_off = basespi - align_down(basespi, nr_irqs)
align_mask = nr_irqs - 1

in the example:

basespi = 0x16A, nr_irqs = 8
align_off = 0x16A-(0X16A & ~7) = 2

bitmap_find_next_zero_area_off() returns the vector at offset 6, which 
allocates the multiple MSI at 0x170, basespi + offset, aligned according 
to the 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;
> >   		}
> >   	}
>
> 	M.

thank you,

Christian