Re: [PATCH v5 5/5] vfio: selftests: igb: Recover after DMA-read faults

[David Matlack <dmatlack@google.com>]

On 2026-07-20 03:51 PM, Alex Williamson wrote:
> On Mon, 20 Jul 2026 15:42:27 -0600
> Alex Williamson <alex.williamson@nvidia.com> wrote:
> 
> > On Mon, 20 Jul 2026 14:27:12 -0700
> > Josh Hilke <jrhilke@google.com> wrote:
> > 
> > > On Tue, Jul 14, 2026 at 2:12 PM <sashiko-bot@kernel.org> wrote:  
> > > >
> > > > Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> > > > - [Medium] Test flakiness due to fixed heuristic delay instead of polling on EAGAIN during VFIO_DEVICE_RESET
> > > > --
> > > >
> > > > --- Patch [5]: [PATCH v5 5/5] vfio: selftests: igb: Recover after DMA-read faults ---
> > > > commit a631c99ce1be37dbbdda0314b730b2c6bd947b50
> > > > Author: Alex Williamson <alex.williamson@nvidia.com>
> > > >
> > > > vfio: selftests: igb: Recover after DMA-read faults
> > > >
> > > > This commit adds recovery logic to the igb selftest when an intentional DMA
> > > > read fault causes the device to hang. It introduces a reset and re-init
> > > > sequence using VFIO_DEVICE_RESET, including a 10ms delay to allow kernel
> > > > error handlers to release the device lock.
> > > >    
> > > > > diff --git a/tools/testing/selftests/vfio/lib/drivers/igb/igb.c b/tools/testing/selftests/vfio/lib/drivers/igb/igb.c
> > > > > --- a/tools/testing/selftests/vfio/lib/drivers/igb/igb.c
> > > > > +++ b/tools/testing/selftests/vfio/lib/drivers/igb/igb.c    
> > > >
> > > > [ ... ]
> > > >    
> > > > > @@ -500,7 +522,28 @@ static int igb_memcpy_wait(struct vfio_pci_device *device)
> > > > >
> > > > >       igb_irq_enable(igb);
> > > > >
> > > > > -     return (status & 1) ? 0 : -ETIMEDOUT;
> > > > > +     if (status & 1)
> > > > > +             return 0;
> > > > > +
> > > > > +     /*
> > > > > +      * The descriptor never completed.  On real 82576 hardware this
> > > > > +      * typically follows a DMA-read fault from one of the intentional
> > > > > +      * unmapped-IOVA tests; the fault leaves the descriptor engine
> > > > > +      * unable to service subsequent valid descriptors.  CTRL.RST alone
> > > > > +      * reinitializes the queue registers but leaves the engine wedged
> > > > > +      * for the current process, so a broader VFIO_DEVICE_RESET (FLR)
> > > > > +      * is required.
> > > > > +      *
> > > > > +      * Delay before requesting reset so PCIe/IOMMU/AER error handling
> > > > > +      * triggered by the just-observed DMA fault can release the device
> > > > > +      * lock VFIO_DEVICE_RESET contends for.  The 10 ms value is
> > > > > +      * heuristic.  The current memcpy still fails with -ETIMEDOUT;
> > > > > +      * recovery only ensures the next memcpy starts from a usable state.
> > > > > +      */
> > > > > +     usleep(10000);
> > > > > +     igb_error_reset_and_reinit(device);    
> > > >
> > > > [Severity: Medium]
> > > > Will this fixed heuristic delay cause test flakiness under heavy load?
> > > >
> > > > If the kernel's PCIe/IOMMU/AER error handling takes longer than 10ms to
> > > > complete (for example, when KASAN or other heavy debugging options are
> > > > enabled), the device lock might still be held.
> > > >
> > > > Looking at the kernel side in drivers/vfio/pci/vfio_pci_core.c:
> > > > vfio_pci_ioctl_reset(), lock contention causes pci_try_reset_function() to
> > > > return -EAGAIN:
> > > >
> > > >         ret = pci_try_reset_function(vdev->pdev);
> > > >         if (__vfio_pci_memory_enabled(vdev))
> > > >                 vfio_pci_dma_buf_move(vdev, false);
> > > >         up_write(&vdev->memory_lock);
> > > >
> > > >         return ret;
> > > >
> > > > Since igb_error_reset_and_reinit() calls vfio_pci_device_reset(), which
> > > > asserts that the ioctl returns 0, would this lock contention cause an
> > > > immediate test suite failure?
> > > >
> > > > Should this code explicitly poll for -EAGAIN from the ioctl instead of
> > > > relying entirely on a fixed sleep?
> > > >    
> > > > > +     return -ETIMEDOUT;    
> > > 
> > > Alex, I'm guessing 10ms is what worked for you when testing with the
> > > physical hardware? If it's working fine, I don't think it needs to be
> > > changed.  
> > 
> > Yes, the delay was found empirically and indeed we could make this more
> > robust if we retried on -EAGAIN.  I'd be in favor of doing that as a
> > follow-up.  Thanks,
> 
> Oh, some further context, vfio_pci_device_reset() asserts the return is
> zero, which IIRC, is why I accepted the heuristic delay rather than
> expand the scope with a new reset callback within the framework.

Let's fix vfio_pci_device_reset() to retry on -EAGAIN. Josh can you apply
something like this on top in v6?

diff --git a/tools/testing/selftests/vfio/lib/drivers/igb/igb.c b/tools/testing/selftests/vfio/lib/drivers/igb/igb.c
index f50d450d3e94..25cac663de1d 100644
--- a/tools/testing/selftests/vfio/lib/drivers/igb/igb.c
+++ b/tools/testing/selftests/vfio/lib/drivers/igb/igb.c
@@ -533,14 +533,7 @@ static int igb_memcpy_wait(struct vfio_pci_device *device)
         * reinitializes the queue registers but leaves the engine wedged
         * for the current process, so a broader VFIO_DEVICE_RESET (FLR)
         * is required.
-        *
-        * Delay before requesting reset so PCIe/IOMMU/AER error handling
-        * triggered by the just-observed DMA fault can release the device
-        * lock VFIO_DEVICE_RESET contends for.  The 10 ms value is
-        * heuristic.  The current memcpy still fails with -ETIMEDOUT;
-        * recovery only ensures the next memcpy starts from a usable state.
         */
-       usleep(10000);
        igb_error_reset_and_reinit(device);

        return -ETIMEDOUT;
diff --git a/tools/testing/selftests/vfio/lib/include/libvfio/vfio_pci_device.h b/tools/testing/selftests/vfio/lib/include/libvfio/vfio_pci_device.h
index 2e67afc0d580..27bdf561925f 100644
--- a/tools/testing/selftests/vfio/lib/include/libvfio/vfio_pci_device.h
+++ b/tools/testing/selftests/vfio/lib/include/libvfio/vfio_pci_device.h
@@ -41,6 +41,7 @@ struct vfio_pci_device {
 struct vfio_pci_device *vfio_pci_device_init(const char *bdf, struct iommu *iommu);
 void vfio_pci_device_cleanup(struct vfio_pci_device *device);

+int __vfio_pci_device_reset(struct vfio_pci_device *device);
 void vfio_pci_device_reset(struct vfio_pci_device *device);

 void vfio_pci_config_access(struct vfio_pci_device *device, bool write,
diff --git a/tools/testing/selftests/vfio/lib/vfio_pci_device.c b/tools/testing/selftests/vfio/lib/vfio_pci_device.c
index 7b8394d0ac50..45704a2c9485 100644
--- a/tools/testing/selftests/vfio/lib/vfio_pci_device.c
+++ b/tools/testing/selftests/vfio/lib/vfio_pci_device.c
@@ -221,9 +221,23 @@ void vfio_pci_config_access(struct vfio_pci_device *device, bool write,
                       write ? "write to" : "read from", config);
 }

+int __vfio_pci_device_reset(struct vfio_pci_device *device)
+{
+       if (ioctl(device->fd, VFIO_DEVICE_RESET))
+               return -errno;
+
+       return 0;
+}
+
 void vfio_pci_device_reset(struct vfio_pci_device *device)
 {
-       ioctl_assert(device->fd, VFIO_DEVICE_RESET, NULL);
+       int r;
+
+       do {
+               r = __vfio_pci_device_reset(device);
+       } while (r == -EAGAIN);
+
+       VFIO_ASSERT_EQ(r, 0, "ioctl(device->fd, VFIO_DEVICE_RESET) failed\n");
 }

 static unsigned int vfio_pci_get_group_from_dev(const char *bdf)