[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <865xtf4woi.wl-maz@kernel.org>
Date: Mon, 08 Jul 2024 18:31:57 +0100
From: Marc Zyngier <maz@...nel.org>
To: Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>
Cc: tglx@...utronix.de,
linux-arm-kernel@...ts.infradead.org,
linux-kernel@...r.kernel.org
Subject: Re: MSIs not freed in GICv3 ITS driver
Mani,
On Mon, 08 Jul 2024 16:39:33 +0100,
Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org> wrote:
>
> Hi Marc, Thomas,
>
> I'm seeing a weird behavior with GICv3 ITS driver while allocating MSIs from
> PCIe devices. When the PCIe driver (I'm using virtio_pci_common.c) tries to
> allocate non power of 2 MSIs (like 3), then the GICv3 MSI driver always rounds
> the MSI count to power of 2 to find the order. In this case, the order becomes 2
> in its_alloc_device_irq().
That's because we can only allocate EventIDs as a number of ID
bits. So you can't have *1* MSI, nor 3. You can have 2, 4, 8, or
2^24. This is a power-of-two architecture.
> So 4 entries are allocated by bitmap_find_free_region().
Assuming you're calling about its_alloc_device_irq(), it looks like a
bug. Or rather, some laziness on my part. The thing is, this bitmap is
only dealing with sub-allocation in the pool that has been given to
the endpoint. So the power-of-two crap doesn't really matter unless
you are dealing with Multi-MSI, which has actual alignment
requirements.
>
> But since the PCIe driver has only requested 3 MSIs, its_irq_domain_alloc()
> will only allocate 3 MSIs, leaving one bitmap entry unused.
>
> And when the driver frees the MSIs using pci_free_irq_vectors(), only 3
> allocated MSIs were freed and their bitmap entries were also released. But the
> entry for the additional bitmap was never released. Due to this,
> its_free_device() was also never called, resulting in the ITS device not getting
> freed.
>
> So when the PCIe driver tries to request the MSIs again (PCIe device being
> removed and inserted back), because the ITS device was not freed previously,
> MSIs were again requested for the same ITS device. And due to the stale bitmap
> entry, the ITS driver refuses to allocate 4 MSIs as only 3 bitmap entries were
> available. This forces the PCIe driver to reduce the MSI count, which is sub
> optimal.
>
> This behavior might be applicable to other irqchip drivers handling MSI as well.
> I want to know if this behavior is already known with MSI and irqchip drivers?
>
> For fixing this issue, the PCIe drivers could always request MSIs of power of 2,
> and use a dummy MSI handler for the extra number of MSIs allocated. This could
> also be done in the generic MSI driver itself to avoid changes in the PCIe
> drivers. But I wouldn't say it is the best possible fix.
No, that's terrible. This is just papering over a design mistake, and
I refuse to go down that road.
>
> Is there any other way to address this issue? Or am I missing something
> completely?
Well, since each endpoint handled by an ITS has its allocation tracked
by a bitmap, it makes more sense to precisely track the allocation.
Here's a quick hack that managed to survive a VM boot. It may even
work. The only problem with it is that it probably breaks a Multi-MSi
device sitting behind a non-transparent bridge that would get its MSIs
allocated after another device. In this case, we wouldn't honor the
required alignment and things would break.
So take this as a proof of concept. If that works, I'll think of how
to deal with this crap in a more suitable way...
M.
diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
index 3c755d5dad6e6..43479c9e7f8d2 100644
--- a/drivers/irqchip/irq-gic-v3-its.c
+++ b/drivers/irqchip/irq-gic-v3-its.c
@@ -3475,15 +3475,16 @@ static void its_free_device(struct its_device *its_dev)
static int its_alloc_device_irq(struct its_device *dev, int nvecs, irq_hw_number_t *hwirq)
{
- int idx;
+ unsigned long idx;
/* Find a free LPI region in lpi_map and allocate them. */
- idx = bitmap_find_free_region(dev->event_map.lpi_map,
- dev->event_map.nr_lpis,
- get_count_order(nvecs));
- if (idx < 0)
+ idx = bitmap_find_next_zero_area(dev->event_map.lpi_map,
+ dev->event_map.nr_lpis, 0, nvecs, 0);
+ if (idx >= dev->event_map.nr_lpis)
return -ENOSPC;
+ bitmap_set(dev->event_map.lpi_map, idx, nvecs);
+
*hwirq = dev->event_map.lpi_base + idx;
return 0;
@@ -3653,9 +3654,9 @@ static void its_irq_domain_free(struct irq_domain *domain, unsigned int virq,
struct its_node *its = its_dev->its;
int i;
- bitmap_release_region(its_dev->event_map.lpi_map,
- its_get_event_id(irq_domain_get_irq_data(domain, virq)),
- get_count_order(nr_irqs));
+ bitmap_clear(its_dev->event_map.lpi_map,
+ its_get_event_id(irq_domain_get_irq_data(domain, virq)),
+ nr_irqs);
for (i = 0; i < nr_irqs; i++) {
struct irq_data *data = irq_domain_get_irq_data(domain,
--
Without deviation from the norm, progress is not possible.
Powered by blists - more mailing lists