Re: [PATCH v8 08/28] vfio: add region cache

[John Levon <levon@movementarian.org>]

On Fri, Apr 04, 2025 at 06:18:20PM +0100, John Levon wrote:

> On Fri, Apr 04, 2025 at 06:57:46PM +0200, Cédric Le Goater wrote:
> 
> > > > why not populate vbasedev->regions[index] in vfio_get_all_regions() ?
> > > 
> > > Good question. I presume it's not possible for us to ever look up a region
> > > that has somehow appeared *after* vfio_prepare_device() ?
> > > 
> > > We'd end up off the end of the array in that case anyway.
> > 
> > I was confused. I thought we were caching VFIORegions ...
> > 
> > Anyway, this is an optimisation and I fail to understand where
> > the VFIO_DEVICE_GET_REGION_INFO ioctl is called on a hot path.
> > 
> > Is it for interrupts ? Please explain.
> > 
> > Do you have figures ?
> 
> That's a great question that I don't know the answer to (like much of this code
> I just inherited it). Let me try to investigate.

I found one reason. hw/vfio/pci.c stores VFIOPCIDevice::config_offset so it
doesn't need to do a get region info on every config space access.

But after the refactoring, vfio_io_region_read() gets passed a region index (the
idea of a "region offset" isn't meaningful to vfio-user).

Without the cache, the kernel vfio implementation:

```
867 static int vfio_io_region_write(VFIODevice *vbasedev, uint8_t index, off_t off,  
868                                 uint32_t size, void *data, bool post)            
869 {                                                                                
870     struct vfio_region_info *info = vbasedev->regions[index];                    
871     int ret;                                                                     
872                                                                                  
873     ret = pwrite(vbasedev->fd, data, size, info->offset + off);                  
```

would have to look up the region offset every time.

regards
john