[PATCH] PCI: brcmstb: Reserve only the MSI vectors that are handed out

[PATCH] PCI: brcmstb: Reserve only the MSI vectors that are handed out

[Sangwoo Han]

brcm_msi_alloc() reserves a naturally aligned power-of-two region with
bitmap_find_free_region(order_base_2(nr_irqs)), but the irqdomain core
releases the vectors of a block one at a time:

  /* irq_domain_free_irqs_hierarchy() */
  for (i = 0; i < nr_irqs; i++)
          if (irq_domain_get_irq_data(domain, irq_base + i))
                  domain->ops->free(domain, irq_base + i, 1);

That loop is the only caller of an irq_domain's ops->free(), so
brcm_irq_domain_free() always sees nr_irqs == 1 and
bitmap_release_region() clears exactly one bit per call. A request whose
vector count is not a power of two therefore reserves
roundup_pow_of_two(nr_irqs) bits but releases only nr_irqs of them, and
the difference stays set for the lifetime of the controller.

Multi-MSI regions are order-aligned and the controller has at most 32
MSIs, so the pool is quickly exhausted. Observed on a BCM2712 with a
5-vector endpoint behind a 4-port PCIe switch: the switch ports take
hwirq 0x0-0x3 and the endpoint's block walks 0x8 -> 0x10 -> 0x18 across
three driver reloads until no aligned order-3 region is left.
pci_alloc_irq_vectors() then falls back to a single vector for the rest
of the boot, silently multiplexing the endpoint's four completion
interrupts onto one hwirq.

Devices that ask for a non-power-of-two vector count are not exotic:
wil6210 asks for 3, the MHI modems for 5 (Quectel EM1xx, Foxconn SDX55,
Telit FN990, MediaTek MV3x) or 7 (Qualcomm v1), ath11k WCN6750 for 28
and ptp_ocp for 17. MSI-X is unaffected because it allocates one
descriptor per vector with nvec_used == 1, so the order is always zero.

Reserve exactly the vectors that are handed out, at a base found with
bitmap_find_next_zero_area(), and clear exactly the vectors that are
freed. The number of reserved bits then matches the number the core
releases, whatever arity it uses.

The base still has to be aligned: PCI Local Bus Specification 3.0
(section 6.8.1.6) lets the endpoint encode the vector number in the low
order_base_2(nr_irqs) bits of the Message Data register, and in this
controller those bits are the hwirq itself. The alignment mask has to be
roundup_pow_of_two(nr_irqs) - 1 rather than nr_irqs - 1:
bitmap_find_next_zero_area() requires a mask of the form 2^k - 1. No
align_offset is needed because the bitmap index is the value the
endpoint ORs in, see brcm_msi_compose_msi_msg().

For a power-of-two nr_irqs the alignment and the region length are both
nr_irqs, so the base returned is the same as before and those
allocations are unaffected.

Fixes: 198acab1772f ("PCI: brcmstb: Enable Multi-MSI")
Cc: stable@vger.kernel.org
Signed-off-by: Sangwoo Han <sangwoo.han@nearthlab.com>
---

Notes:
    Tested on a BCM2712 (Raspberry Pi 5) with a 5-vector endpoint behind a
    4-port PCIe switch, running 6.12.25 where brcm_msi_alloc() and
    brcm_msi_free() are byte-identical to mainline. Without the patch the
    endpoint's block walks 0x8 -> 0x10 -> 0x18 over three driver reloads and
    then falls back to a single vector for the rest of the boot; with it the
    block returns to 0x8 on all of eight reload cycles and the inner
    domain's mapped count round-trips cleanly.
    
    Compile-tested on mainline for arm64 with W=1 and C=1 (sparse); no new
    warnings.

 drivers/pci/controller/pcie-brcmstb.c | 17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
index 8a0c353d2a..6c5166666d 100644
--- a/drivers/pci/controller/pcie-brcmstb.c
+++ b/drivers/pci/controller/pcie-brcmstb.c
@@ -595,11 +595,22 @@ static struct irq_chip brcm_msi_bottom_irq_chip = {
 
 static int brcm_msi_alloc(struct brcm_msi *msi, unsigned int nr_irqs)
 {
+	/*
+	 * brcm_msi_compose_msi_msg() puts hwirq in the low order bits of the
+	 * message data, which a Multi-MSI endpoint rewrites per vector, so a
+	 * block's base must be aligned to the Multiple Message Enable count.
+	 */
+	unsigned long align_mask = roundup_pow_of_two(nr_irqs) - 1;
 	int hwirq;
 
 	mutex_lock(&msi->lock);
-	hwirq = bitmap_find_free_region(msi->used, msi->nr,
-					order_base_2(nr_irqs));
+	hwirq = bitmap_find_next_zero_area(msi->used, msi->nr, 0, nr_irqs,
+					   align_mask);
+	if (hwirq >= msi->nr) {
+		mutex_unlock(&msi->lock);
+		return -ENOSPC;
+	}
+	bitmap_set(msi->used, hwirq, nr_irqs);
 	mutex_unlock(&msi->lock);
 
 	return hwirq;
@@ -609,7 +620,7 @@ static void brcm_msi_free(struct brcm_msi *msi, unsigned long hwirq,
 			  unsigned int nr_irqs)
 {
 	mutex_lock(&msi->lock);
-	bitmap_release_region(msi->used, hwirq, order_base_2(nr_irqs));
+	bitmap_clear(msi->used, hwirq, nr_irqs);
 	mutex_unlock(&msi->lock);
 }
 
-- 
2.53.0

Re: [PATCH] PCI: brcmstb: Reserve only the MSI vectors that are handed out

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The patch only reserves `nr_irqs` bits instead of the full power-of-two block in the controller's bitmap, breaking hardware IRQ isolation and allowing cross-device IRQ collisions if trailing vectors are reallocated.
--

--- Patch [1]: [PATCH] PCI: brcmstb: Reserve only the MSI vectors that are handed out ---
commit 627a58c7a939ca8de261414f13dbb9fd516289b4
Author: Sangwoo Han <sangwoo.han@nearthlab.com>
Subject: PCI: brcmstb: Reserve only the MSI vectors that are handed out

This commit modifies the Broadcom STB PCIe MSI allocator to reserve and free
exactly the requested number of interrupt vectors rather than rounding up to
the nearest power of two. This aims to prevent exhaustion of the 32-vector
pool when endpoints request non-power-of-two vector counts.

> diff --git a/drivers/pci/controller/pcie-brcmstb.c b/drivers/pci/controller/pcie-brcmstb.c
> --- a/drivers/pci/controller/pcie-brcmstb.c
> +++ b/drivers/pci/controller/pcie-brcmstb.c
> @@ -595,11 +595,22 @@ static struct irq_chip brcm_msi_bottom_irq_chip = {
>  
>  static int brcm_msi_alloc(struct brcm_msi *msi, unsigned int nr_irqs)
>  {

[ ... ]

> -	hwirq = bitmap_find_free_region(msi->used, msi->nr,
> -					order_base_2(nr_irqs));
> +	hwirq = bitmap_find_next_zero_area(msi->used, msi->nr, 0, nr_irqs,
> +					   align_mask);
> +	if (hwirq >= msi->nr) {
> +		mutex_unlock(&msi->lock);
> +		return -ENOSPC;
> +	}
> +	bitmap_set(msi->used, hwirq, nr_irqs);

[Severity: High]
Does this leave the unallocated trailing vectors in the power-of-two block
vulnerable to cross-device IRQ collisions?

When a device requests a non-power-of-two number of MSIs, the generic PCI
core rounds the allocation up and programs the device's Multiple Message
Enable capability to the next power of two. The device is then
hardware-authorized to modify the lower bits of its MSI Message Data to
distinguish between vectors.

Because bitmap_set only reserves exactly nr_irqs bits, the remaining bits in
the device's hardware window remain marked as free in the allocator. If
another device is subsequently assigned one of these trailing vectors, can
the first device's interrupts be incorrectly routed to the second device?

>  	mutex_unlock(&msi->lock);
>  
>  	return hwirq;
>  }
>  
>  static void brcm_msi_free(struct brcm_msi *msi, unsigned long hwirq,
>  			  unsigned int nr_irqs)
>  {
>  	mutex_lock(&msi->lock);
> -	bitmap_release_region(msi->used, hwirq, order_base_2(nr_irqs));
> +	bitmap_clear(msi->used, hwirq, nr_irqs);

[Severity: High]
If the allocation needs to reserve the full power-of-two block to prevent
collisions, does this also need to clear the full power-of-two block when
the vectors are freed?

>  	mutex_unlock(&msi->lock);
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260730072215.2090974-1-sangwoo.han@nearthlab.com?part=1

Re: [PATCH] PCI: brcmstb: Reserve only the MSI vectors that are handed out

[Han / 한상우Sangwoo]

> Because bitmap_set only reserves exactly nr_irqs bits, the remaining bits in
> the device's hardware window remain marked as free in the allocator. If
> another device is subsequently assigned one of these trailing vectors, can
> the first device's interrupts be incorrectly routed to the second device?

No. The PCI/MSI core masks every vector the endpoint is capable of and
then unmasks only the ones that were handed out, so the endpoint cannot
signal the trailing vectors of its window.

msi_capability_init() masks the full capable range:

  /* drivers/pci/msi/msi.c */
  /* All MSIs are unmasked by default; mask them all */
  entry = msi_first_desc(&dev->dev, MSI_DESC_ALL);
  pci_msi_mask(entry, msi_multi_mask(entry));

And each allocated vector is unmasked individually:

  /* drivers/pci/msi/irqdomain.c */
  pci_msi_unmask(desc, BIT(data->irq - desc->irq));

This is observable in config space on the endpoint I tested. With 5
vectors handed out at hwirq 0x8 and Multiple Message Enable programmed to
8, the Mask Bits register reads 0x0000ffe0: bits 0-4 (the vectors in use)
open, bits 5-15 masked. Messages 5-7, which would land on hwirq 0xd-0xf,
are blocked at the device.

Reserving the full power-of-two block does not provide isolation either;
it just never releases it. The irqdomain core calls ops->free() once per
vector with nr_irqs == 1, so bitmap_release_region(order_base_2(1))
clears a single bit per call, and roundup_pow_of_two(nr_irqs) - nr_irqs
bits stay set for the lifetime of the controller. That is the leak this
patch fixes.

irqchip/gic-v2m.c and irqchip/irq-alpine-msi.c already reserve exactly
nr_irqs this way. For instance, gic-v2m moved to
bitmap_find_next_zero_area_off() + bitmap_set(nr_irqs) in commit
2ef3886ce626 ("irqchip/gic-v2m: Handle Multiple MSI base IRQ Alignment").

Thanks,
Sangwoo