RE: [PATCH v4 17/27] vfio/cxl: Virtualize the CXL DVSEC

[Manish Honap]

> -----Original Message-----
> From: Alex Williamson <alex@xxxxxxxxxxx>
> Sent: Friday, August 28, 2026 10:10 PM
> 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 17/27] vfio/cxl: Virtualize the CXL DVSEC
> 
> External email: Use caution opening links or attachments
> 
> 
> On Thu, 13 Aug 2026 15:06:21 +0530
> <mhonap@xxxxxxxxxx> wrote:
> 
> > From: Manish Honap <mhonap@xxxxxxxxxx>
> >
> > Serve reads of the CXL DVSEC body from the per-open shadow and keep
> > guest writes in the shadow rather than letting them reach the
> > hardware, so a guest cannot reprogram the device through the DVSEC.
> > Accesses outside the CXL DVSEC return -ENODEV and take the default
> > DVSEC handling, so a device that also exposes a vendor DVSEC is unaffected.
> >
> > Route each shadow write through the CXL r4.0 field class rather than
> > storing it verbatim: Control stays programmable, Status is
> > write-1-to-clear, and Capability, Lock and the Range registers keep
> > their firmware snapshot. The guest can no longer set Config Lock or
> > scribble the capability and range fields.
> >
> > Signed-off-by: Manish Honap <mhonap@xxxxxxxxxx>
> > ---
> >  drivers/vfio/pci/cxl/vfio_cxl_core.c | 88 ++++++++++++++++++++++++++++
> >  drivers/vfio/pci/vfio_pci_config.c   | 36 +++++++++++-
> >  include/linux/vfio_pci_core.h        |  5 ++
> >  include/uapi/linux/pci_regs.h        |  1 +
> >  4 files changed, 129 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/vfio/pci/cxl/vfio_cxl_core.c
> > b/drivers/vfio/pci/cxl/vfio_cxl_core.c
> > index 2e516a0929c6..9fed909cb9d3 100644
> > --- a/drivers/vfio/pci/cxl/vfio_cxl_core.c
> > +++ b/drivers/vfio/pci/cxl/vfio_cxl_core.c
> > @@ -148,11 +148,99 @@ static void vfio_cxl_close_device(struct
> vfio_pci_core_device *vdev)
> >       cxl->dvsec_shadow = NULL;
> >  }
> >
> > +/* Read a 16-bit DVSEC field from the shadow; @off is DVSEC-relative.
> > +*/ static u16 vfio_cxl_dvsec16(struct vfio_cxl_state *cxl, u32 off) {
> > +     u32 dw = cxl->dvsec_shadow[off / sizeof(u32)];
> > +
> > +     return (dw >> (8 * (off % sizeof(u32)))) & 0xffff; }
> 
> I don't understand the contract this function is providing.  It claims to read a
> 16-bit field, but an odd offset either crosses between 16-bit fields or drops the
> upper byte.  The contract would be more clear as:
> 
>         return (off & 2) ? (dw >> 16) : (dw & 0xffff);
> 
> The comment could also specify _aligned_ 16-bit fields.
> 
> > +
> > +/*
> > + * Apply the CXL r4.0 8.1.3 write class for the 16-bit DVSEC register at @off.
> > + * Control is programmable, Status is write-1-to-clear, and
> > +Capability, Lock and
> > + * the Range registers stay fixed at their firmware snapshot.
> > + */
> > +static u16 vfio_cxl_dvsec_field(u32 off, u16 old, u16 wval, u16
> > +wmask) {
> > +     switch (off) {
> > +     case PCI_DVSEC_CXL_CTRL:
> > +             /*
> > +              * CXL.mem stays enabled for as long as the guest owns the device.
> > +              * The HDM decoder maps the guest window to device memory, so a
> > +              * store to it while CXL.mem is disabled completes on the device as
> > +              * an error that the host fabric reports as an SError, which is
> > +              * fatal. The spec does not pin down accesses to a decoder whose
> > +              * CXL.mem is off and many hosts SError, so ignore a guest request
> > +              * to clear the enable and keep the bit set.
> > +              */
> > +             return ((old & ~wmask) | (wval & wmask)) |
> PCI_DVSEC_CXL_MEM_ENABLE;
> > +     case PCI_DVSEC_CXL_CTRL2:
> > +             return (old & ~wmask) | (wval & wmask);
> > +     case PCI_DVSEC_CXL_STATUS:
> > +     case PCI_DVSEC_CXL_STATUS2:
> > +             return old & ~(wval & wmask);
> > +     default:
> > +             return old;
> > +     }
> > +}
> > +
> > +/* Config accesses never cross a dword, so a single shadow entry
> > +covers them. */ static int vfio_cxl_config_read(struct vfio_pci_core_device
> *vdev, int pos,
> > +                             int count, __le32 *val) {
> > +     struct vfio_cxl_state *cxl = vdev->cxl;
> > +     int boff = (pos - cxl->dvsec) % sizeof(u32);
> > +     __le32 dword;
> > +
> > +     if (pos < cxl->dvsec || pos >= cxl->dvsec + cxl->dvsec_len)
> > +             return -ENODEV;
> > +
> > +     dword = cpu_to_le32(cxl->dvsec_shadow[(pos - cxl->dvsec) /
> sizeof(u32)]);
> > +     memcpy(val, (u8 *)&dword + boff, count);
> > +
> > +     return count;
> > +}
> > +
> > +static int vfio_cxl_config_write(struct vfio_pci_core_device *vdev, int pos,
> > +                              int count, __le32 val) {
> > +     struct vfio_cxl_state *cxl = vdev->cxl;
> > +     int idx = (pos - cxl->dvsec) / sizeof(u32);
> > +     int boff = (pos - cxl->dvsec) % sizeof(u32);
> > +     u32 off = idx * sizeof(u32);
> > +     __le32 le_wval = 0, le_wmask = 0;
> > +     u32 old, wval, wmask;
> > +     u16 lo, hi;
> > +
> > +     if (pos < cxl->dvsec || pos >= cxl->dvsec + cxl->dvsec_len)
> > +             return -ENODEV;
> > +
> > +     /*
> > +      * Place the guest bytes and a matching byte mask at the write offset,
> > +      * then let the per-field class decide what actually lands in the shadow.
> > +      * The hardware is never touched.
> > +      */
> > +     memcpy((u8 *)&le_wval + boff, &val, count);
> > +     memset((u8 *)&le_wmask + boff, 0xff, count);
> > +     old = cxl->dvsec_shadow[idx];
> > +     wval = le32_to_cpu(le_wval);
> > +     wmask = le32_to_cpu(le_wmask);
> > +
> > +     lo = vfio_cxl_dvsec_field(off, old, wval, wmask);
> > +     hi = vfio_cxl_dvsec_field(off + 2, old >> 16, wval >> 16, wmask >> 16);
> > +     cxl->dvsec_shadow[idx] = lo | ((u32)hi << 16);
> > +
> > +     return count;
> > +}
> > +
> >  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,
> > +     .config_read    = vfio_cxl_config_read,
> > +     .config_write   = vfio_cxl_config_write,
> >       .owner          = THIS_MODULE,
> >  };
> >
> > diff --git a/drivers/vfio/pci/vfio_pci_config.c
> > b/drivers/vfio/pci/vfio_pci_config.c
> > index a10ed733f0e3..f088e4ce5e07 100644
> > --- a/drivers/vfio/pci/vfio_pci_config.c
> > +++ b/drivers/vfio/pci/vfio_pci_config.c
> > @@ -1102,6 +1102,39 @@ void vfio_pci_uninit_perm_bits(void)
> >       free_perm_bits(&ecap_perms[PCI_EXT_CAP_ID_PWR]);
> >  }
> >
> > +/*
> > + * DVSEC accesses are dispatched to vfio-cxl for a CXL device. It
> > +handles the
> > + * CXL DVSEC and returns -ENODEV for any other DVSEC, for which the
> > +default
> > + * handling then applies.
> > + */
> > +static int vfio_pci_dvsec_config_read(struct vfio_pci_core_device *vdev, int
> pos,
> > +                                   int count, struct perm_bits *perm,
> > +                                   int offset, __le32 *val) {
> > +     if (vdev->cxl_ops && vdev->cxl_ops->config_read) {
> > +             int ret = vdev->cxl_ops->config_read(vdev, pos, count,
> > +val);
> > +
> > +             if (ret != -ENODEV)
> > +                     return ret;
> > +     }
> > +
> > +     return vfio_direct_config_read(vdev, pos, count, perm, offset,
> > +val); }
> > +
> > +static int vfio_pci_dvsec_config_write(struct vfio_pci_core_device *vdev,
> > +                                    int pos, int count, struct perm_bits *perm,
> > +                                    int offset, __le32 val) {
> > +     if (vdev->cxl_ops && vdev->cxl_ops->config_write) {
> > +             int ret = vdev->cxl_ops->config_write(vdev, pos, count,
> > +val);
> > +
> > +             if (ret != -ENODEV)
> > +                     return ret;
> > +     }
> > +
> > +     return vfio_raw_config_write(vdev, pos, count, perm, offset,
> > +val); }
> > +
> >  int __init vfio_pci_init_perm_bits(void)  {
> >       int ret;
> > @@ -1121,7 +1154,8 @@ int __init vfio_pci_init_perm_bits(void)
> >       ret |= init_pci_ext_cap_err_perm(&ecap_perms[PCI_EXT_CAP_ID_ERR]);
> >       ret |=
> init_pci_ext_cap_pwr_perm(&ecap_perms[PCI_EXT_CAP_ID_PWR]);
> >       ecap_perms[PCI_EXT_CAP_ID_VNDR].writefn = vfio_raw_config_write;
> > -     ecap_perms[PCI_EXT_CAP_ID_DVSEC].writefn = vfio_raw_config_write;
> > +     ecap_perms[PCI_EXT_CAP_ID_DVSEC].readfn =
> vfio_pci_dvsec_config_read;
> > +     ecap_perms[PCI_EXT_CAP_ID_DVSEC].writefn =
> > + vfio_pci_dvsec_config_write;
> >
> >       if (ret)
> >               vfio_pci_uninit_perm_bits(); diff --git
> > a/include/linux/vfio_pci_core.h b/include/linux/vfio_pci_core.h index
> > 7354dae1dd85..b9202cb29d96 100644
> > --- a/include/linux/vfio_pci_core.h
> > +++ b/include/linux/vfio_pci_core.h
> > @@ -72,6 +72,11 @@ struct vfio_cxl_ops {
> >       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);
> > +     int     (*config_read)(struct vfio_pci_core_device *vdev, int pos,
> > +                            int count, __le32 *val);
> > +     int     (*config_write)(struct vfio_pci_core_device *vdev, int pos,
> > +                             int count, __le32 val);
> > +
> >       /* Pinned per bound CXL device so vfio-cxl cannot unload under usage */
> >       struct module *owner;
> >  };
> > diff --git a/include/uapi/linux/pci_regs.h
> > b/include/uapi/linux/pci_regs.h index 4f5942ebd29b..00ca1c777b44
> > 100644
> > --- a/include/uapi/linux/pci_regs.h
> > +++ b/include/uapi/linux/pci_regs.h
> > @@ -1359,6 +1359,7 @@
> >  #define  PCI_DVSEC_CXL_CTRL                          0xC
> >  #define   PCI_DVSEC_CXL_CACHE_ENABLE                 _BITUL(0)
> >  #define   PCI_DVSEC_CXL_MEM_ENABLE                   _BITUL(2)
> > +#define  PCI_DVSEC_CXL_STATUS                                0xE
> >  #define  PCI_DVSEC_CXL_CTRL2                         0x10
> >  #define   PCI_DVSEC_CXL_DISABLE_CACHING                      _BITUL(0)
> >  #define   PCI_DVSEC_CXL_INIT_CACHE_WBI                       _BITUL(1)
> 
> This is still not the correct implementation.  We're only doing virtualized and
> writable bit manipulation, with some RW1C manipulation that doesn't require
> CXL callouts or module dependencies.  This could all live in vfio_pci_config.c
> gated on fields in the core vdev, ex.
> vdev->cxl_dvsec, vdev->cxl_dvsec_len.  We can setup a vdev->cxl_perms
> field with similar use and initialization to msi_perms.  This would also avoid the
> CXL DVSEC shadow setup in the previous patch as vconfig already shadows this
> range.
> 
> The only CXL callouts I see later in the series are for reset, where we can make
> much more discrete ops callouts for store flags to avoid direct CXL module
> dependencies.  Thanks,
> 
> Alex

Agreed on the suggestion. I will update this patch to handle this in vfio_pci_config.c

Manish