Re: [PATCH v6 08/31] vfio/pci: Add a generic excluded-range list

[Alex Williamson <alex@shazbot.org>]

On Fri, 16 Oct 2026 21:48:15 +0530
<mhonap@nvidia.com> wrote:

> From: Manish Honap <mhonap@nvidia.com>
> 
> A provider such as vfio-cxl needs to keep sub-ranges of a BAR off the
> direct guest path and serve them through its own trap. vfio-pci-core
> already has one such window, the MSI-X table, open-coded in
> vfio_pci_bar_rw().
> 
> Add vfio_pci_core_add_excluded_range(). Each BAR has its own list,
> sorted by start, and an insert that overlaps a range on the same BAR or
> ends past the BAR fails with -EINVAL. Ranges are added only while the
> device is being opened and are freed in vfio_pci_core_disable(), so
> readers on an open device take no lock.
> 
> The MSI-X table is virtualized in vfio_pci_bar_rw() by an open-coded
> x_start/x_end window that fills reads with -1 and drops writes. Register
> the MSI-X table as a read and write excluded range when the MSI-X
> capability is parsed in vfio_pci_core_enable(), and walk the BAR's
> ranges in order in vfio_pci_bar_rw(), so an access that spans several
> ranges is handled in one pass. Only the part of the table inside its BAR
> is registered, so a device whose MSI-X capability points outside the BAR
> still opens, as before. The ROM BAR keeps its trailing -1 window,
> outside the list.
> 
> No behavioral change for an MSI-X table in a standard BAR: an access to
> it still reads -1 and drops writes.
> 
> Assisted-by: LLM
> Signed-off-by: Manish Honap <mhonap@nvidia.com>
> ---
>  drivers/vfio/pci/vfio_pci_core.c | 97 ++++++++++++++++++++++++++++++++
>  drivers/vfio/pci/vfio_pci_priv.h |  8 +++
>  drivers/vfio/pci/vfio_pci_rdwr.c | 67 +++++++++++++++++++---
>  include/linux/vfio_pci_core.h    | 20 +++++++
>  4 files changed, 185 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/vfio/pci/vfio_pci_core.c b/drivers/vfio/pci/vfio_pci_core.c
> index e50b36751d9a..bee3d1b4d8d2 100644
> --- a/drivers/vfio/pci/vfio_pci_core.c
> +++ b/drivers/vfio/pci/vfio_pci_core.c
> @@ -588,6 +588,8 @@ static const struct dev_pm_ops vfio_pci_core_pm_ops = {
>  			   NULL)
>  };
>  
> +static void vfio_pci_free_excluded_ranges(struct vfio_pci_core_device *vdev);
> +
>  int vfio_pci_core_enable(struct vfio_pci_core_device *vdev)
>  {
>  	struct pci_dev *pdev = vdev->pdev;
> @@ -644,6 +646,7 @@ int vfio_pci_core_enable(struct vfio_pci_core_device *vdev)
>  
>  	msix_pos = pdev->msix_cap;
>  	if (msix_pos) {
> +		u64 bar_len = 0;
>  		u16 flags;
>  		u32 table;
>  
> @@ -654,6 +657,30 @@ int vfio_pci_core_enable(struct vfio_pci_core_device *vdev)
>  		vdev->msix_offset = table & PCI_MSIX_TABLE_OFFSET;
>  		vdev->msix_size = ((flags & PCI_MSIX_FLAGS_QSIZE) + 1) * 16;
>  		vdev->has_dyn_msix = pci_msix_can_alloc_dyn(pdev);
> +
> +		/*
> +		 * Virtualize the MSI-X table through the excluded-range list:
> +		 * reads fill -1 and writes are dropped so the guest never
> +		 * reaches the hardware table directly. BAR access never goes
> +		 * past the BAR, so only the part of the table inside the BAR
> +		 * needs a range; a table placed outside its BAR needs none.
> +		 */

AI comments.

> +		if (vdev->msix_bar < PCI_STD_NUM_BARS)
> +			bar_len = pci_resource_len(pdev, vdev->msix_bar);
> +		if (vdev->msix_offset < bar_len) {
> +			u64 len = min_t(u64, vdev->msix_size,
> +					bar_len - vdev->msix_offset);
> +
> +			ret = vfio_pci_core_add_excluded_range(vdev, vdev->msix_bar,
> +							       vdev->msix_offset, len,
> +							       VFIO_PCI_EXCLUDE_READ |
> +							       VFIO_PCI_EXCLUDE_WRITE);

This is not a terribly elegant flow and I think the AI comment above
misses the main point of what the apparently redundant BAR range and
size cap test are trying to accomplish, compatibility.

In the current code, it doesn't matter if BIR is bogus or the offset
doesn't land in the BAR length or size overflows, those just generate
non-matches when we read or write.

I think this would be cleaner as a separate function that wraps the new
function, returning success (perhaps even with a pci_warn_once) for an
out of range BIR or offset, and performs silent cleanup on a range that
exceeds the BAR, with a human generated comment explaining the
compatibility argument.

> +			if (ret) {
> +				vfio_pci_free_excluded_ranges(vdev);
> +				vfio_config_free(vdev);
> +				goto out_free_zdev;

This is mixing local cleanup with an unwind goto and it bites us later
in the series when we end up leaving this in place and adding:

out_free_config:
        /* A failed first open never reaches vfio_pci_core_disable(). */
        vfio_pci_core_free_dev_regions(vdev);
        vfio_pci_core_unmap_bars(vdev);
        vfio_pci_free_excluded_ranges(vdev);
        vfio_config_free(vdev);

after the cxl open.

> +			}
> +		}
>  	} else {
>  		vdev->msix_bar = 0xFF;
>  		vdev->has_dyn_msix = false;
> @@ -740,6 +767,7 @@ void vfio_pci_core_disable(struct vfio_pci_core_device *vdev)
>  	vdev->region = NULL; /* don't krealloc a freed pointer */
>  
>  	vfio_config_free(vdev);
> +	vfio_pci_free_excluded_ranges(vdev);

Order is wrong, should be reverse of setup order.  Also doesn't match
the correct ordering used in the error unwind above.

>  
>  	for (i = 0; i < PCI_STD_NUM_BARS; i++) {
>  		bar = i + PCI_STD_RESOURCES;
> @@ -1012,6 +1040,72 @@ static int msix_mmappable_cap(struct vfio_pci_core_device *vdev,
>  	return vfio_info_add_capability(caps, &header, sizeof(header));
>  }
>  
> +/**
> + * vfio_pci_core_add_excluded_range() - keep a BAR sub-range off direct access

"Exclude a BAR sub-range from user access"

> + * @vdev: vfio PCI device being opened
> + * @bar: BAR index, below PCI_STD_NUM_BARS
> + * @start: offset of the range in @bar
> + * @size: length of the range in bytes
> + * @flags: VFIO_PCI_EXCLUDE_* paths the range is excluded from
> + *
> + * Call only while the device is being opened, from vfio_pci_core_enable() or
> + * a provider's open hook; vfio_pci_core_disable() frees the ranges. Ranges on
> + * one BAR must not overlap and must lie inside the BAR.

Are we pushing an overly complicated model just to align with MSI-X?

I think MSI-X is configured on open_device because it's trivial.  Just
a couple config reads and extract the info.  Now it's becoming more
complicated with allocations and lists, and I'm not sure why it doesn't
at the same time move to the .init and .release callbacks instead.

The AI comment also skips the important note reminding us that the
exclude list is lockless and must be immutable while the device is
opened, wherever we decide that boundary should be.

> + *
> + * Return: 0 on success, -EINVAL for an invalid or overlapping range or one
> + * that ends past the BAR, or -ENOMEM.
> + */
> +int vfio_pci_core_add_excluded_range(struct vfio_pci_core_device *vdev, int bar,
> +				     u64 start, u64 size, u32 flags)
> +{
> +	const u32 valid = VFIO_PCI_EXCLUDE_MMAP | VFIO_PCI_EXCLUDE_READ |
> +			  VFIO_PCI_EXCLUDE_WRITE;
> +	struct vfio_pci_excluded_range *range, *pos;
> +	struct list_head *head;
> +
> +	if (bar < 0 || bar >= PCI_STD_NUM_BARS || !size ||
> +	    start + size < start || !flags || (flags & ~valid))
> +		return -EINVAL;
> +	if (start + size > pci_resource_len(vdev->pdev, bar))
> +		return -EINVAL;
> +
> +	/* Keep the list sorted by start; find the first range after @start. */
> +	head = &vdev->excluded_ranges[bar];
> +	list_for_each_entry(pos, head, entry) {
> +		if (start < pos->start + pos->size && start + size > pos->start)
> +			return -EINVAL;
> +		if (pos->start > start)
> +			break;
> +	}
> +
> +	range = kmalloc_obj(*range);
> +	if (!range)
> +		return -ENOMEM;
> +
> +	*range = (struct vfio_pci_excluded_range) {
> +		.start = start,
> +		.size = size,
> +		.flags = flags,
> +	};
> +	list_add_tail(&range->entry, &pos->entry);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(vfio_pci_core_add_excluded_range);
> +
> +static void vfio_pci_free_excluded_ranges(struct vfio_pci_core_device *vdev)
> +{
> +	struct vfio_pci_excluded_range *range, *tmp;
> +
> +	for (int bar = 0; bar < PCI_STD_NUM_BARS; bar++) {
> +		list_for_each_entry_safe(range, tmp, &vdev->excluded_ranges[bar],
> +					 entry) {
> +			list_del(&range->entry);
> +			kfree(range);
> +		}
> +	}
> +}
> +
>  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,
> @@ -2317,6 +2411,8 @@ int vfio_pci_core_init_dev(struct vfio_device *core_vdev)
>  	INIT_LIST_HEAD(&vdev->dmabufs);
>  	init_rwsem(&vdev->memory_lock);
>  	xa_init(&vdev->ctx);
> +	for (int bar = 0; bar < PCI_STD_NUM_BARS; bar++)
> +		INIT_LIST_HEAD(&vdev->excluded_ranges[bar]);
>  
>  	ret = vfio_pci_core_cxl_init(vdev);
>  	if (ret)
> @@ -2332,6 +2428,7 @@ void vfio_pci_core_release_dev(struct vfio_device *core_vdev)
>  		container_of(core_vdev, struct vfio_pci_core_device, vdev);
>  
>  	vfio_pci_core_cxl_release(vdev);
> +	vfio_pci_free_excluded_ranges(vdev);
>  
>  	mutex_destroy(&vdev->igate);
>  	mutex_destroy(&vdev->ioeventfds_lock);
> diff --git a/drivers/vfio/pci/vfio_pci_priv.h b/drivers/vfio/pci/vfio_pci_priv.h
> index 4e7162234a2e..53544e0cbedf 100644
> --- a/drivers/vfio/pci/vfio_pci_priv.h
> +++ b/drivers/vfio/pci/vfio_pci_priv.h
> @@ -44,6 +44,14 @@ ssize_t vfio_pci_config_rw_single(struct vfio_pci_core_device *vdev,
>  ssize_t vfio_pci_bar_rw(struct vfio_pci_core_device *vdev, char __user *buf,
>  			size_t count, loff_t *ppos, bool iswrite);
>  
> +/* A BAR sub-range kept off direct access; see vfio_pci_core_add_excluded_range() */
> +struct vfio_pci_excluded_range {
> +	struct list_head	entry;
> +	u64			start;
> +	u64			size;
> +	u32			flags;
> +};

If we used struct range as a member here we could use range_contains()
and range_overlaps(), for finding a matching range below and avoiding
conflicts above (mind the inclusive end).

> +
>  #ifdef CONFIG_VFIO_PCI_VGA
>  ssize_t vfio_pci_vga_rw(struct vfio_pci_core_device *vdev, char __user *buf,
>  			size_t count, loff_t *ppos, bool iswrite);
> diff --git a/drivers/vfio/pci/vfio_pci_rdwr.c b/drivers/vfio/pci/vfio_pci_rdwr.c
> index 7f14dd46de17..93823c6391f6 100644
> --- a/drivers/vfio/pci/vfio_pci_rdwr.c
> +++ b/drivers/vfio/pci/vfio_pci_rdwr.c
> @@ -198,6 +198,57 @@ ssize_t vfio_pci_core_do_io_rw(struct vfio_pci_core_device *vdev, bool test_mem,
>  }
>  EXPORT_SYMBOL_GPL(vfio_pci_core_do_io_rw);
>  
> +/*
> + * Access @count bytes of @bar at @pos, walking the BAR's excluded ranges in
> + * order: a range excluded for this direction reads as -1 and drops writes,
> + * and the bytes around it reach the device. The MSI-X table is one such
> + * range, and a BAR can hold several, for example the MSI-X table and a CXL
> + * HDM decoder block.
> + */
> +static ssize_t vfio_pci_bar_do_io_rw(struct vfio_pci_core_device *vdev,
> +				     int bar, bool test_mem, void __iomem *io,
> +				     char __user *buf, loff_t pos,
> +				     size_t count, bool iswrite,
> +				     enum vfio_pci_io_width max_width)
> +{
> +	u32 want = iswrite ? VFIO_PCI_EXCLUDE_WRITE : VFIO_PCI_EXCLUDE_READ;
> +	struct vfio_pci_excluded_range *range;
> +	ssize_t done = 0;
> +	ssize_t ret;
> +
> +	list_for_each_entry(range, &vdev->excluded_ranges[bar], entry) {
> +		u64 end = range->start + range->size;
> +		size_t chunk;
> +
> +		if (!(range->flags & want) || end <= pos)
> +			continue;
> +		if (range->start >= pos + count)
> +			break;
> +
> +		/* Up to the end of this range; do_io_rw() fills it. */
> +		chunk = min_t(u64, count, end - pos);
> +		ret = vfio_pci_core_do_io_rw(vdev, test_mem, io, buf, pos, chunk,
> +					     range->start, end, iswrite,
> +					     max_width);
> +		if (ret < 0)
> +			return done ?: ret;
> +
> +		done += ret;
> +		pos += ret;
> +		buf += ret;
> +		count -= ret;
> +		if (!count)
> +			return done;
> +	}
> +
> +	ret = vfio_pci_core_do_io_rw(vdev, test_mem, io, buf, pos, count, 0, 0,
> +				     iswrite, max_width);
> +	if (ret < 0)
> +		return done ?: ret;
> +
> +	return done + ret;
> +}

This is mixing fault semantics.  Currently on fault we return the
errno, regardless of work that's been done.  With this, we potentially
return a short read/write on fault if later errors occur.  That short
return is not necessarily representative of the length of the actual
operation, a partial operation in the last call to core_do_io_rw is
discarded.

Therefore, I think both of the above (ret < 0) return branches should
just return ret, not the potentially incorrect done value.


> +
>  ssize_t vfio_pci_bar_rw(struct vfio_pci_core_device *vdev, char __user *buf,
>  			size_t count, loff_t *ppos, bool iswrite)
>  {
> @@ -256,13 +307,15 @@ ssize_t vfio_pci_bar_rw(struct vfio_pci_core_device *vdev, char __user *buf,
>  		}
>  	}
>  
> -	if (bar == vdev->msix_bar) {
> -		x_start = vdev->msix_offset;
> -		x_end = vdev->msix_offset + vdev->msix_size;
> -	}
> -
> -	done = vfio_pci_core_do_io_rw(vdev, res->flags & IORESOURCE_MEM, io, buf, pos,
> -				      count, x_start, x_end, iswrite, max_width);
> +	if (bar == PCI_ROM_RESOURCE)
> +		done = vfio_pci_core_do_io_rw(vdev, res->flags & IORESOURCE_MEM,
> +					      io, buf, pos, count, x_start,
> +					      x_end, iswrite, max_width);
> +	else
> +		done = vfio_pci_bar_do_io_rw(vdev, bar,
> +					     res->flags & IORESOURCE_MEM, io,
> +					     buf, pos, count, iswrite,
> +					     max_width);

Seems like the wrong refactor to add a ROM specific branch, we should
probably instead precede the excluded range loop by a test for a
standard BAR and non-empty excluded range list.  That might mean the
new function is reduced to only the excluded range handling and all
non-excluded range handling falls through to a common call to the core
function.

>  
>  	if (done >= 0)
>  		*ppos += done;
> diff --git a/include/linux/vfio_pci_core.h b/include/linux/vfio_pci_core.h
> index 7f3a2bcb5830..70acb03d466e 100644
> --- a/include/linux/vfio_pci_core.h
> +++ b/include/linux/vfio_pci_core.h
> @@ -162,6 +162,13 @@ struct vfio_pci_core_device {
>  	struct notifier_block	nb;
>  	struct rw_semaphore	memory_lock;
>  	struct list_head	dmabufs;
> +	/*
> +	 * Excluded BAR sub-ranges, one list per BAR, sorted by start and not
> +	 * overlapping. The lists are filled only while the device is being
> +	 * opened and emptied in vfio_pci_core_disable(); every reader runs on
> +	 * an open device in between, so readers take no lock.
> +	 */

I'll stop noting these, but I expect all gratuitous AI comments to be
removed by default and evaluated as a human whether a comment is
warranted.  AI assistance doesn't relieve authors of the responsibility
to perform a thorough human self review.  /dismount-soapbox
Thanks,

Alex

> +	struct list_head	excluded_ranges[PCI_STD_NUM_BARS];
>  };
>  
>  enum vfio_pci_io_width {
> @@ -172,6 +179,19 @@ enum vfio_pci_io_width {
>  };
>  
>  /* Will be exported for vfio pci drivers usage */
> +/*
> + * A provider can keep a BAR sub-range off the direct guest path, reached only
> + * through its own trap. The flags select which paths are excluded: mmap, and
> + * region read and write (an excluded read fills -1, an excluded write is
> + * dropped).
> + */
> +#define VFIO_PCI_EXCLUDE_MMAP	BIT(0)
> +#define VFIO_PCI_EXCLUDE_READ	BIT(1)
> +#define VFIO_PCI_EXCLUDE_WRITE	BIT(2)
> +
> +int vfio_pci_core_add_excluded_range(struct vfio_pci_core_device *vdev, int bar,
> +				     u64 start, u64 size, u32 flags);
> +
>  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,