Re: [PATCH] vfio: Reuse file f_inode as vfio device inode

[Yan Zhao <yan.y.zhao@intel.com>]

On Wed, Jun 26, 2024 at 04:36:26PM +0800, Tian, Kevin wrote:
> > From: Zhao, Yan Y <yan.y.zhao@intel.com>
> > Sent: Thursday, June 20, 2024 6:15 PM
> > 
> > On Mon, Jun 17, 2024 at 05:53:32PM +0800, Yan Zhao wrote:
> > ...
> > > diff --git a/drivers/vfio/group.c b/drivers/vfio/group.c
> > > index ded364588d29..aaef188003b6 100644
> > > --- a/drivers/vfio/group.c
> > > +++ b/drivers/vfio/group.c
> > > @@ -268,31 +268,14 @@ static struct file *vfio_device_open_file(struct
> > vfio_device *device)
> > >  	if (ret)
> > >  		goto err_free;
> > >
> > > -	/*
> > > -	 * We can't use anon_inode_getfd() because we need to modify
> > > -	 * the f_mode flags directly to allow more than just ioctls
> > > -	 */
> > > -	filep = anon_inode_getfile("[vfio-device]", &vfio_device_fops,
> > > -				   df, O_RDWR);
> > > +	filep = vfio_device_get_pseudo_file(device);
> > If getting an inode from vfio_fs_type is not a must, maybe we could use
> > anon_inode_create_getfile() here?
> > Then changes to group.c and vfio_main.c can be simplified as below:
> 
> not familiar with file system, but at a glance the anon_inodefs is similar
> to vfio's own pseudo fs so it might work. anyway what is required here
> is to have an unique inode per vfio device to hold an unique address space
> and anon_inode_create_getfile() appears to achieve it.
Right.
 
> > 
> > diff --git a/drivers/vfio/group.c b/drivers/vfio/group.c
> > index ded364588d29..7f2f7871403f 100644
> > --- a/drivers/vfio/group.c
> > +++ b/drivers/vfio/group.c
> > @@ -269,29 +269,22 @@ static struct file *vfio_device_open_file(struct
> > vfio_device *device)
> >                 goto err_free;
> > 
> >         /*
> > -        * We can't use anon_inode_getfd() because we need to modify
> > -        * the f_mode flags directly to allow more than just ioctls
> > +        * Get a unique inode from anon_inodefs
> >          */
> > -       filep = anon_inode_getfile("[vfio-device]", &vfio_device_fops,
> > -                                  df, O_RDWR);
> > +       filep = anon_inode_create_getfile("[vfio-device]", &vfio_device_fops, df,
> > +                                         O_RDWR, NULL);
> >         if (IS_ERR(filep)) {
> >                 ret = PTR_ERR(filep);
> >                 goto err_close_device;
> >         }
> > -
> > -       /*
> > -        * TODO: add an anon_inode interface to do this.
> > -        * Appears to be missing by lack of need rather than
> > -        * explicitly prevented.  Now there's need.
> > -        */
> 
> why removing this comment?
I found it's confusing, as now an anon_inode is available but
"filep->f_mode |= (FMODE_PREAD | FMODE_PWRITE)" still cannot be avoided.
To avoid it, the file needs be opened through do_dentry_open() interface,
which is not easily achieved.


> >         filep->f_mode |= (FMODE_PREAD | FMODE_PWRITE);
> > 
> >         /*
> > -        * Use the pseudo fs inode on the device to link all mmaps
> > -        * to the same address space, allowing us to unmap all vmas
> > -        * associated to this device using unmap_mapping_range().
> > +        * mmaps are linked to the address space of the filep->f_inode.
> > +        * Save the inode in device->inode to allow unmap_mapping_range() to
> > +        * unmap all vmas.
> >          */
> > -       filep->f_mapping = device->inode->i_mapping;
> > +       device->inode = filep->f_inode;
> > 
> >         if (device->group->type == VFIO_NO_IOMMU)
> >                 dev_warn(device->dev, "vfio-noiommu device opened by user "
> > 
> > diff --git a/drivers/vfio/vfio_main.c b/drivers/vfio/vfio_main.c
> > index a5a62d9d963f..c9dac788411b 100644
> > --- a/drivers/vfio/vfio_main.c
> > +++ b/drivers/vfio/vfio_main.c
> > @@ -192,8 +192,6 @@ static void vfio_device_release(struct device *dev)
> >         if (device->ops->release)
> >                 device->ops->release(device);
> > 
> > -       iput(device->inode);
> > -       simple_release_fs(&vfio.vfs_mount, &vfio.fs_count);
> >         kvfree(device);
> >  }
> > 
> > @@ -248,22 +246,6 @@ static struct file_system_type vfio_fs_type = {
> >         .kill_sb = kill_anon_super,
> >  };
> 
> then vfio_fs_type can be removed too.
Right. Thanks for catching it.

> > 
> > -static struct inode *vfio_fs_inode_new(void)
> > -{
> > -       struct inode *inode;
> > -       int ret;
> > -
> > -       ret = simple_pin_fs(&vfio_fs_type, &vfio.vfs_mount, &vfio.fs_count);
> > -       if (ret)
> > -               return ERR_PTR(ret);
> > -
> > -       inode = alloc_anon_inode(vfio.vfs_mount->mnt_sb);
> > -       if (IS_ERR(inode))
> > -               simple_release_fs(&vfio.vfs_mount, &vfio.fs_count);
> > -
> > -       return inode;
> > -}
> > -
> >  /*
> >   * Initialize a vfio_device so it can be registered to vfio core.
> >   */
> > @@ -282,11 +264,6 @@ static int vfio_init_device(struct vfio_device *device,
> > struct device *dev,
> >         init_completion(&device->comp);
> >         device->dev = dev;
> >         device->ops = ops;
> > -       device->inode = vfio_fs_inode_new();
> > -       if (IS_ERR(device->inode)) {
> > -               ret = PTR_ERR(device->inode);
> > -               goto out_inode;
> > -       }
> > 
> >         if (ops->init) {
> >                 ret = ops->init(device);
> > @@ -301,9 +278,6 @@ static int vfio_init_device(struct vfio_device *device,
> > struct device *dev,
> >         return 0;
> > 
> >  out_uninit:
> > -       iput(device->inode);
> > -       simple_release_fs(&vfio.vfs_mount, &vfio.fs_count);
> > -out_inode:
> >         vfio_release_device_set(device);
> >         ida_free(&vfio.device_ida, device->index);
> >         return ret;
> > 
> > 
> > > diff --git a/drivers/vfio/vfio.h b/drivers/vfio/vfio.h
> > > index 50128da18bca..1f8915f79fbb 100644
> > > --- a/drivers/vfio/vfio.h
> > > +++ b/drivers/vfio/vfio.h
> > > @@ -35,6 +35,7 @@ struct vfio_device_file *
> > >  vfio_allocate_device_file(struct vfio_device *device);
> > >
> > >  extern const struct file_operations vfio_device_fops;
> > > +struct file *vfio_device_get_pseudo_file(struct vfio_device *device);
> > >
> > >  #ifdef CONFIG_VFIO_NOIOMMU
> > >  extern bool vfio_noiommu __read_mostly;
> > > @@ -420,6 +421,7 @@ static inline void vfio_cdev_cleanup(void)
> > >  {
> > >  }
> > >  #endif /* CONFIG_VFIO_DEVICE_CDEV */
> > > +struct file *vfio_device_get_pseduo_file(struct vfio_device *device);
> > Sorry, this line was included by mistake.