Re: [PATCH v3 03/23] vfio: add per-region fd support

[Cédric Le Goater]

On 6/10/25 14:34, John Levon wrote:

On Tue, Jun 10, 2025 at 09:42:41AM +0200, Cédric Le Goater wrote:

On 6/7/25 02:10, John Levon wrote:

For vfio-user, each region has its own fd rather than sharing
vbasedev's. Add the necessary plumbing to support this, and use the
correct fd in vfio_region_mmap().

@@ -172,10 +174,11 @@ struct VFIODeviceIOOps {
       /**
        * @get_region_info
        *
-     * Fill in @info with information on the region given by @info->index.
+     * Fill in @info (and optionally @fd) with information on the region given
+     * by @info->index.
        */

Could you please update the whole documentation of the VFIODeviceIOOps
struct and document each parameter ?

Sorry, not sure what you're asking for. This struct was fully documented in
38bf025d ("vfio: add device IO ops vector")
and its subsequent patches.

yes. See the formatting of VFIODeviceOps :

    /**
     * @vfio_save_config
     *
     * Save device config state
     *
     * @vdev: #VFIODevice for which to save the config
     * @f: #QEMUFile where to send the data
     * @errp: pointer to Error*, to store an error if it happens.
     *
     * Returns zero to indicate success and negative for error
     */
    int (*vfio_save_config)(VFIODevice *vdev, QEMUFile *f, Error **errp);


No big deal. It can come later.

@@ -360,6 +369,7 @@ void vfio_device_init(VFIODevice *vbasedev, int type, 
VFIODeviceOps *ops,
       vbasedev->io_ops = &vfio_device_io_ops_ioctl;
       vbasedev->dev = dev;
       vbasedev->fd = -1;
+    vbasedev->use_region_fds = false;

why not extend vfio_device_init() with a 'region_fd_cache' bool
parameter instead ?

I could do, but you previously asked me not to add an "io_ops" parameter to this
function and instead directly change it here - why is this parameter different?

Because I thought we could allocate vbasedev->region_fds[] directly under
vfio_device_init().
 >> and avoid the extra VFIODevice attribute.

I don't get this - we still need to record the boolean in the vbasedev.

but we can't because we don't have the vbasedev->num_regions value yet.
So forget this proposal.

That said, there are a few contortions around the instance_init()
handler, vfio_device_init() and the realize() handler that I find
confusing. I will see if it can be improved when the code is stable
again.

Thanks,

C.