RE: [PATCH v3 07/11] vfio/pci: Add CONFIG_VFIO_PCI_CXL with bind-time CXL Type-2 acquisition

> -----Original Message-----
> From: Alex Williamson <[email protected]>
> Sent: 11 July 2026 03:53
> To: Manish Honap <[email protected]>
> Cc: [email protected]; [email protected]; [email protected];
> [email protected]; Ankit Agrawal <[email protected]>;
> [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; Neo Jia <[email protected]>; Krishnakant Jaju
> <[email protected]>; Vikram Sethi <[email protected]>; Zhi Wang
> <[email protected]>; [email protected]; [email protected];
> [email protected]; [email protected]; linux-
> [email protected]; [email protected]
> Subject: Re: [PATCH v3 07/11] vfio/pci: Add CONFIG_VFIO_PCI_CXL with
> bind-time CXL Type-2 acquisition
> 
> External email: Use caution opening links or attachments
> 
> 
> On Thu, 25 Jun 2026 22:24:03 +0530
> <[email protected]> wrote:
> 
> > From: Manish Honap <[email protected]>
> >
> > Wire vfio-pci-core to acquire CXL Type-2 device state at PCI bind and
> > release it at PCI unbind, mirroring the existing vfio_pci_zdev_*
> > integration model.
> 
> I don't follow, zpci has the following matching hooks:
> 
>  - vfio_pci_zdev_open_device
>  - vfio_pci_zdev_close_device
> 
> Plus it also has:
> 
>  - vfio_pci_info_zdev_add_caps
> 
> What about those suggest "zdev state" is acquired at bind and released
> on unbind?
> 
> >  Four lifecycle hooks are introduced — vfio_pci_cxl_acquire / _release
> > / _open / _close — with !-config stubs that return -ENODEV / 0 / 0 /
> > no-op respectively so vfio-pci behaviour is unchanged when
> > CONFIG_VFIO_PCI_CXL=n.
> >
> > vfio_pci_cxl_acquire() implements the bind sequence:
> >
> >   - pcie_is_cxl() and CXL Device DVSEC discovery (-ENODEV if absent
> >     or if MEM_CAPABLE clear — caller falls back to plain vfio-pci)
> >   - devm_cxl_dev_state_create() with struct vfio_pci_cxl_state
> >     embedding cxl_dev_state at offset 0 (required by the 7-arg
> >     macro's static_assert in include/cxl/cxl.h)
> >   - pci_enable_device_mem(), cxl_pci_setup_regs(), cxl_get_hdm_info()
> >     (rejecting hdm_count != 1), cxl_regblock_get_bar_info(),
> >     cxl_await_range_active()
> 
> The cover letter claims otherwise:
> 
>   "- cxl_await_range_active stays in cxl-core probe; not exported, vfio
>   does not call it."
> 
> It's exported in 2/ and called below.
> 
> >   - devm_cxl_passthrough_create() to snapshot the DVSEC body, HDM
> >     block, and CM cap-array shadows owned by cxl-core
> >   - pci_disable_device() — clears PCI_COMMAND_MASTER but NOT
> >     PCI_COMMAND_MEMORY, so cxl-core MMIO accesses from the next step
> >     still succeed
> >   - devm_cxl_probe_mem() to register the cxl_memdev, enumerate the
> >     endpoint port, and attach the firmware-committed autoregion
> >   - request_mem_region() + memremap_wb() of the autoregion's HPA so
> >     the HDM VFIO region can serve guest accesses through it
> 
> How does this interact with:
> 
>  - The device making use of low power states while idle
>  - Repeatability per tenant instance
>  - Protection of tenant data per instance
> 
> The culmination of all of these, plus the basic housekeeping of
> maintaining the lightest touch on the device, including keeping the
> device in the minimum state of functionality outside of an actual user,
> is why I would expect to perform acquire/release as part of open/close.

The cover letter description is wrong. During my earlier thought process,
creating a region during probe seemed a correct option as any later requirement
where region needs to be already created at probe emerged, we will have easier
way to handle it. I will give some more thought to update v4 to acquire
CXL state in open_device() and release it in close_device(), matching zpci.

> 
> Could a low power transition invalidate the state established by
> acquire, leading to the issue Richard encountered?

okay, I will take care of this part. Since vfio-cxl-reset support was not
present in this series, I was testing with reset mechanism commented out
in vfio layer. I will incorporate reset support in next series and test this
case before posting the v4.

> 
> Also, on the direct calls to cxl functions, I thought one of our goals
> was to avoid vfio-pci statically pulling in CXL module dependencies.
> To achieve that, it seems like at some point we need to detect that we
> have a CXL device (pcie_is_cxl(pdev)), do a request_module() to load
> vfio-cxl, where the init function would register callback ops with vfio-
> pci-core and each dependent device would acquire a reference to the
> vfio-cxl module.
> 

okay, I will add a separate vfio-cxl.ko. The user will never mention vfio-cxl.
vfio-cxl.ko just needs to be loaded before the VFIO fd is
opened. Rest of the sequence will be as suggested: vfio-pci-core sees
pcie_is_cxl(pdev) == true, do a request_module for vfio-cxl...

If vfio-cxl.ko is not loaded, the device opens as a plain PCI device with
no CXL capabilities.

So, the "enlighten vfio-pci about CXL" goal is still fully intact: the
CXL-aware code is in the vfio tree (drivers/vfio/pci/cxl/vfio.c), owned
by vfio but invoked by vfio-pci-core. The device binding model will still
be same as earlier (bind to vfio-pci)

> > diff --git a/include/linux/vfio_pci_core.h
> > b/include/linux/vfio_pci_core.h index 89165b769e5c..541c1911e090
> > 100644
> > --- a/include/linux/vfio_pci_core.h
> > +++ b/include/linux/vfio_pci_core.h
> > @@ -142,6 +142,13 @@ struct vfio_pci_core_device {
> >       struct notifier_block   nb;
> >       struct rw_semaphore     memory_lock;
> >       struct list_head        dmabufs;
> > +     /*
> > +      * Opaque pointer to struct vfio_pci_cxl_state (defined in
> > +      * drivers/vfio/pci/cxl/vfio_cxl_priv.h).  Set by
> > +      * vfio_pci_cxl_acquire() at PCI bind; NULL on non-CXL devices
> > +      * and when CONFIG_VFIO_PCI_CXL=n.
> > +      */
> > +     void                    *cxl;
> 
> Use a forward declaration rather than void, that avoids half your
> comment.  The remainder of the comment is just explaining the obvious
> parts of the code, unnecessary.  Thanks,

okay; I will rectify this.

> 
> Alex