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(). 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
