Re: [PATCH v2 1/3] vfio/pci: Set up bar resources and maps in vfio_pci_core_enable()

[Alex Williamson]

On Fri, 24 Apr 2026 16:15:06 +0100
Matt Evans <mattev@xxxxxxxx> wrote:
> On 23/04/2026 22:30, Alex Williamson wrote:
> > On Thu, 23 Apr 2026 11:25:07 -0700
> > Matt Evans <mattev@xxxxxxxx> wrote:
> >> +
> >> +		if (pci_resource_len(pdev, i) == 0)
> >> +			continue;
> >> +
> >> +		ret = pci_request_selected_regions(pdev, 1 << bar, "vfio");
> >> +		if (ret) {
> >> +			pci_warn(vdev->pdev, "Failed to reserve region %d\n", bar);
> >> +			continue;
> >> +		}
> >> +		vdev->have_bar_resource[bar] = true;
> >> +
> >> +		io = pci_iomap(pdev, bar, 0);
> >> +		if (io)
> >> +			vdev->barmap[bar] = io;
> >> +		else
> >> +			pci_warn(vdev->pdev, "Failed to iomap region %d\n", bar);
> >> +	}
> >> +}  
> > 
> > I see you making the point in the cover letter about the resource
> > request vs the iomap resource, but we currently handle these together.
> > If either fails, setup barmap fails and the path returns error.  I
> > don't see any justification for now allowing the request resource to
> > succeed but the iomap fails.  
> 
> The primary effect was to let consumers see -EBUSY for a resource 
> reservation failure or -ENOMEM for an iomap failure (whether through 
> this patch's vfio_pci_core_setup_barmap() or the next patch's helpers), 
> and that keeps the error signatures identical.
> 
> A weak secondary effect was that a BAR that gets resource but fails for 
> whatever reason to iomap it can still be used by most clients (assuming 
> the general usage is to mmap).  The system's pretty sick if this is the 
> case, so as I say it's weak.

Right, I don't see that's really a necessary add at this point.  In
fact while we expect users to access through the mmap when available,
we don't actually have a way to specify that mmap works w/o read/write,
which is effectively what this proposes is a valid state.

> 
> OK, if you prefer the combined approach and don't feel the subsequent 
> single-semantic check helpers (need mapping, need resource) are clearer 
> to read then I'll recombine them, though:
> 
>   - If vfio_pci_core_map_bars() just sets barmap[n] iff both resource 
> acquisition and iomap succeed, then a later check can only return one 
> error from either cause.  I'll go with -ENOMEM unless you prefer -EBUSY. 
>   Using something else can again make userspace see previously-unseen 
> error values.
> 
>   - IMHO vfio_pci_core_setup_barmap() should still be renamed (in a 2nd 
> patch) since it doesn't do any setting up anymore.  Cosmetic, but 
> cleaner to parse when the callers use vfio_pci_core_check_barmap_valid() no?

I'm not sure how important it is to preserve the identical errno, but
we can actually do that too.  Make the enable time "setup" function
store the ERR_PTR in the barmap and change the current callers from
"setup" to "get-iomap", where get-iomap is a __must_check return that
callers test against IS_ERR_OR_NULL().

Or maybe better, collapse NULL into -ENODEV so callers only test
IS_ERR().

There's even one user in vfio_pci_bar_rw() where this provides a minor
simplification.  Likely the others are just wrapping the get-iomap call
in the ERR/NULL test to get the equivalent behavior.  Thoughts?  Thanks,

Alex