On 2022-12-09 14:47, Jason Gunthorpe wrote:
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;
Isn't the problem with this that it's super-early, and a device's MSI domain may not actually be resolved until someone starts requesting MSIs for it? Maybe Thomas' ongoing per-device stuff changes that, but I'm not sure :/
Furthermore, even if the system does have a topology with multiple heterogeneous MSI controllers reachable by devices behind the same IOMMU, and the IRQ layer decides to choose each device's MSI parent at random, that's hardly the IOMMU layer's problem, and shouldn't prevent the devices being usable by kernel drivers for whom MSI isolation doesn't matter.
Thanks, Robin.
+ + 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);
