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.
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. 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.
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,
Alex
> 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;
> };
>
> /**
