Re: [PATCH v5 09/27] vfio/pci: Add a generic excluded-range list

[Jonathan Cameron <jic23@kernel.org>]

On Thu, 17 Sep 2026 00:05:22 +0530
<mhonap@nvidia.com> wrote:

> From: Manish Honap <mhonap@nvidia.com>
> 
> 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. Now that
> a generic excluded-range list expresses the same fill/drop behavior,
> register the MSI-X table as a read and write excluded range instead of
> special-casing it in the read/write path.
> 
> Add the range when the MSI-X capability is parsed in
> vfio_pci_core_enable() and clear the list in vfio_pci_core_disable()
> alongside the config teardown. The read/write path now relies solely on
> vfio_pci_bar_find_exclusion(), so MSI-X and a provider's (e.g. vfio-cxl)
> trapped registers share one mechanism.
> 
> No functional change.
> 
> Assisted-by: LLM
> Signed-off-by: Manish Honap <mhonap@nvidia.com>
A few things inline that might tidy up the implementation and make
it a little more readable. Also you've use object allocators in some
places but not universally.  

Jonathan

> ---
>  drivers/vfio/pci/vfio_pci_core.c | 209 +++++++++++++++++++++++++++++++
>  drivers/vfio/pci/vfio_pci_priv.h |   9 ++
>  drivers/vfio/pci/vfio_pci_rdwr.c |   8 ++
>  include/linux/vfio_pci_core.h    |  14 +++
>  4 files changed, 240 insertions(+)
> 
> diff --git a/drivers/vfio/pci/vfio_pci_core.c b/drivers/vfio/pci/vfio_pci_core.c
> index 9a75c30b67e2..9e4fa5d088a4 100644
> --- a/drivers/vfio/pci/vfio_pci_core.c
> +++ b/drivers/vfio/pci/vfio_pci_core.c
> @@ -24,6 +24,7 @@
>  #include <linux/pci.h>
>  #include <linux/pm_runtime.h>
>  #include <linux/slab.h>
> +#include <linux/sort.h>
>  #include <linux/types.h>
>  #include <linux/uaccess.h>
>  #include <linux/vgaarb.h>
> @@ -1012,6 +1013,204 @@ static int msix_mmappable_cap(struct vfio_pci_core_device *vdev,
>  	return vfio_info_add_capability(caps, &header, sizeof(header));
>  }
>  
> +struct vfio_pci_excluded_range {
> +	struct list_head	entry;
> +	int			bar;
> +	u64			start;
> +	u64			size;
> +	u32			flags;
> +};
> +
> +int vfio_pci_core_add_excluded_range(struct vfio_pci_core_device *vdev, int bar,
> +				     u64 start, u64 size, u32 flags)
> +{
> +	struct vfio_pci_excluded_range *range;
> +
> +	range = kzalloc_obj(*range);
> +	if (!range)
> +		return -ENOMEM;
> +
I'd be tempted to do kmalloc_obj and
	*range = (struct vfio_pci_excluded_range) {
		.bar = bar,
		.start = start,
		.size = size,
		.flags = flags,
	};
	list_add_tail(...)

> +	range->bar = bar;
> +	range->start = start;
> +	range->size = size;
> +	range->flags = flags;
> +	list_add_tail(&range->entry, &vdev->excluded_ranges);
> +
> +	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;
> +
> +	list_for_each_entry_safe(range, tmp, &vdev->excluded_ranges, entry) {
> +		list_del(&range->entry);
> +		kfree(range);
> +	}
> +}


> +
> +/* A page-aligned mmap hole, derived from an mmap-excluded range. */
> +struct vfio_pci_mmap_hole {
> +	u64 start;
> +	u64 end;
> +};
This is just a range. Do we need another define for this specific case?

> +/*
> + * Advertise the BAR as mmappable minus every page-aligned mmap-excluded hole.
> + * A BAR can carry several holes at unrelated offsets (for example an MSI-X
> + * table and one or more trapped CXL component sub-blocks, which the CXL spec
> + * locates by pointer, not at fixed offsets).
> + * Collect the holes, page-align and sort them, coalesce any that overlap or
> + * touch, and advertise the gaps.
> + */
> +static int vfio_pci_excluded_sparse_cap(struct vfio_pci_core_device *vdev,
> +					int index, struct vfio_info_cap *caps)
> +{
> +	u64 bar_len = pci_resource_len(vdev->pdev, index);
> +	struct vfio_region_info_cap_sparse_mmap *sparse;
> +	struct vfio_pci_excluded_range *range;
> +	struct vfio_pci_mmap_hole *holes;
> +	int nr_holes = 0, nr_areas = 0, i, j;

Generally avoid mixing declarations with assignment with those that don't
on a single line. It is harder to read than splitting them into each
time of declaration.

> +	size_t size;
> +	u64 pos;
> +	int ret;
> +
> +	list_for_each_entry(range, &vdev->excluded_ranges, entry)
> +		if (range->bar == index &&
> +		    (range->flags & VFIO_PCI_EXCLUDE_MMAP))
> +			nr_holes++;
> +
> +	if (!nr_holes)
> +		return 0;
> +
	struct vfio_pci_mmap_hole *holes __free(kfree) =
		kzalloc_objs(*holes, nr_holes);
or something like that.  Kees is well his way to getting rid of almost
all places where objects are allocated in non typesafe ways.

> +	holes = kmalloc_array(nr_holes, sizeof(*holes), GFP_KERNEL);
> +	if (!holes)
> +		return -ENOMEM;
> +
> +	/*
> +	 * mmap is page granular, so each hole rounds out to the page boundaries
> +	 * enclosing its excluded sub-range. The byte-granular exclusion still
> +	 * governs the fault and read/write paths; only the advertised mmap areas
> +	 * round to whole pages.
> +	 */
> +	i = 0;
> +	list_for_each_entry(range, &vdev->excluded_ranges, entry) {
> +		if (range->bar != index ||
> +		    !(range->flags & VFIO_PCI_EXCLUDE_MMAP))
> +			continue;

		holes[i++] = (struct vfio_pci_mmap_hole) {
			.start = ALIGN_DOWN(range->start, PAGE_SIZE),
			.end = ALIGN(range->start + range->size, PAGE_SIZE),
		};

Mind you vfio_pci_mmap_hole seems a lot like a range. So maybe just use a
struct range then you get 

		holes[i++] = DEFINE_RANGE(ALIGN_DOWN(range->start, PAGE_SIZE),
					  ALIGN(range->start + range->size, PAGE_SIZE));


> +		holes[i].start = ALIGN_DOWN(range->start, PAGE_SIZE);
> +		holes[i].end = ALIGN(range->start + range->size, PAGE_SIZE);
> +		i++;
> +	}
> +
> +	sort(holes, nr_holes, sizeof(*holes), vfio_pci_mmap_hole_cmp, NULL);
> +
> +	/* Coalesce holes that overlap or touch after page alignment. */
> +	for (i = 0, j = 0; i < nr_holes; i++) {
> +		if (j && holes[i].start <= holes[j - 1].end)
> +			holes[j - 1].end = max(holes[j - 1].end, holes[i].end);
> +		else
> +			holes[j++] = holes[i];
> +	}
> +	nr_holes = j;
> +
> +	/* One mmappable area per gap: before, between, and after the holes. */
> +	for (i = 0, pos = 0; i < nr_holes; i++) {
> +		if (holes[i].start > pos)
> +			nr_areas++;
> +		pos = holes[i].end;
> +	}
> +	if (pos < bar_len)
> +		nr_areas++;
> +
> +	size = struct_size(sparse, areas, nr_areas);

	struct vfio_region_info_cap_sparse_mmap *sparse =
		kzalloc_flex(*sparse, areas, nr_areas);

> +	sparse = kzalloc(size, GFP_KERNEL);
> +	if (!sparse) {
> +		kfree(holes);
with the __free above this can simply return.
> +		return -ENOMEM;
> +	}
> +
> +	sparse->header.id = VFIO_REGION_INFO_CAP_SPARSE_MMAP;
> +	sparse->header.version = 1;
> +	sparse->nr_areas = nr_areas;
> +
> +	for (i = 0, j = 0, pos = 0; i < nr_holes; i++) {
> +		if (holes[i].start > pos) {
> +			sparse->areas[j].offset = pos;
> +			sparse->areas[j].size = holes[i].start - pos;
> +			j++;
Bit of a long line because of the indent but I'd still go for setting whole
structure in one shot - so something like:

			sparse->areas[j++] = (struct vfio_region_sparse_mmap_area) {
				.offset = pos,
				.size = holes[i].start - pos,
			};
> +		}
> +		pos = holes[i].end;
> +	}
> +	if (pos < bar_len) {
> +		sparse->areas[j].offset = pos;
> +		sparse->areas[j].size = bar_len - pos;

Similar for setting it in one go so we only do the indexing
once + looks more like above (where this trick is more advantageous!)

		sparse->areas[j] = (struct vfio_sparse_mmap_area) {
			.offset = pos,
			.size = bar_len - pos, 
		};


> +	}
> +
> +	kfree(holes);

This looks like a good place to use __free magic to simplify things.

> +	ret = vfio_info_add_capability(caps, &sparse->header, size);
> +	kfree(sparse);
> +	return ret;
> +}
> +