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

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

[Christian Bruel]

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.

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.

Change-Id: I316a580755cd1b1684929d2540295f4a45f0532d
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;
+	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;
+		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;
 		}
 	}
-- 
2.34.1

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

[Marc Zyngier]

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.

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