RE: [PATCH v5 11/27] vfio/pci: Virtualize the CXL DVSEC in vfio_pci_config.c

[Manish Honap]

> -----Original Message-----
> From: Jonathan Cameron <jic23@xxxxxxxxxx>
> Sent: Saturday, September 26, 2026 3:46 AM
> To: Manish Honap <mhonap@xxxxxxxxxx>
> Cc: alex@xxxxxxxxxxx; jgg@xxxxxxxx; Ankit Agrawal <ankita@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
> 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
> <mhonap@xxxxxxxxxx> wrote:
> 
> > From: Manish Honap <mhonap@xxxxxxxxxx>
> >
> > 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 <mhonap@xxxxxxxxxx>
> 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);
> >