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

Alex Williamson
2026-04-24T17:20:15Z

On Fri, 24 Apr 2026 16:15:06 +0100
Matt Evans <mattev@meta.com> wrote:
> On 23/04/2026 22:30, Alex Williamson wrote:
> > On Thu, 23 Apr 2026 11:25:07 -0700
> > Matt Evans <mattev@meta.com> 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