> -----Original Message-----
> From: Jonathan Cameron <[email protected]>
> Sent: Saturday, September 26, 2026 3:46 AM
> To: Manish Honap <[email protected]>
> Cc: [email protected]; [email protected]; Ankit Agrawal <[email protected]>;
> [email protected]; [email protected]; Srirangan
> Madhavan <[email protected]>; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected]; Yishai
> Hadas <[email protected]>; Shameer Kolothum Thodi
> <[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]>; linux-
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; linux-
> [email protected]; [email protected]
> Subject: Re: [PATCH v5 11/27] vfio/pci: Virtualize the CXL DVSEC in
> vfio_pci_config.c
>
> External email: Use caution opening links or attachments
>
>
> On Thu, 17 Sep 2026 00:05:24 +0530
> <[email protected]> wrote:
>
> > From: Manish Honap <[email protected]>
> >
> > A CXL Type-2 device is reprogrammable through its CXL DVSEC: a guest
> > could set Config Lock, toggle CXL.cache and CXL.mem enable, or rewrite
> > the HDM range registers that govern host memory decode. Virtualize the
> > DVSEC so the guest sees a shadow it cannot use to reprogram the hardware.
> >
> > Build a per-device cxl_perm permission map, modeled on msi_perm, when
> > the device is bound through the CXL provider (e.g. vfio-cxl). The
> > whole CXL DVSEC is served from the vconfig shadow; only Control and
> > Control2 are guest programmable, while Capability, Status, Lock and
> > the Range registers keep their firmware snapshot. A vendor DVSEC on
> > the same device is
> > unaffected: the map is selected only for the CXL DVSEC offset.
> >
> > Control2 carries the CXL reset and cache write-back-invalidate
> > initiate bits as self-clearing doorbells. vfio never forwards them to
> > hardware, so a custom writefn synthesizes their completion in the
> > shadow: the initiate bit self-clears and the matching Status2 bit
> > (Cache Invalid or Reset Done) is set, so a guest following the spec
> > reset sequence (INIT_CACHE_WBI, poll Cache Invalid, INIT_CXL_RST, poll
> > Reset Done) progresses instead of timing out. The host performs the
> > real cache write-back and reset at the vfio reset points.
> >
> > Assisted-by: LLM
> > Signed-off-by: Manish Honap <[email protected]>
> A few minor comments inline.
>
> J
> > ---
> > drivers/vfio/pci/vfio_pci_config.c | 132
> ++++++++++++++++++++++++++++-
> > include/linux/vfio_pci_core.h | 3 +
> > 2 files changed, 133 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/vfio/pci/vfio_pci_config.c
> > b/drivers/vfio/pci/vfio_pci_config.c
> > index 9914f3ac69ae..9a020a768055 100644
> > --- a/drivers/vfio/pci/vfio_pci_config.c
> > +++ b/drivers/vfio/pci/vfio_pci_config.c
>
> > +
> > +static int init_cxl_dvsec_perm(struct perm_bits *perm, int len) {
> > + int i;
> > +
> > + if (alloc_perm_bits(perm, len))
> > + return -ENOMEM;
> > +
> > + perm->writefn = vfio_cxl_dvsec_write;
> > +
> > + /* Serve the whole CXL DVSEC from the shadow. */
> > + for (i = 0; i < len; i++)
> for (int i = 0;
>
> Is mostly acceptable in the kernel these days and keeps the scope tightly
> defined.
Okay, I will add this.
>
> > + p_setb(perm, i, (u8)ALL_VIRT, NO_WRITE);
> > +
> > + /*
> > + * Control and Control2 are guest programmable; Capability, Status,
> > + * Lock and the Range registers keep their firmware snapshot, so the
> > + * guest cannot set Config Lock or rewrite the capability and ranges.
> > + */
> > + p_setw(perm, PCI_DVSEC_CXL_CTRL, (u16)ALL_VIRT, (u16)ALL_WRITE);
> > + p_setw(perm, PCI_DVSEC_CXL_CTRL2, (u16)ALL_VIRT,
> > + (u16)ALL_WRITE);
> > +
> > + return 0;
> > +}
> > +
> > +/* Virtualize the CXL DVSEC so a guest cannot reprogram the device
> > +through it. */ static int vfio_cxl_dvsec_init(struct
> > +vfio_pci_core_device *vdev) {
> > + struct pci_dev *pdev = vdev->pdev;
> > + u32 dword;
> > + u16 dvsec;
> > + int len, ret;
> > +
> > + dvsec = pci_find_dvsec_capability(pdev, PCI_VENDOR_ID_CXL,
> > + PCI_DVSEC_CXL_DEVICE);
> > + if (!dvsec)
> > + return 0;
> > +
> > + ret = pci_read_config_dword(pdev, dvsec + PCI_DVSEC_HEADER1,
> &dword);
> > + if (ret)
> > + return pcibios_err_to_errno(ret);
> > + len = PCI_DVSEC_HEADER1_LEN(dword);
> > +
> > + /*
> > + * The virtualization writes fixed DVSEC offsets up to Status2 (the
> > reset
> > + * doorbell stamps it). A device that reports a shorter DVSEC is not a
> > + * usable Type-2 function; leave it as plain vfio-pci rather than
> > index the
> > + * device-length-sized perm allocation past its end.
> > + */
> > + if (len < PCI_DVSEC_CXL_STATUS2 + 2)
> > + return 0;
> > +
> > + vdev->cxl_perm = kmalloc_obj(struct perm_bits,
> > + GFP_KERNEL_ACCOUNT);
>
> I'd use *vdev->cxl_perm instead of struct perm_bits just because that saves
> anyone checking types.
Okay, I will update this in v6.
>
> > + if (!vdev->cxl_perm)
> > + return -ENOMEM;
> > +
> > + ret = init_cxl_dvsec_perm(vdev->cxl_perm, len);
> > + if (ret) {
> > + kfree(vdev->cxl_perm);
> > + vdev->cxl_perm = NULL;
> > + return ret;
> > + }
> > +
> > + vdev->cxl_dvsec = dvsec;
> > + vdev->cxl_dvsec_len = len;
> > +
> > + return 0;
> > +}
> > +
> > int vfio_config_init(struct vfio_pci_core_device *vdev) {
> > struct pci_dev *pdev = vdev->pdev; @@ -1842,6 +1955,12 @@ int
> > vfio_config_init(struct vfio_pci_core_device *vdev)
> > if (ret)
> > goto out;
> >
> > + if (vdev->cxl_ops) {
> > + ret = vfio_cxl_dvsec_init(vdev);
> > + if (ret)
> > + goto out;
> > + }
> > +
> > return 0;
> >
> > out:
> > @@ -1863,6 +1982,12 @@ void vfio_config_free(struct
> vfio_pci_core_device *vdev)
> > kfree(vdev->msi_perm);
> > vdev->msi_perm = NULL;
> > }
> > + if (vdev->cxl_perm) {
> > + free_perm_bits(vdev->cxl_perm);
> > + kfree(vdev->cxl_perm);
> > + vdev->cxl_perm = NULL;
> > + vdev->cxl_dvsec = 0;
>
> I'd define a vfio_cxl_dvsec_exit() or _unint() for this so it is clear it
> pairs with
> vfio_cxl_dvsec_init() above.
Ack. Alex pointed out that cxl_dvsec_len is unused, so the exit helper clears
only
cxl_perm and cxl_dvsec.
>
>
> > + }
> > }
> >
> > /*
> > @@ -1926,12 +2051,15 @@ ssize_t vfio_pci_config_rw_single(struct
> vfio_pci_core_device *vdev,
> > * of the extended capability list. Use default, ro
> > * access, which will virtualize the id and next
> > values.
> > */
> > + cap_start = vfio_find_cap_start(vdev, *ppos);
> > +
> > if (cap_id > PCI_EXT_CAP_ID_MAX)
> > perm = &direct_ro_perms;
> > + else if (cap_id == PCI_EXT_CAP_ID_DVSEC &&
> > vdev->cxl_perm
> &&
> > + cap_start == vdev->cxl_dvsec)
>
> If it is only used here (I haven't read on in series)
> vfio_find_cap_start(vdev, *ppos) ==
> vdev->cxl_dvsec) seems
> fine to me. It's only just over 80 chars and hopeful this bit of the kernel
> is
> flexible on that!
cap_start is also used after the branch for the offset into the capability.
I will keep the variable in v6.
>
> > + perm = vdev->cxl_perm;
> > else
> > perm = &ecap_perms[cap_id];
> > -
> > - cap_start = vfio_find_cap_start(vdev, *ppos);
> > } else {
> > WARN_ON(cap_id > PCI_CAP_ID_MAX);
> >
