> From: Alex Williamson <alex.williamson@xxxxxxxxxx>
> Sent: Tuesday, May 23, 2023 4:25 AM
>
> On Sat, 13 May 2023 06:28:14 -0700
> Yi Liu <yi.l.liu@xxxxxxxxx> wrote:
>
> > into vfio_device_group_open(). This is more consistent with what will
> > be done in vfio device cdev path.
>
> Same comment regarding flowing commit subject into body on this series.
Yes, I've modified the commit message in my local branch.:-)
> >
> > Reviewed-by: Kevin Tian <kevin.tian@xxxxxxxxx>
> > Tested-by: Terrence Xu <terrence.xu@xxxxxxxxx>
> > Tested-by: Nicolin Chen <nicolinc@xxxxxxxxxx>
> > Tested-by: Yanting Jiang <yanting.jiang@xxxxxxxxx>
> > Tested-by: Shameer Kolothum <shameerali.kolothum.thodi@xxxxxxxxxx>
> > Signed-off-by: Yi Liu <yi.l.liu@xxxxxxxxx>
> > ---
> > drivers/vfio/group.c | 6 ++++++
> > drivers/vfio/iommufd.c | 32 +++++++++++++++++++-------------
> > drivers/vfio/vfio.h | 9 +++++++++
> > 3 files changed, 34 insertions(+), 13 deletions(-)
> >
> > diff --git a/drivers/vfio/group.c b/drivers/vfio/group.c
> > index a17584e8be15..cfd0b9254bbc 100644
> > --- a/drivers/vfio/group.c
> > +++ b/drivers/vfio/group.c
> > @@ -192,6 +192,12 @@ 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) {
> > + ret = vfio_iommufd_compat_probe_noiommu(device,
> > + df->iommufd);
> > + if (ret)
> > + goto out_put_kvm;
> > + }
> >
> > ret = vfio_device_open(df);
> > if (ret) {
> > diff --git a/drivers/vfio/iommufd.c b/drivers/vfio/iommufd.c
> > index a18e920be164..7a654a1437f0 100644
> > --- a/drivers/vfio/iommufd.c
> > +++ b/drivers/vfio/iommufd.c
> > @@ -46,6 +46,24 @@ static void vfio_iommufd_noiommu_unbind(struct vfio_device
> *vdev)
> > }
> > }
> >
> > +int vfio_iommufd_compat_probe_noiommu(struct vfio_device *device,
> > + struct iommufd_ctx *ictx)
> > +{
> > + u32 ioas_id;
> > +
> > + if (!capable(CAP_SYS_RAWIO))
> > + return -EPERM;
> > +
> > + /*
> > + * Require no compat ioas to be assigned to proceed. The basic
> > + * statement is that the user cannot have done something that
> > + * implies they expected translation to exist
> > + */
> > + if (!iommufd_vfio_compat_ioas_get_id(ictx, &ioas_id))
> > + return -EPERM;
> > + return 0;
> > +}
>
> I think the purpose of this function is to keep the iommufd namespace
> out of the group,
Yes.
> but we're muddying it as a general grab bag of
> noiommu validation. What if the caller retained the RAWIO test and
> comment, and this function simply became a wrapper around the iommufd
> function, ex:
>
> bool vfio_iommufd_device_has_compat_ioas(struct vfio_device *device,
> struct iommufd_ctx *ictx)
> {
> u32 ioas_id;
>
> return !iommufd_vfio_compat_ioas_get_id(ictx, &ioas_id));
> }
Sure. This looks better.
Regards,
Yi Liu
> Thanks,
> Alex
>
> > +
> > int vfio_iommufd_bind(struct vfio_device *vdev, struct iommufd_ctx *ictx)
> > {
> > u32 ioas_id;
> > @@ -54,20 +72,8 @@ int vfio_iommufd_bind(struct vfio_device *vdev, struct
> iommufd_ctx *ictx)
> >
> > lockdep_assert_held(&vdev->dev_set->lock);
> >
> > - if (vfio_device_is_noiommu(vdev)) {
> > - if (!capable(CAP_SYS_RAWIO))
> > - return -EPERM;
> > -
> > - /*
> > - * Require no compat ioas to be assigned to proceed. The basic
> > - * statement is that the user cannot have done something that
> > - * implies they expected translation to exist
> > - */
> > - if (!iommufd_vfio_compat_ioas_get_id(ictx, &ioas_id))
> > - return -EPERM;
> > -
> > + if (vfio_device_is_noiommu(vdev))
> > return vfio_iommufd_noiommu_bind(vdev, ictx, &device_id);
> > - }
> >
> > ret = vdev->ops->bind_iommufd(vdev, ictx, &device_id);
> > if (ret)
> > diff --git a/drivers/vfio/vfio.h b/drivers/vfio/vfio.h
> > index 785afc40ece8..8884b557fb26 100644
> > --- a/drivers/vfio/vfio.h
> > +++ b/drivers/vfio/vfio.h
> > @@ -234,9 +234,18 @@ static inline void vfio_container_cleanup(void)
> > #endif
> >
> > #if IS_ENABLED(CONFIG_IOMMUFD)
> > +int vfio_iommufd_compat_probe_noiommu(struct vfio_device *device,
> > + struct iommufd_ctx *ictx);
> > int vfio_iommufd_bind(struct vfio_device *device, struct iommufd_ctx *ictx);
> > void vfio_iommufd_unbind(struct vfio_device *device);
> > #else
> > +static inline int
> > +vfio_iommufd_compat_probe_noiommu(struct vfio_device *device,
> > + struct iommufd_ctx *ictx)
> > +{
> > + return -EOPNOTSUPP;
> > +}
> > +
> > static inline int vfio_iommufd_bind(struct vfio_device *device,
> > struct iommufd_ctx *ictx)
> > {
