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

["K V P, Satyanarayana" <satyanarayana.k.v.p@intel.com>]

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",