Hi Alex,
On 24/04/2026 18:20, Alex Williamson wrote:
On Fri, 24 Apr 2026 16:15:06 +0100
Matt Evans <[email protected]> wrote:
On 23/04/2026 22:30, Alex Williamson wrote:
On Thu, 23 Apr 2026 11:25:07 -0700
Matt Evans <[email protected]> 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().
I like that! (My mental model of ERR_PTR was one of function return
values and must admit it didn't occur to me to stash one longer-term in
barmap.)
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,
Just implemented a vfio_pci_core_get_iomap() and I think it's a lot
nicer. I also prefer the helper returning the BAR map pointer rather
than the open-coded vdev->barmap[]s. I've moved the helper [back] to a
header, since the additional index checks will then fold out at compile
time in some cases.
Thanks much for the suggestion, posting a v3 shortly.
Matt
