> -----Original Message-----
> From: Alex Williamson <[email protected]>
> Sent: Friday, August 28, 2026 10:10 PM
> To: Manish Honap <[email protected]>
> Cc: [email protected]; Ankit Agrawal <[email protected]>; [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]; [email protected]
> 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
> <[email protected]> wrote:
>
> > From: Manish Honap <[email protected]>
> >
> > 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 <[email protected]>
> > ---
> > 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
