RE: [RFC PATCH 00/19] vfio/pci: Handle PCI error recovery and report state to userspace

[Shameer Kolothum Thodi]

> -----Original Message-----
> From: Alex Williamson <alex@xxxxxxxxxxx>
> Sent: 04 September 2026 20:09
> To: Shameer Kolothum Thodi <skolothumtho@xxxxxxxxxx>
> Cc: kvm@xxxxxxxxxxxxxxx; linux-pci@xxxxxxxxxxxxxxx; linux-
> kernel@xxxxxxxxxxxxxxx; jgg@xxxxxxxx; kevin.tian@xxxxxxxxx;
> kbusch@xxxxxxxx; michal.winiarski@xxxxxxxxx;
> satyanarayana.k.v.p@xxxxxxxxx; Sonang Patel <sonangp@xxxxxxxxxx>;
> Nathan Chen <nathanc@xxxxxxxxxx>; Matt Ochs <mochs@xxxxxxxxxx>;
> alex@xxxxxxxxxxx
> Subject: Re: [RFC PATCH 00/19] vfio/pci: Handle PCI error recovery and report
> state to userspace
> 
> External email: Use caution opening links or attachments
> 
> 
> On Tue, 1 Sep 2026 10:31:58 +0100
> Shameer Kolothum <skolothumtho@xxxxxxxxxx> wrote:
> 
> > Hi,
> >
> > Currently, vfio-pci takes almost no part in PCI error recovery. It
> > implements error_detected() and neither of the other two callbacks. That
> > one callback ignores the pci_channel_state_t it is given, signals the
> > error eventfd, and returns PCI_ERS_RESULT_CAN_RECOVER for every error, a
> > permanent failure included. Nothing implements slot_reset() or resume(),
> > so vfio-pci never learns that the host reset the device, or that
> > recovery finished.
> >
> > Userspace gets one eventfd signal with nothing attached to it. It cannot
> > tell a non-fatal error the host recovered from apart from a permanent
> > failure, and it is never told when recovery is over. With nothing to go
> > on, QEMU assumes the worst and calls
> vm_stop(RUN_STATE_INTERNAL_ERROR),
> > which the VM cannot come back from.
> >
> > Any device assigned through vfio-pci can hit this. A non-fatal
> > uncorrectable error is reported, the host AER path recovers the device
> > fine, and the VM is killed anyway.
> >
> > This series lets userspace observe host recovery state, and keeps it off
> > the device while recovery is running. With that state visible, userspace
> > can decide what to do with the guest rather than assuming the worst.
> >
> > The approach here comes from an earlier discussion with Alex.
> >
> > https://lore.kernel.org/qemu-
> devel/20260707161234.23ed28db@xxxxxxxxxx/
> > https://lore.kernel.org/all/20260818083754.7ccf76d9@xxxxxxxxxxx/
> >
> > Design
> > ------
> >
> > The VMM watches recovery. It does not take part in it. The kernel runs
> > the recovery sequence and tells userspace what happened and when it is
> > done.
> >
> > vfio-pci already has error_detected(). This series extends it and adds
> > the other two callbacks:
> >
> >   - error_detected() now records the channel state, blocks new device
> >     access, revokes BAR mappings and exported DMA-BUFs, and quiesces
> >     INTx. It still signals err_trigger as it does today. It votes on
> >     severity rather than always claiming it can recover: CAN_RECOVER for
> >     a non-fatal error, NEED_RESET for a frozen channel, DISCONNECT for a
> >     permanent failure, and NONE if our own quiesce failed, which leaves
> >     the rest of the domain alone.
> >   - slot_reset() is new. It restores config state after the host has
> >     reset the device. Nothing does that today, which is why a device
> >     comes back from an AER reset with its config lost.
> >   - resume() is new. It restores PCI_COMMAND, unblocks access and wakes
> >     waiters.
> >   - A new device feature reports the state and carries an eventfd.
> >
> > A non-fatal error gets the same quiesce as a frozen one. The host has not
> > finished deciding what the error was, and can still escalate to a reset,
> > so the device is not the user's again until resume() says so.
> >
> > The support is opt-in. Until userspace installs the recovery eventfd,
> > generic vfio-pci behaves as it does today. error_detected() takes its
> > existing path and signals the same eventfd. VFIO variant driver support
> > is not added for now.
> >
> > The uAPI is VFIO_DEVICE_FEATURE_PCI_ERROR_RECOVERY. It carries the
> > eventfd and reports a status word plus a sequence number, so userspace
> > can tell coalesced notifications apart. IN_PROGRESS is set while a
> > recovery is running. CHANNEL_FROZEN says the link went down.
> > DEVICE_RESET says the host reset the device. FAILED says the device
> > cannot be used again until close and reopen. ENABLED says userspace has
> > opted in.
> >
> > A non-fatal recovery can complete before userspace reacts to the eventfd,
> > so IN_PROGRESS may already be clear by the time the feature is read. Work
> > from the sequence number and the status bits rather than expecting to
> > catch the event while it runs.
> >
> > Patches
> > -------
> >
> >   1-3    the groundwork: the recovery state fields, the open and close
> >          lifecycle so a callback never sees a half built or half torn
> >          down device, and the access guards the rest of the series uses
> >   4-13   close the access paths one at a time: function reset, config
> >          space, ioeventfd, BAR faults, BAR and ROM, interrupts, hot
> >          reset, runtime PM, info queries, DMA-BUF
> >   14-18  the error handler callbacks: slot reset, the INTx helpers and
> >          the quiesce that uses them, then resume and error_detected
> >   19     the uAPI a user opts in through
> >
> > Locking
> > -------
> >
> > Blocking access is the hard part of this series, and it comes down to
> > one rule.
> >
> >   recovery_lock can be held while publishing state, and while draining
> >   operations that are already under way. It cannot be held across a reset,
> >   or across anything else that reaches pci_bus_sem.
> >
> > The reason is the order AER arrives in. It enters the driver already
> > holding device_lock, and pci_bus_sem too when the device sits under a
> > bridge with a subordinate bus, and only then takes recovery_lock. A
> > secondary bus reset reaches pci_bus_sem. So a vfio path which holds
> > recovery_lock across a reset ends up taking those two the other way
> > round.
> >
> > Seven places needed reshaping for this rule: device close, slot_reset(),
> > open, VFIO_DEVICE_RESET, the guest triggered config space FLR,
> > VFIO_DEVICE_SET_IRQS, and a guest write putting the device back in D0,
> > which reaches pci_bus_sem through pcie_aspm_pm_state_change().
> >
> > Most access takes recovery_lock for reading and checks whether a recovery
> > or a reset is blocking the device. A few places cannot take the lock and
> > read that state directly instead. All of them fail safe. A stale read
> > costs an extra refusal or retry, never an unguarded access.
> >
> > Interrupt teardown is the one deliberate exception. It flushes the global
> > virqfd workqueue with recovery_lock held, which can make the hold last as
> > long as a reset on another vfio device. It costs latency, not
> > correctness.
> >
> > I am not sure this is the best way to handle it, and would welcome
> > suggestions.
> 
> 
> Thanks for tackling this, Shameer.  The recovery_lock wrapping all
> these accesses does make me nervous, both in lock complexity and
> overhead.  Wouldn't it be a better solution to decouple the user
> interface from the device by replacing the access path via SRCU then
> doing zap/move/interrupt teardown?
> 
> Such a solution would have utility beyond the error path.  We could use
> it for surprise removal/DPC, we could allow a policy to remove the
> device from the user on unbind, in place of or in addition to the
> request eventfd we use currently.  In the error case, the intention
> would be to temporarily suspend access to the device, but if it falls
> off the bus after recovery, it may turn into a permanent removal.
> 
> What do you think?  Thanks,

Agree. As it stands, it looks not that maintainable due to the lock
complexity and dependencies.  Let me look at replacing the
recovery_lock with SRCU and see how that evolves.

The generalisation makes sense too. I will keep this series to the error
path but make sure the mechanism is not tied to it.

Thanks,
Shameer