> From: Tian, Kevin <[email protected]>
> Sent: Wednesday, June 14, 2023 1:42 PM
>
> > From: Liu, Yi L <[email protected]>
> > Sent: Wednesday, June 14, 2023 11:24 AM
> >
> > > From: Alex Williamson <[email protected]>
> > > Sent: Wednesday, June 14, 2023 4:11 AM
> > >
> > > On Tue, 13 Jun 2023 14:35:09 -0300
> > > Jason Gunthorpe <[email protected]> wrote:
> > >
> > > > On Tue, Jun 13, 2023 at 11:15:11AM -0600, Alex Williamson wrote:
> > > > > [Sorry for breaking threading, replying to my own message id with
> > > > > reply
> > > > > content from Yi since the Cc list got broken]
> > > >
> > > > Yikes it is really busted, I think I fixed it?
> > > >
> > > > > If we renamed your function above to vfio_device_has_iommu_group(),
> > > > > couldn't we just wrap device_add like below instead to not have cdev
> > > > > setup for a noiommu device, generate an error for a physical device
> > w/o
> > > > > IOMMU backing, and otherwise setup the cdev device?
> > > > >
> > > > > static inline int vfio_device_add(struct vfio_device *device, enum
> > vfio_group_type
> > > type)
> > > > > {
> > > > > #if IS_ENABLED(CONFIG_VFIO_GROUP)
> > > > > if (device->group->type == VFIO_NO_IOMMU)
> > > > > return device_add(&device->device);
> > > >
> > > > vfio_device_is_noiommu() embeds the IS_ENABLED
> > >
> > > But patch 23/ makes the definition of struct vfio_group conditional on
> > > CONFIG_VFIO_GROUP, so while CONFIG_VFIO_NOIOMMU depends on
> > > CONFIG_VFIO_GROUP and the result could be determined, I think the
> > > compiler is still unhappy about the undefined reference. We'd need a
> > > !CONFIG_VFIO_GROUP stub for the function.
> > >
> > > > > #else
> > > > > if (type == VFIO_IOMMU && !vfio_device_has_iommu_group(device))
> > > > > return -EINVAL;
> > > > > #endif
> > > >
> > > > The require test is this from the group code:
> > > >
> > > > if (!device_iommu_capable(dev, IOMMU_CAP_CACHE_COHERENCY))
> > {
> > > >
> > > > We could lift it out of the group code and call it from vfio_main.c
> > > > like:
> > > >
> > > > if (type == VFIO_IOMMU && !vfio_device_is_noiommu(vdev)
> > > && !device_iommu_capable(dev,
> > > > IOMMU_CAP_CACHE_COHERENCY))
> > > > FAIL
> > >
> > > Ack. Thanks,
> >
> > So, what I got is:
> >
> > 1) Add bellow check in __vfio_register_dev() to fail the physical devices
> > that
> > don't have IOMMU protection.
> >
> > /*
> > * noiommu device is a special type supported by the group interface.
> > * Such type represents the physical devices that are not iommu
> > backed.
> > */
> > if (type == VFIO_IOMMU && !vfio_device_is_noiommu(device)) &&
> > !vfio_device_has_iommu_group(device))
> > return -EINVAL; //or maybe -EOPNOTSUPP?
> >
> > Nit: require a vfio_device_is_noiommu() stub which returns false for
> > the VFIO_GROUP unset case.
>
> device_iommu_capable(dev, IOMMU_CAP_CACHE_COHERENCY) is valid
> only for cases with iommu groups. So that check already covers the
> group verification indirectly.
Okay. This IOMMU_CAP_CACHE_COHERENCY check is missed in the cdev
path.
> With that I think Jason's suggestion is to lift that test into main.c:
>
> int vfio_register_group_dev(struct vfio_device *device)
> {
> /*
> * VFIO always sets IOMMU_CACHE because we offer no way for userspace to
> * restore cache coherency. It has to be checked here because it is only
> * valid for cases where we are using iommu groups.
> */
> if (type == VFIO_IOMMU && !vfio_device_is_noiommu(device) &&
> !device_iommu_capable(dev, IOMMU_CAP_CACHE_COHERENCY))
> return ERR_PTR(-EINVAL);
vfio_device_is_noiommu() needs to be called after vfio_device_set_group().
Otherwise, it's always false. So still needs to call it in the
__vfio_register_dev().
> return __vfio_register_dev(device, VFIO_IOMMU);
> }
>
> >
> > 2) Have below functions to add device
> >
> > #if IS_ENABLED(CONFIG_VFIO_DEVICE_CDEV)
> > static inline int vfio_device_add(struct vfio_device *device)
> > {
> > if (vfio_device_is_noiommu(device))
> > return device_add(&device->device);
> > vfio_init_device_cdev(device);
> > return cdev_device_add(&device->cdev, &device->device);
> > }
> >
> > static inline void vfio_device_del(struct vfio_device *device)
> > {
> > if (vfio_device_is_noiommu(device))
> > return device_del(&device->device);
> > cdev_device_del(&device->cdev, &device->device);
> > }
>
> Correct
Regards,
Yi Liu
