On Tue, 14 Oct 2025 16:00:26 +0200 John Levon <[email protected]> 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
