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@xxxxxxxxxx>
> ---
>   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(