| Message ID | 20221124122702.26507-6-yi.l.liu@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Move group specific code into group.c | expand |
> From: Liu, Yi L <yi.l.liu@intel.com> > Sent: Thursday, November 24, 2022 8:27 PM > > This prepares for moving group specific code to separate file. > > Signed-off-by: Yi Liu <yi.l.liu@intel.com> > --- > drivers/vfio/vfio_main.c | 7 ++++--- > 1 file changed, 4 insertions(+), 3 deletions(-) > > diff --git a/drivers/vfio/vfio_main.c b/drivers/vfio/vfio_main.c > index edcfa8a61096..fcb9f778fc9b 100644 > --- a/drivers/vfio/vfio_main.c > +++ b/drivers/vfio/vfio_main.c > @@ -878,9 +878,6 @@ static struct file *vfio_device_open(struct vfio_device > *device) > */ > filep->f_mode |= (FMODE_PREAD | FMODE_PWRITE); > > - if (device->group->type == VFIO_NO_IOMMU) > - dev_warn(device->dev, "vfio-noiommu device opened by > user " > - "(%s:%d)\n", current->comm, task_pid_nr(current)); > /* > * On success the ref of device is moved to the file and > * put in vfio_device_fops_release() > @@ -927,6 +924,10 @@ static int vfio_group_ioctl_get_device_fd(struct > vfio_group *group, > goto err_put_fdno; > } > > + if (group->type == VFIO_NO_IOMMU) > + dev_warn(device->dev, "vfio-noiommu device opened by > user " > + "(%s:%d)\n", current->comm, task_pid_nr(current)); > + > fd_install(fdno, filep); > return fdno; > Do we want to support no-iommu mode in future cdev path? If yes keeping the check in vfio_device_open() makes more sense. Just replace direct device->group reference with a helper e.g.: vfio_device_group_noiommu()
On 2022/11/28 16:17, Tian, Kevin wrote: >> From: Liu, Yi L <yi.l.liu@intel.com> >> Sent: Thursday, November 24, 2022 8:27 PM >> >> This prepares for moving group specific code to separate file. >> >> Signed-off-by: Yi Liu <yi.l.liu@intel.com> >> --- >> drivers/vfio/vfio_main.c | 7 ++++--- >> 1 file changed, 4 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/vfio/vfio_main.c b/drivers/vfio/vfio_main.c >> index edcfa8a61096..fcb9f778fc9b 100644 >> --- a/drivers/vfio/vfio_main.c >> +++ b/drivers/vfio/vfio_main.c >> @@ -878,9 +878,6 @@ static struct file *vfio_device_open(struct vfio_device >> *device) >> */ >> filep->f_mode |= (FMODE_PREAD | FMODE_PWRITE); >> >> - if (device->group->type == VFIO_NO_IOMMU) >> - dev_warn(device->dev, "vfio-noiommu device opened by >> user " >> - "(%s:%d)\n", current->comm, task_pid_nr(current)); >> /* >> * On success the ref of device is moved to the file and >> * put in vfio_device_fops_release() >> @@ -927,6 +924,10 @@ static int vfio_group_ioctl_get_device_fd(struct >> vfio_group *group, >> goto err_put_fdno; >> } >> >> + if (group->type == VFIO_NO_IOMMU) >> + dev_warn(device->dev, "vfio-noiommu device opened by >> user " >> + "(%s:%d)\n", current->comm, task_pid_nr(current)); >> + >> fd_install(fdno, filep); >> return fdno; >> > > Do we want to support no-iommu mode in future cdev path? > > If yes keeping the check in vfio_device_open() makes more sense. Just > replace direct device->group reference with a helper e.g.: > > vfio_device_group_noiommu() I didn't see a reason cdev cannot support no-iommu mode. so a helper to check noiommu is reasonable.
On 2022/11/28 17:19, Yi Liu wrote: > On 2022/11/28 16:17, Tian, Kevin wrote: >>> From: Liu, Yi L <yi.l.liu@intel.com> >>> Sent: Thursday, November 24, 2022 8:27 PM >>> >>> This prepares for moving group specific code to separate file. >>> >>> Signed-off-by: Yi Liu <yi.l.liu@intel.com> >>> --- >>> drivers/vfio/vfio_main.c | 7 ++++--- >>> 1 file changed, 4 insertions(+), 3 deletions(-) >>> >>> diff --git a/drivers/vfio/vfio_main.c b/drivers/vfio/vfio_main.c >>> index edcfa8a61096..fcb9f778fc9b 100644 >>> --- a/drivers/vfio/vfio_main.c >>> +++ b/drivers/vfio/vfio_main.c >>> @@ -878,9 +878,6 @@ static struct file *vfio_device_open(struct vfio_device >>> *device) >>> */ >>> filep->f_mode |= (FMODE_PREAD | FMODE_PWRITE); >>> >>> - if (device->group->type == VFIO_NO_IOMMU) >>> - dev_warn(device->dev, "vfio-noiommu device opened by >>> user " >>> - "(%s:%d)\n", current->comm, task_pid_nr(current)); >>> /* >>> * On success the ref of device is moved to the file and >>> * put in vfio_device_fops_release() >>> @@ -927,6 +924,10 @@ static int vfio_group_ioctl_get_device_fd(struct >>> vfio_group *group, >>> goto err_put_fdno; >>> } >>> >>> + if (group->type == VFIO_NO_IOMMU) >>> + dev_warn(device->dev, "vfio-noiommu device opened by >>> user " >>> + "(%s:%d)\n", current->comm, task_pid_nr(current)); >>> + >>> fd_install(fdno, filep); >>> return fdno; >>> >> >> Do we want to support no-iommu mode in future cdev path? >> >> If yes keeping the check in vfio_device_open() makes more sense. Just >> replace direct device->group reference with a helper e.g.: >> >> vfio_device_group_noiommu() > > I didn't see a reason cdev cannot support no-iommu mode. so a helper to > check noiommu is reasonable. This check should be done after opening device and the file. Current vfio_device_open() opens device first and then open file. Open file is group path specific, not needed in future device cdev path. So if want to have this check in the common function, the open device and open file order should be swapped. However, it is not necessary here. So may just drop this patch and consider it in future device cdev series.
On Thu, Dec 01, 2022 at 03:08:12PM +0800, Yi Liu wrote: > On 2022/11/28 17:19, Yi Liu wrote: > > On 2022/11/28 16:17, Tian, Kevin wrote: > > > > From: Liu, Yi L <yi.l.liu@intel.com> > > > > Sent: Thursday, November 24, 2022 8:27 PM > > > > > > > > This prepares for moving group specific code to separate file. > > > > > > > > Signed-off-by: Yi Liu <yi.l.liu@intel.com> > > > > --- > > > > drivers/vfio/vfio_main.c | 7 ++++--- > > > > 1 file changed, 4 insertions(+), 3 deletions(-) > > > > > > > > diff --git a/drivers/vfio/vfio_main.c b/drivers/vfio/vfio_main.c > > > > index edcfa8a61096..fcb9f778fc9b 100644 > > > > --- a/drivers/vfio/vfio_main.c > > > > +++ b/drivers/vfio/vfio_main.c > > > > @@ -878,9 +878,6 @@ static struct file *vfio_device_open(struct vfio_device > > > > *device) > > > > */ > > > > filep->f_mode |= (FMODE_PREAD | FMODE_PWRITE); > > > > > > > > - if (device->group->type == VFIO_NO_IOMMU) > > > > - dev_warn(device->dev, "vfio-noiommu device opened by > > > > user " > > > > - "(%s:%d)\n", current->comm, task_pid_nr(current)); > > > > /* > > > > * On success the ref of device is moved to the file and > > > > * put in vfio_device_fops_release() > > > > @@ -927,6 +924,10 @@ static int vfio_group_ioctl_get_device_fd(struct > > > > vfio_group *group, > > > > goto err_put_fdno; > > > > } > > > > > > > > + if (group->type == VFIO_NO_IOMMU) > > > > + dev_warn(device->dev, "vfio-noiommu device opened by > > > > user " > > > > + "(%s:%d)\n", current->comm, task_pid_nr(current)); > > > > + > > > > fd_install(fdno, filep); > > > > return fdno; > > > > > > > > > > Do we want to support no-iommu mode in future cdev path? > > > > > > If yes keeping the check in vfio_device_open() makes more sense. Just > > > replace direct device->group reference with a helper e.g.: > > > > > > vfio_device_group_noiommu() > > > > I didn't see a reason cdev cannot support no-iommu mode. so a helper to > > check noiommu is reasonable. > > This check should be done after opening device and the file. Current > vfio_device_open() opens device first and then open file. Open file is > group path specific, not needed in future device cdev path. So if want to > have this check in the common function, the open device and open file > order should be swapped. However, it is not necessary here. So may just > drop this patch and consider it in future device cdev series. the point here was to remove the device->group touches from vfio_main.c, which this does and seems appropraite. cdev no-iommu mode is going to be quite different since it will work without groups Jason
diff --git a/drivers/vfio/vfio_main.c b/drivers/vfio/vfio_main.c index edcfa8a61096..fcb9f778fc9b 100644 --- a/drivers/vfio/vfio_main.c +++ b/drivers/vfio/vfio_main.c @@ -878,9 +878,6 @@ static struct file *vfio_device_open(struct vfio_device *device) */ filep->f_mode |= (FMODE_PREAD | FMODE_PWRITE); - if (device->group->type == VFIO_NO_IOMMU) - dev_warn(device->dev, "vfio-noiommu device opened by user " - "(%s:%d)\n", current->comm, task_pid_nr(current)); /* * On success the ref of device is moved to the file and * put in vfio_device_fops_release() @@ -927,6 +924,10 @@ static int vfio_group_ioctl_get_device_fd(struct vfio_group *group, goto err_put_fdno; } + if (group->type == VFIO_NO_IOMMU) + dev_warn(device->dev, "vfio-noiommu device opened by user " + "(%s:%d)\n", current->comm, task_pid_nr(current)); + fd_install(fdno, filep); return fdno;
This prepares for moving group specific code to separate file. Signed-off-by: Yi Liu <yi.l.liu@intel.com> --- drivers/vfio/vfio_main.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)