On Wed, Jun 26, 2024 at 04:36:26PM +0800, Tian, Kevin wrote: > > From: Zhao, Yan Y <yan.y.zhao@xxxxxxxxx> > > 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.