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

[Alex Williamson <alex@shazbot.org>]

On Tue, 1 Sep 2026 10:31:58 +0100
Shameer Kolothum <skolothumtho@nvidia.com> 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@nvidia.com/
> https://lore.kernel.org/all/20260818083754.7ccf76d9@shazbot.org/
> 
> 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,

Alex