Re: [RFC] iommufd: Use accurate dev_id in the PRI forwarding path

[Yi Liu <yi.l.liu@intel.com>]

On 2025/3/6 11:30, Yi Liu wrote:
> On 2025/3/6 11:10, Tian, Kevin wrote:
>>> From: Jason Gunthorpe <jgg@nvidia.com>
>>> Sent: Thursday, March 6, 2025 1:19 AM
>>>
>>> On Wed, Mar 05, 2025 at 05:03:56AM -0800, Yi Liu wrote:
>>>> Hi Jason, Kevin, Baolu,
>>>>
>>>> This is more a query, the patch here does not really work as there
>>>> is locking issue. So I'd like to hear from you if any good idea.
>>>>
>>>> Detail as below:
>>>>
>>>> Existing code has a problem when the PRI happens on a device that has
>>>> alias. The PRI reporting path uses the idev stored in the handle. While
>>>> this idev is the first idev that attachs to the domain.  If the PRI
>>>> happens on devices other than the first attached device, then the idev
>>>> stored in handle does not match.
>>>
>>> Do you actually care about this? Can we say that devices that have
>>> aliases do not allow PRI?
> 
> It would be simpler if we can enforce it.

how about something like the below?

diff --git a/drivers/iommu/iommu-priv.h b/drivers/iommu/iommu-priv.h
index b4508423e13b..faefe06c212c 100644
--- a/drivers/iommu/iommu-priv.h
+++ b/drivers/iommu/iommu-priv.h
@@ -32,6 +32,8 @@ void iommu_device_unregister_bus(struct iommu_device *iommu,
  				 const struct bus_type *bus,
  				 struct notifier_block *nb);

+bool iommu_group_is_singular(struct iommu_group *group);
+
  struct iommu_attach_handle *iommu_attach_handle_get(struct iommu_group 
*group,
  						    ioasid_t pasid,
  						    unsigned int type);
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 0ee17893810f..7b24b49110c6 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -1040,6 +1040,18 @@ void *iommu_group_get_iommudata(struct iommu_group 
*group)
  }
  EXPORT_SYMBOL_GPL(iommu_group_get_iommudata);

+bool iommu_group_is_singular(struct iommu_group *group)
+{
+	bool res;
+
+	mutex_lock(&group->mutex);
+	res = list_count_nodes(&group->devices) == 1;
+	mutex_unlock(&group->mutex);
+
+	return res;
+}
+EXPORT_SYMBOL_NS_GPL(iommu_group_is_singular, "IOMMUFD_INTERNAL");
+
  /**
   * iommu_group_set_iommudata - set iommu_data for a group
   * @group: the group
diff --git a/drivers/iommu/iommufd/device.c b/drivers/iommu/iommufd/device.c
index 92dc1ca010dc..6a1ca3874806 100644
--- a/drivers/iommu/iommufd/device.c
+++ b/drivers/iommu/iommufd/device.c
@@ -591,6 +591,9 @@ int iommufd_hw_pagetable_attach(struct 
iommufd_hw_pagetable *hwpt,
  	struct iommufd_hwpt_paging *hwpt_paging = find_hwpt_paging(hwpt);
  	int rc;

+	if (hwpt->fault && !iommu_group_is_singular(idev->igroup->group))
+		return -EOPNOTSUPP;
+
  	mutex_lock(&idev->igroup->lock);

  	if (idev->igroup->hwpt != NULL && idev->igroup->hwpt != hwpt) {
@@ -720,6 +723,9 @@ iommufd_device_do_replace(struct iommufd_device *idev,
  	unsigned int num_devices;
  	int rc;

+	if (hwpt->fault && !iommu_group_is_singular(idev->igroup->group))
+		return -EOPNOTSUPP;
+
  	mutex_lock(&idev->igroup->lock);

  	if (igroup->hwpt == NULL) {
diff --git a/drivers/iommu/iommufd/hw_pagetable.c 
b/drivers/iommu/iommufd/hw_pagetable.c
index 268315b1d8bc..bd2bab5a3b86 100644
--- a/drivers/iommu/iommufd/hw_pagetable.c
+++ b/drivers/iommu/iommufd/hw_pagetable.c
@@ -405,6 +405,11 @@ int iommufd_hwpt_alloc(struct iommufd_ucmd *ucmd)
  	if (cmd->flags & IOMMU_HWPT_FAULT_ID_VALID) {
  		struct iommufd_fault *fault;

+		if (!iommu_group_is_singular(idev->igroup->group)) {
+			rc = -EOPNOTSUPP;
+			goto out_hwpt;
+		}
+
  		fault = iommufd_get_fault(ucmd, cmd->fault_id);
  		if (IS_ERR(fault)) {
  			rc = PTR_ERR(fault);

-- 
Regards,
Yi Liu