Re: [PATCH v5 18/27] vfio/cxl: Expose the HDM memory region to the guest

[Alex Williamson]

On Thu, 17 Sep 2026 00:05:31 +0530
<mhonap@xxxxxxxxxx> wrote:

> From: Manish Honap <mhonap@xxxxxxxxxx>
> 
> A CXL Type-2 guest maps the device HDM memory to use its coherent
> CXL.mem. The HDM memory is a host physical range with no struct page,
> so the guest and KVM need a write-back mapping of it.
> 
> Register the range as an mmap-able region under
> VFIO_REGION_TYPE_PCI_VENDOR_TYPE with the CXL vendor id rather than a
> bespoke region type, per open because vfio_pci_core_disable() tears
> down all dynamic regions on close.
> 
> Own the resolved host physical range exclusively (IORESOURCE_EXCLUSIVE)
> so nothing, /dev/mem included, can map a conflicting cacheable alias
> that would fault the host once the range is mapped write-back.  There
> is no devm form of the exclusive request, so pair it with a devm
> release action.
> 
> vfio_pci_zap_bars() only unmaps the fixed PCI BAR range, so revoke
> mmap-capable device-specific regions there too.  Otherwise a
> Memory-Space disable, a D3 transition, or a reset would leave the guest
> a live mapping into quiesced device memory; the fault handler re-gates
> on device state before it inserts a pfn again.
> 
> Assisted-by: LLM
> Signed-off-by: Manish Honap <mhonap@xxxxxxxxxx>
> ---
>  drivers/vfio/pci/cxl/vfio_cxl_core.c | 186 +++++++++++++++++++++++++++
>  drivers/vfio/pci/vfio_pci_core.c     |  37 ++++++
>  include/linux/vfio_pci_core.h        |   1 +
>  include/uapi/linux/vfio.h            |   4 +
>  4 files changed, 228 insertions(+)
> 
> diff --git a/drivers/vfio/pci/cxl/vfio_cxl_core.c b/drivers/vfio/pci/cxl/vfio_cxl_core.c
> index 5c8a63833a43..5b65cac30aba 100644
> --- a/drivers/vfio/pci/cxl/vfio_cxl_core.c
> +++ b/drivers/vfio/pci/cxl/vfio_cxl_core.c
> @@ -5,9 +5,13 @@
>   * Copyright (c) 2026 NVIDIA Corporation & Affiliates
>   */
>  
> +#include <linux/cleanup.h>
> +#include <linux/io.h>
> +#include <linux/mm.h>
>  #include <linux/module.h>
>  #include <linux/pci.h>
>  #include <linux/range.h>
> +#include <linux/uaccess.h>
>  #include <linux/vfio_pci_core.h>
>  #include <cxl/cxl.h>
>  #include <cxl/pci.h>
> @@ -17,13 +21,144 @@
>   * @cxlds: CXL device state; kept first for devm_cxl_dev_state_create()
>   * @cxlmd: memory device joined to the CXL topology at bind
>   * @hpa_range: host physical range of the HDM region
> + * @hdm_valid: true when host CPU access to the HDM range is safe; under memory_lock
>   */
>  struct vfio_cxl_state {
>  	struct cxl_dev_state cxlds;
>  	struct cxl_memdev *cxlmd;
>  	struct range hpa_range;
> +	bool hdm_valid;
>  };
>  
> +static unsigned long vfio_cxl_mem_pgoff(struct vm_area_struct *vma,
> +					unsigned long addr)
> +{
> +	unsigned long mask = (1U << (VFIO_PCI_OFFSET_SHIFT - PAGE_SHIFT)) - 1;
> +
> +	return (vma->vm_pgoff & mask) + ((addr - vma->vm_start) >> PAGE_SHIFT);
> +}
> +
> +static vm_fault_t vfio_cxl_mem_huge_fault(struct vm_fault *vmf,
> +					  unsigned int order)
> +{
> +	struct vm_area_struct *vma = vmf->vma;
> +	struct vfio_pci_core_device *vdev = vma->vm_private_data;
> +	struct vfio_cxl_state *cxl = vdev->cxl;
> +	unsigned long addr = ALIGN_DOWN(vmf->address, PAGE_SIZE << order);
> +	unsigned long pfn = PHYS_PFN(cxl->hpa_range.start) +
> +			    vfio_cxl_mem_pgoff(vma, addr);
> +	vm_fault_t ret = VM_FAULT_FALLBACK;
> +
> +	if (is_aligned_for_order(vma, addr, pfn, order)) {
> +		scoped_guard(rwsem_read, &vdev->memory_lock) {
> +			/*
> +			 * Insert a PFN only for a known-good decoder whose
> +			 * media is ready and whose Memory-Space is enabled.
> +			 */
> +			if (__vfio_pci_memory_enabled(vdev) &&
> +			    cxl->hdm_valid && cxl->cxlds.media_ready)
> +				ret = vfio_pci_vmf_insert_pfn(vdev, vmf, pfn,
> +							      order);
> +			else
> +				ret = VM_FAULT_SIGBUS;
> +		}
> +	}
> +
> +	return ret;
> +}
> +
> +static vm_fault_t vfio_cxl_mem_fault(struct vm_fault *vmf)
> +{
> +	return vfio_cxl_mem_huge_fault(vmf, 0);
> +}
> +
> +static const struct vm_operations_struct vfio_cxl_mem_vm_ops = {
> +	.fault = vfio_cxl_mem_fault,
> +#ifdef CONFIG_ARCH_SUPPORTS_HUGE_PFNMAP
> +	.huge_fault = vfio_cxl_mem_huge_fault,
> +#endif
> +};
> +
> +static int vfio_cxl_mem_mmap(struct vfio_pci_core_device *vdev,
> +			     struct vfio_pci_region *region,
> +			     struct vm_area_struct *vma)
> +{
> +	unsigned long mask = (1U << (VFIO_PCI_OFFSET_SHIFT - PAGE_SHIFT)) - 1;
> +	u64 req_start = (vma->vm_pgoff & mask) << PAGE_SHIFT;
> +	u64 req_len = vma->vm_end - vma->vm_start;
> +
> +	if (req_start + req_len > region->size)
> +		return -EINVAL;
> +
> +	/*
> +	 * CXL.mem is coherent memory, so leave the mapping write-back cacheable.
> +	 */
> +	vm_flags_set(vma, VM_IO | VM_PFNMAP | VM_DONTEXPAND | VM_DONTDUMP);
> +	vma->vm_ops = &vfio_cxl_mem_vm_ops;
> +	vma->vm_private_data = vdev;
> +
> +	return 0;
> +}
> +
> +static ssize_t vfio_cxl_mem_rw(struct vfio_pci_core_device *vdev,
> +			       char __user *buf, size_t count, loff_t *ppos,
> +			       bool iswrite)
> +{
> +	struct vfio_cxl_state *cxl = vdev->cxl;
> +	u64 pos = *ppos & VFIO_PCI_OFFSET_MASK;
> +	void *mem;
> +	ssize_t done;
> +
> +	if (pos >= range_len(&cxl->hpa_range))
> +		return -EINVAL;
> +	count = min_t(size_t, count, range_len(&cxl->hpa_range) - pos);
> +
> +	scoped_guard(rwsem_read, &vdev->memory_lock) {
> +		/*
> +		 * Same gate as the fault path: only touch the HDM range with
> +		 * the decoder in a known-good state AND Memory-Space enabled,
> +		 * or a host CPU access aborts as a fatal SError.
> +		 */
> +		if (!cxl->hdm_valid || !__vfio_pci_memory_enabled(vdev))
> +			return -EIO;
> +
> +		mem = memremap(cxl->hpa_range.start + pos, count, MEMREMAP_WB);
> +		if (!mem)
> +			return -ENOMEM;
> +		if (iswrite)
> +			done = copy_from_user(mem, buf, count) ? -EFAULT : count;
> +		else
> +			done = copy_to_user(buf, mem, count) ? -EFAULT : count;
> +		memunmap(mem);
> +	}
> +	if (done > 0)
> +		*ppos += done;
> +
> +	return done;
> +}
> +
> +/*
> + * The CXL regions carry no per-region state (region->data is the shared,
> + * devm-managed vfio_cxl_state), so releasing a region is a no-op.
> + */
> +static void vfio_cxl_region_release(struct vfio_pci_core_device *vdev,
> +				    struct vfio_pci_region *region)
> +{
> +}
> +
> +static const struct vfio_pci_regops vfio_cxl_mem_regops = {
> +	.rw = vfio_cxl_mem_rw,
> +	.mmap = vfio_cxl_mem_mmap,
> +	.release = vfio_cxl_region_release,
> +};
> +
> +static void vfio_cxl_release_hpa(void *data)
> +{
> +	struct vfio_cxl_state *cxl = data;
> +
> +	release_mem_region(cxl->hpa_range.start, range_len(&cxl->hpa_range));
> +}
> +
>  static int vfio_cxl_init_device(struct vfio_pci_core_device *vdev)
>  {
>  	struct pci_dev *pdev = vdev->pdev;
> @@ -120,6 +255,21 @@ static int vfio_cxl_init_device(struct vfio_pci_core_device *vdev)
>  		goto err;
>  	}
>  
> +	/*
> +	 * Claim the range IORESOURCE_EXCLUSIVE so no conflicting cacheable
> +	 * alias can fault the host once it is mapped write-back; there is no
> +	 * devm form, so pair it with a devm release action.
> +	 */
> +	if (!request_mem_region_exclusive(cxl->hpa_range.start,
> +					  range_len(&cxl->hpa_range),
> +					  "vfio-cxl-hdm")) {
> +		ret = -EBUSY;
> +		goto err;
> +	}
> +	ret = devm_add_action_or_reset(&pdev->dev, vfio_cxl_release_hpa, cxl);
> +	if (ret)
> +		goto err;
> +
>  	cxl->cxlmd = cxlmd;
>  	devres_close_group(&pdev->dev, NULL);
>  
> @@ -143,13 +293,49 @@ static void vfio_cxl_release_device(struct vfio_pci_core_device *vdev)
>  	vdev->cxl = NULL;
>  }
>  
> +static int vfio_cxl_add_region(struct vfio_pci_core_device *vdev, u32 subtype,
> +			       const struct vfio_pci_regops *ops, size_t size,
> +			       u32 flags)
> +{
> +	u32 type = VFIO_REGION_TYPE_PCI_VENDOR_TYPE | PCI_VENDOR_ID_CXL;
> +
> +	return vfio_pci_core_register_dev_region(vdev, type, subtype, ops,
> +						 size, flags, vdev->cxl);
> +}
> +
>  static int vfio_cxl_open_device(struct vfio_pci_core_device *vdev)
>  {
> +	struct vfio_cxl_state *cxl = vdev->cxl;
> +	int ret;
> +
> +	/*
> +	 * vfio_pci_core_disable() frees all dynamic regions on close, so register
> +	 * them here per open rather than at bind. A failed first open never
> +	 * reaches close_device(), so unwind on error.

There's no unwind here.

> +	 */
> +	ret = vfio_cxl_add_region(vdev, VFIO_REGION_SUBTYPE_CXL_MEM,
> +				  &vfio_cxl_mem_regops, range_len(&cxl->hpa_range),
> +				  VFIO_REGION_INFO_FLAG_READ |
> +				  VFIO_REGION_INFO_FLAG_WRITE |
> +				  VFIO_REGION_INFO_FLAG_MMAP);
> +	if (ret)
> +		return ret;
> +
> +	/*
> +	 * The decoder is firmware-committed, so host access to the HDM range is
> +	 * safe. Open the access gate; reset and power transitions clear it until
> +	 * the decoder is restored.
> +	 */
> +	cxl->hdm_valid = true;
> +
>  	return 0;
>  }
>  
>  static void vfio_cxl_close_device(struct vfio_pci_core_device *vdev)
>  {
> +	struct vfio_cxl_state *cxl = vdev->cxl;
> +
> +	cxl->hdm_valid = false;
>  }

If hdm_valid is set on open_device and cleared on close_device, how is
this not redundant to open_count?  It's totally superfluous everywhere
it's tested in this patch.

>  
>  static void vfio_cxl_reset_prepare(struct vfio_pci_core_device *vdev)
> diff --git a/drivers/vfio/pci/vfio_pci_core.c b/drivers/vfio/pci/vfio_pci_core.c
> index ddd6807893fd..8913a9e24302 100644
> --- a/drivers/vfio/pci/vfio_pci_core.c
> +++ b/drivers/vfio/pci/vfio_pci_core.c
> @@ -1295,6 +1295,23 @@ int vfio_pci_core_register_dev_region(struct vfio_pci_core_device *vdev,
>  }
>  EXPORT_SYMBOL_GPL(vfio_pci_core_register_dev_region);
>  
> +/*
> + * Unregister the most recently registered dynamic region. Used to unwind a
> + * partially built region set on an open-time error; regions are otherwise
> + * released together in vfio_pci_core_disable().
> + */
> +void vfio_pci_core_unregister_dev_region(struct vfio_pci_core_device *vdev)
> +{
> +	struct vfio_pci_region *region;
> +
> +	if (WARN_ON(!vdev->num_regions))
> +		return;
> +
> +	region = &vdev->region[--vdev->num_regions];
> +	region->ops->release(vdev, region);
> +}
> +EXPORT_SYMBOL_GPL(vfio_pci_core_unregister_dev_region);

This is not a good interface, implicitly popping the last dev region
added.  The whole interface should change to push and pop semantics if
we're doing this, but we shouldn't do this.  It would be better to ask
to remove a specific region, identified by some compliment of the
creation parameters, with a memmove + collapse on the array.  Or
perhaps this justifies another data structure.

This is also unused in this patch.  Why's it added here?  This patch
already does too much.  We could have lead with read/write access and
followed with mmap support in another patch.

> +
>  static int vfio_pci_info_atomic_cap(struct vfio_pci_core_device *vdev,
>  				    struct vfio_info_cap *caps)
>  {
> @@ -1972,8 +1989,28 @@ static void vfio_pci_zap_bars(struct vfio_pci_core_device *vdev)
>  	loff_t start = VFIO_PCI_INDEX_TO_OFFSET(VFIO_PCI_BAR0_REGION_INDEX);
>  	loff_t end = VFIO_PCI_INDEX_TO_OFFSET(VFIO_PCI_ROM_REGION_INDEX);
>  	loff_t len = end - start;
> +	unsigned int i;
>  
>  	unmap_mapping_range(core_vdev->inode->i_mapping, start, len, true);
> +
> +	/*
> +	 * The unmap above covers the PCI BARs; mmap-capable device-specific
> +	 * regions (e.g. a vfio-cxl HDM window) sit above that range, so revoke
> +	 * them here too, or a Memory-Space disable, D3/PM transition, or reset
> +	 * would leave the guest a live mapping into quiesced device memory.
> +	 * Callers hold memory_lock, so the region array is stable.

memory_lock doesn't protect the region array, it's allocated at
init/open and considered stable.  Like v4, this is arguing where it's
convenient that we need to guard against PM transitions while also
preventing PM transitions.  Why don't we just support PM?

> +	 */
> +	for (i = 0; i < vdev->num_regions; i++) {
> +		struct vfio_pci_region *region = &vdev->region[i];
> +		loff_t roff;
> +
> +		if (!(region->flags & VFIO_REGION_INFO_FLAG_MMAP))
> +			continue;
> +
> +		roff = VFIO_PCI_INDEX_TO_OFFSET(VFIO_PCI_NUM_REGIONS + i);
> +		unmap_mapping_range(core_vdev->inode->i_mapping, roff,
> +				    region->size, true);
> +	}

There's an assumption here that doesn't seem well founded that a device
specific region that support mmap is necessarily dependent on the
memory enable state of the device.  We could have chosen to report the
coherent memory nvgrace uses as a device specific region, we know it's
not tied to memory enable.  We could support mmap on something like the
IGD OpRegion if we chose to map a whole page for it.  There's an
implicit dependency here that I don't see baked in to simply supporting
mmap.  Thanks,

Alex

>  }
>  
>  void vfio_pci_zap_and_down_write_memory_lock(struct vfio_pci_core_device *vdev)
> diff --git a/include/linux/vfio_pci_core.h b/include/linux/vfio_pci_core.h
> index 475a0ecf9e4f..39a28cc6ae8c 100644
> --- a/include/linux/vfio_pci_core.h
> +++ b/include/linux/vfio_pci_core.h
> @@ -198,6 +198,7 @@ int vfio_pci_core_register_dev_region(struct vfio_pci_core_device *vdev,
>  				      unsigned int type, unsigned int subtype,
>  				      const struct vfio_pci_regops *ops,
>  				      size_t size, u32 flags, void *data);
> +void vfio_pci_core_unregister_dev_region(struct vfio_pci_core_device *vdev);
>  void vfio_pci_core_close_device(struct vfio_device *core_vdev);
>  int vfio_pci_core_init_dev(struct vfio_device *core_vdev);
>  void vfio_pci_core_release_dev(struct vfio_device *core_vdev);
> diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
> index e41437fa17ad..1bf86763c0f7 100644
> --- a/include/uapi/linux/vfio.h
> +++ b/include/uapi/linux/vfio.h
> @@ -370,6 +370,10 @@ struct vfio_region_info_cap_type {
>   */
>  #define VFIO_REGION_SUBTYPE_IBM_NVLINK2_ATSD	(1)
>  
> +/* CXL Type-2 device (0x1e98) sub-types for VFIO_REGION_TYPE_PCI_VENDOR_TYPE */
> +/* CXL.mem HDM region of a Type-2 device, mmap-able */
> +#define VFIO_REGION_SUBTYPE_CXL_MEM		(1)
> +
>  /* sub-types for VFIO_REGION_TYPE_GFX */
>  #define VFIO_REGION_SUBTYPE_GFX_EDID            (1)
>