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

[Alex Williamson]

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