> From: Alex Williamson <alex.williamson@xxxxxxxxxx>
> Sent: Wednesday, June 14, 2023 4:11 AM
>
> On Tue, 13 Jun 2023 14:35:09 -0300
> Jason Gunthorpe <jgg@xxxxxxxxxx> 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.
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);
}
#else
blabla
#endif
Regards,
Yi Liu
