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

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 at 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().

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