Re: [PATCH v3 08/11] vfio/pci/cxl: Add HDM + COMP_REGS regions and DVSEC clipping shim

[Alex Williamson <alex@shazbot.org>]

On Thu, 25 Jun 2026 22:24:04 +0530
<mhonap@nvidia.com> wrote:
> diff --git a/drivers/vfio/pci/vfio_pci_config.c b/drivers/vfio/pci/vfio_pci_config.c
> index a10ed733f0e3..b9f30a33515a 100644
> --- a/drivers/vfio/pci/vfio_pci_config.c
> +++ b/drivers/vfio/pci/vfio_pci_config.c
> @@ -1898,8 +1898,15 @@ ssize_t vfio_pci_config_rw_single(struct vfio_pci_core_device *vdev,
>  	/*
>  	 * Chop accesses into aligned chunks containing no more than a
>  	 * single capability.  Caller increments to the next chunk.
> +	 *
> +	 * For CXL Type-2 devices also clip at the CXL Device DVSEC body
> +	 * boundary so the generic perm-bits path handles the DVSEC
> +	 * header bytes and the CXL hook handles the body bytes; without
> +	 * this clip a 32-bit access at dvsec + 0x08 would span the
> +	 * generic Header2 word and the CXL CAPABILITY word.
>  	 */
>  	count = min(count, vfio_pci_cap_remaining_dword(vdev, *ppos));
> +	count = min(count, vfio_pci_cxl_config_boundary(vdev, *ppos));
>  	if (count >= 4 && !(*ppos % 4))
>  		count = 4;
>  	else if (count >= 2 && !(*ppos % 2))
> @@ -1909,6 +1916,30 @@ ssize_t vfio_pci_config_rw_single(struct vfio_pci_core_device *vdev,
>  
>  	ret = count;
>  
> +	/*
> +	 * Give the CXL Type-2 hook first claim on this access: if the
> +	 * range lies inside the CXL Device DVSEC body, forward it to
> +	 * cxl-core's register-virtualization helpers instead of the
> +	 * standard perm-bits path.  -ENOENT means "not for me; use the
> +	 * default path"; any other negative value is a hard error.
> +	 */
> +	if (vdev->cxl) {
> +		__le32 le_val = 0;
> +		ssize_t cxl_ret;
> +
> +		if (iswrite && copy_from_user(&le_val, buf, count))
> +			return -EFAULT;
> +		cxl_ret = vfio_pci_cxl_config_rw(vdev, *ppos, count, &le_val,
> +						 iswrite);
> +		if (cxl_ret >= 0) {
> +			if (!iswrite && copy_to_user(buf, &le_val, count))
> +				return -EFAULT;
> +			return cxl_ret;
> +		}
> +		if (cxl_ret != -ENOENT)
> +			return cxl_ret;
> +	}
> +

I think the solution here is just to set the .readfn and .writefn for
PCI_EXT_CAP_ID_DVSEC to dvsec specific handlers, rather than the raw
write and direct read handlers.  The new handlers would detect whether
the reference is to the CXL DVSEC body, possibly via ranges stored in
vdev->cxl, and either call through to CXL handlers via cxl_ops
(previously suggested), or fall through to the raw/direct handlers.

>  	cap_id = vdev->pci_config_map[*ppos];
>  
>  	if (cap_id == PCI_CAP_ID_INVALID) {
> diff --git a/drivers/vfio/pci/vfio_pci_core.c b/drivers/vfio/pci/vfio_pci_core.c
> index 05ab4ae59157..2d2dae278d1e 100644
> --- a/drivers/vfio/pci/vfio_pci_core.c
> +++ b/drivers/vfio/pci/vfio_pci_core.c
> @@ -501,6 +501,23 @@ static void vfio_pci_core_map_bars(struct vfio_pci_core_device *vdev)
>  		if (!pci_resource_len(pdev, i))
>  			continue;
>  
> +		/*
> +		 * cxl-core already holds request_mem_region() on the CXL
> +		 * component register sub-range of this BAR.  Skip the
> +		 * full-BAR request so we do not collide with that
> +		 * sub-region; vfio still owns the BAR via the driver
> +		 * binding and the iomap below succeeds without a region
> +		 * claim.
> +		 */
> +		if (vdev->cxl && bar == vfio_pci_cxl_get_component_reg_bar(vdev)) {
> +			vdev->barmap[bar] = pci_iomap(pdev, bar, 0);
> +			if (!vdev->barmap[bar]) {
> +				pci_dbg(pdev, "Failed to iomap region %d\n", bar);
> +				vdev->barmap[bar] = IOMEM_ERR_PTR(-ENOMEM);
> +			}
> +			continue;
> +		}
> +
>  		if (pci_request_selected_regions(pdev, 1 << bar, "vfio")) {
>  			pci_dbg(pdev, "Failed to reserve region %d\n", bar);
>  			vdev->barmap[bar] = IOMEM_ERR_PTR(-EBUSY);
> @@ -701,7 +718,10 @@ void vfio_pci_core_disable(struct vfio_pci_core_device *vdev)
>  		if (IS_ERR_OR_NULL(vdev->barmap[bar]))
>  			continue;
>  		pci_iounmap(pdev, vdev->barmap[bar]);
> -		pci_release_selected_regions(pdev, 1 << bar);
> +		/* Mirror the asymmetric setup-time skip in map_bars(). */
> +		if (!(vdev->cxl &&
> +		      i == vfio_pci_cxl_get_component_reg_bar(vdev)))
> +			pci_release_selected_regions(pdev, 1 << bar);

It would be much less ugly to create
vfio_pci_{request,release}_selected_region() wrappers that mask whether
the region is actually requested or released than to disrupt the code
flow like this.  Likewise below, think about creating wrappers that do
the right thing for cxl and are no-ops otherwise.  For example, embed
the vdev->cxl test into the function to cleanup the callers.  Thanks,

Alex

>  		vdev->barmap[bar] = NULL;
>  	}
>  
> @@ -1051,6 +1071,16 @@ static int vfio_pci_ioctl_get_info(struct vfio_pci_core_device *vdev,
>  	info.num_regions = VFIO_PCI_NUM_REGIONS + vdev->num_regions;
>  	info.num_irqs = VFIO_PCI_NUM_IRQS;
>  
> +	if (vdev->cxl) {
> +		ret = vfio_pci_cxl_get_info(vdev, &caps);
> +		if (ret) {
> +			pci_warn(vdev->pdev,
> +				 "Failed to add CXL info capability\n");
> +			return ret;
> +		}
> +		info.flags |= VFIO_DEVICE_FLAGS_CXL;
> +	}
> +
>  	ret = vfio_pci_info_zdev_add_caps(vdev, &caps);
>  	if (ret && ret != -ENODEV) {
>  		pci_warn(vdev->pdev,
> @@ -1093,6 +1123,12 @@ int vfio_pci_ioctl_get_region_info(struct vfio_device *core_vdev,
>  	struct pci_dev *pdev = vdev->pdev;
>  	int i, ret;
>  
> +	if (vdev->cxl) {
> +		ret = vfio_pci_cxl_get_region_info(vdev, info, caps);
> +		if (ret != -ENOTTY)
> +			return ret;
> +	}
> +
>  	switch (info->index) {
>  	case VFIO_PCI_CONFIG_REGION_INDEX:
>  		info->offset = VFIO_PCI_INDEX_TO_OFFSET(info->index);