RE: [PATCH v11 21/23] vfio: Determine noiommu device in __vfio_register_dev()

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



> From: Alex Williamson <alex.williamson@xxxxxxxxxx>
> Sent: Tuesday, May 23, 2023 7:04 AM
> 
> On Sat, 13 May 2023 06:28:25 -0700
> Yi Liu <yi.l.liu@xxxxxxxxx> wrote:
> 
> > This is to make the cdev path and group path consistent for the noiommu
> > devices registration. If vfio_noiommu is disabled, such registration
> > should fail. However, this check is vfio_device_set_group() which is part
> > of the vfio_group code. If the vfio_group code is compiled out, noiommu
> > devices would be registered even vfio_noiommu is disabled.
> >
> > This adds vfio_device_set_noiommu() which can fail and calls it in the
> > device registration. For now, it never fails as long as
> > vfio_device_set_group() is successful. But when the vfio_group code is
> > compiled out, vfio_device_set_noiommu() would fail the noiommu devices
> > when vfio_noiommu is disabled.
> 
> I'm lost.  After the next patch we end up with the following when
> CONFIG_VFIO_GROUP is set:
> 
> static inline int vfio_device_set_noiommu(struct vfio_device *device)
> {
>         device->noiommu = IS_ENABLED(CONFIG_VFIO_NOIOMMU) &&
>                           device->group->type == VFIO_NO_IOMMU;
>         return 0;
> }
> 
> I think this is relying on the fact that vfio_device_set_group() which
> is called immediately prior to this function would have performed the
> testing for noiommu and failed prior to this function being called and
> therefore there is no error return here.

Yes.

> 
> Note also here that I think CONFIG_VFIO_NOIOMMU was only being tested
> here previously so that a smart enough compiler would optimize out the
> entire function, we can never set a VFIO_NO_IOMMU type when
> !CONFIG_VFIO_NOIOMMU.

You are right. VFIO_NO_IOMMU type is only set when vfio_noiommu==true.

> That's no longer the case if the function is
> refactored like this.
> 
> When !CONFIG_VFIO_GROUP:
> 
> static inline int vfio_device_set_noiommu(struct vfio_device *device)
> {
>         struct iommu_group *iommu_group;
> 
>         iommu_group = iommu_group_get(device->dev);
>         if (!iommu_group) {
>                 if (!IS_ENABLED(CONFIG_VFIO_NOIOMMU) || !vfio_noiommu)
>                         return -EINVAL;
>                 device->noiommu = true;
>         } else {
>                 iommu_group_put(iommu_group);
>                 device->noiommu = false;
>         }
> 
>         return 0;
> }
> 
> Here again, the NOIOMMU config option is irrelevant, vfio_noiommu can
> only be true if the config option is enabled.  Therefore if there's no
> IOMMU group and the module option to enable noiommu is not set, return
> an error.

Yes. I think I missed the part that the vfio_noiommu option can only be
true when CONFIG_VFIO_NOIOMMU is enabled. That's why the check
is "IS_ENABLED(CONFIG_VFIO_NOIOMMU) && device->group->type == VFIO_NO_IOMMU;".
This appears that the two conditions are orthogonal.

> 
> It's really quite ugly that in one mode we rely on this function to
> generate the error and in the other mode it happens prior to getting
> here.
> 
> The above could be simplified to something like:
> 
> 	iommu_group = iommu_group_get(device->dev);
> 	if (!iommu_group && !vfio_iommu)
> 		return -EINVAL;
> 
> 	device->noiommu = !iommu_group;
> 	iommu_group_put(iommu_group); /* Accepts NULL */
> 	return 0;
> 
> Which would actually work regardless of CONFIG_VFIO_GROUP, where maybe
> this could then be moved before vfio_device_set_group() and become the
> de facto exit point for invalid noiommu configurations and maybe we
> could remove the test from the group code (with a comment to note that
> it's been tested prior)?  Thanks,

Yes, this looks much clearer. I think this new logic must be called before
vfio_device_set_group(). Otherwise,  iommu_group_get () may return
the faked groups. Then it would need to work differently per CONFIG_VFIO_GROUP
configuration.

Regards,
Yi Liu
> 
> > As noiommu devices is checked and there are multiple places which needs
> > to test noiommu devices, this also adds a flag to mark noiommu devices.
> > Hence the callers of vfio_device_is_noiommu() can be converted to test
> > vfio_device->noiommu.
> >
> > Reviewed-by: Kevin Tian <kevin.tian@xxxxxxxxx>
> > Tested-by: Nicolin Chen <nicolinc@xxxxxxxxxx>
> > Tested-by: Yanting Jiang <yanting.jiang@xxxxxxxxx>
> > Signed-off-by: Yi Liu <yi.l.liu@xxxxxxxxx>
> > ---
> >  drivers/vfio/device_cdev.c |  4 ++--
> >  drivers/vfio/group.c       |  2 +-
> >  drivers/vfio/iommufd.c     | 10 +++++-----
> >  drivers/vfio/vfio.h        |  7 ++++---
> >  drivers/vfio/vfio_main.c   |  6 +++++-
> >  include/linux/vfio.h       |  1 +
> >  6 files changed, 18 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/vfio/device_cdev.c b/drivers/vfio/device_cdev.c
> > index 3f14edb80a93..6d7f50ee535d 100644
> > --- a/drivers/vfio/device_cdev.c
> > +++ b/drivers/vfio/device_cdev.c
> > @@ -111,7 +111,7 @@ long vfio_device_ioctl_bind_iommufd(struct vfio_device_file
> *df,
> >  	if (df->group)
> >  		return -EINVAL;
> >
> > -	if (vfio_device_is_noiommu(device) && !capable(CAP_SYS_RAWIO))
> > +	if (device->noiommu && !capable(CAP_SYS_RAWIO))
> >  		return -EPERM;
> >
> >  	ret = vfio_device_block_group(device);
> > @@ -157,7 +157,7 @@ long vfio_device_ioctl_bind_iommufd(struct vfio_device_file
> *df,
> >  	device->cdev_opened = true;
> >  	mutex_unlock(&device->dev_set->lock);
> >
> > -	if (vfio_device_is_noiommu(device))
> > +	if (device->noiommu)
> >  		dev_warn(device->dev, "noiommu device is bound to iommufd by user
> "
> >  			 "(%s:%d)\n", current->comm, task_pid_nr(current));
> >  	return 0;
> > diff --git a/drivers/vfio/group.c b/drivers/vfio/group.c
> > index 7aacbd9d08c9..bf4335bce892 100644
> > --- a/drivers/vfio/group.c
> > +++ b/drivers/vfio/group.c
> > @@ -192,7 +192,7 @@ static int vfio_device_group_open(struct vfio_device_file *df)
> >  		vfio_device_group_get_kvm_safe(device);
> >
> >  	df->iommufd = device->group->iommufd;
> > -	if (df->iommufd && vfio_device_is_noiommu(device) && device->open_count
> == 0) {
> > +	if (df->iommufd && device->noiommu && device->open_count == 0) {
> >  		ret = vfio_iommufd_compat_probe_noiommu(device,
> >  							df->iommufd);
> >  		if (ret)
> > diff --git a/drivers/vfio/iommufd.c b/drivers/vfio/iommufd.c
> > index 799ea322a7d4..dfe706f1e952 100644
> > --- a/drivers/vfio/iommufd.c
> > +++ b/drivers/vfio/iommufd.c
> > @@ -71,7 +71,7 @@ int vfio_iommufd_bind(struct vfio_device_file *df)
> >
> >  	lockdep_assert_held(&vdev->dev_set->lock);
> >
> > -	if (vfio_device_is_noiommu(vdev))
> > +	if (vdev->noiommu)
> >  		return vfio_iommufd_noiommu_bind(vdev, ictx, &df->devid);
> >
> >  	return vdev->ops->bind_iommufd(vdev, ictx, &df->devid);
> > @@ -86,7 +86,7 @@ int vfio_iommufd_compat_attach_ioas(struct vfio_device *vdev,
> >  	lockdep_assert_held(&vdev->dev_set->lock);
> >
> >  	/* compat noiommu does not need to do ioas attach */
> > -	if (vfio_device_is_noiommu(vdev))
> > +	if (vdev->noiommu)
> >  		return 0;
> >
> >  	ret = iommufd_vfio_compat_ioas_get_id(ictx, &ioas_id);
> > @@ -103,7 +103,7 @@ void vfio_iommufd_unbind(struct vfio_device_file *df)
> >
> >  	lockdep_assert_held(&vdev->dev_set->lock);
> >
> > -	if (vfio_device_is_noiommu(vdev)) {
> > +	if (vdev->noiommu) {
> >  		vfio_iommufd_noiommu_unbind(vdev);
> >  		return;
> >  	}
> > @@ -116,7 +116,7 @@ int vfio_iommufd_attach(struct vfio_device *vdev, u32 *pt_id)
> >  {
> >  	lockdep_assert_held(&vdev->dev_set->lock);
> >
> > -	if (vfio_device_is_noiommu(vdev))
> > +	if (vdev->noiommu)
> >  		return 0;
> >
> >  	return vdev->ops->attach_ioas(vdev, pt_id);
> > @@ -126,7 +126,7 @@ void vfio_iommufd_detach(struct vfio_device *vdev)
> >  {
> >  	lockdep_assert_held(&vdev->dev_set->lock);
> >
> > -	if (!vfio_device_is_noiommu(vdev))
> > +	if (!vdev->noiommu)
> >  		vdev->ops->detach_ioas(vdev);
> >  }
> >
> > diff --git a/drivers/vfio/vfio.h b/drivers/vfio/vfio.h
> > index 50553f67600f..c8579d63b2b9 100644
> > --- a/drivers/vfio/vfio.h
> > +++ b/drivers/vfio/vfio.h
> > @@ -106,10 +106,11 @@ bool vfio_device_has_container(struct vfio_device *device);
> >  int __init vfio_group_init(void);
> >  void vfio_group_cleanup(void);
> >
> > -static inline bool vfio_device_is_noiommu(struct vfio_device *vdev)
> > +static inline int vfio_device_set_noiommu(struct vfio_device *device)
> >  {
> > -	return IS_ENABLED(CONFIG_VFIO_NOIOMMU) &&
> > -	       vdev->group->type == VFIO_NO_IOMMU;
> > +	device->noiommu = IS_ENABLED(CONFIG_VFIO_NOIOMMU) &&
> > +			  device->group->type == VFIO_NO_IOMMU;
> > +	return 0;
> >  }
> >
> >  #if IS_ENABLED(CONFIG_VFIO_CONTAINER)
> > diff --git a/drivers/vfio/vfio_main.c b/drivers/vfio/vfio_main.c
> > index 8c3f26b4929b..8979f320d620 100644
> > --- a/drivers/vfio/vfio_main.c
> > +++ b/drivers/vfio/vfio_main.c
> > @@ -289,8 +289,12 @@ static int __vfio_register_dev(struct vfio_device *device,
> >  	if (ret)
> >  		return ret;
> >
> > +	ret = vfio_device_set_noiommu(device);
> > +	if (ret)
> > +		goto err_out;
> > +
> >  	ret = dev_set_name(&device->device, "%svfio%d",
> > -			   vfio_device_is_noiommu(device) ? "noiommu-" : "", device-
> >index);
> > +			   device->noiommu ? "noiommu-" : "", device->index);
> >  	if (ret)
> >  		goto err_out;
> >
> > diff --git a/include/linux/vfio.h b/include/linux/vfio.h
> > index cf9d082a623c..fa13889e763f 100644
> > --- a/include/linux/vfio.h
> > +++ b/include/linux/vfio.h
> > @@ -68,6 +68,7 @@ struct vfio_device {
> >  	bool iommufd_attached;
> >  #endif
> >  	bool cdev_opened:1;
> > +	bool noiommu:1;
> >  };
> >
> >  /**





[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[Index of Archives]     [Kernel Development]     [Kernel Newbies]     [IDE]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite Info]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux ATA RAID]     [Samba]     [Linux Media]     [Device Mapper]

  Powered by Linux