On Thu, 7 Mar 2024 08:28:45 +0000
"Tian, Kevin" <kevin.tian@xxxxxxxxx> wrote:
> > From: Alex Williamson <alex.williamson@xxxxxxxxxx>
> > Sent: Thursday, March 7, 2024 5:15 AM
> >
> > Currently for devices requiring masking at the irqchip for INTx, ie.
> > devices without DisINTx support, the IRQ is enabled in request_irq()
> > and subsequently disabled as necessary to align with the masked status
> > flag. This presents a window where the interrupt could fire between
> > these events, resulting in the IRQ incrementing the disable depth twice.
>
> Can you elaborate the last point about disable depth?
Each irq_desc maintains a depth field, a disable increments the depth,
an enable decrements. On the disable transition from 0 to 1 the IRQ
chip is disabled, on the enable transition from 1 to 0 the IRQ chip is
enabled.
Therefore if an interrupt fires between request_irq() and
disable_irq(), vfio_intx_handler() will disable the IRQ (depth 0->1).
Note that masked is not tested here, the interrupt is expected to be
exclusive for non-pci_2_3 devices. @masked would be redundantly set to
true. The setup call path would increment depth to 2. The order these
happen is not important so long as the interrupt is in-flight before
the setup path disables the IRQ.
> > This would be unrecoverable for a user since the masked flag prevents
> > nested enables through vfio.
>
> What is 'nested enables'?
In the case above we have masked true and disable depth 2. If the user
now unmasks the interrupt then depth is reduced to 1, the IRQ is still
disabled, and masked is false. The masked value is now out of sync
with the IRQ line and prevents the user from unmasking again. The
disable depth is stuck at 1.
Nested enables would be if we allowed the user to unmask a line that we
think is already unmasked.
> > Instead, invert the logic using IRQF_NO_AUTOEN such that exclusive INTx
> > is never auto-enabled, then unmask as required.
> >
> > Fixes: 89e1f7d4c66d ("vfio: Add PCI device driver")
> > Signed-off-by: Alex Williamson <alex.williamson@xxxxxxxxxx>
>
> But this patch looks good to me:
>
> Reviewed-by: Kevin Tian <kevin.tian@xxxxxxxxx>
>
> with one nit...
>
> >
> > + /*
> > + * Devices without DisINTx support require an exclusive interrupt,
> > + * IRQ masking is performed at the IRQ chip. The masked status is
>
> "exclusive interrupt, with IRQ masking performed at..."
TBH, the difference is too subtle for me. With my version above you
could replace the comma with a period, I think it has the same meaning.
However, "...exclusive interrupt, with IRQ masking performed at..."
almost suggests that we need a specific type of exclusive interrupt
with this property. There's nothing unique about the exclusive
interrupt, we could arbitrarily decide we want an exclusive interrupt
for DisINTx masking if we wanted to frustrate a lot of users.
Performing masking at the IRQ chip is actually what necessitates the
exclusive interrupt here. Thanks,
Alex
