RE: [RFC PATCH 09/19] vfio/pci: Serialize interrupt operations with recovery

[Shameer Kolothum Thodi]

> -----Original Message-----
> From: K V P, Satyanarayana <satyanarayana.k.v.p@xxxxxxxxx>
> Sent: 03 September 2026 07:34
> To: Shameer Kolothum Thodi <skolothumtho@xxxxxxxxxx>;
> kvm@xxxxxxxxxxxxxxx; linux-pci@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx
> Cc: alex@xxxxxxxxxxx; jgg@xxxxxxxx; kevin.tian@xxxxxxxxx;
> kbusch@xxxxxxxx; michal.winiarski@xxxxxxxxx; Sonang Patel
> <sonangp@xxxxxxxxxx>; Nathan Chen <nathanc@xxxxxxxxxx>; Matt Ochs
> <mochs@xxxxxxxxxx>
> Subject: Re: [RFC PATCH 09/19] vfio/pci: Serialize interrupt operations with
> recovery
> 
> External email: Use caution opening links or attachments
> 
> 
> On 01-Sep-26 3:02 PM, Shameer Kolothum wrote:
> > Hold recovery_lock for reading around INTx, MSI and MSI-X capability
> > queries and configuration changes. ERR and REQ are software-only
> > indexes and stay available while recovery blocks device access. INTx
> > is covered by the same test even though its count comes from the
> > virtual config space, so that one rule applies to every index which can reach
> hardware.
> >
> > The test is on the index alone, so a blocked device also refuses the
> > few requests on those indexes which would not have touched it:
> > signalling an eventfd for test purposes, and adding or removing the
> > virqfd behind INTx masking. Both return -EIO until access is
> > unblocked, which for a non-fatal error is the time the host takes to
> > log it. Reading the flags or the count of a request is not enough to
> > tell whether it reaches the device, and refusing a few extra requests
> > for the length of an error event is cheaper than getting that classification
> wrong.
> >
> > Copy the IRQ payload from userspace before taking recovery_lock. The
> > copy can fault, and with userfaultfd the fault is serviced by
> > userspace, so holding the lock across it would let a user stall
> > error_detected() for as long as it likes. The count read and the
> > interrupt operation each take the lock for themselves.
> >
> > Assisted-by: Claude:claude-opus-5
> > Signed-off-by: Shameer Kolothum <skolothumtho@xxxxxxxxxx>
> > ---
> >   drivers/vfio/pci/vfio_pci_core.c | 55
> ++++++++++++++++++++++++++++++++
> >   1 file changed, 55 insertions(+)
> >
> > diff --git a/drivers/vfio/pci/vfio_pci_core.c
> > b/drivers/vfio/pci/vfio_pci_core.c
> > index 0b1b2398dc88..876ff51d6987 100644
> > --- a/drivers/vfio/pci/vfio_pci_core.c
> > +++ b/drivers/vfio/pci/vfio_pci_core.c
> > @@ -1313,11 +1313,29 @@ int vfio_pci_ioctl_get_region_info(struct
> vfio_device *core_vdev,
> >   }
> >   EXPORT_SYMBOL_GPL(vfio_pci_ioctl_get_region_info);
> >
> > +/*
> > + * Which IRQ indexes can reach the device. ERR and REQ are software only.
> > + * An index added later gets no access guard until it is listed here.
> > + */
> > +static bool vfio_pci_irq_index_is_device(u32 index) {
> > +     switch (index) {
> > +     case VFIO_PCI_INTX_IRQ_INDEX:
> > +     case VFIO_PCI_MSI_IRQ_INDEX:
> > +     case VFIO_PCI_MSIX_IRQ_INDEX:
> > +             return true;
> > +     default:
> > +             return false;
> > +     }
> > +}
> > +
> >   static int vfio_pci_ioctl_get_irq_info(struct vfio_pci_core_device *vdev,
> >                                      struct vfio_irq_info __user *arg)
> >   {
> >       unsigned long minsz = offsetofend(struct vfio_irq_info, count);
> >       struct vfio_irq_info info;
> > +     bool device_irq;
> > +     int ret;
> >
> >       if (copy_from_user(&info, arg, minsz))
> >               return -EFAULT;
> > @@ -1336,7 +1354,15 @@ static int vfio_pci_ioctl_get_irq_info(struct
> > vfio_pci_core_device *vdev,
> >
> >       info.flags = VFIO_IRQ_INFO_EVENTFD;
> >
> > +     device_irq = vfio_pci_irq_index_is_device(info.index);
> > +     if (device_irq) {
> > +             ret = vfio_pci_core_access_begin(vdev);
> > +             if (ret)
> > +                     return ret;
> > +     }
> >       info.count = vfio_pci_get_irq_count(vdev, info.index);
> > +     if (device_irq)
> > +             vfio_pci_core_access_end(vdev);
> >
> >       if (info.index == VFIO_PCI_INTX_IRQ_INDEX)
> >               info.flags |=
> > @@ -1353,13 +1379,23 @@ static int vfio_pci_ioctl_set_irqs(struct
> vfio_pci_core_device *vdev,
> >       unsigned long minsz = offsetofend(struct vfio_irq_set, count);
> >       struct vfio_irq_set hdr;
> >       u8 *data = NULL;
> > +     bool device_irq;
> >       int max, ret = 0;
> >       size_t data_size = 0;
> >
> >       if (copy_from_user(&hdr, arg, minsz))
> >               return -EFAULT;
> >
> > +     device_irq = vfio_pci_irq_index_is_device(hdr.index);
> > +     if (device_irq) {
> > +             ret = vfio_pci_core_access_begin(vdev);
> > +             if (ret)
> > +                     return ret;
> > +     }
> >       max = vfio_pci_get_irq_count(vdev, hdr.index);
> > +     /* Dropped for the user copy below, which can fault under userfaultfd. */
> > +     if (device_irq)
> > +             vfio_pci_core_access_end(vdev);
> 
> Can we have some thing like this.
> 
> if (device_irq) {
> 
> access_begin
> 
> max = vfio_pci_get_irq_count(vdev, hdr.index);
> 
> access_end
> 
> } else {
> 
> max = vfio_pci_get_irq_count(vdev, hdr.index);
> 
> }

Ok. I think that will read better.

> >       ret = vfio_set_irqs_validate_and_prepare(&hdr, max,
> VFIO_PCI_NUM_IRQS,
> >                                                &data_size); @@
> > -1372,12 +1408,31 @@ static int vfio_pci_ioctl_set_irqs(struct
> vfio_pci_core_device *vdev,
> >                       return PTR_ERR(data);
> >       }
> >
> > +     /*
> > +      * Interrupt teardown reaches vfio_virqfd_disable(), which flushes the
> > +      * global virqfd cleanup workqueue, so recovery_lock is held here for
> > +      * as long as work queued by any vfio device takes. Shutdown work waits
> > +      * for its inject worker, and an ioeventfd inject takes that device's
> > +      * memory_lock, so the wait can last as long as a reset there. That is
> > +      * only a wait. Nothing on that workqueue takes recovery_lock, which is
> > +      * why the ioeventfd write path reads the recovery state without it. A
> > +      * callback there which used the vfio_pci_core_iowrite*() accessors
> > +      * would break that and deadlock against a queued writer.
> > +      */
> > +     if (device_irq) {
> > +             ret = vfio_pci_core_access_begin(vdev);
> > +             if (ret)
> > +                     goto out_free;
> > +     }
> >       mutex_lock(&vdev->igate);
> 
> We are using a semaphore wait in vfio_pci_core_access_begin() and
> immediately after that using a mutex_lock().
> 
> Try to optimize this if possible.

Hmm... I am not sure how we can optimise by avoiding one for
the other. 

This path takes recovery_lock for reading, and only the error callbacks
take it for writing. So, the read side keeps recovery out, but two SET_IRQS
calls both take the read lock and both go through. igate is what keeps
those two apart. Probably worth adding a comment here to explain
it better.

Thanks,
Shameer