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
