Re: [PATCH iommufd 4/9] iommufd: Convert to msi_device_has_secure_msi()

[Jason Gunthorpe <jgg@xxxxxxxxxx>]

On Fri, Dec 09, 2022 at 06:01:14AM +0000, Tian, Kevin wrote:
> > From: Jason Gunthorpe <jgg@xxxxxxxxxx>
> > Sent: Friday, December 9, 2022 4:27 AM
> >
> > @@ -170,7 +170,7 @@ static int iommufd_device_setup_msi(struct
> > iommufd_device *idev,
> >  	 * interrupt outside this iommufd context.
> >  	 */
> >  	if (!device_iommu_capable(idev->dev, IOMMU_CAP_INTR_REMAP)
> > &&
> > -	    !irq_domain_check_msi_remap()) {
> > +	    !msi_device_has_secure_msi(idev->dev)) {
> >  		if (!allow_unsafe_interrupts)
> >  			return -EPERM;
> > 
> 
> this is where iommufd and vfio diverge.
> 
> vfio has a check to ensure all devices in the group has secure_msi.
> 
> but iommufd only imposes the check per device.

Ah, that is an interesting, though pedantic point.

So, let us do this and address the other point about vfio as well:

+++ b/drivers/iommu/iommu.c
@@ -941,6 +941,28 @@ static bool iommu_is_attach_deferred(struct device *dev)
        return false;
 }
 
+static int iommu_group_add_device_list(struct iommu_group *group,
+                                      struct group_device *group_dev)
+{
+       struct group_device *existing;
+
+       lockdep_assert_held(&group->mutex);
+
+       existing = list_first_entry_or_null(&group->devices,
+                                           struct group_device, list);
+
+       /*
+        * It is a driver bug to create groups with different irq_domain
+        * properties.
+        */
+       if (existing && msi_device_has_isolated_msi(existing->dev) !=
+                               msi_device_has_isolated_msi(group_dev->dev))
+               return -EINVAL;
+
+       list_add_tail(&group_dev->list, &group->devices);
+       return 0;
+}
+
 /**
  * iommu_group_add_device - add a device to an iommu group
  * @group: the group into which to add the device (reference should be held)
@@ -992,7 +1014,7 @@ int iommu_group_add_device(struct iommu_group *group, struct device *dev)
        dev->iommu_group = group;
 
        mutex_lock(&group->mutex);
-       list_add_tail(&device->list, &group->devices);
+       iommu_group_add_device_list(group, device);
        if (group->domain  && !iommu_is_attach_deferred(dev))
                ret = __iommu_attach_device(group->domain, dev);
        mutex_unlock(&group->mutex);