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

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

[Shameer Kolothum]

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.

Testing
-------

Basic sanity tests performed on a GB200 with an NVIDIA GPU assigned.

Kernel branch:
https://github.com/shamiali2008/linux/commits/vfio-aer-rfc-v1

QEMU test branch is here(This is just to test the kernel sereis):
https://github.com/shamiali2008/qemu-master/tree/master-vfio-aer-rfc-test

Software AER injection was performed using a modified pcieaer_inject
module.

Non-fatal path (pci_channel_io_normal):

./aer-inject nonfatal.conf

qemu-system-aarch64: warning: vfio 0018:06:00.0: AER error signaled; host recovery in progress
qemu-system-aarch64: warning: vfio 0018:06:00.0: AER recovery completed successfully (seq=1)

Fatal path (pci_channel_io_frozen):

./aer-inject fatal.conf

qemu-system-aarch64: warning: vfio 0018:06:00.0: AER error signaled; host recovery in progress
qemu-system-aarch64: warning: vfio 0018:06:00.0: AER recovery started (seq=2, channel frozen), device access blocked
qemu-system-aarch64: warning: vfio 0018:06:00.0: AER recovery completed with device reset (seq=2).

Please take a look and let me know your feedback.

Thanks,
Shameer

Shameer Kolothum (19):
  vfio/pci: Add PCI error recovery support state
  vfio/pci: Serialize generic device lifetime with recovery
  vfio/pci: Add PCI recovery access guards
  vfio/pci: Serialize function reset with recovery
  vfio/pci: Serialize config access with recovery
  vfio/pci: Serialize ioeventfd writes with recovery
  vfio/pci: Retry BAR faults after temporary recovery
  vfio/pci: Serialize BAR and ROM access with recovery
  vfio/pci: Serialize interrupt operations with recovery
  vfio/pci: Serialize hot reset with recovery
  vfio/pci: Serialize runtime PM with recovery
  vfio/pci: Serialize physical device information queries with recovery
  vfio/pci: Serialize DMA-BUF export with recovery
  vfio/pci: Add generic PCI error slot reset handling
  vfio/pci: Add INTx helpers for PCI recovery
  vfio/pci: Quiesce INTx during PCI recovery
  vfio/pci: Add generic PCI error resume handling
  vfio/pci: Coordinate generic device access with host recovery
  vfio/pci: Expose and enable host PCI error recovery

 drivers/vfio/pci/vfio_pci_priv.h   |  10 +
 include/linux/vfio_pci_core.h      |  60 ++
 include/uapi/linux/vfio.h          |  69 +++
 drivers/vfio/pci/vfio_pci.c        |   1 +
 drivers/vfio/pci/vfio_pci_config.c | 147 +++--
 drivers/vfio/pci/vfio_pci_core.c   | 883 ++++++++++++++++++++++++++++-
 drivers/vfio/pci/vfio_pci_dmabuf.c |  26 +-
 drivers/vfio/pci/vfio_pci_intrs.c  | 159 ++++++
 drivers/vfio/pci/vfio_pci_rdwr.c   | 142 +++--
 9 files changed, 1402 insertions(+), 95 deletions(-)

-- 
2.43.0

[RFC PATCH 01/19] vfio/pci: Add PCI error recovery support state

[Shameer Kolothum]

Add the state, the lock and the wait queue that the rest of the series
needs. Nothing uses any of it yet. The lifecycle and the access guards
come next, the recovery callbacks later.

The flags word records what happened in one event. IN_PROGRESS while
recovery is running, FROZEN if the channel was frozen, RESET if the host
reset the device, and FAILED if it did not recover. These are internal.
A later patch reports the same set to userspace along with a sequence
number, so an event can be told from the one before it.

Where both locks are held, recovery_lock goes outside memory_lock.
Recovery has to shut out new device access and wait for whatever is
already running before it can take memory_lock and revoke the BAR
mappings. The other way round deadlocks. An access path sits on
memory_lock waiting for recovery_lock, while recovery sits on
recovery_lock waiting for memory_lock. So nothing may take recovery_lock
when it already holds memory_lock.

Only the generic vfio-pci driver advertises support, so variant drivers
carry on as they do now. Userspace activation is a separate switch, so
even on generic vfio-pci nothing changes until userspace asks for it.

Signed-off-by: Shameer Kolothum <skolothumtho@nvidia.com>
---
 include/linux/vfio_pci_core.h    | 59 ++++++++++++++++++++++++++++++++
 drivers/vfio/pci/vfio_pci.c      |  1 +
 drivers/vfio/pci/vfio_pci_core.c |  2 ++
 3 files changed, 62 insertions(+)

diff --git a/include/linux/vfio_pci_core.h b/include/linux/vfio_pci_core.h
index 9a1674c152aa..42a77ed6b93c 100644
--- a/include/linux/vfio_pci_core.h
+++ b/include/linux/vfio_pci_core.h
@@ -95,6 +95,11 @@ static inline int vfio_pci_core_get_dmabuf_phys(
 }
 #endif
 
+#define VFIO_PCI_RECOVERY_IN_PROGRESS	BIT(0)
+#define VFIO_PCI_RECOVERY_FROZEN	BIT(1)
+#define VFIO_PCI_RECOVERY_RESET		BIT(2)
+#define VFIO_PCI_RECOVERY_FAILED	BIT(3)
+
 struct vfio_pci_core_device {
 	struct vfio_device	vdev;
 	struct pci_dev		*pdev;
@@ -129,6 +134,7 @@ struct vfio_pci_core_device {
 	bool			disable_idle_d3:1;
 	bool			nointxmask:1;
 	bool			disable_vga:1;
+	bool			pci_recovery_supported:1;
 	/* Flags modified at runtime - dedicated storage unit */
 	bool			needs_reset;
 	bool			pm_intx_masked;
@@ -147,7 +153,60 @@ struct vfio_pci_core_device {
 	struct list_head		sriov_pfs_item;
 	struct vfio_pci_core_device	*sriov_pf_core_dev;
 	struct notifier_block	nb;
+	/*
+	 * Serializes host PCI error recovery with device access and the
+	 * open/close lifecycle. recovery_lock nests outside memory_lock.
+	 */
+	struct rw_semaphore	recovery_lock;
 	struct rw_semaphore	memory_lock;
+	/*
+	 * PCI error recovery state, written under recovery_lock held for
+	 * writing except where noted.
+	 *
+	 * Some readers cannot take recovery_lock. An interrupt handler cannot
+	 * sleep. The ioeventfd write runs on a workqueue which is flushed with
+	 * the lock held, so it would block behind a queued writer. A
+	 * wait-queue condition cannot take it either. Some checks would refuse
+	 * work against ordinary device traffic if they took it. Work deferred
+	 * past the guard has none to take, since it reaches pci_bus_sem.
+	 *
+	 * Those readers use READ_ONCE() on pci_recovery_flags,
+	 * pci_recovery_enabled, pci_recovery_access_blocked and
+	 * pci_recovery_device_open instead. All of them fail safe. A stale
+	 * read costs an extra refusal or retry, never an unguarded access.
+	 *
+	 * recovery_lock does not exclude those readers, so publish each field
+	 * with a single store of its final value rather than clearing and
+	 * then setting. That keeps the states a reader can observe to ones
+	 * which are meaningful on their own.
+	 *
+	 * device_open and access_blocked mean different things. device_open
+	 * says the device is open and its per-open state, vconfig included,
+	 * is allocated. access_blocked says a recovery or reset is blocking
+	 * access right now.
+	 *
+	 * access_blocked is only ever set while device_open is set. Nothing
+	 * sets it without testing device_open first under recovery_lock, and
+	 * close clears access_blocked before it clears device_open, so a
+	 * block never outlives the open which created it. A reader which
+	 * finds access_blocked set can rely on the per-open state being
+	 * there with it.
+	 */
+	u32			pci_recovery_flags;
+	u64			pci_recovery_sequence;
+	/* PCI_COMMAND value saved before recovery quiesces the device. */
+	u16			pci_recovery_command;
+	/* Userspace enabled recovery for this device open. */
+	bool			pci_recovery_enabled;
+	/* pci_recovery_command contains a restorable value. */
+	bool			pci_recovery_command_valid;
+	/* A recovery or reset transaction is blocking physical access. */
+	bool			pci_recovery_access_blocked;
+	/* Device initialization completed and close teardown has not started. */
+	bool			pci_recovery_device_open;
+	/* May be set while recovery_lock is held for reading during ROM unmap. */
+	bool			pci_recovery_rom_disable;
+	wait_queue_head_t	pci_recovery_wait;
 	struct list_head	dmabufs;
 };
 
diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c
index 830369ff878d..46544dbe70d0 100644
--- a/drivers/vfio/pci/vfio_pci.c
+++ b/drivers/vfio/pci/vfio_pci.c
@@ -139,6 +139,7 @@ static int vfio_pci_init_dev(struct vfio_device *core_vdev)
 	 */
 	vdev->nointxmask = nointxmask;
 	vdev->disable_idle_d3 = disable_idle_d3;
+	vdev->pci_recovery_supported = true;
 #ifdef CONFIG_VFIO_PCI_VGA
 	vdev->disable_vga = disable_vga;
 #endif
diff --git a/drivers/vfio/pci/vfio_pci_core.c b/drivers/vfio/pci/vfio_pci_core.c
index 6757054e9d87..e0be5ddf7039 100644
--- a/drivers/vfio/pci/vfio_pci_core.c
+++ b/drivers/vfio/pci/vfio_pci_core.c
@@ -2196,7 +2196,9 @@ int vfio_pci_core_init_dev(struct vfio_device *core_vdev)
 	if (ret && ret != -EOPNOTSUPP)
 		return ret;
 	INIT_LIST_HEAD(&vdev->dmabufs);
+	init_rwsem(&vdev->recovery_lock);
 	init_rwsem(&vdev->memory_lock);
+	init_waitqueue_head(&vdev->pci_recovery_wait);
 	xa_init(&vdev->ctx);
 
 	return 0;
-- 
2.43.0

Re: [RFC PATCH 01/19] vfio/pci: Add PCI error recovery support state

[sashiko-bot]

> Add the state, the lock and the wait queue that the rest of the series
> needs. Nothing uses any of it yet. The lifecycle and the access guards
> come next, the recovery callbacks later.
> 
> The flags word records what happened in one event. IN_PROGRESS while
> recovery is running, FROZEN if the channel was frozen, RESET if the host
> reset the device, and FAILED if it did not recover. These are internal.
> A later patch reports the same set to userspace along with a sequence
> number, so an event can be told from the one before it.
> 
> Where both locks are held, recovery_lock goes outside memory_lock.
> Recovery has to shut out new device access and wait for whatever is
> already running before it can take memory_lock and revoke the BAR
> mappings. The other way round deadlocks. An access path sits on
> memory_lock waiting for recovery_lock, while recovery sits on
> recovery_lock waiting for memory_lock. So nothing may take recovery_lock
> when it already holds memory_lock.
> 
> Only the generic vfio-pci driver advertises support, so variant drivers
> carry on as they do now. Userspace activation is a separate switch, so
> even on generic vfio-pci nothing changes until userspace asks for it.
> 
> Signed-off-by: Shameer Kolothum <skolothumtho@nvidia.com>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260901093217.8539-1-skolothumtho@nvidia.com?part=1

[RFC PATCH 02/19] vfio/pci: Serialize generic device lifetime with recovery

[Shameer Kolothum]

vfio_pci_core_disable() frees vconfig while holding only the vfio
device_set mutex. The PCI error callbacks never take that one. They run
under the PCI device_lock instead, and vfio's close path does not hold
that. So a callback still running when close starts can walk into state
which is being freed.

Publish a device_open flag under recovery_lock. enable() clears it before
it touches the device, finish_enable() sets it once vfio_config_init()
has allocated vconfig, and prepare_close() clears it again before the
teardown frees vconfig. All three take recovery_lock for writing, so a
callback either gets there first and close waits for it, or it finds the
flag clear and does nothing. The access guards added later test the same
flag.

recovery_lock is not held across vfio_pci_core_disable(). A later patch
has error_detected() take it from under pci_bus_sem, and disable() gets
to pci_reset_bus(), which takes pci_bus_sem the other way round.

access_blocked is only ever set while device_open is set. Nothing sets it
without testing device_open first, and close clears access_blocked before
it clears device_open. If close left it set, nothing could clear it
afterwards.
The transaction which set it cannot clear it once device_open is gone,
and every path which refuses work on a blocked device would go on
refusing. Clear it before device_open so a lock-free reader never sees
it set on a device which is closed.

open() now refuses a disconnected device with -ENODEV. That is new.

Signed-off-by: Shameer Kolothum <skolothumtho@nvidia.com>
---
 drivers/vfio/pci/vfio_pci_core.c | 69 +++++++++++++++++++++++++++++++-
 1 file changed, 68 insertions(+), 1 deletion(-)

diff --git a/drivers/vfio/pci/vfio_pci_core.c b/drivers/vfio/pci/vfio_pci_core.c
index e0be5ddf7039..8de586e4bb73 100644
--- a/drivers/vfio/pci/vfio_pci_core.c
+++ b/drivers/vfio/pci/vfio_pci_core.c
@@ -591,10 +591,23 @@ static const struct dev_pm_ops vfio_pci_core_pm_ops = {
 int vfio_pci_core_enable(struct vfio_pci_core_device *vdev)
 {
 	struct pci_dev *pdev = vdev->pdev;
+	bool supported = vdev->pci_recovery_supported;
 	int ret;
 	u16 cmd;
 	u8 msix_pos;
 
+	if (supported) {
+		down_write(&vdev->recovery_lock);
+		if (pci_dev_is_disconnected(pdev)) {
+			up_write(&vdev->recovery_lock);
+			return -ENODEV;
+		}
+
+		vdev->pci_recovery_command_valid = false;
+		WRITE_ONCE(vdev->pci_recovery_device_open, false);
+		up_write(&vdev->recovery_lock);
+	}
+
 	if (!vdev->disable_idle_d3) {
 		ret = pm_runtime_resume_and_get(&pdev->dev);
 		if (ret < 0)
@@ -815,7 +828,40 @@ void vfio_pci_core_disable(struct vfio_pci_core_device *vdev)
 }
 EXPORT_SYMBOL_GPL(vfio_pci_core_disable);
 
-void vfio_pci_core_close_device(struct vfio_device *core_vdev)
+static void vfio_pci_core_prepare_close(struct vfio_pci_core_device *vdev)
+{
+	if (!vdev->pci_recovery_supported)
+		return;
+
+	down_write(&vdev->recovery_lock);
+	WRITE_ONCE(vdev->pci_recovery_enabled, false);
+	vdev->pci_recovery_command_valid = false;
+	/*
+	 * Clear access_blocked before device_open, so a lock-free reader
+	 * never sees it set on a device which is no longer open. A
+	 * transaction which is still running cannot clear it once
+	 * device_open is gone, and paths which refuse work on a blocked
+	 * device would then refuse it for good.
+	 */
+	WRITE_ONCE(vdev->pci_recovery_access_blocked, false);
+	WRITE_ONCE(vdev->pci_recovery_device_open, false);
+	WRITE_ONCE(vdev->pci_recovery_flags, 0);
+
+	/*
+	 * Publish the closing state and drop recovery_lock before any
+	 * teardown. Recovery is disabled and its state cleared, so
+	 * slot_reset() and resume() become no-ops and a later
+	 * error_detected() only follows the legacy notification path.
+	 * Holding the lock across vfio_pci_core_disable() protects nothing
+	 * and inverts the lock order. disable() reaches pci_reset_bus(),
+	 * which takes pci_bus_sem, while error_detected() takes
+	 * recovery_lock from under pci_bus_sem.
+	 */
+	up_write(&vdev->recovery_lock);
+	wake_up_all(&vdev->pci_recovery_wait);
+}
+
+static void vfio_pci_core_finish_close(struct vfio_device *core_vdev)
 {
 	struct vfio_pci_core_device *vdev =
 		container_of(core_vdev, struct vfio_pci_core_device, vdev);
@@ -838,6 +884,15 @@ void vfio_pci_core_close_device(struct vfio_device *core_vdev)
 	vfio_pci_eventfd_replace_locked(vdev, &vdev->req_trigger, NULL);
 	mutex_unlock(&vdev->igate);
 }
+
+void vfio_pci_core_close_device(struct vfio_device *core_vdev)
+{
+	struct vfio_pci_core_device *vdev =
+		container_of(core_vdev, struct vfio_pci_core_device, vdev);
+
+	vfio_pci_core_prepare_close(vdev);
+	vfio_pci_core_finish_close(core_vdev);
+}
 EXPORT_SYMBOL_GPL(vfio_pci_core_close_device);
 
 void vfio_pci_core_finish_enable(struct vfio_pci_core_device *vdev)
@@ -852,6 +907,18 @@ void vfio_pci_core_finish_enable(struct vfio_pci_core_device *vdev)
 		vdev->sriov_pf_core_dev->vf_token->users++;
 		mutex_unlock(&vdev->sriov_pf_core_dev->vf_token->lock);
 	}
+
+	if (vdev->pci_recovery_supported) {
+		down_write(&vdev->recovery_lock);
+		WRITE_ONCE(vdev->pci_recovery_flags, 0);
+		vdev->pci_recovery_sequence = 0;
+		WRITE_ONCE(vdev->pci_recovery_enabled, false);
+		/* Close clears this too. Start unblocked either way. */
+		WRITE_ONCE(vdev->pci_recovery_access_blocked, false);
+		WRITE_ONCE(vdev->pci_recovery_device_open, true);
+		WRITE_ONCE(vdev->pci_recovery_rom_disable, false);
+		up_write(&vdev->recovery_lock);
+	}
 }
 EXPORT_SYMBOL_GPL(vfio_pci_core_finish_enable);
 
-- 
2.43.0

Re: [RFC PATCH 02/19] vfio/pci: Serialize generic device lifetime with recovery

[sashiko-bot]

> vfio_pci_core_disable() frees vconfig while holding only the vfio
> device_set mutex. The PCI error callbacks never take that one. They run
> under the PCI device_lock instead, and vfio's close path does not hold
> that. So a callback still running when close starts can walk into state
> which is being freed.
> 
> Publish a device_open flag under recovery_lock. enable() clears it before
> it touches the device, finish_enable() sets it once vfio_config_init()
> has allocated vconfig, and prepare_close() clears it again before the
> teardown frees vconfig. All three take recovery_lock for writing, so a
> callback either gets there first and close waits for it, or it finds the
> flag clear and does nothing. The access guards added later test the same
> flag.
> 
> recovery_lock is not held across vfio_pci_core_disable(). A later patch
> has error_detected() take it from under pci_bus_sem, and disable() gets
> to pci_reset_bus(), which takes pci_bus_sem the other way round.
> 
> access_blocked is only ever set while device_open is set. Nothing sets it
> without testing device_open first, and close clears access_blocked before
> it clears device_open. If close left it set, nothing could clear it
> afterwards.
> The transaction which set it cannot clear it once device_open is gone,
> and every path which refuses work on a blocked device would go on
> refusing. Clear it before device_open so a lock-free reader never sees
> it set on a device which is closed.
> 
> open() now refuses a disconnected device with -ENODEV. That is new.
> 
> Signed-off-by: Shameer Kolothum <skolothumtho@nvidia.com>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260901093217.8539-1-skolothumtho@nvidia.com?part=2

Re: [RFC PATCH 02/19] vfio/pci: Serialize generic device lifetime with recovery

[K V P, Satyanarayana]

On 01-Sep-26 3:02 PM, Shameer Kolothum wrote:
> vfio_pci_core_disable() frees vconfig while holding only the vfio
> device_set mutex. The PCI error callbacks never take that one. They run
> under the PCI device_lock instead, and vfio's close path does not hold
> that. So a callback still running when close starts can walk into state
> which is being freed.
>
> Publish a device_open flag under recovery_lock. enable() clears it before
> it touches the device, finish_enable() sets it once vfio_config_init()
> has allocated vconfig, and prepare_close() clears it again before the
> teardown frees vconfig. All three take recovery_lock for writing, so a
> callback either gets there first and close waits for it, or it finds the
> flag clear and does nothing. The access guards added later test the same
> flag.
>
> recovery_lock is not held across vfio_pci_core_disable(). A later patch
> has error_detected() take it from under pci_bus_sem, and disable() gets
> to pci_reset_bus(), which takes pci_bus_sem the other way round.
>
> access_blocked is only ever set while device_open is set. Nothing sets it
> without testing device_open first, and close clears access_blocked before
> it clears device_open. If close left it set, nothing could clear it
> afterwards.
> The transaction which set it cannot clear it once device_open is gone,
> and every path which refuses work on a blocked device would go on
> refusing. Clear it before device_open so a lock-free reader never sees
> it set on a device which is closed.
>
> open() now refuses a disconnected device with -ENODEV. That is new.
>
> Signed-off-by: Shameer Kolothum <skolothumtho@nvidia.com>
> ---
>   drivers/vfio/pci/vfio_pci_core.c | 69 +++++++++++++++++++++++++++++++-
>   1 file changed, 68 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/vfio/pci/vfio_pci_core.c b/drivers/vfio/pci/vfio_pci_core.c
> index e0be5ddf7039..8de586e4bb73 100644
> --- a/drivers/vfio/pci/vfio_pci_core.c
> +++ b/drivers/vfio/pci/vfio_pci_core.c
> @@ -591,10 +591,23 @@ static const struct dev_pm_ops vfio_pci_core_pm_ops = {
>   int vfio_pci_core_enable(struct vfio_pci_core_device *vdev)
>   {
>   	struct pci_dev *pdev = vdev->pdev;
> +	bool supported = vdev->pci_recovery_supported;
>   	int ret;
>   	u16 cmd;
>   	u8 msix_pos;
>   
> +	if (supported) {
> +		down_write(&vdev->recovery_lock);
> +		if (pci_dev_is_disconnected(pdev)) {
> +			up_write(&vdev->recovery_lock);
> +			return -ENODEV;
> +		}
> +
> +		vdev->pci_recovery_command_valid = false;
> +		WRITE_ONCE(vdev->pci_recovery_device_open, false);
> +		up_write(&vdev->recovery_lock);
> +	}
> +

Can we use scoped_guard()/guard() instead of down_write()/up_write()? 
The same comment applies across all similar sections.

- Satya.

>   	if (!vdev->disable_idle_d3) {
>   		ret = pm_runtime_resume_and_get(&pdev->dev);
>   		if (ret < 0)
> @@ -815,7 +828,40 @@ void vfio_pci_core_disable(struct vfio_pci_core_device *vdev)
>   }
>   EXPORT_SYMBOL_GPL(vfio_pci_core_disable);
>   
> -void vfio_pci_core_close_device(struct vfio_device *core_vdev)
> +static void vfio_pci_core_prepare_close(struct vfio_pci_core_device *vdev)
> +{
> +	if (!vdev->pci_recovery_supported)
> +		return;
> +
> +	down_write(&vdev->recovery_lock);
> +	WRITE_ONCE(vdev->pci_recovery_enabled, false);
> +	vdev->pci_recovery_command_valid = false;
> +	/*
> +	 * Clear access_blocked before device_open, so a lock-free reader
> +	 * never sees it set on a device which is no longer open. A
> +	 * transaction which is still running cannot clear it once
> +	 * device_open is gone, and paths which refuse work on a blocked
> +	 * device would then refuse it for good.
> +	 */
> +	WRITE_ONCE(vdev->pci_recovery_access_blocked, false);
> +	WRITE_ONCE(vdev->pci_recovery_device_open, false);
> +	WRITE_ONCE(vdev->pci_recovery_flags, 0);
> +
> +	/*
> +	 * Publish the closing state and drop recovery_lock before any
> +	 * teardown. Recovery is disabled and its state cleared, so
> +	 * slot_reset() and resume() become no-ops and a later
> +	 * error_detected() only follows the legacy notification path.
> +	 * Holding the lock across vfio_pci_core_disable() protects nothing
> +	 * and inverts the lock order. disable() reaches pci_reset_bus(),
> +	 * which takes pci_bus_sem, while error_detected() takes
> +	 * recovery_lock from under pci_bus_sem.
> +	 */
> +	up_write(&vdev->recovery_lock);
> +	wake_up_all(&vdev->pci_recovery_wait);
> +}
> +
> +static void vfio_pci_core_finish_close(struct vfio_device *core_vdev)
>   {
>   	struct vfio_pci_core_device *vdev =
>   		container_of(core_vdev, struct vfio_pci_core_device, vdev);
> @@ -838,6 +884,15 @@ void vfio_pci_core_close_device(struct vfio_device *core_vdev)
>   	vfio_pci_eventfd_replace_locked(vdev, &vdev->req_trigger, NULL);
>   	mutex_unlock(&vdev->igate);
>   }
> +
> +void vfio_pci_core_close_device(struct vfio_device *core_vdev)
> +{
> +	struct vfio_pci_core_device *vdev =
> +		container_of(core_vdev, struct vfio_pci_core_device, vdev);
> +
> +	vfio_pci_core_prepare_close(vdev);
> +	vfio_pci_core_finish_close(core_vdev);
> +}
>   EXPORT_SYMBOL_GPL(vfio_pci_core_close_device);
>   
>   void vfio_pci_core_finish_enable(struct vfio_pci_core_device *vdev)
> @@ -852,6 +907,18 @@ void vfio_pci_core_finish_enable(struct vfio_pci_core_device *vdev)
>   		vdev->sriov_pf_core_dev->vf_token->users++;
>   		mutex_unlock(&vdev->sriov_pf_core_dev->vf_token->lock);
>   	}
> +
> +	if (vdev->pci_recovery_supported) {
> +		down_write(&vdev->recovery_lock);
> +		WRITE_ONCE(vdev->pci_recovery_flags, 0);
> +		vdev->pci_recovery_sequence = 0;
> +		WRITE_ONCE(vdev->pci_recovery_enabled, false);
> +		/* Close clears this too. Start unblocked either way. */
> +		WRITE_ONCE(vdev->pci_recovery_access_blocked, false);
> +		WRITE_ONCE(vdev->pci_recovery_device_open, true);
> +		WRITE_ONCE(vdev->pci_recovery_rom_disable, false);
> +		up_write(&vdev->recovery_lock);
> +	}
>   }
>   EXPORT_SYMBOL_GPL(vfio_pci_core_finish_enable);
>   

RE: [RFC PATCH 02/19] vfio/pci: Serialize generic device lifetime with recovery

[Shameer Kolothum Thodi]

> -----Original Message-----
> From: K V P, Satyanarayana <satyanarayana.k.v.p@intel.com>
> Sent: 01 September 2026 14:14
> To: Shameer Kolothum Thodi <skolothumtho@nvidia.com>;
> kvm@vger.kernel.org; linux-pci@vger.kernel.org; linux-kernel@vger.kernel.org
> Cc: alex@shazbot.org; jgg@ziepe.ca; kevin.tian@intel.com;
> kbusch@meta.com; michal.winiarski@intel.com; Sonang Patel
> <sonangp@nvidia.com>; Nathan Chen <nathanc@nvidia.com>; Matt Ochs
> <mochs@nvidia.com>
> Subject: Re: [RFC PATCH 02/19] vfio/pci: Serialize generic device lifetime with
> recovery
> 
> External email: Use caution opening links or attachments
> 
> 
> On 01-Sep-26 3:02 PM, Shameer Kolothum wrote:
> > vfio_pci_core_disable() frees vconfig while holding only the vfio
> > device_set mutex. The PCI error callbacks never take that one. They
> > run under the PCI device_lock instead, and vfio's close path does not
> > hold that. So a callback still running when close starts can walk into
> > state which is being freed.
> >
> > Publish a device_open flag under recovery_lock. enable() clears it
> > before it touches the device, finish_enable() sets it once
> > vfio_config_init() has allocated vconfig, and prepare_close() clears
> > it again before the teardown frees vconfig. All three take
> > recovery_lock for writing, so a callback either gets there first and
> > close waits for it, or it finds the flag clear and does nothing. The
> > access guards added later test the same flag.
> >
> > recovery_lock is not held across vfio_pci_core_disable(). A later
> > patch has error_detected() take it from under pci_bus_sem, and
> > disable() gets to pci_reset_bus(), which takes pci_bus_sem the other way
> round.
> >
> > access_blocked is only ever set while device_open is set. Nothing sets
> > it without testing device_open first, and close clears access_blocked
> > before it clears device_open. If close left it set, nothing could
> > clear it afterwards.
> > The transaction which set it cannot clear it once device_open is gone,
> > and every path which refuses work on a blocked device would go on
> > refusing. Clear it before device_open so a lock-free reader never sees
> > it set on a device which is closed.
> >
> > open() now refuses a disconnected device with -ENODEV. That is new.
> >
> > Signed-off-by: Shameer Kolothum <skolothumtho@nvidia.com>
> > ---
> >   drivers/vfio/pci/vfio_pci_core.c | 69 +++++++++++++++++++++++++++++++-
> >   1 file changed, 68 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/vfio/pci/vfio_pci_core.c
> > b/drivers/vfio/pci/vfio_pci_core.c
> > index e0be5ddf7039..8de586e4bb73 100644
> > --- a/drivers/vfio/pci/vfio_pci_core.c
> > +++ b/drivers/vfio/pci/vfio_pci_core.c
> > @@ -591,10 +591,23 @@ static const struct dev_pm_ops
> vfio_pci_core_pm_ops = {
> >   int vfio_pci_core_enable(struct vfio_pci_core_device *vdev)
> >   {
> >       struct pci_dev *pdev = vdev->pdev;
> > +     bool supported = vdev->pci_recovery_supported;
> >       int ret;
> >       u16 cmd;
> >       u8 msix_pos;
> >
> > +     if (supported) {
> > +             down_write(&vdev->recovery_lock);
> > +             if (pci_dev_is_disconnected(pdev)) {
> > +                     up_write(&vdev->recovery_lock);
> > +                     return -ENODEV;
> > +             }
> > +
> > +             vdev->pci_recovery_command_valid = false;
> > +             WRITE_ONCE(vdev->pci_recovery_device_open, false);
> > +             up_write(&vdev->recovery_lock);
> > +     }
> > +
> 
> Can we use scoped_guard()/guard() instead of down_write()/up_write()?
> The same comment applies across all similar sections.

Sure, thanks. I will convert these in the next respin wherever it
works out.

Most of the sections take the lock and drop it in the same block, so
they convert cleanly. A few will not, I guess. Some of the callers
drop the lock part way through on purpose, so that the work which
follows runs outside the lock and keeps recovery_lock below pci_bus_sem.
I will double check if there is a way to convert those as well.

Thanks,
Shameer

[RFC PATCH 03/19] vfio/pci: Add PCI recovery access guards

[Shameer Kolothum]

Add a pair of helpers to wrap each operation which touches the device.
access_begin() takes recovery_lock for reading and refuses if the device
is not open, or if recovery is blocking access. The callers come in later
patches.

access_end() drops the lock without looking at the recovery state, so
only call it after access_begin() returned 0. On failure the lock is
already gone. Both helpers key off pci_recovery_supported, which is
fixed for the lifetime of the device, so the pair stays balanced.

The lock is taken even when userspace has not enabled recovery. Enabling
takes recovery_lock for writing, which waits for anything already in
flight. Without that, an operation which started before enable could
still be touching the device when the first error arrives, and there would
be nothing to wait on.

access_blocked is checked either way. Nothing sets it yet, so nothing
which works today gets rejected.

The cost is one rwsem acquire per guarded access on devices that never
turn recovery on. For BAR traffic that is once per width-sized access,
alongside the memory_lock read already taken there.

Signed-off-by: Shameer Kolothum <skolothumtho@nvidia.com>
---
 drivers/vfio/pci/vfio_pci_priv.h |  3 +++
 drivers/vfio/pci/vfio_pci_core.c | 21 +++++++++++++++++++++
 2 files changed, 24 insertions(+)

diff --git a/drivers/vfio/pci/vfio_pci_priv.h b/drivers/vfio/pci/vfio_pci_priv.h
index 4e7162234a2e..6daf51669d05 100644
--- a/drivers/vfio/pci/vfio_pci_priv.h
+++ b/drivers/vfio/pci/vfio_pci_priv.h
@@ -73,6 +73,9 @@ u16 vfio_pci_memory_lock_and_enable(struct vfio_pci_core_device *vdev);
 void vfio_pci_memory_unlock_and_restore(struct vfio_pci_core_device *vdev,
 					u16 cmd);
 
+int vfio_pci_core_access_begin(struct vfio_pci_core_device *vdev);
+void vfio_pci_core_access_end(struct vfio_pci_core_device *vdev);
+
 #ifdef CONFIG_VFIO_PCI_IGD
 bool vfio_pci_is_intel_display(struct pci_dev *pdev);
 int vfio_pci_igd_init(struct vfio_pci_core_device *vdev);
diff --git a/drivers/vfio/pci/vfio_pci_core.c b/drivers/vfio/pci/vfio_pci_core.c
index 8de586e4bb73..4194d44d6530 100644
--- a/drivers/vfio/pci/vfio_pci_core.c
+++ b/drivers/vfio/pci/vfio_pci_core.c
@@ -1747,6 +1747,27 @@ static ssize_t vfio_pci_rw(struct vfio_pci_core_device *vdev, char __user *buf,
 	return ret;
 }
 
+int vfio_pci_core_access_begin(struct vfio_pci_core_device *vdev)
+{
+	if (!vdev->pci_recovery_supported)
+		return 0;
+
+	down_read(&vdev->recovery_lock);
+	if (unlikely(!vdev->pci_recovery_device_open ||
+		     vdev->pci_recovery_access_blocked)) {
+		up_read(&vdev->recovery_lock);
+		return -EIO;
+	}
+
+	return 0;
+}
+
+void vfio_pci_core_access_end(struct vfio_pci_core_device *vdev)
+{
+	if (vdev->pci_recovery_supported)
+		up_read(&vdev->recovery_lock);
+}
+
 ssize_t vfio_pci_core_read(struct vfio_device *core_vdev, char __user *buf,
 		size_t count, loff_t *ppos)
 {
-- 
2.43.0

Re: [RFC PATCH 03/19] vfio/pci: Add PCI recovery access guards

[sashiko-bot]

> Add a pair of helpers to wrap each operation which touches the device.
> access_begin() takes recovery_lock for reading and refuses if the device
> is not open, or if recovery is blocking access. The callers come in later
> patches.
> 
> access_end() drops the lock without looking at the recovery state, so
> only call it after access_begin() returned 0. On failure the lock is
> already gone. Both helpers key off pci_recovery_supported, which is
> fixed for the lifetime of the device, so the pair stays balanced.
> 
> The lock is taken even when userspace has not enabled recovery. Enabling
> takes recovery_lock for writing, which waits for anything already in
> flight. Without that, an operation which started before enable could
> still be touching the device when the first error arrives, and there would
> be nothing to wait on.
> 
> access_blocked is checked either way. Nothing sets it yet, so nothing
> which works today gets rejected.
> 
> The cost is one rwsem acquire per guarded access on devices that never
> turn recovery on. For BAR traffic that is once per width-sized access,
> alongside the memory_lock read already taken there.
> 
> Signed-off-by: Shameer Kolothum <skolothumtho@nvidia.com>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260901093217.8539-1-skolothumtho@nvidia.com?part=3

[RFC PATCH 04/19] vfio/pci: Serialize function reset with recovery

[Shameer Kolothum]

Add a function reset helper and use it for VFIO_DEVICE_RESET. A later
patch routes the guest triggered config space FLR through it as well.
That path never did the power state transition, so make it optional.

With recovery enabled, take recovery_lock for writing, refuse the reset
with -EBUSY if access is already blocked, otherwise block access and drop
the lock again before revoking mappings or running the reset.
recovery_lock cannot be held across the reset because a reset method can
take pci_bus_sem, and the PCI error callbacks take recovery_lock from
under it.

Dropping it is safe in both directions. The error callbacks hold
recovery_lock for their whole body, so one already running has finished
before the reset starts. One which arrives while the lock is down runs
its own event, and the PCI core calls it with the device lock held, which
pci_try_reset_function() also takes, so it cannot overlap the reset
itself.

Only unblock access at the end for a reset which is still the one
blocking it. An event which started meanwhile owns the state from then
on, and resume() is what ends it.

With recovery not enabled, leave access_blocked alone. Two concurrent
resets still serialize on memory_lock, same as today. Setting the flag
for a device which never opted in would turn a working VFIO_DEVICE_RESET
into -EBUSY.

Access stays blocked until the reset is done and memory state is back,
and the wait queue is woken once it clears. A later patch adds the BAR
fault path, which waits there rather than failing the fault while a
reset is in flight.

Assisted-by: Claude:claude-opus-5
Signed-off-by: Shameer Kolothum <skolothumtho@nvidia.com>
---
 drivers/vfio/pci/vfio_pci_priv.h |  3 ++
 drivers/vfio/pci/vfio_pci_core.c | 85 +++++++++++++++++++++++++++++---
 2 files changed, 82 insertions(+), 6 deletions(-)

diff --git a/drivers/vfio/pci/vfio_pci_priv.h b/drivers/vfio/pci/vfio_pci_priv.h
index 6daf51669d05..8a7f9fe22386 100644
--- a/drivers/vfio/pci/vfio_pci_priv.h
+++ b/drivers/vfio/pci/vfio_pci_priv.h
@@ -41,6 +41,9 @@ ssize_t vfio_pci_config_rw_single(struct vfio_pci_core_device *vdev,
 				  char __user *buf, size_t count, loff_t *ppos,
 				  bool iswrite);
 
+int vfio_pci_try_reset_function(struct vfio_pci_core_device *vdev,
+				bool reset_power_state);
+
 ssize_t vfio_pci_bar_rw(struct vfio_pci_core_device *vdev, char __user *buf,
 			size_t count, loff_t *ppos, bool iswrite);
 
diff --git a/drivers/vfio/pci/vfio_pci_core.c b/drivers/vfio/pci/vfio_pci_core.c
index 4194d44d6530..3645daa8891f 100644
--- a/drivers/vfio/pci/vfio_pci_core.c
+++ b/drivers/vfio/pci/vfio_pci_core.c
@@ -1379,14 +1379,53 @@ static int vfio_pci_ioctl_set_irqs(struct vfio_pci_core_device *vdev,
 	return ret;
 }
 
-static int vfio_pci_ioctl_reset(struct vfio_pci_core_device *vdev,
-				void __user *arg)
+int vfio_pci_try_reset_function(struct vfio_pci_core_device *vdev,
+				bool reset_power_state)
 {
+	struct pci_dev *pdev = vdev->pdev;
+	bool enabled = false;
+	bool supported = vdev->pci_recovery_supported;
 	int ret;
 
-	if (!vdev->reset_works)
-		return -EINVAL;
+	/*
+	 * Claim the device against recovery before resetting it. The PCI
+	 * error callbacks hold recovery_lock for their whole body, so taking
+	 * it for writing here waits for one already running, and
+	 * access_blocked keeps a later one away while the lock is dropped.
+	 */
+	if (supported) {
+		down_write(&vdev->recovery_lock);
+		if (!vdev->pci_recovery_device_open) {
+			ret = -ENODEV;
+			goto out_recovery;
+		}
 
+		enabled = vdev->pci_recovery_enabled;
+
+		/*
+		 * Only claim access_blocked when recovery is enabled.
+		 * error_detected() returns early for a device which has not
+		 * enabled it, so there is nothing to exclude, and claiming it
+		 * anyway would fail the second of two concurrent
+		 * VFIO_DEVICE_RESET calls with -EBUSY.
+		 */
+		if (enabled) {
+			if (vdev->pci_recovery_access_blocked) {
+				ret = -EBUSY;
+				goto out_recovery;
+			}
+			WRITE_ONCE(vdev->pci_recovery_access_blocked, true);
+		}
+		up_write(&vdev->recovery_lock);
+	}
+
+	/*
+	 * On a device which supports recovery, taking recovery_lock for
+	 * writing above waited for anything already past its access check,
+	 * and if recovery is enabled access_blocked keeps new ones out. Do
+	 * not hold recovery_lock while taking memory_lock or running a reset
+	 * method, since a reset can take pci_bus_sem.
+	 */
 	vfio_pci_zap_and_down_write_memory_lock(vdev);
 
 	/*
@@ -1398,15 +1437,49 @@ static int vfio_pci_ioctl_reset(struct vfio_pci_core_device *vdev,
 	 * reset without restoring the original state (saved locally in
 	 * 'vdev->pm_save').
 	 */
-	vfio_pci_set_power_state(vdev, PCI_D0);
+	if (reset_power_state)
+		vfio_pci_set_power_state(vdev, PCI_D0);
 
 	vfio_pci_dma_buf_move(vdev, true);
-	ret = pci_try_reset_function(vdev->pdev);
+	ret = pci_try_reset_function(pdev);
 	if (__vfio_pci_memory_enabled(vdev))
 		vfio_pci_dma_buf_move(vdev, false);
 	up_write(&vdev->memory_lock);
 
+	if (enabled) {
+		down_write(&vdev->recovery_lock);
+		/*
+		 * An error callback can have started an event while the lock
+		 * was down. Leave the state to it. Only unblock access for a
+		 * reset which is still the one holding it.
+		 */
+		if (vdev->pci_recovery_device_open &&
+		    !(vdev->pci_recovery_flags & (VFIO_PCI_RECOVERY_IN_PROGRESS |
+						  VFIO_PCI_RECOVERY_FAILED)))
+			WRITE_ONCE(vdev->pci_recovery_access_blocked, false);
+		up_write(&vdev->recovery_lock);
+		/*
+		 * Access is blocked for the length of the reset, so anything
+		 * waiting for it to clear has to be woken here. A later patch
+		 * adds the BAR fault path which waits on this.
+		 */
+		wake_up_all(&vdev->pci_recovery_wait);
+	}
+
 	return ret;
+
+out_recovery:
+	up_write(&vdev->recovery_lock);
+	return ret;
+}
+
+static int vfio_pci_ioctl_reset(struct vfio_pci_core_device *vdev,
+				void __user *arg)
+{
+	if (!vdev->reset_works)
+		return -EINVAL;
+
+	return vfio_pci_try_reset_function(vdev, true);
 }
 
 static int vfio_pci_ioctl_get_pci_hot_reset_info(
-- 
2.43.0

Re: [RFC PATCH 04/19] vfio/pci: Serialize function reset with recovery

[sashiko-bot]

> Add a function reset helper and use it for VFIO_DEVICE_RESET. A later
> patch routes the guest triggered config space FLR through it as well.
> That path never did the power state transition, so make it optional.
> 
> With recovery enabled, take recovery_lock for writing, refuse the reset
> with -EBUSY if access is already blocked, otherwise block access and drop
> the lock again before revoking mappings or running the reset.
> recovery_lock cannot be held across the reset because a reset method can
> take pci_bus_sem, and the PCI error callbacks take recovery_lock from
> under it.
> 
> Dropping it is safe in both directions. The error callbacks hold
> recovery_lock for their whole body, so one already running has finished
> before the reset starts. One which arrives while the lock is down runs
> its own event, and the PCI core calls it with the device lock held, which
> [ ... ]
> fault path, which waits there rather than failing the fault while a
> reset is in flight.
> 
> Assisted-by: Claude:claude-opus-5
> Signed-off-by: Shameer Kolothum <skolothumtho@nvidia.com>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260901093217.8539-1-skolothumtho@nvidia.com?part=4

Re: [RFC PATCH 04/19] vfio/pci: Serialize function reset with recovery

[K V P, Satyanarayana]

On 01-Sep-26 3:02 PM, Shameer Kolothum wrote:
> Add a function reset helper and use it for VFIO_DEVICE_RESET. A later
> patch routes the guest triggered config space FLR through it as well.
> That path never did the power state transition, so make it optional.
>
> With recovery enabled, take recovery_lock for writing, refuse the reset
> with -EBUSY if access is already blocked, otherwise block access and drop
> the lock again before revoking mappings or running the reset.
> recovery_lock cannot be held across the reset because a reset method can
> take pci_bus_sem, and the PCI error callbacks take recovery_lock from
> under it.
>
> Dropping it is safe in both directions. The error callbacks hold
> recovery_lock for their whole body, so one already running has finished
> before the reset starts. One which arrives while the lock is down runs
> its own event, and the PCI core calls it with the device lock held, which
> pci_try_reset_function() also takes, so it cannot overlap the reset
> itself.
>
> Only unblock access at the end for a reset which is still the one
> blocking it. An event which started meanwhile owns the state from then
> on, and resume() is what ends it.
>
> With recovery not enabled, leave access_blocked alone. Two concurrent
> resets still serialize on memory_lock, same as today. Setting the flag
> for a device which never opted in would turn a working VFIO_DEVICE_RESET
> into -EBUSY.
>
> Access stays blocked until the reset is done and memory state is back,
> and the wait queue is woken once it clears. A later patch adds the BAR
> fault path, which waits there rather than failing the fault while a
> reset is in flight.
>
> Assisted-by: Claude:claude-opus-5
> Signed-off-by: Shameer Kolothum <skolothumtho@nvidia.com>
> ---
>   drivers/vfio/pci/vfio_pci_priv.h |  3 ++
>   drivers/vfio/pci/vfio_pci_core.c | 85 +++++++++++++++++++++++++++++---
>   2 files changed, 82 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/vfio/pci/vfio_pci_priv.h b/drivers/vfio/pci/vfio_pci_priv.h
> index 6daf51669d05..8a7f9fe22386 100644
> --- a/drivers/vfio/pci/vfio_pci_priv.h
> +++ b/drivers/vfio/pci/vfio_pci_priv.h
> @@ -41,6 +41,9 @@ ssize_t vfio_pci_config_rw_single(struct vfio_pci_core_device *vdev,
>   				  char __user *buf, size_t count, loff_t *ppos,
>   				  bool iswrite);
>   
> +int vfio_pci_try_reset_function(struct vfio_pci_core_device *vdev,
> +				bool reset_power_state);
> +
>   ssize_t vfio_pci_bar_rw(struct vfio_pci_core_device *vdev, char __user *buf,
>   			size_t count, loff_t *ppos, bool iswrite);
>   
> diff --git a/drivers/vfio/pci/vfio_pci_core.c b/drivers/vfio/pci/vfio_pci_core.c
> index 4194d44d6530..3645daa8891f 100644
> --- a/drivers/vfio/pci/vfio_pci_core.c
> +++ b/drivers/vfio/pci/vfio_pci_core.c
> @@ -1379,14 +1379,53 @@ static int vfio_pci_ioctl_set_irqs(struct vfio_pci_core_device *vdev,
>   	return ret;
>   }
>   
> -static int vfio_pci_ioctl_reset(struct vfio_pci_core_device *vdev,
> -				void __user *arg)
> +int vfio_pci_try_reset_function(struct vfio_pci_core_device *vdev,
> +				bool reset_power_state)
>   {
> +	struct pci_dev *pdev = vdev->pdev;
> +	bool enabled = false;
> +	bool supported = vdev->pci_recovery_supported;

Can we use a helper function to get pci recovery is supported or not?

Maintainability will be easy with helper function than direct assignment.

-Satya.

>   	int ret;
>   
> -	if (!vdev->reset_works)
> -		return -EINVAL;
> +	/*
> +	 * Claim the device against recovery before resetting it. The PCI
> +	 * error callbacks hold recovery_lock for their whole body, so taking
> +	 * it for writing here waits for one already running, and
> +	 * access_blocked keeps a later one away while the lock is dropped.
> +	 */
> +	if (supported) {
> +		down_write(&vdev->recovery_lock);
> +		if (!vdev->pci_recovery_device_open) {
> +			ret = -ENODEV;
> +			goto out_recovery;
> +		}
>   
> +		enabled = vdev->pci_recovery_enabled;
> +
> +		/*
> +		 * Only claim access_blocked when recovery is enabled.
> +		 * error_detected() returns early for a device which has not
> +		 * enabled it, so there is nothing to exclude, and claiming it
> +		 * anyway would fail the second of two concurrent
> +		 * VFIO_DEVICE_RESET calls with -EBUSY.
> +		 */
> +		if (enabled) {
> +			if (vdev->pci_recovery_access_blocked) {
> +				ret = -EBUSY;
> +				goto out_recovery;
> +			}
> +			WRITE_ONCE(vdev->pci_recovery_access_blocked, true);
> +		}
> +		up_write(&vdev->recovery_lock);
> +	}
> +
> +	/*
> +	 * On a device which supports recovery, taking recovery_lock for
> +	 * writing above waited for anything already past its access check,
> +	 * and if recovery is enabled access_blocked keeps new ones out. Do
> +	 * not hold recovery_lock while taking memory_lock or running a reset
> +	 * method, since a reset can take pci_bus_sem.
> +	 */
>   	vfio_pci_zap_and_down_write_memory_lock(vdev);
>   
>   	/*
> @@ -1398,15 +1437,49 @@ static int vfio_pci_ioctl_reset(struct vfio_pci_core_device *vdev,
>   	 * reset without restoring the original state (saved locally in
>   	 * 'vdev->pm_save').
>   	 */
> -	vfio_pci_set_power_state(vdev, PCI_D0);
> +	if (reset_power_state)
> +		vfio_pci_set_power_state(vdev, PCI_D0);
>   
>   	vfio_pci_dma_buf_move(vdev, true);
> -	ret = pci_try_reset_function(vdev->pdev);
> +	ret = pci_try_reset_function(pdev);
>   	if (__vfio_pci_memory_enabled(vdev))
>   		vfio_pci_dma_buf_move(vdev, false);
>   	up_write(&vdev->memory_lock);
>   
> +	if (enabled) {
> +		down_write(&vdev->recovery_lock);
> +		/*
> +		 * An error callback can have started an event while the lock
> +		 * was down. Leave the state to it. Only unblock access for a
> +		 * reset which is still the one holding it.
> +		 */
> +		if (vdev->pci_recovery_device_open &&
> +		    !(vdev->pci_recovery_flags & (VFIO_PCI_RECOVERY_IN_PROGRESS |
> +						  VFIO_PCI_RECOVERY_FAILED)))
> +			WRITE_ONCE(vdev->pci_recovery_access_blocked, false);
> +		up_write(&vdev->recovery_lock);
> +		/*
> +		 * Access is blocked for the length of the reset, so anything
> +		 * waiting for it to clear has to be woken here. A later patch
> +		 * adds the BAR fault path which waits on this.
> +		 */
> +		wake_up_all(&vdev->pci_recovery_wait);
> +	}
> +
>   	return ret;
> +
> +out_recovery:
> +	up_write(&vdev->recovery_lock);
> +	return ret;
> +}
> +
> +static int vfio_pci_ioctl_reset(struct vfio_pci_core_device *vdev,
> +				void __user *arg)
> +{
> +	if (!vdev->reset_works)
> +		return -EINVAL;
> +
> +	return vfio_pci_try_reset_function(vdev, true);
>   }
>   
>   static int vfio_pci_ioctl_get_pci_hot_reset_info(

RE: [RFC PATCH 04/19] vfio/pci: Serialize function reset with recovery

[Shameer Kolothum Thodi]

> -----Original Message-----
> From: K V P, Satyanarayana <satyanarayana.k.v.p@intel.com>
> Sent: 02 September 2026 07:06
> To: Shameer Kolothum Thodi <skolothumtho@nvidia.com>;
> kvm@vger.kernel.org; linux-pci@vger.kernel.org; linux-kernel@vger.kernel.org
> Cc: alex@shazbot.org; jgg@ziepe.ca; kevin.tian@intel.com;
> kbusch@meta.com; michal.winiarski@intel.com; Sonang Patel
> <sonangp@nvidia.com>; Nathan Chen <nathanc@nvidia.com>; Matt Ochs
> <mochs@nvidia.com>
> Subject: Re: [RFC PATCH 04/19] vfio/pci: Serialize function reset with recovery

[...]
> > +int vfio_pci_try_reset_function(struct vfio_pci_core_device *vdev,
> > +                             bool reset_power_state)
> >   {
> > +     struct pci_dev *pdev = vdev->pdev;
> > +     bool enabled = false;
> > +     bool supported = vdev->pci_recovery_supported;
> 
> Can we use a helper function to get pci recovery is supported or not?
> 
> Maintainability will be easy with helper function than direct assignment.

Ok. Maybe we can add something similar to vfio_device_cdev_opened()
and I don’t think that local "supported" is required here.

Thanks,
Shameer

[RFC PATCH 05/19] vfio/pci: Serialize config access with recovery

[Shameer Kolothum]

Hold recovery_lock for reading across each config space operation, so
recovery can shut out new ones and wait for whatever is already running.
The user copies stay outside the lock, since a copy can fault.

Take the lock in the dispatcher rather than around the individual
hardware accessors. That means once recovery blocks access every config
read fails with -EIO, even a read served entirely from vconfig which
never touches the device. Userspace which wants to know what is going on
reads the device feature instead. That one stays available during an
event.

The PCIe and AF capability writes no longer reset the device themselves,
and the power management write no longer moves it to D0 itself. They
record what was asked for and the dispatcher does it after dropping
recovery_lock. Both take pci_bus_sem, which AER already holds when it
calls into the driver, so doing either inside the lock would be the wrong
order. A reset method reaches it directly, and a D0 transition reaches it
through pci_set_full_power_state() calling
pcie_aspm_pm_state_change(). The lower power states take neither, so
those still run in the writefn. The writefn declaration says so.

Both stay best effort, as the guest requested FLR always was. The result
is not reported back through the config write. With recovery enabled they
are dropped while a recovery or reset is already in flight, since that
leaves the device in D0 and reset anyway. The reset helper tests the
recovery state for itself. The power up does not, so the dispatcher
tests it before that one.

Assisted-by: Claude:claude-opus-5
Signed-off-by: Shameer Kolothum <skolothumtho@nvidia.com>
---
 drivers/vfio/pci/vfio_pci_config.c | 147 ++++++++++++++++++++---------
 1 file changed, 102 insertions(+), 45 deletions(-)

diff --git a/drivers/vfio/pci/vfio_pci_config.c b/drivers/vfio/pci/vfio_pci_config.c
index 9914f3ac69ae..3365100acf21 100644
--- a/drivers/vfio/pci/vfio_pci_config.c
+++ b/drivers/vfio/pci/vfio_pci_config.c
@@ -99,6 +99,12 @@ static const u16 pci_ext_cap_length[PCI_EXT_CAP_ID_MAX + 1] = {
 	[PCI_EXT_CAP_ID_DVSEC]	=	0xFF,
 };
 
+/* What a config write asked for which has to wait for the access guard. */
+struct vfio_pci_config_deferred {
+	bool	flr;		/* a function-level reset */
+	bool	power_up;	/* a transition to D0 */
+};
+
 /*
  * Read/Write Permission Bits - one bit for each bit in capability
  * Any field can be read if it exists, but what is read depends on
@@ -111,8 +117,17 @@ struct perm_bits {
 	u8	*write;		/* writeable bits */
 	int	(*readfn)(struct vfio_pci_core_device *vdev, int pos, int count,
 			  struct perm_bits *perm, int offset, __le32 *val);
+	/*
+	 * @deferred records work the write asked for which a writefn must not
+	 * do itself. Both a reset method and a transition to D0 acquire
+	 * pci_bus_sem, which AER already holds when it enters the driver, so
+	 * doing either here would invert the lock order against recovery_lock.
+	 * The dispatcher does them after dropping recovery_lock. Callers zero
+	 * it, and a writefn only sets a field on a success return.
+	 */
 	int	(*writefn)(struct vfio_pci_core_device *vdev, int pos, int count,
-			   struct perm_bits *perm, int offset, __le32 val);
+			   struct perm_bits *perm, int offset, __le32 val,
+			   struct vfio_pci_config_deferred *deferred);
 };
 
 #define	NO_VIRT		0
@@ -200,7 +215,8 @@ static int vfio_default_config_read(struct vfio_pci_core_device *vdev, int pos,
 
 static int vfio_default_config_write(struct vfio_pci_core_device *vdev, int pos,
 				     int count, struct perm_bits *perm,
-				     int offset, __le32 val)
+				     int offset, __le32 val,
+				     struct vfio_pci_config_deferred *deferred)
 {
 	__le32 virt = 0, write = 0;
 
@@ -272,7 +288,8 @@ static int vfio_direct_config_read(struct vfio_pci_core_device *vdev, int pos,
 /* Raw access skips any kind of virtualization */
 static int vfio_raw_config_write(struct vfio_pci_core_device *vdev, int pos,
 				 int count, struct perm_bits *perm,
-				 int offset, __le32 val)
+				 int offset, __le32 val,
+				 struct vfio_pci_config_deferred *deferred)
 {
 	int ret;
 
@@ -299,7 +316,8 @@ static int vfio_raw_config_read(struct vfio_pci_core_device *vdev, int pos,
 /* Virt access uses only virtualization */
 static int vfio_virt_config_write(struct vfio_pci_core_device *vdev, int pos,
 				  int count, struct perm_bits *perm,
-				  int offset, __le32 val)
+				  int offset, __le32 val,
+				  struct vfio_pci_config_deferred *deferred)
 {
 	memcpy(vdev->vconfig + pos, &val, count);
 	return count;
@@ -563,7 +581,8 @@ static bool vfio_need_bar_restore(struct vfio_pci_core_device *vdev)
 
 static int vfio_basic_config_write(struct vfio_pci_core_device *vdev, int pos,
 				   int count, struct perm_bits *perm,
-				   int offset, __le32 val)
+				   int offset, __le32 val,
+				   struct vfio_pci_config_deferred *deferred)
 {
 	struct pci_dev *pdev = vdev->pdev;
 	__le16 *virt_cmd;
@@ -613,7 +632,8 @@ static int vfio_basic_config_write(struct vfio_pci_core_device *vdev, int pos,
 			vfio_bar_restore(vdev);
 	}
 
-	count = vfio_default_config_write(vdev, pos, count, perm, offset, val);
+	count = vfio_default_config_write(vdev, pos, count, perm, offset, val,
+					  deferred);
 	if (count < 0) {
 		if (offset == PCI_COMMAND)
 			up_write(&vdev->memory_lock);
@@ -727,9 +747,11 @@ static void vfio_lock_and_set_power_state(struct vfio_pci_core_device *vdev,
 
 static int vfio_pm_config_write(struct vfio_pci_core_device *vdev, int pos,
 				int count, struct perm_bits *perm,
-				int offset, __le32 val)
+				int offset, __le32 val,
+				struct vfio_pci_config_deferred *deferred)
 {
-	count = vfio_default_config_write(vdev, pos, count, perm, offset, val);
+	count = vfio_default_config_write(vdev, pos, count, perm, offset, val,
+					  deferred);
 	if (count < 0)
 		return count;
 
@@ -738,8 +760,15 @@ static int vfio_pm_config_write(struct vfio_pci_core_device *vdev, int pos,
 
 		switch (le32_to_cpu(val) & PCI_PM_CTRL_STATE_MASK) {
 		case 0:
-			state = PCI_D0;
-			break;
+			/*
+			 * Going to D0 reaches pci_set_full_power_state(),
+			 * which takes pci_bus_sem through
+			 * pcie_aspm_pm_state_change(). Leave it to the
+			 * dispatcher. The lower states do not, so they run
+			 * here.
+			 */
+			deferred->power_up = true;
+			return count;
 		case 1:
 			state = PCI_D1;
 			break;
@@ -799,7 +828,8 @@ static int __init init_pci_cap_pm_perm(struct perm_bits *perm)
 
 static int vfio_vpd_config_write(struct vfio_pci_core_device *vdev, int pos,
 				 int count, struct perm_bits *perm,
-				 int offset, __le32 val)
+				 int offset, __le32 val,
+				 struct vfio_pci_config_deferred *deferred)
 {
 	struct pci_dev *pdev = vdev->pdev;
 	__le16 *paddr = (__le16 *)(vdev->vconfig + pos - offset + PCI_VPD_ADDR);
@@ -812,7 +842,8 @@ static int vfio_vpd_config_write(struct vfio_pci_core_device *vdev, int pos,
 	 * of PCI_VPD_ADDR, then the PCI_VPD_ADDR_F bit is written and we
 	 * have work to do.
 	 */
-	count = vfio_default_config_write(vdev, pos, count, perm, offset, val);
+	count = vfio_default_config_write(vdev, pos, count, perm, offset, val,
+					  deferred);
 	if (count < 0 || offset > PCI_VPD_ADDR + 1 ||
 	    offset + count <= PCI_VPD_ADDR + 1)
 		return count;
@@ -881,21 +912,24 @@ static int __init init_pci_cap_pcix_perm(struct perm_bits *perm)
 
 static int vfio_exp_config_write(struct vfio_pci_core_device *vdev, int pos,
 				 int count, struct perm_bits *perm,
-				 int offset, __le32 val)
+				 int offset, __le32 val,
+				 struct vfio_pci_config_deferred *deferred)
 {
 	__le16 *ctrl = (__le16 *)(vdev->vconfig + pos -
 				  offset + PCI_EXP_DEVCTL);
 	int readrq = le16_to_cpu(*ctrl) & PCI_EXP_DEVCTL_READRQ;
 
-	count = vfio_default_config_write(vdev, pos, count, perm, offset, val);
+	count = vfio_default_config_write(vdev, pos, count, perm, offset, val,
+					  deferred);
 	if (count < 0)
 		return count;
 
 	/*
 	 * The FLR bit is virtualized, if set and the device supports PCIe
-	 * FLR, issue a reset_function.  Regardless, clear the bit, the spec
-	 * requires it to be always read as zero.  NB, reset_function might
-	 * not use a PCIe FLR, we don't have that level of granularity.
+	 * FLR, request a function reset once recovery_lock has been
+	 * released. Regardless, clear the bit, the spec requires it to be
+	 * always read as zero. NB, reset_function might not use a PCIe FLR,
+	 * we don't have that level of granularity.
 	 */
 	if (*ctrl & cpu_to_le16(PCI_EXP_DEVCTL_BCR_FLR)) {
 		u32 cap;
@@ -907,14 +941,8 @@ static int vfio_exp_config_write(struct vfio_pci_core_device *vdev, int pos,
 						 pos - offset + PCI_EXP_DEVCAP,
 						 &cap);
 
-		if (!ret && (cap & PCI_EXP_DEVCAP_FLR)) {
-			vfio_pci_zap_and_down_write_memory_lock(vdev);
-			vfio_pci_dma_buf_move(vdev, true);
-			pci_try_reset_function(vdev->pdev);
-			if (__vfio_pci_memory_enabled(vdev))
-				vfio_pci_dma_buf_move(vdev, false);
-			up_write(&vdev->memory_lock);
-		}
+		if (!ret && (cap & PCI_EXP_DEVCAP_FLR))
+			deferred->flr = true;
 	}
 
 	/*
@@ -968,19 +996,22 @@ static int __init init_pci_cap_exp_perm(struct perm_bits *perm)
 
 static int vfio_af_config_write(struct vfio_pci_core_device *vdev, int pos,
 				int count, struct perm_bits *perm,
-				int offset, __le32 val)
+				int offset, __le32 val,
+				struct vfio_pci_config_deferred *deferred)
 {
 	u8 *ctrl = vdev->vconfig + pos - offset + PCI_AF_CTRL;
 
-	count = vfio_default_config_write(vdev, pos, count, perm, offset, val);
+	count = vfio_default_config_write(vdev, pos, count, perm, offset, val,
+					  deferred);
 	if (count < 0)
 		return count;
 
 	/*
 	 * The FLR bit is virtualized, if set and the device supports AF
-	 * FLR, issue a reset_function.  Regardless, clear the bit, the spec
-	 * requires it to be always read as zero.  NB, reset_function might
-	 * not use an AF FLR, we don't have that level of granularity.
+	 * FLR, request a function reset once recovery_lock has been
+	 * released. Regardless, clear the bit, the spec requires it to be
+	 * always read as zero. NB, reset_function might not use an AF FLR,
+	 * we don't have that level of granularity.
 	 */
 	if (*ctrl & PCI_AF_CTRL_FLR) {
 		u8 cap;
@@ -992,14 +1023,8 @@ static int vfio_af_config_write(struct vfio_pci_core_device *vdev, int pos,
 						pos - offset + PCI_AF_CAP,
 						&cap);
 
-		if (!ret && (cap & PCI_AF_CAP_FLR) && (cap & PCI_AF_CAP_TP)) {
-			vfio_pci_zap_and_down_write_memory_lock(vdev);
-			vfio_pci_dma_buf_move(vdev, true);
-			pci_try_reset_function(vdev->pdev);
-			if (__vfio_pci_memory_enabled(vdev))
-				vfio_pci_dma_buf_move(vdev, false);
-			up_write(&vdev->memory_lock);
-		}
+		if (!ret && (cap & PCI_AF_CAP_FLR) && (cap & PCI_AF_CAP_TP))
+			deferred->flr = true;
 	}
 
 	return count;
@@ -1168,9 +1193,11 @@ static int vfio_msi_config_read(struct vfio_pci_core_device *vdev, int pos,
 
 static int vfio_msi_config_write(struct vfio_pci_core_device *vdev, int pos,
 				 int count, struct perm_bits *perm,
-				 int offset, __le32 val)
+				 int offset, __le32 val,
+				 struct vfio_pci_config_deferred *deferred)
 {
-	count = vfio_default_config_write(vdev, pos, count, perm, offset, val);
+	count = vfio_default_config_write(vdev, pos, count, perm, offset, val,
+					  deferred);
 	if (count < 0)
 		return count;
 
@@ -1889,6 +1916,8 @@ ssize_t vfio_pci_config_rw_single(struct vfio_pci_core_device *vdev,
 	struct perm_bits *perm;
 	__le32 val = 0;
 	int cap_start = 0, offset;
+	int access_ret;
+	struct vfio_pci_config_deferred deferred = {};
 	u8 cap_id;
 	ssize_t ret;
 
@@ -1957,14 +1986,42 @@ ssize_t vfio_pci_config_rw_single(struct vfio_pci_core_device *vdev,
 		if (copy_from_user(&val, buf, count))
 			return -EFAULT;
 
-		ret = perm->writefn(vdev, *ppos, count, perm, offset, val);
+		access_ret = vfio_pci_core_access_begin(vdev);
+		if (access_ret)
+			return access_ret;
+		ret = perm->writefn(vdev, *ppos, count, perm, offset, val,
+				    &deferred);
+		vfio_pci_core_access_end(vdev);
+		if (ret < 0)
+			return ret;
+		/*
+		 * Both of these take pci_bus_sem, so run them with the access
+		 * guard dropped. The reset re-checks the recovery state for
+		 * itself. The power up does not, so check it here.
+		 *
+		 * Both are best effort, as the guest-requested FLR has always
+		 * been. The result is not reported back through the config
+		 * write. Without recovery enabled the only failure is -EAGAIN
+		 * from device lock contention, exactly as before. With it they
+		 * are dropped while a recovery or reset transaction is in
+		 * flight, which leaves the device in D0 and reset anyway.
+		 */
+		if (deferred.power_up &&
+		    !(vdev->pci_recovery_supported &&
+		      READ_ONCE(vdev->pci_recovery_access_blocked)))
+			vfio_lock_and_set_power_state(vdev, PCI_D0);
+		if (deferred.flr)
+			vfio_pci_try_reset_function(vdev, false);
 	} else {
-		if (perm->readfn) {
+		access_ret = vfio_pci_core_access_begin(vdev);
+		if (access_ret)
+			return access_ret;
+		if (perm->readfn)
 			ret = perm->readfn(vdev, *ppos, count,
 					   perm, offset, &val);
-			if (ret < 0)
-				return ret;
-		}
+		vfio_pci_core_access_end(vdev);
+		if (ret < 0)
+			return ret;
 
 		if (copy_to_user(buf, &val, count))
 			return -EFAULT;
-- 
2.43.0

Re: [RFC PATCH 05/19] vfio/pci: Serialize config access with recovery

[sashiko-bot]

> Hold recovery_lock for reading across each config space operation, so
> recovery can shut out new ones and wait for whatever is already running.
> The user copies stay outside the lock, since a copy can fault.
> 
> Take the lock in the dispatcher rather than around the individual
> hardware accessors. That means once recovery blocks access every config
> read fails with -EIO, even a read served entirely from vconfig which
> never touches the device. Userspace which wants to know what is going on
> reads the device feature instead. That one stays available during an
> event.
> 
> The PCIe and AF capability writes no longer reset the device themselves,
> and the power management write no longer moves it to D0 itself. They
> record what was asked for and the dispatcher does it after dropping
> recovery_lock. Both take pci_bus_sem, which AER already holds when it
> calls into the driver, so doing either inside the lock would be the wrong
> order. A reset method reaches it directly, and a D0 transition reaches it
> through pci_set_full_power_state() calling
> pcie_aspm_pm_state_change(). The lower power states take neither, so
> those still run in the writefn. The writefn declaration says so.
> 
> Both stay best effort, as the guest requested FLR always was. The result
> is not reported back through the config write. With recovery enabled they
> are dropped while a recovery or reset is already in flight, since that
> leaves the device in D0 and reset anyway. The reset helper tests the
> recovery state for itself. The power up does not, so the dispatcher
> tests it before that one.
> 
> Assisted-by: Claude:claude-opus-5
> Signed-off-by: Shameer Kolothum <skolothumtho@nvidia.com>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260901093217.8539-1-skolothumtho@nvidia.com?part=5

Re: [RFC PATCH 05/19] vfio/pci: Serialize config access with recovery

[K V P, Satyanarayana]

On 01-Sep-26 3:02 PM, Shameer Kolothum wrote:
> Hold recovery_lock for reading across each config space operation, so
> recovery can shut out new ones and wait for whatever is already running.
> The user copies stay outside the lock, since a copy can fault.
>
> Take the lock in the dispatcher rather than around the individual
> hardware accessors. That means once recovery blocks access every config
> read fails with -EIO, even a read served entirely from vconfig which
> never touches the device. Userspace which wants to know what is going on
> reads the device feature instead. That one stays available during an
> event.
>
> The PCIe and AF capability writes no longer reset the device themselves,
> and the power management write no longer moves it to D0 itself. They
> record what was asked for and the dispatcher does it after dropping
> recovery_lock. Both take pci_bus_sem, which AER already holds when it
> calls into the driver, so doing either inside the lock would be the wrong
> order. A reset method reaches it directly, and a D0 transition reaches it
> through pci_set_full_power_state() calling
> pcie_aspm_pm_state_change(). The lower power states take neither, so
> those still run in the writefn. The writefn declaration says so.
>
> Both stay best effort, as the guest requested FLR always was. The result
> is not reported back through the config write. With recovery enabled they
> are dropped while a recovery or reset is already in flight, since that
> leaves the device in D0 and reset anyway. The reset helper tests the
> recovery state for itself. The power up does not, so the dispatcher
> tests it before that one.
>
> Assisted-by: Claude:claude-opus-5
> Signed-off-by: Shameer Kolothum <skolothumtho@nvidia.com>
> ---
>   drivers/vfio/pci/vfio_pci_config.c | 147 ++++++++++++++++++++---------
>   1 file changed, 102 insertions(+), 45 deletions(-)
>
> diff --git a/drivers/vfio/pci/vfio_pci_config.c b/drivers/vfio/pci/vfio_pci_config.c
> index 9914f3ac69ae..3365100acf21 100644
> --- a/drivers/vfio/pci/vfio_pci_config.c
> +++ b/drivers/vfio/pci/vfio_pci_config.c
> @@ -99,6 +99,12 @@ static const u16 pci_ext_cap_length[PCI_EXT_CAP_ID_MAX + 1] = {
>   	[PCI_EXT_CAP_ID_DVSEC]	=	0xFF,
>   };
>   
> +/* What a config write asked for which has to wait for the access guard. */
> +struct vfio_pci_config_deferred {
> +	bool	flr;		/* a function-level reset */
> +	bool	power_up;	/* a transition to D0 */
> +};
> +
>   /*
>    * Read/Write Permission Bits - one bit for each bit in capability
>    * Any field can be read if it exists, but what is read depends on
> @@ -111,8 +117,17 @@ struct perm_bits {
>   	u8	*write;		/* writeable bits */
>   	int	(*readfn)(struct vfio_pci_core_device *vdev, int pos, int count,
>   			  struct perm_bits *perm, int offset, __le32 *val);
> +	/*
> +	 * @deferred records work the write asked for which a writefn must not
> +	 * do itself. Both a reset method and a transition to D0 acquire
> +	 * pci_bus_sem, which AER already holds when it enters the driver, so
> +	 * doing either here would invert the lock order against recovery_lock.
> +	 * The dispatcher does them after dropping recovery_lock. Callers zero
> +	 * it, and a writefn only sets a field on a success return.
> +	 */
>   	int	(*writefn)(struct vfio_pci_core_device *vdev, int pos, int count,
> -			   struct perm_bits *perm, int offset, __le32 val);
> +			   struct perm_bits *perm, int offset, __le32 val,
> +			   struct vfio_pci_config_deferred *deferred);
>   };
>   
>   #define	NO_VIRT		0
> @@ -200,7 +215,8 @@ static int vfio_default_config_read(struct vfio_pci_core_device *vdev, int pos,
>   
>   static int vfio_default_config_write(struct vfio_pci_core_device *vdev, int pos,
>   				     int count, struct perm_bits *perm,
> -				     int offset, __le32 val)
> +				     int offset, __le32 val,
> +				     struct vfio_pci_config_deferred *deferred)
>   {
>   	__le32 virt = 0, write = 0;
>   
> @@ -272,7 +288,8 @@ static int vfio_direct_config_read(struct vfio_pci_core_device *vdev, int pos,
>   /* Raw access skips any kind of virtualization */
>   static int vfio_raw_config_write(struct vfio_pci_core_device *vdev, int pos,
>   				 int count, struct perm_bits *perm,
> -				 int offset, __le32 val)
> +				 int offset, __le32 val,
> +				 struct vfio_pci_config_deferred *deferred)
>   {
>   	int ret;
>   
> @@ -299,7 +316,8 @@ static int vfio_raw_config_read(struct vfio_pci_core_device *vdev, int pos,
>   /* Virt access uses only virtualization */
>   static int vfio_virt_config_write(struct vfio_pci_core_device *vdev, int pos,
>   				  int count, struct perm_bits *perm,
> -				  int offset, __le32 val)
> +				  int offset, __le32 val,
> +				  struct vfio_pci_config_deferred *deferred)
>   {
>   	memcpy(vdev->vconfig + pos, &val, count);
>   	return count;
> @@ -563,7 +581,8 @@ static bool vfio_need_bar_restore(struct vfio_pci_core_device *vdev)
>   
>   static int vfio_basic_config_write(struct vfio_pci_core_device *vdev, int pos,
>   				   int count, struct perm_bits *perm,
> -				   int offset, __le32 val)
> +				   int offset, __le32 val,
> +				   struct vfio_pci_config_deferred *deferred)
>   {
>   	struct pci_dev *pdev = vdev->pdev;
>   	__le16 *virt_cmd;
> @@ -613,7 +632,8 @@ static int vfio_basic_config_write(struct vfio_pci_core_device *vdev, int pos,
>   			vfio_bar_restore(vdev);
>   	}
>   
> -	count = vfio_default_config_write(vdev, pos, count, perm, offset, val);
> +	count = vfio_default_config_write(vdev, pos, count, perm, offset, val,
> +					  deferred);
>   	if (count < 0) {
>   		if (offset == PCI_COMMAND)
>   			up_write(&vdev->memory_lock);
> @@ -727,9 +747,11 @@ static void vfio_lock_and_set_power_state(struct vfio_pci_core_device *vdev,
>   
>   static int vfio_pm_config_write(struct vfio_pci_core_device *vdev, int pos,
>   				int count, struct perm_bits *perm,
> -				int offset, __le32 val)
> +				int offset, __le32 val,
> +				struct vfio_pci_config_deferred *deferred)
>   {
> -	count = vfio_default_config_write(vdev, pos, count, perm, offset, val);
> +	count = vfio_default_config_write(vdev, pos, count, perm, offset, val,
> +					  deferred);
>   	if (count < 0)
>   		return count;
>   
> @@ -738,8 +760,15 @@ static int vfio_pm_config_write(struct vfio_pci_core_device *vdev, int pos,
>   
>   		switch (le32_to_cpu(val) & PCI_PM_CTRL_STATE_MASK) {
>   		case 0:
> -			state = PCI_D0;
> -			break;
> +			/*
> +			 * Going to D0 reaches pci_set_full_power_state(),
> +			 * which takes pci_bus_sem through
> +			 * pcie_aspm_pm_state_change(). Leave it to the
> +			 * dispatcher. The lower states do not, so they run
> +			 * here.
> +			 */
> +			deferred->power_up = true;
> +			return count;
>   		case 1:
>   			state = PCI_D1;
>   			break;
> @@ -799,7 +828,8 @@ static int __init init_pci_cap_pm_perm(struct perm_bits *perm)
>   
>   static int vfio_vpd_config_write(struct vfio_pci_core_device *vdev, int pos,
>   				 int count, struct perm_bits *perm,
> -				 int offset, __le32 val)
> +				 int offset, __le32 val,
> +				 struct vfio_pci_config_deferred *deferred)
>   {
>   	struct pci_dev *pdev = vdev->pdev;
>   	__le16 *paddr = (__le16 *)(vdev->vconfig + pos - offset + PCI_VPD_ADDR);
> @@ -812,7 +842,8 @@ static int vfio_vpd_config_write(struct vfio_pci_core_device *vdev, int pos,
>   	 * of PCI_VPD_ADDR, then the PCI_VPD_ADDR_F bit is written and we
>   	 * have work to do.
>   	 */
> -	count = vfio_default_config_write(vdev, pos, count, perm, offset, val);
> +	count = vfio_default_config_write(vdev, pos, count, perm, offset, val,
> +					  deferred);
>   	if (count < 0 || offset > PCI_VPD_ADDR + 1 ||
>   	    offset + count <= PCI_VPD_ADDR + 1)
>   		return count;
> @@ -881,21 +912,24 @@ static int __init init_pci_cap_pcix_perm(struct perm_bits *perm)
>   
>   static int vfio_exp_config_write(struct vfio_pci_core_device *vdev, int pos,
>   				 int count, struct perm_bits *perm,
> -				 int offset, __le32 val)
> +				 int offset, __le32 val,
> +				 struct vfio_pci_config_deferred *deferred)
>   {
>   	__le16 *ctrl = (__le16 *)(vdev->vconfig + pos -
>   				  offset + PCI_EXP_DEVCTL);
>   	int readrq = le16_to_cpu(*ctrl) & PCI_EXP_DEVCTL_READRQ;
>   
> -	count = vfio_default_config_write(vdev, pos, count, perm, offset, val);
> +	count = vfio_default_config_write(vdev, pos, count, perm, offset, val,
> +					  deferred);
>   	if (count < 0)
>   		return count;
>   
>   	/*
>   	 * The FLR bit is virtualized, if set and the device supports PCIe
> -	 * FLR, issue a reset_function.  Regardless, clear the bit, the spec
> -	 * requires it to be always read as zero.  NB, reset_function might
> -	 * not use a PCIe FLR, we don't have that level of granularity.
> +	 * FLR, request a function reset once recovery_lock has been
> +	 * released. Regardless, clear the bit, the spec requires it to be
> +	 * always read as zero. NB, reset_function might not use a PCIe FLR,
> +	 * we don't have that level of granularity.
>   	 */
>   	if (*ctrl & cpu_to_le16(PCI_EXP_DEVCTL_BCR_FLR)) {
>   		u32 cap;
> @@ -907,14 +941,8 @@ static int vfio_exp_config_write(struct vfio_pci_core_device *vdev, int pos,
>   						 pos - offset + PCI_EXP_DEVCAP,
>   						 &cap);
>   
> -		if (!ret && (cap & PCI_EXP_DEVCAP_FLR)) {
> -			vfio_pci_zap_and_down_write_memory_lock(vdev);
> -			vfio_pci_dma_buf_move(vdev, true);
> -			pci_try_reset_function(vdev->pdev);
> -			if (__vfio_pci_memory_enabled(vdev))
> -				vfio_pci_dma_buf_move(vdev, false);
> -			up_write(&vdev->memory_lock);
> -		}
> +		if (!ret && (cap & PCI_EXP_DEVCAP_FLR))
> +			deferred->flr = true;
>   	}
>   
>   	/*
> @@ -968,19 +996,22 @@ static int __init init_pci_cap_exp_perm(struct perm_bits *perm)
>   
>   static int vfio_af_config_write(struct vfio_pci_core_device *vdev, int pos,
>   				int count, struct perm_bits *perm,
> -				int offset, __le32 val)
> +				int offset, __le32 val,
> +				struct vfio_pci_config_deferred *deferred)
>   {
>   	u8 *ctrl = vdev->vconfig + pos - offset + PCI_AF_CTRL;
>   
> -	count = vfio_default_config_write(vdev, pos, count, perm, offset, val);
> +	count = vfio_default_config_write(vdev, pos, count, perm, offset, val,
> +					  deferred);
>   	if (count < 0)
>   		return count;
>   
>   	/*
>   	 * The FLR bit is virtualized, if set and the device supports AF
> -	 * FLR, issue a reset_function.  Regardless, clear the bit, the spec
> -	 * requires it to be always read as zero.  NB, reset_function might
> -	 * not use an AF FLR, we don't have that level of granularity.
> +	 * FLR, request a function reset once recovery_lock has been
> +	 * released. Regardless, clear the bit, the spec requires it to be
> +	 * always read as zero. NB, reset_function might not use an AF FLR,
> +	 * we don't have that level of granularity.
>   	 */
>   	if (*ctrl & PCI_AF_CTRL_FLR) {
>   		u8 cap;
> @@ -992,14 +1023,8 @@ static int vfio_af_config_write(struct vfio_pci_core_device *vdev, int pos,
>   						pos - offset + PCI_AF_CAP,
>   						&cap);
>   
> -		if (!ret && (cap & PCI_AF_CAP_FLR) && (cap & PCI_AF_CAP_TP)) {
> -			vfio_pci_zap_and_down_write_memory_lock(vdev);
> -			vfio_pci_dma_buf_move(vdev, true);
> -			pci_try_reset_function(vdev->pdev);
> -			if (__vfio_pci_memory_enabled(vdev))
> -				vfio_pci_dma_buf_move(vdev, false);
> -			up_write(&vdev->memory_lock);
> -		}
> +		if (!ret && (cap & PCI_AF_CAP_FLR) && (cap & PCI_AF_CAP_TP))
> +			deferred->flr = true;
>   	}
>   
>   	return count;
> @@ -1168,9 +1193,11 @@ static int vfio_msi_config_read(struct vfio_pci_core_device *vdev, int pos,
>   
>   static int vfio_msi_config_write(struct vfio_pci_core_device *vdev, int pos,
>   				 int count, struct perm_bits *perm,
> -				 int offset, __le32 val)
> +				 int offset, __le32 val,
> +				 struct vfio_pci_config_deferred *deferred)
>   {
> -	count = vfio_default_config_write(vdev, pos, count, perm, offset, val);
> +	count = vfio_default_config_write(vdev, pos, count, perm, offset, val,
> +					  deferred);
>   	if (count < 0)
>   		return count;
>   
> @@ -1889,6 +1916,8 @@ ssize_t vfio_pci_config_rw_single(struct vfio_pci_core_device *vdev,
>   	struct perm_bits *perm;
>   	__le32 val = 0;
>   	int cap_start = 0, offset;
> +	int access_ret;
> +	struct vfio_pci_config_deferred deferred = {};
>   	u8 cap_id;
>   	ssize_t ret;
>   
> @@ -1957,14 +1986,42 @@ ssize_t vfio_pci_config_rw_single(struct vfio_pci_core_device *vdev,
>   		if (copy_from_user(&val, buf, count))
>   			return -EFAULT;
>   
> -		ret = perm->writefn(vdev, *ppos, count, perm, offset, val);
> +		access_ret = vfio_pci_core_access_begin(vdev);
> +		if (access_ret)
> +			return access_ret;
> +		ret = perm->writefn(vdev, *ppos, count, perm, offset, val,
> +				    &deferred);
> +		vfio_pci_core_access_end(vdev);
> +		if (ret < 0)
> +			return ret;
> +		/*
> +		 * Both of these take pci_bus_sem, so run them with the access
> +		 * guard dropped. The reset re-checks the recovery state for
> +		 * itself. The power up does not, so check it here.
> +		 *
> +		 * Both are best effort, as the guest-requested FLR has always
> +		 * been. The result is not reported back through the config
> +		 * write. Without recovery enabled the only failure is -EAGAIN
> +		 * from device lock contention, exactly as before. With it they
> +		 * are dropped while a recovery or reset transaction is in
> +		 * flight, which leaves the device in D0 and reset anyway.
> +		 */
> +		if (deferred.power_up &&
> +		    !(vdev->pci_recovery_supported &&
> +		      READ_ONCE(vdev->pci_recovery_access_blocked)))
> +			vfio_lock_and_set_power_state(vdev, PCI_D0);
> +		if (deferred.flr)
> +			vfio_pci_try_reset_function(vdev, false);
>   	} else {
> -		if (perm->readfn) {
> +		access_ret = vfio_pci_core_access_begin(vdev);
> +		if (access_ret)
> +			return access_ret;
> +		if (perm->readfn)
>   			ret = perm->readfn(vdev, *ppos, count,
>   					   perm, offset, &val);
> -			if (ret < 0)
> -				return ret;
> -		}
> +		vfio_pci_core_access_end(vdev);
> +		if (ret < 0)
> +			return ret;

The else {} is all about perm->readfn. Can we move 
vfio_pci_core_access_begin() and end() inside the if (perm->readfn) ?

We do not need to bring if (ret < 0) out of if(perm->readfn) in that case.

- Satya.

>   
>   		if (copy_to_user(buf, &val, count))
>   			return -EFAULT;

RE: [RFC PATCH 05/19] vfio/pci: Serialize config access with recovery

[Shameer Kolothum Thodi]

> -----Original Message-----
> From: K V P, Satyanarayana <satyanarayana.k.v.p@intel.com>
> Sent: 02 September 2026 07:28
> To: Shameer Kolothum Thodi <skolothumtho@nvidia.com>;
> kvm@vger.kernel.org; linux-pci@vger.kernel.org; linux-kernel@vger.kernel.org
> Cc: alex@shazbot.org; jgg@ziepe.ca; kevin.tian@intel.com;
> kbusch@meta.com; michal.winiarski@intel.com; Sonang Patel
> <sonangp@nvidia.com>; Nathan Chen <nathanc@nvidia.com>; Matt Ochs
> <mochs@nvidia.com>
> Subject: Re: [RFC PATCH 05/19] vfio/pci: Serialize config access with recovery
> 

[...]
> >       } else {
> > -             if (perm->readfn) {
> > +             access_ret = vfio_pci_core_access_begin(vdev);
> > +             if (access_ret)
> > +                     return access_ret;
> > +             if (perm->readfn)
> >                       ret = perm->readfn(vdev, *ppos, count,
> >                                          perm, offset, &val);
> > -                     if (ret < 0)
> > -                             return ret;
> > -             }
> > +             vfio_pci_core_access_end(vdev);
> > +             if (ret < 0)
> > +                     return ret;
> 
> The else {} is all about perm->readfn. Can we move
> vfio_pci_core_access_begin() and end() inside the if (perm->readfn) ?
> 
> We do not need to bring if (ret < 0) out of if(perm->readfn) in that case.

Right. That’s better, will change it.

Thanks,
Shameer

[RFC PATCH 06/19] vfio/pci: Serialize ioeventfd writes with recovery

[Shameer Kolothum]

Share one write helper between the ioeventfd fast path and the threaded
one. It takes memory_lock, checks the recovery state, then writes. The
fast path runs from the virqfd wakeup with a spinlock held, so it
trylocks and hands off to the thread if the lock is busy. The thread can
block.

The ioeventfd write path must not take recovery_lock at all. It is
reached through flush_work() from the virqfd cleanup workqueue, and
VFIO_DEVICE_SET_IRQS later calls vfio_virqfd_disable(), which does that
flush while holding recovery_lock for reading. If the write then blocked
on recovery_lock behind a queued AER writer, all three would be stuck.
The flush waits for the write, the write waits for the AER writer, and
the AER writer waits for the reader driving the flush.

So the recovery state is read lock-free, and the write goes through the
raw vfio_iowrite*() accessors rather than vfio_pci_core_iowrite*(). A
later patch makes those take recovery_lock, which is what this path has
to stay clear of.

memory_lock is still what drains a write already under way, and the
order is what makes it safe. The lock is taken before the flag is read,
so a write which saw the flag clear is already holding the read side,
and recovery waits for it when it takes memory_lock for writing. A write
to an I/O port BAR takes no memory_lock and is not drained. Those are
best effort. So is a write which arrives just after a reset has finished,
since VFIO_DEVICE_RESET releases memory_lock before it retakes
recovery_lock to unblock access.

The check is skipped for drivers which do not advertise support.
pci_recovery_device_open is only ever set when the recovery machinery is
live, so testing it unconditionally would drop every ioeventfd write for
every other vfio-pci-core driver.

Assisted-by: Claude:claude-opus-5
Signed-off-by: Shameer Kolothum <skolothumtho@nvidia.com>
---
 drivers/vfio/pci/vfio_pci_rdwr.c | 91 +++++++++++++++++++++-----------
 1 file changed, 60 insertions(+), 31 deletions(-)

diff --git a/drivers/vfio/pci/vfio_pci_rdwr.c b/drivers/vfio/pci/vfio_pci_rdwr.c
index 7f14dd46de17..20362e2f0166 100644
--- a/drivers/vfio/pci/vfio_pci_rdwr.c
+++ b/drivers/vfio/pci/vfio_pci_rdwr.c
@@ -349,56 +349,85 @@ ssize_t vfio_pci_vga_rw(struct vfio_pci_core_device *vdev, char __user *buf,
 }
 #endif
 
-static void vfio_pci_ioeventfd_do_write(struct vfio_pci_ioeventfd *ioeventfd,
-					bool test_mem)
+static int vfio_pci_ioeventfd_do_write(struct vfio_pci_ioeventfd *ioeventfd,
+				       bool trylock)
 {
+	struct vfio_pci_core_device *vdev = ioeventfd->vdev;
+
+	if (ioeventfd->test_mem) {
+		if (trylock) {
+			if (!down_read_trylock(&vdev->memory_lock))
+				return 1; /* Lock contended, use thread */
+		} else {
+			down_read(&vdev->memory_lock);
+		}
+	}
+
+	/*
+	 * Read the recovery state lock-free rather than under recovery_lock.
+	 * This path runs from the virqfd cleanup workqueue, which is flushed
+	 * from paths that take recovery_lock for reading, so blocking on it
+	 * here would deadlock behind a queued writer.
+	 *
+	 * For a memory BAR, a blocked device still waits for a write already
+	 * under way, through memory_lock. The lock is taken above before the
+	 * flag is read, so a write which saw the flag clear is already
+	 * holding the read side, and the blocker waits for it when it takes
+	 * memory_lock for writing. An I/O port BAR takes no memory_lock, so
+	 * a write which saw the flag clear can still land afterwards. Port
+	 * writes are best effort here.
+	 *
+	 * A write can also be dropped for a short while after a reset has
+	 * finished, since VFIO_DEVICE_RESET releases memory_lock before it
+	 * retakes recovery_lock to unblock access. Closing that would mean
+	 * taking recovery_lock inside memory_lock, which is the wrong way
+	 * round.
+	 *
+	 * pci_recovery_device_open records that the recovery machinery is
+	 * live, so it is only ever set for drivers which advertise support.
+	 * Testing it unconditionally would drop every write for every other
+	 * driver.
+	 *
+	 * The raw vfio_iowrite*() accessors below are used for the same
+	 * reason. This path must not take recovery_lock.
+	 */
+	if (vdev->pci_recovery_supported &&
+	    (!READ_ONCE(vdev->pci_recovery_device_open) ||
+	     READ_ONCE(vdev->pci_recovery_access_blocked)))
+		goto out_memory;
+
+	if (ioeventfd->test_mem && !__vfio_pci_memory_enabled(vdev))
+		goto out_memory;
+
 	switch (ioeventfd->count) {
 	case 1:
-		vfio_pci_core_iowrite8(ioeventfd->vdev, test_mem,
-				       ioeventfd->data, ioeventfd->addr);
+		vfio_iowrite8(ioeventfd->data, ioeventfd->addr);
 		break;
 	case 2:
-		vfio_pci_core_iowrite16(ioeventfd->vdev, test_mem,
-					ioeventfd->data, ioeventfd->addr);
+		vfio_iowrite16(ioeventfd->data, ioeventfd->addr);
 		break;
 	case 4:
-		vfio_pci_core_iowrite32(ioeventfd->vdev, test_mem,
-					ioeventfd->data, ioeventfd->addr);
+		vfio_iowrite32(ioeventfd->data, ioeventfd->addr);
 		break;
 	case 8:
-		vfio_pci_core_iowrite64(ioeventfd->vdev, test_mem,
-					ioeventfd->data, ioeventfd->addr);
+		vfio_iowrite64(ioeventfd->data, ioeventfd->addr);
 		break;
 	}
-}
-
-static int vfio_pci_ioeventfd_handler(void *opaque, void *unused)
-{
-	struct vfio_pci_ioeventfd *ioeventfd = opaque;
-	struct vfio_pci_core_device *vdev = ioeventfd->vdev;
-
-	if (ioeventfd->test_mem) {
-		if (!down_read_trylock(&vdev->memory_lock))
-			return 1; /* Lock contended, use thread */
-		if (!__vfio_pci_memory_enabled(vdev)) {
-			up_read(&vdev->memory_lock);
-			return 0;
-		}
-	}
-
-	vfio_pci_ioeventfd_do_write(ioeventfd, false);
 
+out_memory:
 	if (ioeventfd->test_mem)
 		up_read(&vdev->memory_lock);
-
 	return 0;
 }
 
-static void vfio_pci_ioeventfd_thread(void *opaque, void *unused)
+static int vfio_pci_ioeventfd_handler(void *opaque, void *unused)
 {
-	struct vfio_pci_ioeventfd *ioeventfd = opaque;
+	return vfio_pci_ioeventfd_do_write(opaque, true);
+}
 
-	vfio_pci_ioeventfd_do_write(ioeventfd, ioeventfd->test_mem);
+static void vfio_pci_ioeventfd_thread(void *opaque, void *unused)
+{
+	vfio_pci_ioeventfd_do_write(opaque, false);
 }
 
 int vfio_pci_ioeventfd(struct vfio_pci_core_device *vdev, loff_t offset,
-- 
2.43.0

Re: [RFC PATCH 06/19] vfio/pci: Serialize ioeventfd writes with recovery

[sashiko-bot]

> Share one write helper between the ioeventfd fast path and the threaded
> one. It takes memory_lock, checks the recovery state, then writes. The
> fast path runs from the virqfd wakeup with a spinlock held, so it
> trylocks and hands off to the thread if the lock is busy. The thread can
> block.
> 
> The ioeventfd write path must not take recovery_lock at all. It is
> reached through flush_work() from the virqfd cleanup workqueue, and
> VFIO_DEVICE_SET_IRQS later calls vfio_virqfd_disable(), which does that
> flush while holding recovery_lock for reading. If the write then blocked
> on recovery_lock behind a queued AER writer, all three would be stuck.
> The flush waits for the write, the write waits for the AER writer, and
> the AER writer waits for the reader driving the flush.
> 
> So the recovery state is read lock-free, and the write goes through the
> [ ... ]
> live, so testing it unconditionally would drop every ioeventfd write for
> every other vfio-pci-core driver.
> 
> Assisted-by: Claude:claude-opus-5
> Signed-off-by: Shameer Kolothum <skolothumtho@nvidia.com>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260901093217.8539-1-skolothumtho@nvidia.com?part=6

[RFC PATCH 07/19] vfio/pci: Retry BAR faults after temporary recovery

[Shameer Kolothum]

A guest can fault on a mapped BAR while host recovery is running. Do not
insert the PFN in that case. The device is not usable until recovery is
finished.

Wait whenever access is blocked, not only while a recovery transaction is
in progress. A function reset blocks access without starting one, and
error_detected() blocks it before it publishes the flags, so a fault in
either window would otherwise fail for good. Only a closed device, or one
which has failed for good, ends the fault, which is what
VFIO_PCI_RECOVERY_FAILED records.

On the first attempt the fault lock can be dropped, so drop it, wait for
recovery, and return VM_FAULT_RETRY to bring the fault back later. The
wait is killable. Take a reference on the device registration before
dropping the lock, because the wait outlives the lock and the device
could go away. This is the FAULT_FLAG_ALLOW_RETRY set and
FAULT_FLAG_TRIED clear case.

Once the lock is dropped the VMA may be gone, so return without touching
it. The caller returns early too and skips its debug print, which reads
both the VMA and the device.

When the fault lock cannot be dropped, because the caller did not allow a
retry or this fault has already used one, wait with it held.

Look at the state once more after that wait and return SIGBUS if recovery
is still not done, rather than wait again with the lock held. That fails
a fault which might still have recovered, but the window is narrow.

The wait condition is read without recovery_lock, so it only says when to
look again. Every path which unblocks access wakes the queue, and the
decision itself is taken under the lock on the next look.

It is not a deadlock. Recovery revokes mappings with
unmap_mapping_range(), which does not take mmap_lock. The wait is
killable. SIGBUS is also what a closed device, or one which has failed
for good, returns.

The order matters. The fault takes memory_lock before it releases
recovery_lock, so from the check until the PFN is in it always holds at
least one of the two. Recovery needs both, so it cannot finish revoking
while a fault is part way through. If the fault let go of recovery_lock
before taking memory_lock, recovery could slip into that window and
revoke everything, and the fault would then map a PFN for a device which
was already revoked.

Assisted-by: Claude:claude-opus-5
Signed-off-by: Shameer Kolothum <skolothumtho@nvidia.com>
---
 drivers/vfio/pci/vfio_pci_core.c | 128 ++++++++++++++++++++++++++++++-
 1 file changed, 126 insertions(+), 2 deletions(-)

diff --git a/drivers/vfio/pci/vfio_pci_core.c b/drivers/vfio/pci/vfio_pci_core.c
index 3645daa8891f..d46448662e84 100644
--- a/drivers/vfio/pci/vfio_pci_core.c
+++ b/drivers/vfio/pci/vfio_pci_core.c
@@ -1937,6 +1937,127 @@ vm_fault_t vfio_pci_vmf_insert_pfn(struct vfio_pci_core_device *vdev,
 }
 EXPORT_SYMBOL_GPL(vfio_pci_vmf_insert_pfn);
 
+/*
+ * Whether a fault which found access blocked is worth retrying. Read
+ * without recovery_lock, so it is only a hint about when to look again.
+ * vfio_pci_fault_trylock_once() takes the lock and decides. Read the flags
+ * once so the two tests below see the same value. Every writer which can
+ * make this true wakes pci_recovery_wait.
+ */
+static bool vfio_pci_recovery_done(struct vfio_pci_core_device *vdev)
+{
+	u32 flags = READ_ONCE(vdev->pci_recovery_flags);
+
+	if (!READ_ONCE(vdev->pci_recovery_device_open))
+		return true;
+	if (flags & VFIO_PCI_RECOVERY_IN_PROGRESS)
+		return false;
+	if (flags & VFIO_PCI_RECOVERY_FAILED)
+		return true;
+	return !READ_ONCE(vdev->pci_recovery_access_blocked);
+}
+
+static int vfio_pci_wait_for_recovery(struct vfio_pci_core_device *vdev)
+{
+	return wait_event_killable(vdev->pci_recovery_wait,
+				   vfio_pci_recovery_done(vdev));
+}
+
+/* What one look at the recovery state says the fault should do. */
+enum vfio_pci_fault_action {
+	VFIO_PCI_FAULT_PROCEED,	/* returns with memory_lock held */
+	VFIO_PCI_FAULT_WAIT,	/* recovery is running, may still recover */
+	VFIO_PCI_FAULT_FAIL,	/* closed, or failed for good */
+};
+
+static enum vfio_pci_fault_action
+vfio_pci_fault_trylock_once(struct vfio_pci_core_device *vdev)
+{
+	enum vfio_pci_fault_action action;
+
+	down_read(&vdev->recovery_lock);
+	if (!vdev->pci_recovery_device_open ||
+	    (vdev->pci_recovery_flags & VFIO_PCI_RECOVERY_FAILED)) {
+		action = VFIO_PCI_FAULT_FAIL;
+	} else if (vdev->pci_recovery_access_blocked) {
+		/*
+		 * Blocked for a reason which still ends: a recovery which has
+		 * not failed, or a function reset. Test FAILED above rather
+		 * than IN_PROGRESS here, so a fault does not fail for good
+		 * while a reset is running, or in the window where
+		 * error_detected() has blocked access but not yet published
+		 * the flags.
+		 */
+		action = VFIO_PCI_FAULT_WAIT;
+	} else {
+		down_read(&vdev->memory_lock);
+		action = VFIO_PCI_FAULT_PROCEED;
+	}
+	up_read(&vdev->recovery_lock);
+
+	return action;
+}
+
+/*
+ * Return true with memory_lock held for a fault that may proceed. Otherwise
+ * return false with @ret set to the result the fault handler should return.
+ */
+static bool vfio_pci_core_fault_trylock(struct vfio_pci_core_device *vdev,
+					struct vm_fault *vmf,
+					vm_fault_t *ret)
+{
+	if (!vdev->pci_recovery_supported) {
+		down_read(&vdev->memory_lock);
+		return true;
+	}
+
+	switch (vfio_pci_fault_trylock_once(vdev)) {
+	case VFIO_PCI_FAULT_PROCEED:
+		return true;
+	case VFIO_PCI_FAULT_FAIL:
+		*ret = VM_FAULT_SIGBUS;
+		return false;
+	case VFIO_PCI_FAULT_WAIT:
+		break;
+	}
+
+	if (fault_flag_allow_retry_first(vmf->flags)) {
+		if (vmf->flags & FAULT_FLAG_RETRY_NOWAIT) {
+			*ret = VM_FAULT_RETRY;
+			return false;
+		}
+
+		if (!vfio_device_try_get_registration(&vdev->vdev)) {
+			*ret = VM_FAULT_SIGBUS;
+			return false;
+		}
+
+		release_fault_lock(vmf);
+		vfio_pci_wait_for_recovery(vdev);
+		vfio_device_put_registration(&vdev->vdev);
+		*ret = VM_FAULT_RETRY;
+		return false;
+	}
+
+	/*
+	 * The fault lock cannot be dropped here: either the caller did not
+	 * allow a retry, or this fault has already used one. So wait with
+	 * it held. It is not a deadlock. Recovery revokes mappings through
+	 * unmap_mapping_range(), which never takes mmap_lock. The wait is
+	 * killable.
+	 */
+	if (vfio_pci_wait_for_recovery(vdev)) {
+		*ret = VM_FAULT_NOPAGE;
+		return false;
+	}
+
+	if (vfio_pci_fault_trylock_once(vdev) == VFIO_PCI_FAULT_PROCEED)
+		return true;
+
+	*ret = VM_FAULT_SIGBUS;
+	return false;
+}
+
 static vm_fault_t vfio_pci_mmap_huge_fault(struct vm_fault *vmf,
 					   unsigned int order)
 {
@@ -1948,8 +2069,11 @@ static vm_fault_t vfio_pci_mmap_huge_fault(struct vm_fault *vmf,
 	vm_fault_t ret = VM_FAULT_FALLBACK;
 
 	if (is_aligned_for_order(vma, addr, pfn, order)) {
-		scoped_guard(rwsem_read, &vdev->memory_lock)
-			ret = vfio_pci_vmf_insert_pfn(vdev, vmf, pfn, order);
+		if (!vfio_pci_core_fault_trylock(vdev, vmf, &ret))
+			return ret;
+
+		ret = vfio_pci_vmf_insert_pfn(vdev, vmf, pfn, order);
+		up_read(&vdev->memory_lock);
 	}
 
 	dev_dbg_ratelimited(&vdev->pdev->dev,
-- 
2.43.0

Re: [RFC PATCH 07/19] vfio/pci: Retry BAR faults after temporary recovery

[sashiko-bot]

> A guest can fault on a mapped BAR while host recovery is running. Do not
> insert the PFN in that case. The device is not usable until recovery is
> finished.
> 
> Wait whenever access is blocked, not only while a recovery transaction is
> in progress. A function reset blocks access without starting one, and
> error_detected() blocks it before it publishes the flags, so a fault in
> either window would otherwise fail for good. Only a closed device, or one
> which has failed for good, ends the fault, which is what
> VFIO_PCI_RECOVERY_FAILED records.
> 
> On the first attempt the fault lock can be dropped, so drop it, wait for
> recovery, and return VM_FAULT_RETRY to bring the fault back later. The
> wait is killable. Take a reference on the device registration before
> dropping the lock, because the wait outlives the lock and the device
> [ ... ]
> revoke everything, and the fault would then map a PFN for a device which
> was already revoked.
> 
> Assisted-by: Claude:claude-opus-5
> Signed-off-by: Shameer Kolothum <skolothumtho@nvidia.com>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260901093217.8539-1-skolothumtho@nvidia.com?part=7

[RFC PATCH 08/19] vfio/pci: Serialize BAR and ROM access with recovery

[Shameer Kolothum]

Hold recovery_lock for reading around trapped BAR reads and writes, and
around the ROM mapping, so they do not run while host recovery has access
blocked.

The lock is taken inside the width-specific I/O helpers, one access at a
time, rather than across the whole transfer. copy_to_user() and
copy_from_user() run in the callers of those helpers and so stay outside
it. A user buffer can fault, and with userfaultfd the fault is serviced
by userspace, so holding recovery_lock across the copy would let a user
stall error_detected() for as long as it likes.

VFIO_DEVICE_GET_REGION_INFO probes the ROM the same way, enabling memory
decode and mapping it to see whether the contents are valid, so guard that
too.

Mapping and unmapping the ROM both write config space: pci_map_rom()
enables decode, and assigns the resource first if it has none, and
pci_unmap_rom() disables it again. If recovery has blocked access, do the
iounmap and record the disable in pci_recovery_rom_disable instead.
recovery_lock is held across the decision and the record so recovery
cannot complete in between.

Do the recorded disable from vfio_pci_try_reset_function() once the reset
has finished, and from the resume() handler a later patch adds. Both are
points where whatever blocked access has ended. A closed device is
skipped, since close puts the device back through reset and config
restore without holding recovery_lock.

Assisted-by: Claude:claude-opus-5
Signed-off-by: Shameer Kolothum <skolothumtho@nvidia.com>
---
 drivers/vfio/pci/vfio_pci_core.c | 32 +++++++++++++++++++-
 drivers/vfio/pci/vfio_pci_rdwr.c | 51 +++++++++++++++++++++++++++++++-
 2 files changed, 81 insertions(+), 2 deletions(-)

diff --git a/drivers/vfio/pci/vfio_pci_core.c b/drivers/vfio/pci/vfio_pci_core.c
index d46448662e84..0b1b2398dc88 100644
--- a/drivers/vfio/pci/vfio_pci_core.c
+++ b/drivers/vfio/pci/vfio_pci_core.c
@@ -1244,6 +1244,9 @@ int vfio_pci_ioctl_get_region_info(struct vfio_device *core_vdev,
 			 * Check ROM content is valid. Need to enable memory
 			 * decode for ROM access in pci_map_rom().
 			 */
+			ret = vfio_pci_core_access_begin(vdev);
+			if (ret)
+				return ret;
 			cmd = vfio_pci_memory_lock_and_enable(vdev);
 			io = pci_map_rom(pdev, &size);
 			if (io) {
@@ -1254,6 +1257,7 @@ int vfio_pci_ioctl_get_region_info(struct vfio_device *core_vdev,
 				pci_unmap_rom(pdev, io);
 			}
 			vfio_pci_memory_unlock_and_restore(vdev, cmd);
+			vfio_pci_core_access_end(vdev);
 		} else if (pdev->rom && pdev->romlen) {
 			info->flags = VFIO_REGION_INFO_FLAG_READ;
 			/* Report BAR size as power of two. */
@@ -1379,6 +1383,30 @@ static int vfio_pci_ioctl_set_irqs(struct vfio_pci_core_device *vdev,
 	return ret;
 }
 
+/*
+ * Complete a ROM unmap which could not disable decode through config space.
+ * Call once whatever blocked access has finished. A closed device is skipped.
+ * It runs without recovery_lock, and close puts the device back through reset
+ * and config restore.
+ *
+ * The IORESOURCE_ROM_ENABLE test is what pci_unmap_rom() would have done.
+ * A ROM which firmware left enabled is not ours to turn off.
+ */
+static void vfio_pci_recovery_rom_disable(struct vfio_pci_core_device *vdev)
+{
+	struct pci_dev *pdev = vdev->pdev;
+
+	lockdep_assert_held_write(&vdev->recovery_lock);
+
+	if (!vdev->pci_recovery_device_open ||
+	    !READ_ONCE(vdev->pci_recovery_rom_disable))
+		return;
+
+	if (!(pdev->resource[PCI_ROM_RESOURCE].flags & IORESOURCE_ROM_ENABLE))
+		pci_disable_rom(pdev);
+	WRITE_ONCE(vdev->pci_recovery_rom_disable, false);
+}
+
 int vfio_pci_try_reset_function(struct vfio_pci_core_device *vdev,
 				bool reset_power_state)
 {
@@ -1455,8 +1483,10 @@ int vfio_pci_try_reset_function(struct vfio_pci_core_device *vdev,
 		 */
 		if (vdev->pci_recovery_device_open &&
 		    !(vdev->pci_recovery_flags & (VFIO_PCI_RECOVERY_IN_PROGRESS |
-						  VFIO_PCI_RECOVERY_FAILED)))
+						  VFIO_PCI_RECOVERY_FAILED))) {
+			vfio_pci_recovery_rom_disable(vdev);
 			WRITE_ONCE(vdev->pci_recovery_access_blocked, false);
+		}
 		up_write(&vdev->recovery_lock);
 		/*
 		 * Access is blocked for the length of the reset, so anything
diff --git a/drivers/vfio/pci/vfio_pci_rdwr.c b/drivers/vfio/pci/vfio_pci_rdwr.c
index 20362e2f0166..86fadc999962 100644
--- a/drivers/vfio/pci/vfio_pci_rdwr.c
+++ b/drivers/vfio/pci/vfio_pci_rdwr.c
@@ -42,10 +42,17 @@
 int vfio_pci_core_iowrite##size(struct vfio_pci_core_device *vdev,	\
 			bool test_mem, u##size val, void __iomem *io)	\
 {									\
+	int ret;							\
+									\
+	ret = vfio_pci_core_access_begin(vdev);				\
+	if (ret)							\
+		return ret;						\
+									\
 	if (test_mem) {							\
 		down_read(&vdev->memory_lock);				\
 		if (!__vfio_pci_memory_enabled(vdev)) {			\
 			up_read(&vdev->memory_lock);			\
+			vfio_pci_core_access_end(vdev);			\
 			return -EIO;					\
 		}							\
 	}								\
@@ -54,6 +61,7 @@ int vfio_pci_core_iowrite##size(struct vfio_pci_core_device *vdev,	\
 									\
 	if (test_mem)							\
 		up_read(&vdev->memory_lock);				\
+	vfio_pci_core_access_end(vdev);					\
 									\
 	return 0;							\
 }									\
@@ -68,10 +76,17 @@ VFIO_IOWRITE(64)
 int vfio_pci_core_ioread##size(struct vfio_pci_core_device *vdev,	\
 			bool test_mem, u##size *val, void __iomem *io)	\
 {									\
+	int ret;							\
+									\
+	ret = vfio_pci_core_access_begin(vdev);				\
+	if (ret)							\
+		return ret;						\
+									\
 	if (test_mem) {							\
 		down_read(&vdev->memory_lock);				\
 		if (!__vfio_pci_memory_enabled(vdev)) {			\
 			up_read(&vdev->memory_lock);			\
+			vfio_pci_core_access_end(vdev);			\
 			return -EIO;					\
 		}							\
 	}								\
@@ -80,6 +95,7 @@ int vfio_pci_core_ioread##size(struct vfio_pci_core_device *vdev,	\
 									\
 	if (test_mem)							\
 		up_read(&vdev->memory_lock);				\
+	vfio_pci_core_access_end(vdev);					\
 									\
 	return 0;							\
 }									\
@@ -198,12 +214,41 @@ ssize_t vfio_pci_core_do_io_rw(struct vfio_pci_core_device *vdev, bool test_mem,
 }
 EXPORT_SYMBOL_GPL(vfio_pci_core_do_io_rw);
 
+/*
+ * Undo pci_map_rom(). The iounmap is always safe, but pci_disable_rom() is a
+ * config space write. If recovery has blocked access, do the iounmap now and
+ * record the disable, for whichever of resume() or the reset tail unblocks
+ * access again. recovery_lock spans the decision and the record so recovery
+ * cannot complete in between.
+ */
+static void vfio_pci_unmap_rom(struct vfio_pci_core_device *vdev,
+			       void __iomem *io)
+{
+	struct pci_dev *pdev = vdev->pdev;
+
+	if (!vdev->pci_recovery_supported) {
+		pci_unmap_rom(pdev, io);
+		return;
+	}
+
+	down_read(&vdev->recovery_lock);
+	if (vdev->pci_recovery_device_open &&
+	    !vdev->pci_recovery_access_blocked) {
+		pci_unmap_rom(pdev, io);
+	} else {
+		iounmap(io);
+		WRITE_ONCE(vdev->pci_recovery_rom_disable, true);
+	}
+	up_read(&vdev->recovery_lock);
+}
+
 ssize_t vfio_pci_bar_rw(struct vfio_pci_core_device *vdev, char __user *buf,
 			size_t count, loff_t *ppos, bool iswrite)
 {
 	struct pci_dev *pdev = vdev->pdev;
 	loff_t pos = *ppos & VFIO_PCI_OFFSET_MASK;
 	int bar = VFIO_PCI_OFFSET_TO_INDEX(*ppos);
+	int ret;
 	size_t x_start = 0, x_end = 0;
 	resource_size_t end;
 	void __iomem *io;
@@ -230,7 +275,11 @@ ssize_t vfio_pci_bar_rw(struct vfio_pci_core_device *vdev, char __user *buf,
 		 * filling large ROM BARs much faster.
 		 */
 		if (pci_resource_start(pdev, bar)) {
+			ret = vfio_pci_core_access_begin(vdev);
+			if (ret)
+				return ret;
 			io = pci_map_rom(pdev, &x_start);
+			vfio_pci_core_access_end(vdev);
 		} else {
 			io = ioremap(pdev->rom, pdev->romlen);
 			x_start = pdev->romlen;
@@ -269,7 +318,7 @@ ssize_t vfio_pci_bar_rw(struct vfio_pci_core_device *vdev, char __user *buf,
 
 	if (bar == PCI_ROM_RESOURCE) {
 		if (pci_resource_start(pdev, bar))
-			pci_unmap_rom(pdev, io);
+			vfio_pci_unmap_rom(vdev, io);
 		else
 			iounmap(io);
 	}
-- 
2.43.0

Re: [RFC PATCH 08/19] vfio/pci: Serialize BAR and ROM access with recovery

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Concurrent accesses to PCI_ROM_RESOURCE map and unmap the ROM, which concurrently read/modify/write the ROM_ENABLE bit in PCI config space without serialization. One thread's unmap can disable the ROM while another thread is still performing IO, leading to Master Aborts or AER hardware errors.
--

commit 56bec71818cf734e5593b7903748915f73efa3b3
Author: Shameer Kolothum <skolothumtho@nvidia.com>

vfio/pci: Serialize BAR and ROM access with recovery

This commit holds the recovery_lock for reading around trapped BAR reads
and writes, as well as around ROM mapping. It defers unmapping configuration
changes during host recovery to prevent accesses while recovery blocks them.

> diff --git a/drivers/vfio/pci/vfio_pci_rdwr.c b/drivers/vfio/pci/vfio_pci_rdwr.c
> --- a/drivers/vfio/pci/vfio_pci_rdwr.c
> +++ b/drivers/vfio/pci/vfio_pci_rdwr.c
[ ... ]
> @@ -230,7 +275,11 @@ ssize_t vfio_pci_bar_rw(struct vfio_pci_core_device *vdev, char __user *buf,
>  		 * filling large ROM BARs much faster.
>  		 */
>  		if (pci_resource_start(pdev, bar)) {
> +			ret = vfio_pci_core_access_begin(vdev);
> +			if (ret)
> +				return ret;
>  			io = pci_map_rom(pdev, &x_start);
> +			vfio_pci_core_access_end(vdev);
>  		} else {
>  			io = ioremap(pdev->rom, pdev->romlen);
>  			x_start = pdev->romlen;
[ ... ]
> @@ -269,7 +318,7 @@ ssize_t vfio_pci_bar_rw(struct vfio_pci_core_device *vdev, char __user *buf,
>  
>  	if (bar == PCI_ROM_RESOURCE) {
>  		if (pci_resource_start(pdev, bar))
> -			pci_unmap_rom(pdev, io);
> +			vfio_pci_unmap_rom(vdev, io);

[Severity: High]
This is a pre-existing issue, but does the lack of serialization around ROM
mapping and unmapping expose the host to hardware errors?

When two threads in userspace concurrently call pread on the VFIO device file
descriptor targeting the PCI_ROM_RESOURCE BAR, both threads will enter
vfio_pci_bar_rw and call pci_map_rom. This enables the ROM address decoder in
the PCI config space.

When the first thread finishes reading, it calls vfio_pci_unmap_rom, which
disables ROM decode by clearing the ROM_ENABLE bit in config space.

If the second thread is concurrently executing vfio_pci_core_do_io_rw and
performing IO reads on the memory-mapped ROM, the disabled ROM address decoder
will cause the PCI read transaction to result in a Master Abort or an
Unsupported Request.

Depending on the host AER configuration and hardware, this could trigger
an Advanced Error Reporting interrupt, leading to a system freeze or panic.

Given that unprivileged userspace like a QEMU process can reach this path,
is there a need to serialize these concurrent accesses to prevent a Denial of
Service?

>  		else
>  			iounmap(io);
>  	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260901093217.8539-1-skolothumtho@nvidia.com?part=8

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

[Shameer Kolothum]

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@nvidia.com>
---
 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);
 
 	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);
 
 	ret = vfio_pci_set_irqs_ioctl(vdev, hdr.flags, hdr.index, hdr.start,
 				      hdr.count, data);
 
 	mutex_unlock(&vdev->igate);
+	if (device_irq)
+		vfio_pci_core_access_end(vdev);
+out_free:
 	kfree(data);
 
 	return ret;
-- 
2.43.0

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

[sashiko-bot]

> 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@nvidia.com>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260901093217.8539-1-skolothumtho@nvidia.com?part=9

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

[K V P, Satyanarayana]

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@nvidia.com>
> ---
>   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);

}

>   	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.

- Satya.

>   	ret = vfio_pci_set_irqs_ioctl(vdev, hdr.flags, hdr.index, hdr.start,
>   				      hdr.count, data);
>   
>   	mutex_unlock(&vdev->igate);
> +	if (device_irq)
> +		vfio_pci_core_access_end(vdev);
> +out_free:
>   	kfree(data);
>   
>   	return ret;

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@intel.com>
> Sent: 03 September 2026 07:34
> To: Shameer Kolothum Thodi <skolothumtho@nvidia.com>;
> kvm@vger.kernel.org; linux-pci@vger.kernel.org; linux-kernel@vger.kernel.org
> Cc: alex@shazbot.org; jgg@ziepe.ca; kevin.tian@intel.com;
> kbusch@meta.com; michal.winiarski@intel.com; Sonang Patel
> <sonangp@nvidia.com>; Nathan Chen <nathanc@nvidia.com>; Matt Ochs
> <mochs@nvidia.com>
> 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@nvidia.com>
> > ---
> >   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

[RFC PATCH 10/19] vfio/pci: Serialize hot reset with recovery

[Shameer Kolothum]

Refuse a user hot reset while a recovery transaction blocks access to any
device in the set. Between the AER callbacks memory_lock is not held, so
access_blocked is the only thing marking the device as unavailable.

A device which has failed for good is let through. Nothing is running on
its behalf, and a hot reset covers the whole set, so refusing there would
stop a healthy sibling from being reset because an unrelated device
errored. The failed device stays blocked either way, since the reset does
not clear access_blocked once FAILED is set.

Do not hold recovery_lock while resetting. pci_reset_bus() reaches
pci_bridge_wait_for_secondary_bus(), which takes pci_bus_sem, and AER
already holds pci_bus_sem when it enters the driver and takes
recovery_lock. Holding recovery_lock across the reset would invert that
order for every device in the set.

Assisted-by: Claude:claude-opus-5
Signed-off-by: Shameer Kolothum <skolothumtho@nvidia.com>
---
 drivers/vfio/pci/vfio_pci_core.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/drivers/vfio/pci/vfio_pci_core.c b/drivers/vfio/pci/vfio_pci_core.c
index 876ff51d6987..bd3d79d28f27 100644
--- a/drivers/vfio/pci/vfio_pci_core.c
+++ b/drivers/vfio/pci/vfio_pci_core.c
@@ -2937,6 +2937,27 @@ static int vfio_pci_dev_set_hot_reset(struct vfio_device_set *dev_set,
 			break;
 		}
 
+		/*
+		 * Between the AER callbacks memory_lock is not held, so refuse
+		 * the reset on access_blocked as well. Read it rather than
+		 * take recovery_lock, which would have to be released before
+		 * pci_reset_bus() anyway since that reaches pci_bus_sem.
+		 *
+		 * Let a device which has failed for good through. Nothing is
+		 * running on its behalf, and this reset covers the whole set,
+		 * so refusing there would stop a healthy sibling from being
+		 * reset because an unrelated device errored. The failed one
+		 * stays blocked, since the reset does not unblock a device
+		 * with FAILED set.
+		 */
+		if (vdev->pci_recovery_supported &&
+		    READ_ONCE(vdev->pci_recovery_access_blocked) &&
+		    !(READ_ONCE(vdev->pci_recovery_flags) &
+		      VFIO_PCI_RECOVERY_FAILED)) {
+			ret = -EBUSY;
+			break;
+		}
+
 		/*
 		 * Take the memory write lock for each device and zap BAR
 		 * mappings to prevent the user accessing the device while in
-- 
2.43.0

Re: [RFC PATCH 10/19] vfio/pci: Serialize hot reset with recovery

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] AB-BA R-W-R deadlock between memory_lock and pci_bus_sem during hot reset and AER recovery.
- [High] A race condition allows a user-initiated hot reset to bypass the `pci_recovery_access_blocked` check, leading to a bus reset during an ongoing AER recovery.
--

commit 4d5ab3d1277c9d84f74fe886437d56a38b34b2c4
Author: Shameer Kolothum <skolothumtho@nvidia.com>

vfio/pci: Serialize hot reset with recovery

This patch attempts to serialize user-initiated hot resets with hardware
error recovery by refusing hot resets when recovery blocks access to
the device. It introduces a lockless check of the access_blocked flag
instead of taking recovery_lock to avoid lock inversions.

> diff --git a/drivers/vfio/pci/vfio_pci_core.c b/drivers/vfio/pci/vfio_pci_core.c
> index 876ff51d6987a..bd3d79d28f27a 100644
> --- a/drivers/vfio/pci/vfio_pci_core.c
> +++ b/drivers/vfio/pci/vfio_pci_core.c
> @@ -2937,6 +2937,27 @@ static int vfio_pci_dev_set_hot_reset(struct vfio_device_set *dev_set,
>  			break;
>  		}
>  
> +		/*
> +		 * Between the AER callbacks memory_lock is not held, so refuse
> +		 * the reset on access_blocked as well. Read it rather than
> +		 * take recovery_lock, which would have to be released before
> +		 * pci_reset_bus() anyway since that reaches pci_bus_sem.
> +		 *
> +		 * Let a device which has failed for good through. Nothing is
> +		 * running on its behalf, and this reset covers the whole set,
> +		 * so refusing there would stop a healthy sibling from being
> +		 * reset because an unrelated device errored. The failed one
> +		 * stays blocked, since the reset does not unblock a device
> +		 * with FAILED set.
> +		 */
> +		if (vdev->pci_recovery_supported &&
> +		    READ_ONCE(vdev->pci_recovery_access_blocked) &&
> +		    !(READ_ONCE(vdev->pci_recovery_flags) &
> +		      VFIO_PCI_RECOVERY_FAILED)) {
> +			ret = -EBUSY;
> +			break;
> +		}
> +

[Severity: High]
Does this lockless check of pci_recovery_access_blocked create a race
window before memory_lock is acquired?

If the hot reset thread checks this flag and sees it as false, it can be
preempted. If the AER handler then runs and sets access_blocked to true:

vfio_pci_core_aer_err_detected()
    WRITE_ONCE(vdev->pci_recovery_access_blocked, true);
    ...
    vfio_pci_zap_and_down_write_memory_lock(vdev);
    ...
    up_write(&vdev->memory_lock);

Could the hot reset thread subsequently wake up, successfully acquire
memory_lock in the down_write_trylock() below, and call pci_reset_bus() while
the AER process is in the middle of recovery (e.g., between error_detected and
slot_reset)? 

This could bypass the intended protection and corrupt the recovery state.

>  		/*
>  		 * Take the memory write lock for each device and zap BAR
>  		 * mappings to prevent the user accessing the device while in
>  		 * reset.  Locking multiple devices is prone to deadlock,
>  		 * runaway and unwind if we hit contention.
>  		 */
>  		if (!down_write_trylock(&vdev->memory_lock)) {
>  			ret = -EBUSY;
>  			break;
>  		}

[ ... ]

>  	list_for_each_entry(vdev, &dev_set->device_list, vdev.dev_set_list)
>  		vfio_pci_set_power_state(vdev, PCI_D0);
>  
>  	ret = pci_reset_bus(pdev);

[Severity: High]
Can this sequence lead to an AB-BA R-W-R deadlock between memory_lock and
pci_bus_sem?

In this path, the user-initiated hot reset acquires memory_lock for write, and
then pci_reset_bus() acquires pci_bus_sem for read via
pci_bridge_wait_for_secondary_bus().

However, during AER recovery, pci_walk_bridge() holds pci_bus_sem for read,
and then calls into vfio_pci_core_aer_err_detected() which waits for
memory_lock for write:

vfio_pci_core_aer_err_detected()
    vfio_pci_zap_and_down_write_memory_lock(vdev)
        down_write(&vdev->memory_lock);

If a third thread (e.g., sysfs rescan) requests pci_bus_sem for write, it
blocks new readers (the hot reset thread) to prevent starvation, while waiting
on existing readers (the AER thread).

This creates an unresolvable cycle:
- Hot reset thread holds memory_lock (W), waits for pci_bus_sem (R)
- Third thread waits for pci_bus_sem (W), blocks new readers
- AER thread holds pci_bus_sem (R), waits for memory_lock (W)

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260901093217.8539-1-skolothumtho@nvidia.com?part=10

[RFC PATCH 11/19] vfio/pci: Serialize runtime PM with recovery

[Shameer Kolothum]

Hold recovery_lock for reading around low-power entry and exit. Entry
zaps the BAR mappings and revokes the DMA-BUF exports under memory_lock,
and exit restores the exports. Taking recovery_lock first keeps the same
order the AER callbacks use.

Neither wakes the device. Entry only decrements the runtime PM usage
count, and the suspend which follows runs when the vfio core drops its
own reference after the ioctl returns, outside the lock. Exit takes a
reference without resuming. So neither reaches pci_bus_sem while
recovery_lock is held.

Check the recovery state before the runtime resume in the region read and
write path, but do not hold recovery_lock across it. A resume takes
pci_bus_sem, through pcie_aspm_pm_state_change() and, from D3cold,
through pci_bridge_wait_for_secondary_bus(), and the error callbacks take
recovery_lock from under it.

The check is best effort. It avoids waking a device whose access is
already blocked, and the region access which follows takes recovery_lock
for itself. A recovery which starts after the check is not excluded, and
does not need to be. pcie_do_recovery() runtime resumes every device
under the bridge and holds the reference until it finishes, so a resume
which runs alongside it does no more than take a reference of its own.

Assisted-by: Claude:claude-opus-5
Signed-off-by: Shameer Kolothum <skolothumtho@nvidia.com>
---
 drivers/vfio/pci/vfio_pci_core.c | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/drivers/vfio/pci/vfio_pci_core.c b/drivers/vfio/pci/vfio_pci_core.c
index bd3d79d28f27..95884e713a4b 100644
--- a/drivers/vfio/pci/vfio_pci_core.c
+++ b/drivers/vfio/pci/vfio_pci_core.c
@@ -372,15 +372,21 @@ int vfio_pci_set_power_state(struct vfio_pci_core_device *vdev, pci_power_t stat
 static int vfio_pci_runtime_pm_entry(struct vfio_pci_core_device *vdev,
 				     struct eventfd_ctx *efdctx)
 {
+	int ret;
+
 	/*
 	 * The vdev power related flags are protected with 'memory_lock'
 	 * semaphore.
 	 */
+	ret = vfio_pci_core_access_begin(vdev);
+	if (ret)
+		return ret;
 	vfio_pci_zap_and_down_write_memory_lock(vdev);
 	vfio_pci_dma_buf_move(vdev, true);
 
 	if (vdev->pm_runtime_engaged) {
 		up_write(&vdev->memory_lock);
+		vfio_pci_core_access_end(vdev);
 		return -EINVAL;
 	}
 
@@ -388,6 +394,7 @@ static int vfio_pci_runtime_pm_entry(struct vfio_pci_core_device *vdev,
 	vdev->pm_wake_eventfd_ctx = efdctx;
 	pm_runtime_put_noidle(&vdev->pdev->dev);
 	up_write(&vdev->memory_lock);
+	vfio_pci_core_access_end(vdev);
 
 	return 0;
 }
@@ -483,7 +490,11 @@ static int vfio_pci_core_pm_exit(struct vfio_pci_core_device *vdev, u32 flags,
 	 * already signaled the eventfd and exited low power mode itself.
 	 * pm_runtime_engaged protects the redundant call here.
 	 */
+	ret = vfio_pci_core_access_begin(vdev);
+	if (ret)
+		return ret;
 	vfio_pci_runtime_pm_exit(vdev);
+	vfio_pci_core_access_end(vdev);
 	return 0;
 }
 
@@ -1867,6 +1878,24 @@ static ssize_t vfio_pci_rw(struct vfio_pci_core_device *vdev, char __user *buf,
 	if (index >= VFIO_PCI_NUM_REGIONS + vdev->num_regions)
 		return -EINVAL;
 
+	ret = vfio_pci_core_access_begin(vdev);
+	if (ret)
+		return ret;
+	vfio_pci_core_access_end(vdev);
+
+	/*
+	 * Resume with the guard dropped. A resume takes pci_bus_sem, through
+	 * pcie_aspm_pm_state_change() and, from D3cold, through
+	 * pci_bridge_wait_for_secondary_bus(). The error callbacks take
+	 * recovery_lock from under pci_bus_sem, so holding it here would
+	 * invert the order.
+	 *
+	 * The check above only avoids waking a device whose access is already
+	 * blocked. A recovery which starts in between is not excluded, and
+	 * does not need to be. pcie_do_recovery() has already resumed every
+	 * device under the bridge and holds the reference until it finishes.
+	 * The region access below takes the guard for itself.
+	 */
 	ret = pm_runtime_resume_and_get(&vdev->pdev->dev);
 	if (ret) {
 		pci_info_ratelimited(vdev->pdev, "runtime resume failed %d\n",
-- 
2.43.0

Re: [RFC PATCH 11/19] vfio/pci: Serialize runtime PM with recovery

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Missing `synchronize_irq()` in runtime suspend and missing `pm_runtime_get_if_active()` in shared INTx interrupt handler.
--

commit ff5cf80e830fcc9b546457ca5f098d220807ca16
Author: Shameer Kolothum <skolothumtho@nvidia.com>

vfio/pci: Serialize runtime PM with recovery

This commit serializes runtime PM entry and exit with PCI error recovery by
holding the recovery_lock for reading. It ensures that BAR mappings and
DMA-BUF exports are handled safely without waking the device unnecessarily.

> diff --git a/drivers/vfio/pci/vfio_pci_core.c b/drivers/vfio/pci/vfio_pci_core.c
> index bd3d79d28f27a..95884e713a4bd 100644
> --- a/drivers/vfio/pci/vfio_pci_core.c
> +++ b/drivers/vfio/pci/vfio_pci_core.c

[ ... ]

[Severity: High]
This is a pre-existing issue, but it persists through this patch and seems
important since it can cause an AER panic and host crash.

The runtime suspend callback masks the INTx interrupt but fails to call
synchronize_irq() to wait for any executing handlers. In addition, the
interrupt handler accesses the PCI config space without verifying the device
is active.

In drivers/vfio/pci/vfio_pci_core.c:vfio_pci_core_runtime_suspend():

    vdev->pm_intx_masked = ((vdev->irq_type == VFIO_PCI_INTX_IRQ_INDEX) &&
                            vfio_pci_intx_mask(vdev));

If the VFIO device is runtime suspended while sharing an INTx IRQ line
(IRQF_SHARED), and another device on the shared line triggers an interrupt:

In drivers/vfio/pci/vfio_pci_intrs.c:vfio_intx_handler():

    } else if (!ctx->masked &&  /* may be shared */
               pci_check_and_mask_intx(vdev->pdev)) {

The handler will execute without a pm_runtime_get_if_active() check and
read the PCI config space of the D3cold device. This causes a PCIe Unsupported
Request (UR) error which can crash the host system. The read returns 0xffffffff,
which the handler interprets as a pending interrupt, returning IRQ_HANDLED and
signaling a spurious interrupt to the userspace VM.

Could synchronize_irq() be added to the suspend path, and
pm_runtime_get_if_active() to the interrupt handler to prevent accessing a
powered-off device?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260901093217.8539-1-skolothumtho@nvidia.com?part=11

Re: [RFC PATCH 11/19] vfio/pci: Serialize runtime PM with recovery

[K V P, Satyanarayana]

On 01-Sep-26 3:02 PM, Shameer Kolothum wrote:
> Hold recovery_lock for reading around low-power entry and exit. Entry
> zaps the BAR mappings and revokes the DMA-BUF exports under memory_lock,
> and exit restores the exports. Taking recovery_lock first keeps the same
> order the AER callbacks use.
>
> Neither wakes the device. Entry only decrements the runtime PM usage
> count, and the suspend which follows runs when the vfio core drops its
> own reference after the ioctl returns, outside the lock. Exit takes a
> reference without resuming. So neither reaches pci_bus_sem while
> recovery_lock is held.
>
> Check the recovery state before the runtime resume in the region read and
> write path, but do not hold recovery_lock across it. A resume takes
> pci_bus_sem, through pcie_aspm_pm_state_change() and, from D3cold,
> through pci_bridge_wait_for_secondary_bus(), and the error callbacks take
> recovery_lock from under it.
>
> The check is best effort. It avoids waking a device whose access is
> already blocked, and the region access which follows takes recovery_lock
> for itself. A recovery which starts after the check is not excluded, and
> does not need to be. pcie_do_recovery() runtime resumes every device
> under the bridge and holds the reference until it finishes, so a resume
> which runs alongside it does no more than take a reference of its own.
>
> Assisted-by: Claude:claude-opus-5
> Signed-off-by: Shameer Kolothum <skolothumtho@nvidia.com>
> ---
>   drivers/vfio/pci/vfio_pci_core.c | 29 +++++++++++++++++++++++++++++
>   1 file changed, 29 insertions(+)
>
> diff --git a/drivers/vfio/pci/vfio_pci_core.c b/drivers/vfio/pci/vfio_pci_core.c
> index bd3d79d28f27..95884e713a4b 100644
> --- a/drivers/vfio/pci/vfio_pci_core.c
> +++ b/drivers/vfio/pci/vfio_pci_core.c
> @@ -372,15 +372,21 @@ int vfio_pci_set_power_state(struct vfio_pci_core_device *vdev, pci_power_t stat
>   static int vfio_pci_runtime_pm_entry(struct vfio_pci_core_device *vdev,
>   				     struct eventfd_ctx *efdctx)
>   {
> +	int ret;
> +
>   	/*
>   	 * The vdev power related flags are protected with 'memory_lock'
>   	 * semaphore.
>   	 */
> +	ret = vfio_pci_core_access_begin(vdev);
> +	if (ret)
> +		return ret;
>   	vfio_pci_zap_and_down_write_memory_lock(vdev);
>   	vfio_pci_dma_buf_move(vdev, true);
>   
>   	if (vdev->pm_runtime_engaged) {
>   		up_write(&vdev->memory_lock);
> +		vfio_pci_core_access_end(vdev);
>   		return -EINVAL;
>   	}
>   
> @@ -388,6 +394,7 @@ static int vfio_pci_runtime_pm_entry(struct vfio_pci_core_device *vdev,
>   	vdev->pm_wake_eventfd_ctx = efdctx;
>   	pm_runtime_put_noidle(&vdev->pdev->dev);
>   	up_write(&vdev->memory_lock);
> +	vfio_pci_core_access_end(vdev);
>   
>   	return 0;
>   }
> @@ -483,7 +490,11 @@ static int vfio_pci_core_pm_exit(struct vfio_pci_core_device *vdev, u32 flags,
>   	 * already signaled the eventfd and exited low power mode itself.
>   	 * pm_runtime_engaged protects the redundant call here.
>   	 */
> +	ret = vfio_pci_core_access_begin(vdev);
> +	if (ret)
> +		return ret;
>   	vfio_pci_runtime_pm_exit(vdev);
> +	vfio_pci_core_access_end(vdev);
>   	return 0;
>   }
>   
> @@ -1867,6 +1878,24 @@ static ssize_t vfio_pci_rw(struct vfio_pci_core_device *vdev, char __user *buf,
>   	if (index >= VFIO_PCI_NUM_REGIONS + vdev->num_regions)
>   		return -EINVAL;
>   
> +	ret = vfio_pci_core_access_begin(vdev);
> +	if (ret)
> +		return ret;
> +	vfio_pci_core_access_end(vdev);

Is it really needed? Or some typo?

- Satya.

> +
> +	/*
> +	 * Resume with the guard dropped. A resume takes pci_bus_sem, through
> +	 * pcie_aspm_pm_state_change() and, from D3cold, through
> +	 * pci_bridge_wait_for_secondary_bus(). The error callbacks take
> +	 * recovery_lock from under pci_bus_sem, so holding it here would
> +	 * invert the order.
> +	 *
> +	 * The check above only avoids waking a device whose access is already
> +	 * blocked. A recovery which starts in between is not excluded, and
> +	 * does not need to be. pcie_do_recovery() has already resumed every
> +	 * device under the bridge and holds the reference until it finishes.
> +	 * The region access below takes the guard for itself.
> +	 */
>   	ret = pm_runtime_resume_and_get(&vdev->pdev->dev);
>   	if (ret) {
>   		pci_info_ratelimited(vdev->pdev, "runtime resume failed %d\n",

RE: [RFC PATCH 11/19] vfio/pci: Serialize runtime PM with recovery

[Shameer Kolothum Thodi]

> -----Original Message-----
> From: K V P, Satyanarayana <satyanarayana.k.v.p@intel.com>
> Sent: 03 September 2026 07:43
> To: Shameer Kolothum Thodi <skolothumtho@nvidia.com>;
> kvm@vger.kernel.org; linux-pci@vger.kernel.org; linux-kernel@vger.kernel.org
> Cc: alex@shazbot.org; jgg@ziepe.ca; kevin.tian@intel.com;
> kbusch@meta.com; michal.winiarski@intel.com; Sonang Patel
> <sonangp@nvidia.com>; Nathan Chen <nathanc@nvidia.com>; Matt Ochs
> <mochs@nvidia.com>
> Subject: Re: [RFC PATCH 11/19] vfio/pci: Serialize runtime PM 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 low-power entry and exit. Entry
> > zaps the BAR mappings and revokes the DMA-BUF exports under
> > memory_lock, and exit restores the exports. Taking recovery_lock first
> > keeps the same order the AER callbacks use.
> >
> > Neither wakes the device. Entry only decrements the runtime PM usage
> > count, and the suspend which follows runs when the vfio core drops its
> > own reference after the ioctl returns, outside the lock. Exit takes a
> > reference without resuming. So neither reaches pci_bus_sem while
> > recovery_lock is held.
> >
> > Check the recovery state before the runtime resume in the region read
> > and write path, but do not hold recovery_lock across it. A resume
> > takes pci_bus_sem, through pcie_aspm_pm_state_change() and, from
> > D3cold, through pci_bridge_wait_for_secondary_bus(), and the error
> > callbacks take recovery_lock from under it.
> >
> > The check is best effort. It avoids waking a device whose access is
> > already blocked, and the region access which follows takes
> > recovery_lock for itself. A recovery which starts after the check is
> > not excluded, and does not need to be. pcie_do_recovery() runtime
> > resumes every device under the bridge and holds the reference until it
> > finishes, so a resume which runs alongside it does no more than take a
> reference of its own.
> >
> > Assisted-by: Claude:claude-opus-5
> > Signed-off-by: Shameer Kolothum <skolothumtho@nvidia.com>
> > ---
> >   drivers/vfio/pci/vfio_pci_core.c | 29 +++++++++++++++++++++++++++++
> >   1 file changed, 29 insertions(+)
> >
> > diff --git a/drivers/vfio/pci/vfio_pci_core.c
> > b/drivers/vfio/pci/vfio_pci_core.c
> > index bd3d79d28f27..95884e713a4b 100644
> > --- a/drivers/vfio/pci/vfio_pci_core.c
> > +++ b/drivers/vfio/pci/vfio_pci_core.c
> > @@ -372,15 +372,21 @@ int vfio_pci_set_power_state(struct
> vfio_pci_core_device *vdev, pci_power_t stat
> >   static int vfio_pci_runtime_pm_entry(struct vfio_pci_core_device *vdev,
> >                                    struct eventfd_ctx *efdctx)
> >   {
> > +     int ret;
> > +
> >       /*
> >        * The vdev power related flags are protected with 'memory_lock'
> >        * semaphore.
> >        */
> > +     ret = vfio_pci_core_access_begin(vdev);
> > +     if (ret)
> > +             return ret;
> >       vfio_pci_zap_and_down_write_memory_lock(vdev);
> >       vfio_pci_dma_buf_move(vdev, true);
> >
> >       if (vdev->pm_runtime_engaged) {
> >               up_write(&vdev->memory_lock);
> > +             vfio_pci_core_access_end(vdev);
> >               return -EINVAL;
> >       }
> >
> > @@ -388,6 +394,7 @@ static int vfio_pci_runtime_pm_entry(struct
> vfio_pci_core_device *vdev,
> >       vdev->pm_wake_eventfd_ctx = efdctx;
> >       pm_runtime_put_noidle(&vdev->pdev->dev);
> >       up_write(&vdev->memory_lock);
> > +     vfio_pci_core_access_end(vdev);
> >
> >       return 0;
> >   }
> > @@ -483,7 +490,11 @@ static int vfio_pci_core_pm_exit(struct
> vfio_pci_core_device *vdev, u32 flags,
> >        * already signaled the eventfd and exited low power mode itself.
> >        * pm_runtime_engaged protects the redundant call here.
> >        */
> > +     ret = vfio_pci_core_access_begin(vdev);
> > +     if (ret)
> > +             return ret;
> >       vfio_pci_runtime_pm_exit(vdev);
> > +     vfio_pci_core_access_end(vdev);
> >       return 0;
> >   }
> >
> > @@ -1867,6 +1878,24 @@ static ssize_t vfio_pci_rw(struct
> vfio_pci_core_device *vdev, char __user *buf,
> >       if (index >= VFIO_PCI_NUM_REGIONS + vdev->num_regions)
> >               return -EINVAL;
> >
> > +     ret = vfio_pci_core_access_begin(vdev);
> > +     if (ret)
> > +             return ret;
> > +     vfio_pci_core_access_end(vdev);
> 
> Is it really needed? Or some typo?

The idea was to do a best effort check and do an early return.
Looking at it again, it is not needed for correctness, since the region
access below takes the guard for itself. So it can either be removed,
or replaced by a plain read of the state:

if (vdev->pci_recovery_supported &&
          READ_ONCE(vdev->pci_recovery_access_blocked))
              return -EIO;

Thanks,
Shameer

[RFC PATCH 12/19] vfio/pci: Serialize physical device information queries with recovery

[Shameer Kolothum]

Hold recovery_lock for reading around AtomicOps capability discovery,
which reads config space to work out what the device supports. The rest
of what VFIO_DEVICE_GET_INFO and VFIO_DEVICE_GET_REGION_INFO report is
cached or software only, and remains available without taking
recovery_lock.

Assisted-by: Claude:claude-opus-5
Signed-off-by: Shameer Kolothum <skolothumtho@nvidia.com>
---
 drivers/vfio/pci/vfio_pci_core.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/vfio/pci/vfio_pci_core.c b/drivers/vfio/pci/vfio_pci_core.c
index 95884e713a4b..4447967413e7 100644
--- a/drivers/vfio/pci/vfio_pci_core.c
+++ b/drivers/vfio/pci/vfio_pci_core.c
@@ -1179,7 +1179,13 @@ static int vfio_pci_ioctl_get_info(struct vfio_pci_core_device *vdev,
 		return ret;
 	}
 
+	ret = vfio_pci_core_access_begin(vdev);
+	if (ret) {
+		kfree(caps.buf);
+		return ret;
+	}
 	ret = vfio_pci_info_atomic_cap(vdev, &caps);
+	vfio_pci_core_access_end(vdev);
 	if (ret && ret != -ENODEV) {
 		pci_warn(vdev->pdev,
 			 "Failed to setup AtomicOps info capability\n");
-- 
2.43.0

Re: [RFC PATCH 12/19] vfio/pci: Serialize physical device information queries with recovery

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [High] VFIO_DEVICE_GET_INFO fails completely during device recovery, contradicting the commit message's intent.

Pre-existing issues:
- [High] Concurrent read-modify-write race on the Physical Function's PCI_EXP_DEVCTL2 register when enabling AtomicOps.
--

commit 7eea0a295d5111634553157702a7e697ec8e62ec
Author: Shameer Kolothum <skolothumtho@nvidia.com>

vfio/pci: Serialize physical device information queries with recovery

This commit wraps the AtomicOps capability discovery in VFIO_DEVICE_GET_INFO
with recovery_lock. The lock ensures we do not attempt to read the physical
device configuration space while a PCI recovery is in progress.

> diff --git a/drivers/vfio/pci/vfio_pci_core.c b/drivers/vfio/pci/vfio_pci_core.c
> --- a/drivers/vfio/pci/vfio_pci_core.c
> +++ b/drivers/vfio/pci/vfio_pci_core.c
> @@ -1179,7 +1179,13 @@ static int vfio_pci_ioctl_get_info(struct vfio_pci_core_device *vdev,
>  		return ret;
>  	}
>  
> +	ret = vfio_pci_core_access_begin(vdev);
> +	if (ret) {
> +		kfree(caps.buf);
> +		return ret;

[Severity: High]
Does this break the availability of the cached information mentioned in the
commit message?

The commit message states:
"The rest of what VFIO_DEVICE_GET_INFO and VFIO_DEVICE_GET_REGION_INFO report
is cached or software only, and remains available without taking recovery_lock."

However, if vfio_pci_core_access_begin() fails here (for example, returning
-EIO when a recovery is in progress and access is blocked), we abort the
entire ioctl. Doesn't this deprive userspace of the other cached information
that was supposed to remain available?

Could we skip gathering the atomic capability on access failure rather than
aborting the entire ioctl?

> +	}
>  	ret = vfio_pci_info_atomic_cap(vdev, &caps);

[Severity: High]
This isn't a bug introduced by this patch, but can concurrent ioctls on
multiple Virtual Functions trigger a lockless read-modify-write race on the
shared Physical Function hardware register?

In vfio_pci_info_atomic_cap(), we call pci_enable_atomic_ops_to_root() on the
Physical Function device (retrieved via pci_physfn()). That function modifies
the PF's config space:

pci_enable_atomic_ops_to_root()
    pcie_capability_set_word(dev, PCI_EXP_DEVCTL2, PCI_EXP_DEVCTL2_ATOMIC_REQ);

This performs a lockless read-modify-write sequence on the shared PF hardware
register. Because down_read(&vdev->recovery_lock) only acquires a read lock
on the local VF's rw_semaphore, it provides no mutual exclusion across
different VFs.

If userspace concurrently issues VFIO_DEVICE_GET_INFO ioctls on multiple VFs
of the same PF, could this clobber concurrent configuration updates like ARI
or LTR toggles on the PF?

> +	vfio_pci_core_access_end(vdev);
>  	if (ret && ret != -ENODEV) {
>  		pci_warn(vdev->pdev,
>  			 "Failed to setup AtomicOps info capability\n");

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260901093217.8539-1-skolothumtho@nvidia.com?part=12

[RFC PATCH 13/19] vfio/pci: Serialize DMA-BUF export with recovery

[Shameer Kolothum]

Move the BAR iomap check under recovery_lock and hold the lock for
reading from there until the export is on vdev->dmabufs, so recovery
cannot invalidate the mapping in between. An export attempted while
access is blocked is refused by vfio_pci_core_access_begin() like any
other device access.

Assisted-by: Claude:claude-opus-5
Signed-off-by: Shameer Kolothum <skolothumtho@nvidia.com>
---
 drivers/vfio/pci/vfio_pci_dmabuf.c | 26 ++++++++++++++++++--------
 1 file changed, 18 insertions(+), 8 deletions(-)

diff --git a/drivers/vfio/pci/vfio_pci_dmabuf.c b/drivers/vfio/pci/vfio_pci_dmabuf.c
index c16f460c01d6..a54d199a72c9 100644
--- a/drivers/vfio/pci/vfio_pci_dmabuf.c
+++ b/drivers/vfio/pci/vfio_pci_dmabuf.c
@@ -243,12 +243,8 @@ int vfio_pci_core_feature_dma_buf(struct vfio_pci_core_device *vdev, u32 flags,
 	if (!get_dma_buf.nr_ranges || get_dma_buf.flags)
 		return -EINVAL;
 
-	/*
-	 * For PCI the region_index is the BAR number like everything
-	 * else.  Check that PCI resources have been claimed for it.
-	 */
-	if (get_dma_buf.region_index >= VFIO_PCI_ROM_REGION_INDEX ||
-	    IS_ERR(vfio_pci_core_get_iomap(vdev, get_dma_buf.region_index)))
+	/* For PCI the region_index is the BAR number like everything else. */
+	if (get_dma_buf.region_index >= VFIO_PCI_ROM_REGION_INDEX)
 		return -ENODEV;
 
 	dma_ranges = memdup_array_user(&arg->dma_ranges, get_dma_buf.nr_ranges,
@@ -274,19 +270,30 @@ int vfio_pci_core_feature_dma_buf(struct vfio_pci_core_device *vdev, u32 flags,
 	priv->vdev = vdev;
 	priv->nr_ranges = get_dma_buf.nr_ranges;
 	priv->size = length;
+
+	ret = vfio_pci_core_access_begin(vdev);
+	if (ret)
+		goto err_free_phys;
+
+	/* Check that PCI resources have been claimed for the BAR. */
+	if (IS_ERR(vfio_pci_core_get_iomap(vdev, get_dma_buf.region_index))) {
+		ret = -ENODEV;
+		goto err_access;
+	}
+
 	ret = vdev->pci_ops->get_dmabuf_phys(vdev, &priv->provider,
 					     get_dma_buf.region_index,
 					     priv->phys_vec, dma_ranges,
 					     priv->nr_ranges);
 	if (ret)
-		goto err_free_phys;
+		goto err_access;
 
 	kfree(dma_ranges);
 	dma_ranges = NULL;
 
 	if (!vfio_device_try_get_registration(&vdev->vdev)) {
 		ret = -ENODEV;
-		goto err_free_phys;
+		goto err_access;
 	}
 
 	exp_info.ops = &vfio_pci_dmabuf_ops;
@@ -311,6 +318,7 @@ int vfio_pci_core_feature_dma_buf(struct vfio_pci_core_device *vdev, u32 flags,
 	list_add_tail(&priv->dmabufs_elm, &vdev->dmabufs);
 	dma_resv_unlock(priv->dmabuf->resv);
 	up_write(&vdev->memory_lock);
+	vfio_pci_core_access_end(vdev);
 
 	/*
 	 * dma_buf_fd() consumes the reference, when the file closes the dmabuf
@@ -324,6 +332,8 @@ int vfio_pci_core_feature_dma_buf(struct vfio_pci_core_device *vdev, u32 flags,
 
 err_dev_put:
 	vfio_device_put_registration(&vdev->vdev);
+err_access:
+	vfio_pci_core_access_end(vdev);
 err_free_phys:
 	kfree(priv->phys_vec);
 err_free_priv:
-- 
2.43.0

Re: [RFC PATCH 13/19] vfio/pci: Serialize DMA-BUF export with recovery

[sashiko-bot]

> Move the BAR iomap check under recovery_lock and hold the lock for
> reading from there until the export is on vdev->dmabufs, so recovery
> cannot invalidate the mapping in between. An export attempted while
> access is blocked is refused by vfio_pci_core_access_begin() like any
> other device access.
> 
> Assisted-by: Claude:claude-opus-5
> Signed-off-by: Shameer Kolothum <skolothumtho@nvidia.com>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260901093217.8539-1-skolothumtho@nvidia.com?part=13

[RFC PATCH 14/19] vfio/pci: Add generic PCI error slot reset handling

[Shameer Kolothum]

Add a slot_reset() handler for vfio-pci-core.

Restore the saved PCI state first. The host resets the link without
restoring config space, so on entry the BARs read as zero, and tearing
down MSI-X before the restore would write through a stale table address
the device no longer decodes.

Then tear down the stale interrupt configuration, holding recovery_lock
across it. vfio_pci_core_disable() runs the same interrupt teardown when
the device is closed and takes no igate, relying on there being no other
user by then. vfio_msi_set_vector_signal() frees the per-vector context
with no atomicity between the lookup and the erase, so two callers which
both find it free the irq, the name and the eventfd context twice.
vfio_pci_core_prepare_close() takes recovery_lock for writing before it,
so holding it here keeps close out.

The interrupt teardown ends in a flush of the virqfd cleanup workqueue,
which is shared by every vfio device in the system. So this holds
recovery_lock while waiting for other devices' work to finish.

That cannot deadlock, because none of that work ever asks for
recovery_lock. It can be slow. The flush waits for an ioeventfd write to
complete, and that write waits for its own device's memory_lock, which a
reset on that device holds. So the wait here can last as long as a reset
somewhere else, and anything waiting on this device's recovery_lock waits
with it.

If the teardown fails, record it as FAILED and return
PCI_ERS_RESULT_NONE. Returning DISCONNECT would fail every device under
the bridge for a problem which is local to this one.

The handler does nothing until a later patch starts a recovery
transaction.

Assisted-by: Claude:claude-opus-5
Signed-off-by: Shameer Kolothum <skolothumtho@nvidia.com>
---
 drivers/vfio/pci/vfio_pci_core.c | 72 ++++++++++++++++++++++++++++++++
 1 file changed, 72 insertions(+)

diff --git a/drivers/vfio/pci/vfio_pci_core.c b/drivers/vfio/pci/vfio_pci_core.c
index 4447967413e7..b3ad7ed261e1 100644
--- a/drivers/vfio/pci/vfio_pci_core.c
+++ b/drivers/vfio/pci/vfio_pci_core.c
@@ -2756,6 +2756,77 @@ pci_ers_result_t vfio_pci_core_aer_err_detected(struct pci_dev *pdev,
 }
 EXPORT_SYMBOL_GPL(vfio_pci_core_aer_err_detected);
 
+static pci_ers_result_t vfio_pci_core_aer_slot_reset(struct pci_dev *pdev)
+{
+	struct vfio_pci_core_device *vdev = dev_get_drvdata(&pdev->dev);
+	pci_ers_result_t result = PCI_ERS_RESULT_RECOVERED;
+	int ret = 0;
+
+	down_write(&vdev->recovery_lock);
+	if (!(vdev->pci_recovery_flags & VFIO_PCI_RECOVERY_IN_PROGRESS) ||
+	    !vdev->pci_recovery_device_open) {
+		up_write(&vdev->recovery_lock);
+		return PCI_ERS_RESULT_NONE;
+	}
+
+	/*
+	 * Restore first. aer_root_reset() resets the link with
+	 * PCI_RESET_NO_RESTORE, so on entry the BARs read as zero. Tearing
+	 * down MSI-X before this would have pci_msix_shutdown() write through
+	 * the stale table mapping to an address the device no longer decodes.
+	 */
+	pci_restore_state(pdev);
+
+	/*
+	 * Hold recovery_lock across the interrupt teardown.
+	 * vfio_pci_core_disable() runs the same teardown on close without
+	 * taking igate, and running the per-vector teardown twice frees the
+	 * irq, the name and the eventfd context twice.
+	 * vfio_pci_core_prepare_close() takes recovery_lock for writing
+	 * before it, so holding it here keeps the two apart.
+	 */
+	mutex_lock(&vdev->igate);
+	if (vdev->irq_type < VFIO_PCI_NUM_IRQS)
+		ret = vfio_pci_set_irqs_ioctl(vdev,
+					      VFIO_IRQ_SET_DATA_NONE |
+					      VFIO_IRQ_SET_ACTION_TRIGGER,
+					      vdev->irq_type, 0, 0, NULL);
+	mutex_unlock(&vdev->igate);
+
+	if (ret) {
+		WRITE_ONCE(vdev->pci_recovery_flags,
+			   (vdev->pci_recovery_flags |
+			    VFIO_PCI_RECOVERY_FAILED) &
+			   ~VFIO_PCI_RECOVERY_IN_PROGRESS);
+		vdev->pci_recovery_command_valid = false;
+		/*
+		 * Vote NONE, not DISCONNECT. A DISCONNECT anywhere in the
+		 * domain makes the core skip resume() for every device under
+		 * the bridge and report permanent failure for all of them.
+		 * Our interrupt teardown failing says nothing about the
+		 * others, so record it locally and leave the domain verdict
+		 * alone.
+		 */
+		result = PCI_ERS_RESULT_NONE;
+	} else {
+		WRITE_ONCE(vdev->pci_recovery_flags,
+			   vdev->pci_recovery_flags |
+			   VFIO_PCI_RECOVERY_RESET);
+	}
+
+	up_write(&vdev->recovery_lock);
+	/*
+	 * Whoever clears IN_PROGRESS owes the wake. resume() will not do it,
+	 * since it bails once IN_PROGRESS is clear, and the core skips it
+	 * altogether if the domain verdict is not RECOVERED. On success the
+	 * transaction carries on and resume() wakes.
+	 */
+	if (ret)
+		wake_up_all(&vdev->pci_recovery_wait);
+
+	return result;
+}
+
 int vfio_pci_core_sriov_configure(struct vfio_pci_core_device *vdev,
 				  int nr_virtfn)
 {
@@ -2828,6 +2899,7 @@ EXPORT_SYMBOL_GPL(vfio_pci_core_sriov_configure);
 
 const struct pci_error_handlers vfio_pci_core_err_handlers = {
 	.error_detected = vfio_pci_core_aer_err_detected,
+	.slot_reset = vfio_pci_core_aer_slot_reset,
 };
 EXPORT_SYMBOL_GPL(vfio_pci_core_err_handlers);
 
-- 
2.43.0

Re: [RFC PATCH 14/19] vfio/pci: Add generic PCI error slot reset handling

[sashiko-bot]

> Add a slot_reset() handler for vfio-pci-core.
> 
> Restore the saved PCI state first. The host resets the link without
> restoring config space, so on entry the BARs read as zero, and tearing
> down MSI-X before the restore would write through a stale table address
> the device no longer decodes.
> 
> Then tear down the stale interrupt configuration, holding recovery_lock
> across it. vfio_pci_core_disable() runs the same interrupt teardown when
> the device is closed and takes no igate, relying on there being no other
> user by then. vfio_msi_set_vector_signal() frees the per-vector context
> with no atomicity between the lookup and the erase, so two callers which
> both find it free the irq, the name and the eventfd context twice.
> vfio_pci_core_prepare_close() takes recovery_lock for writing before it,
> so holding it here keeps close out.
> [ ... ]
> The handler does nothing until a later patch starts a recovery
> transaction.
> 
> Assisted-by: Claude:claude-opus-5
> Signed-off-by: Shameer Kolothum <skolothumtho@nvidia.com>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260901093217.8539-1-skolothumtho@nvidia.com?part=14

[RFC PATCH 15/19] vfio/pci: Add INTx helpers for PCI recovery

[Shameer Kolothum]

Add the helpers the recovery callbacks need to keep INTx in step with an
error event, and the two per-context flags they record it in. Nothing
calls them yet.

vfio_pci_intx_recovery_start() masks the line when an event begins.
Nothing has fired at that point, and pci_check_and_mask_intx() only writes
DisINTx when the status register says an interrupt is pending, so it would
find nothing to do and leave the line enabled. Use pci_intx() instead,
which masks whatever the device is doing. __vfio_pci_intx_mask() already
does this for the same reason.

vfio_pci_intx_recovery_finish() replays what the event masked, and any
unmask which arrived while it ran, once the event ends. An interrupt which
was masked and delivered while access was blocked is not replayed. The
user was told about it, and unmasks it as it would outside recovery.

vfio_pci_intx_recovery_command() reconciles INTX_DISABLE with the command
word error_detected() saves. error_detected() sets that bit when it writes
the quiesced command word, without recording it as a mask, so the saved
word and ctx->masked can disagree about it. The helper keeps the bit as
the INTx state has it, for resume() to use when it restores the word.
Restoring the saved bit instead would unmask a line the handler still
believes is masked, and a shared pci_2_3 line would storm until
note_interrupt() disabled it for every device on it.

Assisted-by: Claude:claude-opus-5
Signed-off-by: Shameer Kolothum <skolothumtho@nvidia.com>
---
 drivers/vfio/pci/vfio_pci_priv.h  |   4 ++
 drivers/vfio/pci/vfio_pci_intrs.c | 116 ++++++++++++++++++++++++++++++
 2 files changed, 120 insertions(+)

diff --git a/drivers/vfio/pci/vfio_pci_priv.h b/drivers/vfio/pci/vfio_pci_priv.h
index 8a7f9fe22386..5598e472da4b 100644
--- a/drivers/vfio/pci/vfio_pci_priv.h
+++ b/drivers/vfio/pci/vfio_pci_priv.h
@@ -25,6 +25,10 @@ struct vfio_pci_ioeventfd {
 
 bool vfio_pci_intx_mask(struct vfio_pci_core_device *vdev);
 void vfio_pci_intx_unmask(struct vfio_pci_core_device *vdev);
+void vfio_pci_intx_recovery_start(struct vfio_pci_core_device *vdev);
+void vfio_pci_intx_recovery_finish(struct vfio_pci_core_device *vdev);
+u16 vfio_pci_intx_recovery_command(struct vfio_pci_core_device *vdev,
+				   u16 command);
 
 int vfio_pci_eventfd_replace_locked(struct vfio_pci_core_device *vdev,
 				    struct vfio_pci_eventfd __rcu **peventfd,
diff --git a/drivers/vfio/pci/vfio_pci_intrs.c b/drivers/vfio/pci/vfio_pci_intrs.c
index 64f80f64ff57..c4a075b5bb2e 100644
--- a/drivers/vfio/pci/vfio_pci_intrs.c
+++ b/drivers/vfio/pci/vfio_pci_intrs.c
@@ -29,6 +29,8 @@ struct vfio_pci_irq_ctx {
 	struct virqfd			*mask;
 	char				*name;
 	bool				masked;
+	bool				recovery_masked;
+	bool				unmask_pending;
 	struct irq_bypass_producer	producer;
 };
 
@@ -220,6 +222,40 @@ void vfio_pci_intx_unmask(struct vfio_pci_core_device *vdev)
 	mutex_unlock(&vdev->igate);
 }
 
+/*
+ * Mask INTx because recovery has blocked device access. Returns true if this
+ * call did the masking, which means recovery is the one which must unmask.
+ *
+ * Nothing is normally asserted when a recovery starts, and
+ * pci_check_and_mask_intx() only writes DisINTx when the status register says
+ * an interrupt is pending, so it would leave the line alone. pci_intx() masks
+ * whatever the device is doing, as __vfio_pci_intx_mask() already does for the
+ * same reason.
+ *
+ * Masking a pci_2_3 device goes through config space. If the error left
+ * config space unreadable the write has no effect and the line stays
+ * asserted, which is no worse than not trying. For a non-fatal error config
+ * space still works, and this is what keeps a shared line from storming while
+ * access is blocked.
+ */
+static bool vfio_pci_intx_mask_for_recovery(struct vfio_pci_core_device *vdev,
+					    struct vfio_pci_irq_ctx *ctx)
+{
+	lockdep_assert_held(&vdev->irqlock);
+
+	if (ctx->masked)
+		return false;
+
+	if (!vdev->pci_2_3)
+		disable_irq_nosync(vdev->pdev->irq);
+	else
+		pci_intx(vdev->pdev, 0);
+
+	ctx->masked = true;
+	ctx->recovery_masked = true;
+	return true;
+}
+
 static irqreturn_t vfio_intx_handler(int irq, void *dev_id)
 {
 	struct vfio_pci_irq_ctx *ctx = dev_id;
@@ -247,6 +283,86 @@ static irqreturn_t vfio_intx_handler(int irq, void *dev_id)
 	return ret;
 }
 
+void vfio_pci_intx_recovery_start(struct vfio_pci_core_device *vdev)
+{
+	struct vfio_pci_irq_ctx *ctx;
+	unsigned long flags;
+
+	lockdep_assert_held_write(&vdev->recovery_lock);
+
+	spin_lock_irqsave(&vdev->irqlock, flags);
+	if (!is_intx(vdev))
+		goto out_unlock;
+
+	ctx = vfio_irq_ctx_get(vdev, 0);
+	if (WARN_ON_ONCE(!ctx))
+		goto out_unlock;
+
+	vfio_pci_intx_mask_for_recovery(vdev, ctx);
+
+out_unlock:
+	spin_unlock_irqrestore(&vdev->irqlock, flags);
+}
+
+/*
+ * Replay the masking recovery did, and any unmask which arrived while it was
+ * blocked. Call this only after access_blocked has been cleared, or the
+ * replayed unmask is swallowed and recorded as pending again with nothing
+ * left to replay it.
+ */
+/*
+ * The command word saved before the quiesce can have INTX_DISABLE clear, but
+ * the INTx handler may have masked the line since. Keep the bit as the INTx
+ * state has it, so hardware and ctx->masked agree until
+ * vfio_pci_intx_recovery_finish() replays. Restoring the saved bit instead
+ * would unmask a line the handler still believes is masked, and a shared
+ * pci_2_3 line would then storm until note_interrupt() disables it.
+ */
+u16 vfio_pci_intx_recovery_command(struct vfio_pci_core_device *vdev,
+				   u16 command)
+{
+	struct vfio_pci_irq_ctx *ctx;
+
+	lockdep_assert_held(&vdev->irqlock);
+
+	if (!is_intx(vdev))
+		return command;
+
+	ctx = vfio_irq_ctx_get(vdev, 0);
+	if (ctx && ctx->masked)
+		command |= PCI_COMMAND_INTX_DISABLE;
+
+	return command;
+}
+
+void vfio_pci_intx_recovery_finish(struct vfio_pci_core_device *vdev)
+{
+	struct vfio_pci_irq_ctx *ctx;
+	unsigned long flags;
+	bool replay = false;
+
+	lockdep_assert_held_write(&vdev->recovery_lock);
+
+	mutex_lock(&vdev->igate);
+	spin_lock_irqsave(&vdev->irqlock, flags);
+	if (!is_intx(vdev))
+		goto out_unlock;
+
+	ctx = vfio_irq_ctx_get(vdev, 0);
+	if (WARN_ON_ONCE(!ctx))
+		goto out_unlock;
+
+	replay = ctx->recovery_masked || ctx->unmask_pending;
+	ctx->recovery_masked = false;
+	ctx->unmask_pending = false;
+
+out_unlock:
+	spin_unlock_irqrestore(&vdev->irqlock, flags);
+	if (replay)
+		__vfio_pci_intx_unmask(vdev);
+	mutex_unlock(&vdev->igate);
+}
+
 static int vfio_intx_enable(struct vfio_pci_core_device *vdev,
 			    struct eventfd_ctx *trigger)
 {
-- 
2.43.0

Re: [RFC PATCH 15/19] vfio/pci: Add INTx helpers for PCI recovery

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] vfio_pci_intx_recovery_command() unconditionally sets PCI_COMMAND_INTX_DISABLE without checking if the device supports PCI 2.3 INTx masking.
--

commit e9304e8881d7f9626b1abc86dddca1139e91674f
Author: Shameer Kolothum <skolothumtho@nvidia.com>

vfio/pci: Add INTx helpers for PCI recovery

This commit adds helpers the recovery callbacks need to keep INTx in step
with an error event, and the two per-context flags they record it in.

> diff --git a/drivers/vfio/pci/vfio_pci_intrs.c b/drivers/vfio/pci/vfio_pci_intrs.c
> index 64f80f64ff57a..c4a075b5bb2e3 100644
> --- a/drivers/vfio/pci/vfio_pci_intrs.c
> +++ b/drivers/vfio/pci/vfio_pci_intrs.c

[ ... ]

> @@ -247,6 +283,86 @@ static irqreturn_t vfio_intx_handler(int irq, void *dev_id)
>  	return ret;
>  }

[ ... ]

> +u16 vfio_pci_intx_recovery_command(struct vfio_pci_core_device *vdev,
> +				   u16 command)
> +{
> +	struct vfio_pci_irq_ctx *ctx;
> +
> +	lockdep_assert_held(&vdev->irqlock);
> +
> +	if (!is_intx(vdev))
> +		return command;
> +
> +	ctx = vfio_irq_ctx_get(vdev, 0);
> +	if (ctx && ctx->masked)
> +		command |= PCI_COMMAND_INTX_DISABLE;

[Severity: High]
Is it safe to unconditionally set PCI_COMMAND_INTX_DISABLE here without
checking if the device supports PCI 2.3 INTx masking?

If a host PCI error recovery operation calls vfio_pci_intx_recovery_command()
on a legacy PCI device (where vdev->pci_2_3 is false) whose INTx is currently
masked, this erroneously adds the INTX_DISABLE bit to the saved command word.

Writing to reserved bits in the PCI Command Register on legacy devices
(pre-PCI 2.3) can cause undefined behavior or hardware malfunction. Should
this check vdev->pci_2_3 before setting the disable bit, since other INTx
handling paths in this subsystem correctly verify it?

> +
> +	return command;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260901093217.8539-1-skolothumtho@nvidia.com?part=15

[RFC PATCH 16/19] vfio/pci: Quiesce INTx during PCI recovery

[Shameer Kolothum]

Mask INTx when recovery blocks device access. A PCI 2.3 device shares its
line, so leaving a level interrupt asserted and returning IRQ_NONE would
storm until note_interrupt() disables the line for every device on it.
Mask through pci_check_and_mask_intx() and return IRQ_HANDLED instead. A
device without per-function masking has the line to itself and is quiesced
through genirq.

That is a different job from the masking recovery does for itself, so
vfio_pci_intx_mask_for_recovery() gains a @quiesce argument to tell them
apart. The handler knows an interrupt has fired but not whether this
device is the one asserting the shared line, which is what
pci_check_and_mask_intx() reports, and a false return means the interrupt
belongs to another device. It is also the user who unmasks afterwards,
having been sent the eventfd, so the handler does not record the mask as
one recovery owes.

Record unmask requests received during recovery instead of losing them, so
vfio_pci_intx_recovery_finish() can replay them once the event ends.

The unmask handler checks for a blocked device before the existing INTx
test, so a blocked device is not touched through pci_intx(). It only
dereferences the interrupt context once that test has passed. The DisINTx
emulation calls the unmask path whatever irq_type is set to, and the
context is NULL when INTx is not in use.

Replay from the reset path as well. The unmask eventfd reaches
vfio_pci_intx_unmask_handler() through virqfd, which takes no recovery_lock
and so cannot be refused, and VFIO_DEVICE_RESET blocks access for its
duration. Without a replay there the line stays masked and the guest waits
for an interrupt which cannot arrive, since it only unmasks again after
receiving one.

Assisted-by: Claude:claude-opus-5
Signed-off-by: Shameer Kolothum <skolothumtho@nvidia.com>
---
 drivers/vfio/pci/vfio_pci_core.c  |  1 +
 drivers/vfio/pci/vfio_pci_intrs.c | 61 ++++++++++++++++++++++++++-----
 2 files changed, 53 insertions(+), 9 deletions(-)

diff --git a/drivers/vfio/pci/vfio_pci_core.c b/drivers/vfio/pci/vfio_pci_core.c
index b3ad7ed261e1..658cccecab12 100644
--- a/drivers/vfio/pci/vfio_pci_core.c
+++ b/drivers/vfio/pci/vfio_pci_core.c
@@ -1558,6 +1558,7 @@ int vfio_pci_try_reset_function(struct vfio_pci_core_device *vdev,
 						  VFIO_PCI_RECOVERY_FAILED))) {
 			vfio_pci_recovery_rom_disable(vdev);
 			WRITE_ONCE(vdev->pci_recovery_access_blocked, false);
+			vfio_pci_intx_recovery_finish(vdev);
 		}
 		up_write(&vdev->recovery_lock);
 		/*
diff --git a/drivers/vfio/pci/vfio_pci_intrs.c b/drivers/vfio/pci/vfio_pci_intrs.c
index c4a075b5bb2e..1981a64b6e18 100644
--- a/drivers/vfio/pci/vfio_pci_intrs.c
+++ b/drivers/vfio/pci/vfio_pci_intrs.c
@@ -51,6 +51,15 @@ static bool is_irq_none(struct vfio_pci_core_device *vdev)
 		 vdev->irq_type == VFIO_PCI_MSIX_IRQ_INDEX);
 }
 
+static bool vfio_pci_recovery_blocks_irq(struct vfio_pci_core_device *vdev)
+{
+	if (!vdev->pci_recovery_supported)
+		return false;
+
+	return READ_ONCE(vdev->pci_recovery_enabled) &&
+	       READ_ONCE(vdev->pci_recovery_access_blocked);
+}
+
 static
 struct vfio_pci_irq_ctx *vfio_irq_ctx_get(struct vfio_pci_core_device *vdev,
 					  unsigned long index)
@@ -173,6 +182,16 @@ static int vfio_pci_intx_unmask_handler(void *opaque, void *data)
 	int ret = 0;
 
 	spin_lock_irqsave(&vdev->irqlock, flags);
+	/*
+	 * Check for a blocked device before the INTx test below, so a blocked
+	 * device is not touched through pci_intx(). @ctx is only valid when
+	 * INTx is in use, so record the request only then.
+	 */
+	if (unlikely(vfio_pci_recovery_blocks_irq(vdev))) {
+		if (is_intx(vdev))
+			ctx->unmask_pending = true;
+		goto out_unlock;
+	}
 
 	/*
 	 * Unmasking comes from ioctl or config, so again, have the
@@ -184,6 +203,9 @@ static int vfio_pci_intx_unmask_handler(void *opaque, void *data)
 		goto out_unlock;
 	}
 
+	ctx->unmask_pending = false;
+	ctx->recovery_masked = false;
+
 	if (ctx->masked && !vdev->virq_disabled) {
 		/*
 		 * A pending interrupt here would immediately trigger,
@@ -224,13 +246,16 @@ void vfio_pci_intx_unmask(struct vfio_pci_core_device *vdev)
 
 /*
  * Mask INTx because recovery has blocked device access. Returns true if this
- * call did the masking, which means recovery is the one which must unmask.
+ * call did the masking.
  *
- * Nothing is normally asserted when a recovery starts, and
+ * Set @quiesce when recovery is masking the line itself rather than masking
+ * one delivered interrupt. Nothing is normally asserted at that point, and
  * pci_check_and_mask_intx() only writes DisINTx when the status register says
- * an interrupt is pending, so it would leave the line alone. pci_intx() masks
- * whatever the device is doing, as __vfio_pci_intx_mask() already does for the
- * same reason.
+ * an interrupt is pending, so it would leave the line alone. The interrupt
+ * handler wants that test, since a false return there means the interrupt
+ * belongs to another device on a shared line. @quiesce also records that
+ * recovery is the one which must unmask. A masked interrupt which was
+ * delivered is the user's to unmask, exactly as outside recovery.
  *
  * Masking a pci_2_3 device goes through config space. If the error left
  * config space unreadable the write has no effect and the line stays
@@ -239,7 +264,8 @@ void vfio_pci_intx_unmask(struct vfio_pci_core_device *vdev)
  * access is blocked.
  */
 static bool vfio_pci_intx_mask_for_recovery(struct vfio_pci_core_device *vdev,
-					    struct vfio_pci_irq_ctx *ctx)
+					    struct vfio_pci_irq_ctx *ctx,
+					    bool quiesce)
 {
 	lockdep_assert_held(&vdev->irqlock);
 
@@ -248,11 +274,14 @@ static bool vfio_pci_intx_mask_for_recovery(struct vfio_pci_core_device *vdev,
 
 	if (!vdev->pci_2_3)
 		disable_irq_nosync(vdev->pdev->irq);
-	else
+	else if (quiesce)
 		pci_intx(vdev->pdev, 0);
+	else if (!pci_check_and_mask_intx(vdev->pdev))
+		return false;
 
 	ctx->masked = true;
-	ctx->recovery_masked = true;
+	if (quiesce)
+		ctx->recovery_masked = true;
 	return true;
 }
 
@@ -264,6 +293,19 @@ static irqreturn_t vfio_intx_handler(int irq, void *dev_id)
 	int ret = IRQ_NONE;
 
 	spin_lock_irqsave(&vdev->irqlock, flags);
+	if (unlikely(vfio_pci_recovery_blocks_irq(vdev))) {
+		/*
+		 * Mask rather than return IRQ_NONE with the line still
+		 * asserted. For a shared pci_2_3 line an unhandled level
+		 * interrupt storms until note_interrupt() disables the line
+		 * for every device on it, not just this one.
+		 */
+		if (vfio_pci_intx_mask_for_recovery(vdev, ctx, false))
+			ret = IRQ_HANDLED;
+		else if (ctx->masked && !vdev->pci_2_3)
+			ret = IRQ_HANDLED;
+		goto out_unlock;
+	}
 
 	if (!vdev->pci_2_3) {
 		disable_irq_nosync(vdev->pdev->irq);
@@ -275,6 +317,7 @@ static irqreturn_t vfio_intx_handler(int irq, void *dev_id)
 		ret = IRQ_HANDLED;
 	}
 
+out_unlock:
 	spin_unlock_irqrestore(&vdev->irqlock, flags);
 
 	if (ret == IRQ_HANDLED)
@@ -298,7 +341,7 @@ void vfio_pci_intx_recovery_start(struct vfio_pci_core_device *vdev)
 	if (WARN_ON_ONCE(!ctx))
 		goto out_unlock;
 
-	vfio_pci_intx_mask_for_recovery(vdev, ctx);
+	vfio_pci_intx_mask_for_recovery(vdev, ctx, true);
 
 out_unlock:
 	spin_unlock_irqrestore(&vdev->irqlock, flags);
-- 
2.43.0

Re: [RFC PATCH 16/19] vfio/pci: Quiesce INTx during PCI recovery

[sashiko-bot]

> Mask INTx when recovery blocks device access. A PCI 2.3 device shares its
> line, so leaving a level interrupt asserted and returning IRQ_NONE would
> storm until note_interrupt() disables the line for every device on it.
> Mask through pci_check_and_mask_intx() and return IRQ_HANDLED instead. A
> device without per-function masking has the line to itself and is quiesced
> through genirq.
> 
> That is a different job from the masking recovery does for itself, so
> vfio_pci_intx_mask_for_recovery() gains a @quiesce argument to tell them
> apart. The handler knows an interrupt has fired but not whether this
> device is the one asserting the shared line, which is what
> pci_check_and_mask_intx() reports, and a false return means the interrupt
> belongs to another device. It is also the user who unmasks afterwards,
> having been sent the eventfd, so the handler does not record the mask as
> one recovery owes.
> [ ... ]
> for an interrupt which cannot arrive, since it only unmasks again after
> receiving one.
> 
> Assisted-by: Claude:claude-opus-5
> Signed-off-by: Shameer Kolothum <skolothumtho@nvidia.com>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260901093217.8539-1-skolothumtho@nvidia.com?part=16

[RFC PATCH 17/19] vfio/pci: Add generic PCI error resume handling

[Shameer Kolothum]

Add a resume() handler for vfio-pci-core. It ends the recovery
transaction and makes the device usable again.

INTX_DISABLE is taken from the INTx state rather than from the saved word.
The handler can have masked the line after the word was saved, and
restoring the saved bit would unmask a line it still believes is masked.
The replay is what unmasks it.

Restore the command word saved by error_detected(), unless the device was
reset, in which case slot_reset() already restored the whole config space
and the saved value is stale. Then unblock access, un-revoke exported
DMA-BUFs if the guest still has memory decode enabled, clear IN_PROGRESS
and replay the INTx state recorded during the event.

If the command write fails, keep access blocked and publish FAILED.
IN_PROGRESS is cleared and FAILED set in a single store, so a lock-free
reader never sees the intermediate state, which would read as a
successful completion.

Pay any deferred ROM decode disable first, before the in-progress check.
A failed transaction has already cleared that flag, and the disable would
be lost. The helper skips a closed device on its own.

Until a later patch starts a recovery transaction, only the deferred ROM
disable runs here. The rest returns early.

Signed-off-by: Shameer Kolothum <skolothumtho@nvidia.com>
---
 drivers/vfio/pci/vfio_pci_core.c | 68 ++++++++++++++++++++++++++++++++
 1 file changed, 68 insertions(+)

diff --git a/drivers/vfio/pci/vfio_pci_core.c b/drivers/vfio/pci/vfio_pci_core.c
index 658cccecab12..eed0430c32ee 100644
--- a/drivers/vfio/pci/vfio_pci_core.c
+++ b/drivers/vfio/pci/vfio_pci_core.c
@@ -2828,6 +2828,73 @@ static pci_ers_result_t vfio_pci_core_aer_slot_reset(struct pci_dev *pdev)
 	return result;
 }
 
+static void vfio_pci_core_aer_resume(struct pci_dev *pdev)
+{
+	struct vfio_pci_core_device *vdev = dev_get_drvdata(&pdev->dev);
+	unsigned long irq_flags;
+	u32 flags;
+	int ret = 0;
+
+	down_write(&vdev->recovery_lock);
+
+	/*
+	 * Pay any deferred ROM disable before the in-progress check below,
+	 * which a failed transaction has already cleared, or it would be
+	 * lost.
+	 */
+	vfio_pci_recovery_rom_disable(vdev);
+
+	if (!(vdev->pci_recovery_flags & VFIO_PCI_RECOVERY_IN_PROGRESS))
+		goto out_unlock;
+
+	if (!vdev->pci_recovery_device_open) {
+		vdev->pci_recovery_command_valid = false;
+		WRITE_ONCE(vdev->pci_recovery_flags,
+			   vdev->pci_recovery_flags &
+			   ~VFIO_PCI_RECOVERY_IN_PROGRESS);
+		goto out_unlock;
+	}
+
+	down_write(&vdev->memory_lock);
+	/*
+	 * Restore the command word and clear access_blocked under irqlock.
+	 * The INTx handler writes the same register through
+	 * pci_check_and_mask_intx(), so it must not interleave with the
+	 * restore, and it must not see access blocked cleared while the
+	 * temporary command value is still installed.
+	 *
+	 * INTX_DISABLE comes from the INTx state rather than from the saved
+	 * word, which can be older than the last mask. The replay below is
+	 * what unmasks the line.
+	 */
+	spin_lock_irqsave(&vdev->irqlock, irq_flags);
+	if (!(vdev->pci_recovery_flags & VFIO_PCI_RECOVERY_RESET) &&
+	    vdev->pci_recovery_command_valid) {
+		u16 cmd = vdev->pci_recovery_command;
+
+		cmd = vfio_pci_intx_recovery_command(vdev, cmd);
+		ret = pci_write_config_word(pdev, PCI_COMMAND, cmd);
+	}
+	if (!ret)
+		WRITE_ONCE(vdev->pci_recovery_access_blocked, false);
+	spin_unlock_irqrestore(&vdev->irqlock, irq_flags);
+	if (!ret && __vfio_pci_memory_enabled(vdev))
+		vfio_pci_dma_buf_move(vdev, false);
+	up_write(&vdev->memory_lock);
+
+	vdev->pci_recovery_command_valid = false;
+	flags = vdev->pci_recovery_flags & ~VFIO_PCI_RECOVERY_IN_PROGRESS;
+	if (ret)
+		flags |= VFIO_PCI_RECOVERY_FAILED;
+	WRITE_ONCE(vdev->pci_recovery_flags, flags);
+	if (!ret)
+		vfio_pci_intx_recovery_finish(vdev);
+
+out_unlock:
+	up_write(&vdev->recovery_lock);
+	wake_up_all(&vdev->pci_recovery_wait);
+}
+
 int vfio_pci_core_sriov_configure(struct vfio_pci_core_device *vdev,
 				  int nr_virtfn)
 {
@@ -2901,6 +2968,7 @@ EXPORT_SYMBOL_GPL(vfio_pci_core_sriov_configure);
 const struct pci_error_handlers vfio_pci_core_err_handlers = {
 	.error_detected = vfio_pci_core_aer_err_detected,
 	.slot_reset = vfio_pci_core_aer_slot_reset,
+	.resume = vfio_pci_core_aer_resume,
 };
 EXPORT_SYMBOL_GPL(vfio_pci_core_err_handlers);
 
-- 
2.43.0

Re: [RFC PATCH 17/19] vfio/pci: Add generic PCI error resume handling

[sashiko-bot]

> Add a resume() handler for vfio-pci-core. It ends the recovery
> transaction and makes the device usable again.
> 
> INTX_DISABLE is taken from the INTx state rather than from the saved word.
> The handler can have masked the line after the word was saved, and
> restoring the saved bit would unmask a line it still believes is masked.
> The replay is what unmasks it.
> 
> Restore the command word saved by error_detected(), unless the device was
> reset, in which case slot_reset() already restored the whole config space
> and the saved value is stale. Then unblock access, un-revoke exported
> DMA-BUFs if the guest still has memory decode enabled, clear IN_PROGRESS
> and replay the INTx state recorded during the event.
> 
> If the command write fails, keep access blocked and publish FAILED.
> IN_PROGRESS is cleared and FAILED set in a single store, so a lock-free
> reader never sees the intermediate state, which would read as a
> successful completion.
> 
> Pay any deferred ROM decode disable first, before the in-progress check.
> A failed transaction has already cleared that flag, and the disable would
> be lost. The helper skips a closed device on its own.
> 
> Until a later patch starts a recovery transaction, only the deferred ROM
> disable runs here. The rest returns early.
> 
> Signed-off-by: Shameer Kolothum <skolothumtho@nvidia.com>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260901093217.8539-1-skolothumtho@nvidia.com?part=17

[RFC PATCH 18/19] vfio/pci: Coordinate generic device access with host recovery

[Shameer Kolothum]

Wire up error_detected() for generic vfio-pci devices whose user has
enabled recovery. Devices which have not opted in, and variant drivers,
keep the existing signal-only behaviour.

Block new device access and drain what is already running, then revoke
BAR mappings, revoke exported DMA-BUFs and stop bus mastering, so nothing
touches the device while the host recovers it. A non-fatal error gets the
same treatment 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.

Do not trust a command word which reads as all ones. A device which has
stopped responding still returns success, and writing that value back
would set every command bit while saving it would restore them at the end.
Treat it as a config access failure instead.

Quiesce INTx first. For a device with per-function masking, also save
PCI_COMMAND and write it back with INTX_DISABLE set and bus mastering
cleared, under irqlock so an interrupt handler cannot interleave.

Publish the state in one store. IN_PROGRESS and CHANNEL_FROZEN go out
together so a lock-free reader cannot see an event which is in progress
but not yet marked frozen. FAILED stays set until the device is closed
and reopened.

A frozen channel votes NEED_RESET. A config access failure of our own
votes NONE, which leaves the rest of the recovery domain alone. Only a
permanent channel failure reported to us votes DISCONNECT.

If a ROM unmap raced the blocked interval, its config write is left for
resume() to complete.

A second event which arrives before resume() has finished the first joins
the transaction already running. It keeps the sequence number, the command
word saved before the device was quiesced, and any reset a slot_reset() in
between recorded. Starting again would save the quiesced command word and
restore a device with bus mastering off, and would drop the record of a
reset the host had already performed.

An event which arrives while a VFIO_DEVICE_RESET has access blocked runs
as usual. The PCI core calls this with the device lock held, which
pci_try_reset_function() also takes, so the two cannot overlap the reset
itself, and the reset leaves the state alone once this has claimed it.
Suppressing the event instead would lose a permanent failure or a bus
reset the host went on to perform, which is the state userspace most
needs.

Signed-off-by: Shameer Kolothum <skolothumtho@nvidia.com>
---
 drivers/vfio/pci/vfio_pci_core.c | 166 ++++++++++++++++++++++++++++++-
 1 file changed, 165 insertions(+), 1 deletion(-)

diff --git a/drivers/vfio/pci/vfio_pci_core.c b/drivers/vfio/pci/vfio_pci_core.c
index eed0430c32ee..2d757d6a5fe1 100644
--- a/drivers/vfio/pci/vfio_pci_core.c
+++ b/drivers/vfio/pci/vfio_pci_core.c
@@ -2746,14 +2746,178 @@ pci_ers_result_t vfio_pci_core_aer_err_detected(struct pci_dev *pdev,
 {
 	struct vfio_pci_core_device *vdev = dev_get_drvdata(&pdev->dev);
 	struct vfio_pci_eventfd *eventfd;
+	pci_ers_result_t result = PCI_ERS_RESULT_CAN_RECOVER;
+	unsigned long irq_flags;
+	bool terminal = false;
+	bool nested;
+	u32 flags;
+	int ret;
+
+	if (!vdev->pci_recovery_supported ||
+	    !READ_ONCE(vdev->pci_recovery_enabled))
+		goto out;
+
+	down_write(&vdev->recovery_lock);
+	if (!vdev->pci_recovery_enabled)
+		goto out_unlock;
+
+	/*
+	 * A failed device remains blocked until close and a new open have
+	 * reinitialized it. A later bridge event cannot make the saved VFIO
+	 * state valid again.
+	 */
+	if (vdev->pci_recovery_flags & VFIO_PCI_RECOVERY_FAILED) {
+		result = PCI_ERS_RESULT_NONE;
+		goto out_unlock;
+	}
+
+	if (!vdev->pci_recovery_device_open) {
+		result = PCI_ERS_RESULT_NONE;
+		/*
+		 * PCI core rebroadcasts permanent failure when subtree
+		 * recovery fails. Complete an event which started before
+		 * close so a later open is not permanently stuck on
+		 * IN_PROGRESS.
+		 */
+		if (state == pci_channel_io_perm_failure &&
+		    (vdev->pci_recovery_flags &
+		     VFIO_PCI_RECOVERY_IN_PROGRESS)) {
+			WRITE_ONCE(vdev->pci_recovery_flags,
+				   (vdev->pci_recovery_flags |
+				    VFIO_PCI_RECOVERY_FAILED) &
+				   ~VFIO_PCI_RECOVERY_IN_PROGRESS);
+			vdev->pci_recovery_command_valid = false;
+			terminal = true;
+		}
+		goto out_unlock;
+	}
+
+	WRITE_ONCE(vdev->pci_recovery_access_blocked, true);
+	/*
+	 * A second event before resume() has finished the first joins the
+	 * transaction already running rather than starting one. Keep its
+	 * sequence number, the command word it saved before the device was
+	 * quiesced, and any reset a slot_reset() in between recorded. Reading
+	 * the command word again here would save the quiesced value, and
+	 * restoring that leaves the device with bus mastering off.
+	 */
+	nested = vdev->pci_recovery_flags & VFIO_PCI_RECOVERY_IN_PROGRESS;
+	if (!nested)
+		vdev->pci_recovery_command_valid = false;
+	vfio_pci_intx_recovery_start(vdev);
+	/*
+	 * INTx hardirq and virqfd callbacks cannot take recovery_lock.
+	 * For devices with per-function INTx masking, mask INTx while holding
+	 * irqlock so a callback which passed its blocked-state check is drained
+	 * before the temporary command value is installed. Devices without
+	 * per-function masking were quiesced above through genirq.
+	 */
+	spin_lock_irqsave(&vdev->irqlock, irq_flags);
+	ret = 0;
+	if (state == pci_channel_io_normal && vdev->pci_2_3 && !nested) {
+		u16 command;
 
+		ret = pci_read_config_word(pdev, PCI_COMMAND,
+					   &vdev->pci_recovery_command);
+		/*
+		 * A read from a device which has stopped responding succeeds
+		 * and returns all ones. Writing that back would set every
+		 * command bit, and saving it would restore them at the end.
+		 */
+		if (!ret && PCI_POSSIBLE_ERROR(vdev->pci_recovery_command))
+			ret = -EIO;
+		if (!ret) {
+			command = (vdev->pci_recovery_command &
+				   ~PCI_COMMAND_MASTER) |
+				  PCI_COMMAND_INTX_DISABLE;
+			ret = pci_write_config_word(pdev, PCI_COMMAND, command);
+		}
+		if (!ret)
+			vdev->pci_recovery_command_valid = true;
+	}
+	spin_unlock_irqrestore(&vdev->irqlock, irq_flags);
+	vfio_pci_zap_and_down_write_memory_lock(vdev);
+	vfio_pci_dma_buf_move(vdev, true);
+
+	/*
+	 * Allocate a sequence for a new transaction, and drop the flags the
+	 * previous one left behind for userspace to read. A nested event adds
+	 * to the flags already there. Each path below publishes the result in
+	 * one store, so a lock-free reader never observes a cleared state that
+	 * looks like successful completion.
+	 */
+	flags = vdev->pci_recovery_flags;
+	if (!nested) {
+		if (++vdev->pci_recovery_sequence == 0)
+			vdev->pci_recovery_sequence++;
+		flags = 0;
+	}
+
+	if (state == pci_channel_io_perm_failure) {
+		WRITE_ONCE(vdev->pci_recovery_flags,
+			   (flags | VFIO_PCI_RECOVERY_FAILED) &
+			   ~VFIO_PCI_RECOVERY_IN_PROGRESS);
+		vdev->pci_recovery_command_valid = false;
+		result = PCI_ERS_RESULT_DISCONNECT;
+		terminal = true;
+		goto out_memory;
+	}
+
+	if (state == pci_channel_io_frozen) {
+		WRITE_ONCE(vdev->pci_recovery_flags,
+			   flags | VFIO_PCI_RECOVERY_IN_PROGRESS |
+			   VFIO_PCI_RECOVERY_FROZEN);
+		result = PCI_ERS_RESULT_NEED_RESET;
+		goto out_memory;
+	}
+
+	WRITE_ONCE(vdev->pci_recovery_flags,
+		   flags | VFIO_PCI_RECOVERY_IN_PROGRESS);
+	if (ret)
+		goto out_failed;
+	if (vdev->pci_2_3 || nested)
+		goto out_memory;
+
+	ret = pci_read_config_word(pdev, PCI_COMMAND,
+				   &vdev->pci_recovery_command);
+	if (ret)
+		goto out_failed;
+
+	if (PCI_POSSIBLE_ERROR(vdev->pci_recovery_command)) {
+		ret = -EIO;
+		goto out_failed;
+	}
+
+	ret = pci_write_config_word(pdev, PCI_COMMAND,
+				    vdev->pci_recovery_command &
+				    ~PCI_COMMAND_MASTER);
+	if (ret)
+		goto out_failed;
+
+	vdev->pci_recovery_command_valid = true;
+	goto out_memory;
+
+out_failed:
+	WRITE_ONCE(vdev->pci_recovery_flags,
+		   (vdev->pci_recovery_flags | VFIO_PCI_RECOVERY_FAILED) &
+		   ~VFIO_PCI_RECOVERY_IN_PROGRESS);
+	result = PCI_ERS_RESULT_NONE;
+	terminal = true;
+out_memory:
+	up_write(&vdev->memory_lock);
+out_unlock:
+	up_write(&vdev->recovery_lock);
+	if (terminal)
+		wake_up_all(&vdev->pci_recovery_wait);
+
+out:
 	rcu_read_lock();
 	eventfd = rcu_dereference(vdev->err_trigger);
 	if (eventfd)
 		eventfd_signal(eventfd->ctx);
 	rcu_read_unlock();
 
-	return PCI_ERS_RESULT_CAN_RECOVER;
+	return result;
 }
 EXPORT_SYMBOL_GPL(vfio_pci_core_aer_err_detected);
 
-- 
2.43.0

Re: [RFC PATCH 18/19] vfio/pci: Coordinate generic device access with host recovery

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Writing a reserved bit (INTX_DISABLE) to the PCI Command register on pre-PCI 2.3 devices during AER resume.
--

commit 61aa3eda44efb78a0975edb523d9adc67f239efa
Author: Shameer Kolothum <skolothumtho@nvidia.com>

vfio/pci: Coordinate generic device access with host recovery

This commit wires up error_detected() for generic vfio-pci devices that
have opted into host recovery. It safely coordinates with the host by
blocking device access, draining running requests, revoking mappings,
and stopping bus mastering.

> diff --git a/drivers/vfio/pci/vfio_pci_core.c b/drivers/vfio/pci/vfio_pci_core.c
> index eed0430c32ee6..2d757d6a5fe1b 100644
> --- a/drivers/vfio/pci/vfio_pci_core.c
> +++ b/drivers/vfio/pci/vfio_pci_core.c

[ ... ]

> @@ -2746,14 +2746,178 @@ pci_ers_result_t vfio_pci_core_aer_err_detected(struct pci_dev *pdev,
>  {
>  	struct vfio_pci_core_device *vdev = dev_get_drvdata(&pdev->dev);
>  	struct vfio_pci_eventfd *eventfd;

[ ... ]

> +	ret = pci_write_config_word(pdev, PCI_COMMAND,
> +				    vdev->pci_recovery_command &
> +				    ~PCI_COMMAND_MASTER);
> +	if (ret)
> +		goto out_failed;
> +
> +	vdev->pci_recovery_command_valid = true;

[Severity: Medium]
Will this correctly handle pre-PCI 2.3 devices during AER resume?

Here vdev->pci_recovery_command_valid is set to true for devices even when
the device lacks PCI 2.3 support. Later, during resume, if this flag is true,
it appears vfio_pci_intx_recovery_command() in drivers/vfio/pci/vfio_pci_intrs.c
is invoked and applies the INTX_DISABLE bit unconditionally if the logical
context is masked:

drivers/vfio/pci/vfio_pci_intrs.c:vfio_pci_intx_recovery_command() {
    ...
    if (ctx && ctx->masked)
        command |= PCI_COMMAND_INTX_DISABLE;
    ...
}

Since pre-PCI 2.3 devices do not support INTX_DISABLE, could this write a
reserved bit to the PCI Command register on legacy hardware and cause
undefined behavior?

> +	goto out_memory;
> +
> +out_failed:
> +	WRITE_ONCE(vdev->pci_recovery_flags,
> +		   (vdev->pci_recovery_flags | VFIO_PCI_RECOVERY_FAILED) &
> +		   ~VFIO_PCI_RECOVERY_IN_PROGRESS);

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260901093217.8539-1-skolothumtho@nvidia.com?part=18

[RFC PATCH 19/19] vfio/pci: Expose and enable host PCI error recovery

[Shameer Kolothum]

Add a VFIO device feature that reports whether host PCI recovery is
enabled, whether recovery is in progress, whether the channel was frozen,
whether the host reset the device, and whether recovery failed. A sequence
number lets userspace distinguish events.

Installing a dedicated recovery eventfd enables recovery. The eventfd is
additional to VFIO_PCI_ERR_IRQ_INDEX, which keeps reporting errors as it
does today either way. Recovery can be disabled only when not in progress
and before terminal failure. Reject both enable and disable while device
access is blocked so feature changes cannot race lifecycle teardown or an
explicit reset.

Clear the sequence number and the status bits whichever way the feature is
being changed, so a sequence number always describes an event the eventfd
holding it was notified of. A user which replaces the eventfd would
otherwise read status for an event it never heard about.

Variant drivers return -ENOTTY as they do not advertise recovery support.

Signed-off-by: Shameer Kolothum <skolothumtho@nvidia.com>
---
 include/linux/vfio_pci_core.h    |   1 +
 include/uapi/linux/vfio.h        |  69 +++++++++++++++
 drivers/vfio/pci/vfio_pci_core.c | 142 +++++++++++++++++++++++++++++--
 3 files changed, 206 insertions(+), 6 deletions(-)

diff --git a/include/linux/vfio_pci_core.h b/include/linux/vfio_pci_core.h
index 42a77ed6b93c..fe42089be3fc 100644
--- a/include/linux/vfio_pci_core.h
+++ b/include/linux/vfio_pci_core.h
@@ -145,6 +145,7 @@ struct vfio_pci_core_device {
 	int			ioeventfds_nr;
 	struct vfio_pci_eventfd __rcu *err_trigger;
 	struct vfio_pci_eventfd __rcu *req_trigger;
+	struct vfio_pci_eventfd __rcu *pci_recovery_trigger;
 	struct eventfd_ctx	*pm_wake_eventfd_ctx;
 	struct list_head	dummy_resources_list;
 	struct mutex		ioeventfds_lock;
diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
index e41437fa17ad..ce0cda2dcbbb 100644
--- a/include/uapi/linux/vfio.h
+++ b/include/uapi/linux/vfio.h
@@ -1555,6 +1555,75 @@ struct vfio_device_feature_zpci_err {
 
 #define VFIO_DEVICE_FEATURE_ZPCI_ERROR 13
 
+/*
+ * Report host PCI error recovery state for this device.
+ *
+ * The sequence number is incremented at the start of each event and remains
+ * unchanged for its subsequent state changes. Userspace can therefore
+ * distinguish a new event from completion of the current one and detect
+ * coalesced notifications. It restarts from zero each time recovery is
+ * enabled, so it is only meaningful within one enabled period.
+ *
+ * ENABLED reports that userspace has enabled recovery.
+ * CHANNEL_FROZEN records that recovery started with the PCI channel frozen.
+ * DEVICE_RESET records that the host reset the device. FAILED records that
+ * recovery did not complete successfully. Event status bits remain set after
+ * IN_PROGRESS is cleared. A new event supersedes status from a previous
+ * successful event. FAILED is terminal for the current device open and
+ * remains set until the device is closed and reopened.
+ *
+ * Status bits may also be set while IN_PROGRESS is still set, describing the
+ * event so far. Act on them once IN_PROGRESS is clear. Device access is
+ * refused with -EIO until then.
+ *
+ * When DEVICE_RESET is reported the host reset the device, which tears down
+ * the interrupt configuration the user had established. INTx, MSI and MSI-X
+ * must be re-armed with VFIO_DEVICE_SET_IRQS before interrupts resume.
+ *
+ * VFIO_DEVICE_FEATURE_GET returns the current state and -1 in eventfd. GET is
+ * never refused, including while recovery blocks device access, so that
+ * userspace can read this state during an event. It can wait for a recovery
+ * callback which is already running.
+ *
+ * IN_PROGRESS is not guaranteed to be observable. A recovery which needs no
+ * device reset can complete within microseconds of the notification, before
+ * userspace is scheduled, so a GET which follows the eventfd may already see
+ * IN_PROGRESS clear. Userspace must treat a notification as "an event
+ * occurred" and read the sequence number and the status bits to learn what
+ * happened. It must not wait for IN_PROGRESS to appear set.
+ *
+ * VFIO_DEVICE_FEATURE_SET with a valid eventfd enables recovery
+ * and installs the eventfd as a notification for recovery start and terminal
+ * completion. SET with eventfd -1 disables recovery when none
+ * is in progress and the latest event has not failed. SET returns -EBUSY when
+ * any of those restrictions prevents the requested transition, including while
+ * an explicit VFIO_DEVICE_RESET blocks device access, and -ENODEV if device
+ * close has begun. flags and sequence must be zero for SET.
+ *
+ * This eventfd is separate from VFIO_PCI_ERR_IRQ_INDEX and additional to it.
+ * VFIO_PCI_ERR_IRQ_INDEX keeps reporting errors as it does today whether or
+ * not this feature is enabled, so a user of both receives two notifications
+ * for one event.
+ *
+ * Enabling recovery does not recover an event which is already being handled
+ * for this device. Such an event was declined before it started, so it
+ * completes without notification and without status, even though the host may
+ * reset the device as part of it. Enable recovery before errors occur rather
+ * than in response to one.
+ */
+struct vfio_device_pci_error_recovery {
+	__u32 flags;
+#define VFIO_PCI_ERROR_RECOVERY_IN_PROGRESS	(1U << 0)
+#define VFIO_PCI_ERROR_RECOVERY_CHANNEL_FROZEN	(1U << 1)
+#define VFIO_PCI_ERROR_RECOVERY_DEVICE_RESET	(1U << 2)
+#define VFIO_PCI_ERROR_RECOVERY_FAILED		(1U << 3)
+#define VFIO_PCI_ERROR_RECOVERY_ENABLED		(1U << 4)
+	__s32 eventfd;
+	__aligned_u64 sequence;
+};
+
+#define VFIO_DEVICE_FEATURE_PCI_ERROR_RECOVERY 14
+
 /* -------- API for Type1 VFIO IOMMU -------- */
 
 /**
diff --git a/drivers/vfio/pci/vfio_pci_core.c b/drivers/vfio/pci/vfio_pci_core.c
index 2d757d6a5fe1..c1ea3c868fc5 100644
--- a/drivers/vfio/pci/vfio_pci_core.c
+++ b/drivers/vfio/pci/vfio_pci_core.c
@@ -893,6 +893,10 @@ static void vfio_pci_core_finish_close(struct vfio_device *core_vdev)
 	mutex_lock(&vdev->igate);
 	vfio_pci_eventfd_replace_locked(vdev, &vdev->err_trigger, NULL);
 	vfio_pci_eventfd_replace_locked(vdev, &vdev->req_trigger, NULL);
+	if (vdev->pci_recovery_supported)
+		vfio_pci_eventfd_replace_locked(vdev,
+						&vdev->pci_recovery_trigger,
+						NULL);
 	mutex_unlock(&vdev->igate);
 }
 
@@ -1850,6 +1854,106 @@ static int vfio_pci_core_feature_token(struct vfio_pci_core_device *vdev,
 	return 0;
 }
 
+static int
+vfio_pci_core_feature_error_recovery(struct vfio_pci_core_device *vdev, u32 flags,
+				     struct vfio_device_pci_error_recovery __user *arg,
+				     size_t argsz)
+{
+	struct vfio_device_pci_error_recovery state = { .eventfd = -1 };
+	struct eventfd_ctx *ctx = NULL;
+	bool enable;
+	int ret;
+
+	if (!vdev->pci_recovery_supported)
+		return -ENOTTY;
+
+	ret = vfio_check_feature(flags, argsz,
+				 VFIO_DEVICE_FEATURE_GET |
+				 VFIO_DEVICE_FEATURE_SET, sizeof(state));
+	if (ret != 1)
+		return ret;
+
+	if (flags & VFIO_DEVICE_FEATURE_GET) {
+		down_read(&vdev->recovery_lock);
+		if (vdev->pci_recovery_enabled)
+			state.flags |= VFIO_PCI_ERROR_RECOVERY_ENABLED;
+		if (vdev->pci_recovery_flags & VFIO_PCI_RECOVERY_IN_PROGRESS)
+			state.flags |= VFIO_PCI_ERROR_RECOVERY_IN_PROGRESS;
+		if (vdev->pci_recovery_flags & VFIO_PCI_RECOVERY_FROZEN)
+			state.flags |=
+				VFIO_PCI_ERROR_RECOVERY_CHANNEL_FROZEN;
+		if (vdev->pci_recovery_flags & VFIO_PCI_RECOVERY_RESET)
+			state.flags |= VFIO_PCI_ERROR_RECOVERY_DEVICE_RESET;
+		if (vdev->pci_recovery_flags & VFIO_PCI_RECOVERY_FAILED)
+			state.flags |= VFIO_PCI_ERROR_RECOVERY_FAILED;
+		state.sequence = vdev->pci_recovery_sequence;
+		up_read(&vdev->recovery_lock);
+
+		if (copy_to_user(arg, &state, sizeof(state)))
+			return -EFAULT;
+		return 0;
+	}
+
+	if (copy_from_user(&state, arg, sizeof(state)))
+		return -EFAULT;
+	if (state.flags || state.sequence || state.eventfd < -1)
+		return -EINVAL;
+
+	enable = state.eventfd >= 0;
+	if (enable) {
+		ctx = eventfd_ctx_fdget(state.eventfd);
+		if (IS_ERR(ctx))
+			return PTR_ERR(ctx);
+	}
+
+	down_write(&vdev->recovery_lock);
+	if (!vdev->pci_recovery_device_open) {
+		ret = -ENODEV;
+		goto out_unlock;
+	}
+	if (vdev->pci_recovery_access_blocked) {
+		ret = -EBUSY;
+		goto out_unlock;
+	}
+
+	if (!enable &&
+	    (vdev->pci_recovery_flags &
+	     (VFIO_PCI_RECOVERY_IN_PROGRESS | VFIO_PCI_RECOVERY_FAILED))) {
+		ret = -EBUSY;
+		goto out_unlock;
+	}
+
+	mutex_lock(&vdev->igate);
+	ret = vfio_pci_eventfd_replace_locked(vdev,
+					      &vdev->pci_recovery_trigger,
+					      ctx);
+	mutex_unlock(&vdev->igate);
+	if (ret)
+		goto out_unlock;
+
+	WRITE_ONCE(vdev->pci_recovery_enabled, enable);
+	/*
+	 * Start each enabled period from a clear state, so a sequence number
+	 * and the status bits beside it always describe an event this
+	 * eventfd was notified of. Nothing is in flight to lose. A
+	 * transaction holds access_blocked, which failed this call with
+	 * -EBUSY above.
+	 *
+	 * access_blocked itself is not cleared here, so userspace can never
+	 * disable its way out of a block.
+	 */
+	WRITE_ONCE(vdev->pci_recovery_flags, 0);
+	vdev->pci_recovery_sequence = 0;
+	vdev->pci_recovery_command_valid = false;
+
+out_unlock:
+	up_write(&vdev->recovery_lock);
+	if (ret && ctx)
+		eventfd_ctx_put(ctx);
+
+	return ret;
+}
+
 int vfio_pci_core_ioctl_feature(struct vfio_device *device, u32 flags,
 				void __user *arg, size_t argsz)
 {
@@ -1870,6 +1974,9 @@ int vfio_pci_core_ioctl_feature(struct vfio_device *device, u32 flags,
 		return vfio_pci_core_feature_dma_buf(vdev, flags, arg, argsz);
 	case VFIO_DEVICE_FEATURE_ZPCI_ERROR:
 		return vfio_pci_zdev_feature_err(device, flags, arg, argsz);
+
+	case VFIO_DEVICE_FEATURE_PCI_ERROR_RECOVERY:
+		return vfio_pci_core_feature_error_recovery(vdev, flags, arg, argsz);
 	default:
 		return -ENOTTY;
 	}
@@ -2741,6 +2848,18 @@ void vfio_pci_core_unregister_device(struct vfio_pci_core_device *vdev)
 }
 EXPORT_SYMBOL_GPL(vfio_pci_core_unregister_device);
 
+static void
+vfio_pci_signal_recovery_event(struct vfio_pci_core_device *vdev)
+{
+	struct vfio_pci_eventfd *eventfd;
+
+	rcu_read_lock();
+	eventfd = rcu_dereference(vdev->pci_recovery_trigger);
+	if (eventfd)
+		eventfd_signal(eventfd->ctx);
+	rcu_read_unlock();
+}
+
 pci_ers_result_t vfio_pci_core_aer_err_detected(struct pci_dev *pdev,
 						pci_channel_state_t state)
 {
@@ -2748,6 +2867,7 @@ pci_ers_result_t vfio_pci_core_aer_err_detected(struct pci_dev *pdev,
 	struct vfio_pci_eventfd *eventfd;
 	pci_ers_result_t result = PCI_ERS_RESULT_CAN_RECOVER;
 	unsigned long irq_flags;
+	bool notify_recovery = false;
 	bool terminal = false;
 	bool nested;
 	u32 flags;
@@ -2792,6 +2912,7 @@ pci_ers_result_t vfio_pci_core_aer_err_detected(struct pci_dev *pdev,
 		goto out_unlock;
 	}
 
+	notify_recovery = true;
 	WRITE_ONCE(vdev->pci_recovery_access_blocked, true);
 	/*
 	 * A second event before resume() has finished the first joins the
@@ -2916,6 +3037,8 @@ pci_ers_result_t vfio_pci_core_aer_err_detected(struct pci_dev *pdev,
 	if (eventfd)
 		eventfd_signal(eventfd->ctx);
 	rcu_read_unlock();
+	if (notify_recovery)
+		vfio_pci_signal_recovery_event(vdev);
 
 	return result;
 }
@@ -2981,13 +3104,15 @@ static pci_ers_result_t vfio_pci_core_aer_slot_reset(struct pci_dev *pdev)
 
 	up_write(&vdev->recovery_lock);
 	/*
-	 * Whoever clears IN_PROGRESS owes the wake. resume() will not do it,
-	 * since it bails once IN_PROGRESS is clear, and the core skips it
-	 * altogether if the domain verdict is not RECOVERED. On success the
-	 * transaction carries on and resume() wakes.
+	 * Whoever clears IN_PROGRESS owes the wake and the event. resume()
+	 * will not do it, since it bails once IN_PROGRESS is clear, and the
+	 * core skips it altogether if the domain verdict is not RECOVERED.
+	 * On success the transaction carries on and resume() does both.
 	 */
-	if (ret)
+	if (ret) {
 		wake_up_all(&vdev->pci_recovery_wait);
+		vfio_pci_signal_recovery_event(vdev);
+	}
 
 	return result;
 }
@@ -2996,6 +3121,7 @@ static void vfio_pci_core_aer_resume(struct pci_dev *pdev)
 {
 	struct vfio_pci_core_device *vdev = dev_get_drvdata(&pdev->dev);
 	unsigned long irq_flags;
+	bool notify_recovery = false;
 	u32 flags;
 	int ret = 0;
 
@@ -3011,6 +3137,7 @@ static void vfio_pci_core_aer_resume(struct pci_dev *pdev)
 	if (!(vdev->pci_recovery_flags & VFIO_PCI_RECOVERY_IN_PROGRESS))
 		goto out_unlock;
 
+	notify_recovery = true;
 	if (!vdev->pci_recovery_device_open) {
 		vdev->pci_recovery_command_valid = false;
 		WRITE_ONCE(vdev->pci_recovery_flags,
@@ -3056,7 +3183,10 @@ static void vfio_pci_core_aer_resume(struct pci_dev *pdev)
 
 out_unlock:
 	up_write(&vdev->recovery_lock);
-	wake_up_all(&vdev->pci_recovery_wait);
+	if (notify_recovery) {
+		wake_up_all(&vdev->pci_recovery_wait);
+		vfio_pci_signal_recovery_event(vdev);
+	}
 }
 
 int vfio_pci_core_sriov_configure(struct vfio_pci_core_device *vdev,
-- 
2.43.0

Re: [RFC PATCH 19/19] vfio/pci: Expose and enable host PCI error recovery

[sashiko-bot]

> Add a VFIO device feature that reports whether host PCI recovery is
> enabled, whether recovery is in progress, whether the channel was frozen,
> whether the host reset the device, and whether recovery failed. A sequence
> number lets userspace distinguish events.
> 
> Installing a dedicated recovery eventfd enables recovery. The eventfd is
> additional to VFIO_PCI_ERR_IRQ_INDEX, which keeps reporting errors as it
> does today either way. Recovery can be disabled only when not in progress
> and before terminal failure. Reject both enable and disable while device
> access is blocked so feature changes cannot race lifecycle teardown or an
> explicit reset.
> 
> Clear the sequence number and the status bits whichever way the feature is
> being changed, so a sequence number always describes an event the eventfd
> holding it was notified of. A user which replaces the eventfd would
> otherwise read status for an event it never heard about.
> 
> Variant drivers return -ENOTTY as they do not advertise recovery support.
> 
> Signed-off-by: Shameer Kolothum <skolothumtho@nvidia.com>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260901093217.8539-1-skolothumtho@nvidia.com?part=19

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

[Alex Williamson]

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