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

Matt Evans
2026-04-29T10:15:24Z

Hi Alex,

On 24/04/2026 18:20, Alex Williamson wrote:
>
> 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().

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