Re: [PULL 25/28] vfio: add region info cache

[Alex Williamson <alex@shazbot.org>]

On Tue, 14 Oct 2025 16:00:26 +0200
John Levon <john.levon@nutanix.com> wrote:

> On Tue, Oct 14, 2025 at 07:49:24AM -0600, Alex Williamson wrote:
> 
> > > My apologies - we hit the exact same issue internally, but with a much older
> > > codebase, so I did not realise this could be an upstream problem as well!
> > >
> > > We put this down to a bug in the nvidia driver - surely it shouldn't be
> > > reporting fewer regions than are actually in use. So we applied what we
> > > thought to be a gross hack of boundary checking, and not using the region
> > > cache in case it's beyond num_regions.
> > >
> > > To put it another way, the header file says:
> > >
> > >    217         __u32   num_regions;    /* Max region index + 1 */
> > >
> > > If it's not actually the max region index + 1, what are the expected
> > > semantics of this field, or of region indices more generally? We could not
> > > find any clear documentation on the topic other than this comment.  
> > 
> > '9' only defines the end of the fixed, pre-defined region indexes for
> > vfio-pci, ie. VFIO_PCI_NUM_REGIONS.  Beyond that, we support device specific
> > regions.  The GFX region is one such device specific region.  
> 
> To be clear, nowhere in the code are we hard-coding 9, we ask the underlying
> vfio instance for the num_regions value.
> 
> In this case, it appears that the underlying instance is returning 9, and then
> trying to use index 9.

That sounds like a bug, in vfio_pci_ioctl_get_info() we return
num_regions as (VFIO_PCI_NUM_REGIONS + vdev->num_regions) where
vdev->num_regions is incremented via
vfio_pci_core_register_dev_region().  Any read, write, or mmap access
done through vfio-pci-core would need to increment vdev->num_regions to
support that access.  I think we're dealing with NVIDIA vGPU here,
which afaik is the only driver that uses a region for the display, so
I'd suspect they have a bug and don't use the vfio-pci-core callbacks
here.

Does QEMU's caching model need to fall through to GET_REGION_INFO and
expand the cache for an out-of-bounds index to handle such a bug?

> > There is no fixed limit to the number of regions a device may expose  
> 
> Sure.
> 
> >, nor is vfio_device_info.num_regions necessarily a static value.  
> 
> Ah - is this documented somewhere? And do you have an example of where this
> varies over the lifetime of a device currently?

It's more the case that it's not specifically defined as static over
the lifetime of the device.  I don't think we want device spontaneously
creating new regions, we have no device-check notification mechanism to
userspace to reevaluate regions, but a user initiated action defined to
create a new region seems like a fair vector where num_regions could be
updated.  Also the current working idea would be that regions are only
added, not modified or deleted.

> IOW, are these devices actually changing the num_regions value they report
> sometime after initialization (namely, can it change after
> vfio_device_prepare()) ?

In theory, yes.

> Or would querying num_regions still return the original value, and in fact the
> semantics of the field are "it's not the max region index, but the max region
> index during initialization" or similar ?

Ultimately it's just a marker that *should* indicate there are no
regions to query above (num_regions - 1).  It should be valid at the
time it's queried.  I don't think it would be valid that it could be
dynamic in any other way than a monotonically increasing value.  Thanks,

Alex