RE: [PATCH v4 15/27] vfio/pci: Call CXL open and close hooks around device use

[Manish Honap]

> -----Original Message-----
> From: Alex Williamson <alex@xxxxxxxxxxx>
> Sent: Friday, August 28, 2026 4:33 AM
> To: Manish Honap <mhonap@xxxxxxxxxx>
> Cc: jgg@xxxxxxxx; Ankit Agrawal <ankita@xxxxxxxxxx>; jic23@xxxxxxxxxx;
> dave.jiang@xxxxxxxxx; alejandro.lucero-palau@xxxxxxx; Srirangan Madhavan
> <smadhavan@xxxxxxxxxx>; corbet@xxxxxxx; skhan@xxxxxxxxxxxxxxxxxxx;
> dave@xxxxxxxxxxxx; alison.schofield@xxxxxxxxx; vishal.l.verma@xxxxxxxxx;
> iweiny@xxxxxxxxxx; ming.li@xxxxxxxxxxxx; Yishai Hadas
> <yishaih@xxxxxxxxxx>; Shameer Kolothum Thodi
> <skolothumtho@xxxxxxxxxx>; kevin.tian@xxxxxxxxx; bhelgaas@xxxxxxxxxx;
> dmatlack@xxxxxxxxxx; kees@xxxxxxxxxx; gustavoars@xxxxxxxxxx; Neo Jia
> <cjia@xxxxxxxxxx>; Krishnakant Jaju <kjaju@xxxxxxxxxx>; Vikram Sethi
> <vsethi@xxxxxxxxxx>; Zhi Wang <zhiw@xxxxxxxxxx>; linux-
> doc@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; kvm@xxxxxxxxxxxxxxx;
> linux-cxl@xxxxxxxxxxxxxxx; linux-pci@xxxxxxxxxxxxxxx; linux-
> kselftest@xxxxxxxxxxxxxxx; linux-hardening@xxxxxxxxxxxxxxx; alex@xxxxxxxxxxx
> Subject: Re: [PATCH v4 15/27] vfio/pci: Call CXL open and close hooks around
> device use
> 
> External email: Use caution opening links or attachments
> 
> 
> On Thu, 13 Aug 2026 15:06:19 +0530
> <mhonap@xxxxxxxxxx> wrote:
> 
> > From: Manish Honap <mhonap@xxxxxxxxxx>
> >
> > Volatile CXL register state must be sampled from live hardware when
> > the guest opens the device, not at bind, because a low-power
> > transition between bind and open can leave bind-time values stale.
> 
> But patch 7 prevented such transitions, without really an explanation why.
> Accounting for being in full power state at open here further diminishes the
> requirement to restrict idle lower power states in that patch.
> 
> > Add open_device and close_device to the CXL ops and call them from the
> > common enable and close paths. A failed open unwinds the enable like
> > any other error. The callbacks are stubs here and are filled in next.
> >
> > Signed-off-by: Manish Honap <mhonap@xxxxxxxxxx>
> > ---
> >  drivers/vfio/pci/cxl/vfio_cxl_core.c | 11 +++++++
> >  drivers/vfio/pci/vfio_pci_core.c     | 44 ++++++++++++++++++++++------
> >  include/linux/vfio_pci_core.h        |  2 ++
> >  3 files changed, 48 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/vfio/pci/cxl/vfio_cxl_core.c
> > b/drivers/vfio/pci/cxl/vfio_cxl_core.c
> > index 966b7c22a6ac..d19fd638f538 100644
> > --- a/drivers/vfio/pci/cxl/vfio_cxl_core.c
> > +++ b/drivers/vfio/pci/cxl/vfio_cxl_core.c
> > @@ -100,9 +100,20 @@ static void vfio_cxl_release_device(struct
> vfio_pci_core_device *vdev)
> >       vdev->cxl = NULL;
> >  }
> >
> > +static int vfio_cxl_open_device(struct vfio_pci_core_device *vdev) {
> > +     return 0;
> > +}
> > +
> > +static void vfio_cxl_close_device(struct vfio_pci_core_device *vdev)
> > +{ }
> > +
> >  static const struct vfio_cxl_ops vfio_cxl_ops = {
> >       .init_device    = vfio_cxl_init_device,
> >       .release_device = vfio_cxl_release_device,
> > +     .open_device    = vfio_cxl_open_device,
> > +     .close_device   = vfio_cxl_close_device,
> >       .owner          = THIS_MODULE,
> >  };
> >
> > diff --git a/drivers/vfio/pci/vfio_pci_core.c
> > b/drivers/vfio/pci/vfio_pci_core.c
> > index 49dfbdaf3f05..470730cdc88b 100644
> > --- a/drivers/vfio/pci/vfio_pci_core.c
> > +++ b/drivers/vfio/pci/vfio_pci_core.c
> > @@ -584,6 +584,21 @@ static const struct dev_pm_ops
> vfio_pci_core_pm_ops = {
> >                          NULL)
> >  };
> >
> > +static void vfio_pci_core_unmap_bars(struct vfio_pci_core_device
> > +*vdev) {
> > +     struct pci_dev *pdev = vdev->pdev;
> > +     int i, bar;
> > +
> > +     for (i = 0; i < PCI_STD_NUM_BARS; i++) {
> > +             bar = i + PCI_STD_RESOURCES;
> > +             if (IS_ERR_OR_NULL(vdev->barmap[bar]))
> > +                     continue;
> > +             pci_iounmap(pdev, vdev->barmap[bar]);
> > +             pci_release_selected_regions(pdev, 1 << bar);
> > +             vdev->barmap[bar] = NULL;
> > +     }
> > +}
> > +
> >  int vfio_pci_core_enable(struct vfio_pci_core_device *vdev)  {
> >       struct pci_dev *pdev = vdev->pdev; @@ -660,8 +675,23 @@ int
> > vfio_pci_core_enable(struct vfio_pci_core_device *vdev)
> >
> >       vfio_pci_core_map_bars(vdev);
> >
> > +     if (vdev->cxl_ops) {
> > +             ret = vdev->cxl_ops->open_device(vdev);
> > +             if (ret)
> > +                     goto out_free_config;
> > +     }
> > +
> >       return 0;
> >
> > +out_free_config:
> > +     /*
> > +      * open_device() runs after vfio_config_init() and map_bars() have
> > +      * succeeded, but a failed first open never reaches
> vfio_pci_core_disable().
> > +      * Unwind the common vconfig and BAR state here so the allocations and
> BAR
> > +      * requests are not leaked for a later open to overwrite.
> > +      */
> > +     vfio_config_free(vdev);
> > +     vfio_pci_core_unmap_bars(vdev);
> 
> Unwind in the reverse order, this should start with unmap bars.  The above
> comment is rather gratuitous and looks a bit like AI slop.
> Thanks,
> 
> Alex

Okay, I will resolve this as suggested.

Manish

> 
> >  out_free_zdev:
> >       vfio_pci_zdev_close_device(vdev);
> >  out_free_state:
> > @@ -682,7 +712,7 @@ void vfio_pci_core_disable(struct
> vfio_pci_core_device *vdev)
> >       struct pci_dev *pdev = vdev->pdev;
> >       struct vfio_pci_dummy_resource *dummy_res, *tmp;
> >       struct vfio_pci_ioeventfd *ioeventfd, *ioeventfd_tmp;
> > -     int i, bar;
> > +     int i;
> >
> >       /* For needs_reset */
> >       lockdep_assert_held(&vdev->vdev.dev_set->lock);
> > @@ -737,14 +767,7 @@ void vfio_pci_core_disable(struct
> > vfio_pci_core_device *vdev)
> >
> >       vfio_config_free(vdev);
> >
> > -     for (i = 0; i < PCI_STD_NUM_BARS; i++) {
> > -             bar = i + PCI_STD_RESOURCES;
> > -             if (IS_ERR_OR_NULL(vdev->barmap[bar]))
> > -                     continue;
> > -             pci_iounmap(pdev, vdev->barmap[bar]);
> > -             pci_release_selected_regions(pdev, 1 << bar);
> > -             vdev->barmap[bar] = NULL;
> > -     }
> > +     vfio_pci_core_unmap_bars(vdev);
> >
> >       list_for_each_entry_safe(dummy_res, tmp,
> >                                &vdev->dummy_resources_list, res_next)
> > { @@ -827,6 +850,9 @@ void vfio_pci_core_close_device(struct
> > vfio_device *core_vdev)  #endif
> >       vfio_pci_dma_buf_cleanup(vdev);
> >
> > +     if (vdev->cxl_ops)
> > +             vdev->cxl_ops->close_device(vdev);
> > +
> >       vfio_pci_core_disable(vdev);
> >
> >       mutex_lock(&vdev->igate);
> > diff --git a/include/linux/vfio_pci_core.h
> > b/include/linux/vfio_pci_core.h index 43755b91880f..7354dae1dd85
> > 100644
> > --- a/include/linux/vfio_pci_core.h
> > +++ b/include/linux/vfio_pci_core.h
> > @@ -70,6 +70,8 @@ struct vfio_pci_device_ops {  struct vfio_cxl_ops {
> >       int     (*init_device)(struct vfio_pci_core_device *vdev);
> >       void    (*release_device)(struct vfio_pci_core_device *vdev);
> > +     int     (*open_device)(struct vfio_pci_core_device *vdev);
> > +     void    (*close_device)(struct vfio_pci_core_device *vdev);
> >       /* Pinned per bound CXL device so vfio-cxl cannot unload under usage */
> >       struct module *owner;
> >  };