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

[Matt Evans <mattev@meta.com>]

Hi Alex,

On 30/04/2026 21:13, Alex Williamson wrote:
> 
> On Thu, 30 Apr 2026 03:03:20 -0700
> Matt Evans <mattev@meta.com> wrote:
> 
>> Previously BAR resource requests and the corresponding pci_iomap()
>> were performed on-demand and without synchronisation, which was racy.
>> Rather than add synchronisation, it's simplest to address this by
>> doing both activities from vfio_pci_core_enable().
>>
>> The resource allocation and/or pci_iomap() can still fail; their
>> status is tracked and existing calls to vfio_pci_core_setup_barmap()
>> will fail in a similar way to before.  This keeps the point of failure
>> as observed by userspace the same, i.e. failures to request/map unused
>> BARs are benign.
>>
>> Fixes: 7f5764e179c6 ("vfio: use vfio_pci_core_setup_barmap to map bar in mmap")
>> Fixes: 0d77ed3589ac0 ("vfio/pci: Pull BAR mapping setup from read-write path")
> 
> Neither of these introduced races, they only moved what they were
> already doing into a function or made use of that shared function for
> what they were already doing.  I'm inclined to believe the raciness
> existed from the introduction, 89e1f7d4c66d.
> 
>> Signed-off-by: Matt Evans <mattev@meta.com>
>> ---
>>   drivers/vfio/pci/vfio_pci_core.c | 33 ++++++++++++++++++++++++++++++++
>>   drivers/vfio/pci/vfio_pci_rdwr.c | 29 ++++++++++++----------------
>>   2 files changed, 45 insertions(+), 17 deletions(-)
>>
>> diff --git a/drivers/vfio/pci/vfio_pci_core.c b/drivers/vfio/pci/vfio_pci_core.c
>> index 3f8d093aacf8..eab4f2626b39 100644
>> --- a/drivers/vfio/pci/vfio_pci_core.c
>> +++ b/drivers/vfio/pci/vfio_pci_core.c
>> @@ -482,6 +482,38 @@ static int vfio_pci_core_runtime_resume(struct device *dev)
>>   }
>>   #endif /* CONFIG_PM */
>>   
>> +static void vfio_pci_core_map_bars(struct vfio_pci_core_device *vdev)
>> +{
>> +	struct pci_dev *pdev = vdev->pdev;
>> +	int i;
>> +
>> +	/*
>> +	 * Eager-request BAR resources, and iomap.  Soft failures are
>> +	 * allowed, and consumers must check the barmap before use in
>> +	 * order to give compatible user-visible behaviour with the
>> +	 * previous on-demand allocation method.
>> +	 */
>> +	for (i = 0; i < PCI_STD_NUM_BARS; i++) {
>> +		int bar = i + PCI_STD_RESOURCES;
>> +		void __iomem *io = ERR_PTR(-ENODEV);
> 
> It would collapse the nesting depth to just do:
> 
> 		vdev->barmap[bar] = ERR_PTR(-ENODEV);
> 
> 		if (!pci_resource_len(pdev, i))
> 			continue;
> 
> 		if (pci_request_selected_regions(pdev, 1 << bar, "vfio")) {
> 			pci_dbg(vdev->pdev, "Failed to reserve region %d\n", bar);
> 			vdev->barmap[bar] = ERR_PTR(-EBUSY);
> 			continue;
> 		}
> 
> 		vdev->barmap[bar] = pci_iomap(pdev, bar, 0);
> 		if (!vdev->barmap[bar]) {
> 			pci_dbg(vdev->pdev, "Failed to iomap region %d\n", bar);
> 			vdev->barmap[bar] = ERR_PTR(-ENOMEM);
> 		}
> 
> It's debatable what level to use for the errors, but we were previously
> silent on this, so going all the way to pci_warn() seems unnecessary.

Hm, okay, returned it to a nesting-less format and replaced pci_warn()s 
with pci_dbg().

>> +
>> +		if (pci_resource_len(pdev, i) > 0) {
>> +			if (pci_request_selected_regions(pdev, 1 << bar, "vfio")) {
>> +				pci_warn(vdev->pdev, "Failed to reserve region %d\n", bar);
>> +				io = ERR_PTR(-EBUSY);
>> +			} else {
>> +				io = pci_iomap(pdev, bar, 0);
>> +				if (!io) {
>> +					pci_warn(vdev->pdev, "Failed to iomap region %d\n",
>> +						 bar);
>> +					io = ERR_PTR(-ENOMEM);
>> +				}
>> +			}
>> +		}
>> +		vdev->barmap[bar] = io;
>> +	}
>> +}
>> +
>>   /*
>>    * The pci-driver core runtime PM routines always save the device state
>>    * before going into suspended state. If the device is going into low power
>> @@ -568,6 +600,7 @@ int vfio_pci_core_enable(struct vfio_pci_core_device *vdev)
>>   	if (!vfio_vga_disabled() && vfio_pci_is_vga(pdev))
>>   		vdev->has_vga = true;
>>   
>> +	vfio_pci_core_map_bars(vdev);
>>   
>>   	return 0;
> 
> You're missing the barmap test in vfio_pci_core_disable() now, it's
> still testing for NULL, which is (almost?) never true.  It needs to
> convert to IS_ERR_OR_NULL().

Arrrrgh, yes it does, thank you.  (For the second time, the first being 
the !IS_ERR() typo you caught in patch #3 :(  Thanks there also; it 
slipped by my usual testing routine.)

>> diff --git a/drivers/vfio/pci/vfio_pci_rdwr.c b/drivers/vfio/pci/vfio_pci_rdwr.c
>> index 4251ee03e146..f66ad3d96481 100644
>> --- a/drivers/vfio/pci/vfio_pci_rdwr.c
>> +++ b/drivers/vfio/pci/vfio_pci_rdwr.c
>> @@ -200,25 +200,20 @@ EXPORT_SYMBOL_GPL(vfio_pci_core_do_io_rw);
>>   
>>   int vfio_pci_core_setup_barmap(struct vfio_pci_core_device *vdev, int bar)
>>   {
>> -	struct pci_dev *pdev = vdev->pdev;
>> -	int ret;
>> -	void __iomem *io;
>> -
>> -	if (vdev->barmap[bar])
>> -		return 0;
>> -
>> -	ret = pci_request_selected_regions(pdev, 1 << bar, "vfio");
>> -	if (ret)
>> -		return ret;
>> -
>> -	io = pci_iomap(pdev, bar, 0);
>> -	if (!io) {
>> -		pci_release_selected_regions(pdev, 1 << bar);
>> -		return -ENOMEM;
>> -	}
>> +	/*
>> +	 * The barmap is set up in vfio_pci_core_enable().  Callers
>> +	 * use this function to check that the BAR resources are
>> +	 * requested or that the pci_iomap() was done.
>> +	 */
> 
> Looks like a function level comment to be placed above the function
> definition.  TBH, the comment in the previous function could also be
> pulled up as a function level comment.
> 
>> +	if (bar < 0 || bar >= PCI_STD_NUM_BARS)
> 
> Maybe `if ((unsigned)bar >= PCI_STD_NUM_BARS)` but really author
> preference here.
> 
>> +		return -EINVAL;
>>   
>> -	vdev->barmap[bar] = io;
>> +	/* Did vfio_pci_core_map_bars() set it up yet? */
>> +	if (!vdev->barmap[bar])
>> +		return -ENODEV;
> 
> What hits this?  Should it be a WARN_ON_ONCE?  It would need to be a use
> case that accesses barmap outside of the window between enable and
> disable, where I think we're defining the contract that it's only valid
> between those events.  Both this and the range check could move to the
> iomap implemenation to keep the Fixes: patch reasonably small since
> afaik they're not triggered.  The BAR range test could be WARN_ON_ONCE
> as well, only driver bugs should hit it.  Thanks,

I've reduced the fix patch #1 to just an IS_ERR test (without the null 
or range checks as you suggest).  And indeed WARN_ON_ONCE() is a good 
idea as only tremendous mishaps would lead to these conditions 
triggering (worth testing though).

Also ack on your suggestion on patch #2 to make the call to 
nvgrace_gpu_wait_device_ready() more minimalist, and to order the 2x 
fixes up front.  Posting v4 shortly, cheers!


Thanks,


Matt