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@xxxxxxxxx>
> Sent: 03 September 2026 07:43
> To: Shameer Kolothum Thodi <skolothumtho@xxxxxxxxxx>;
> kvm@xxxxxxxxxxxxxxx; linux-pci@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx
> Cc: alex@xxxxxxxxxxx; jgg@xxxxxxxx; kevin.tian@xxxxxxxxx;
> kbusch@xxxxxxxx; michal.winiarski@xxxxxxxxx; Sonang Patel
> <sonangp@xxxxxxxxxx>; Nathan Chen <nathanc@xxxxxxxxxx>; Matt Ochs
> <mochs@xxxxxxxxxx>
> 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@xxxxxxxxxx>
> > ---
> >   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