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

[Matt Evans <mattev@meta.com>]

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