[PATCH v2 0/3] iommu: Make pasid array per device

[PATCH v2 0/3] iommu: Make pasid array per device

[Lu Baolu]

The PCI PASID enabling interface guarantees that the address space used
by each PASID is unique. This is achieved by checking that the PCI ACS
path is enabled for the device. If the path is not enabled, then the
PASID feature cannot be used.

    if (!pci_acs_path_enabled(pdev, NULL, PCI_ACS_RR | PCI_ACS_UF))
            return -EINVAL;

The PASID array is not an attribute of the IOMMU group. It is more
natural to store the PASID array in the per-device IOMMU data. This
makes the code clearer and easier to understand.

Please help review and suggest.

Change log:
v2:
 - Add an explict check for single-device group in the domain attaching
   device pasid interface. (Jason)
 - Make assert_pasid_dma_ownership() returns true or false. Refactor the
   code in a way that does not change its exsiting behavior. (Kevin)

v1: https://lore.kernel.org/linux-iommu/20230801063125.34995-1-baolu.lu@linux.intel.com/

Lu Baolu (3):
  iommu: Make single-device group for PASID explicit
  iommu: Consolidate pasid dma ownership check
  iommu: Move pasid array from group to device

 include/linux/iommu.h |   2 +
 drivers/iommu/iommu.c | 102 +++++++++++++++++++-----------------------
 2 files changed, 49 insertions(+), 55 deletions(-)

-- 
2.34.1

[PATCH v2 1/3] iommu: Make single-device group for PASID explicit

[Lu Baolu]

The PASID interfaces have always supported only single-device groups.
This was first introduced in commit 26b25a2b98e45 ("iommu: Bind process
address spaces to devices"), and has been kept consistent in subsequent
commits.

However, the core code doesn't explicitly check for this requirement
after commit 201007ef707a8 ("PCI: Enable PASID only when ACS RR & UF
enabled on upstream path"), which made this requirement implicit.

Restore the check to make it explicit that the PASID interfaces only
support devices belonging to single-device groups.

Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
---
 drivers/iommu/iommu.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 71b9c41f2a9e..f1eba60e573f 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -3408,6 +3408,11 @@ int iommu_attach_device_pasid(struct iommu_domain *domain,
 		return -ENODEV;
 
 	mutex_lock(&group->mutex);
+	if (list_count_nodes(&group->devices) != 1) {
+		ret = -EINVAL;
+		goto out_unlock;
+	}
+
 	curr = xa_cmpxchg(&group->pasid_array, pasid, NULL, domain, GFP_KERNEL);
 	if (curr) {
 		ret = xa_err(curr) ? : -EBUSY;
-- 
2.34.1

[PATCH v2 2/3] iommu: Consolidate pasid dma ownership check

[Lu Baolu]

When switching device DMA ownership, it is required that all the device's
pasid DMA be disabled. This is done by checking if the pasid array of the
group is empty. Consolidate all the open code into a single helper. No
intentional functionality change.

Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
---
 drivers/iommu/iommu.c | 23 ++++++++++++++++++-----
 1 file changed, 18 insertions(+), 5 deletions(-)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index f1eba60e573f..d4a06a37ce39 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -3127,6 +3127,19 @@ static bool iommu_is_default_domain(struct iommu_group *group)
 	return false;
 }
 
+/*
+ * Assert no PASID DMA when claiming or releasing group's DMA ownership.
+ * The device pasid interfaces are only for device drivers that have
+ * claimed the DMA ownership. Return true if no pasid DMA setup, otherwise
+ * return false with a WARN().
+ */
+static bool assert_pasid_dma_ownership(struct iommu_group *group)
+{
+	lockdep_assert_held(&group->mutex);
+
+	return !WARN_ON(!xa_empty(&group->pasid_array));
+}
+
 /**
  * iommu_device_use_default_domain() - Device driver wants to handle device
  *                                     DMA through the kernel DMA API.
@@ -3147,7 +3160,7 @@ int iommu_device_use_default_domain(struct device *dev)
 	mutex_lock(&group->mutex);
 	if (group->owner_cnt) {
 		if (group->owner || !iommu_is_default_domain(group) ||
-		    !xa_empty(&group->pasid_array)) {
+		    !assert_pasid_dma_ownership(group)) {
 			ret = -EBUSY;
 			goto unlock_out;
 		}
@@ -3177,7 +3190,7 @@ void iommu_device_unuse_default_domain(struct device *dev)
 		return;
 
 	mutex_lock(&group->mutex);
-	if (!WARN_ON(!group->owner_cnt || !xa_empty(&group->pasid_array)))
+	if (!WARN_ON(!group->owner_cnt) && assert_pasid_dma_ownership(group))
 		group->owner_cnt--;
 
 	mutex_unlock(&group->mutex);
@@ -3211,7 +3224,7 @@ static int __iommu_take_dma_ownership(struct iommu_group *group, void *owner)
 	int ret;
 
 	if ((group->domain && group->domain != group->default_domain) ||
-	    !xa_empty(&group->pasid_array))
+	    !assert_pasid_dma_ownership(group))
 		return -EBUSY;
 
 	ret = __iommu_group_alloc_blocking_domain(group);
@@ -3296,8 +3309,8 @@ EXPORT_SYMBOL_GPL(iommu_device_claim_dma_owner);
 
 static void __iommu_release_dma_ownership(struct iommu_group *group)
 {
-	if (WARN_ON(!group->owner_cnt || !group->owner ||
-		    !xa_empty(&group->pasid_array)))
+	if (WARN_ON(!group->owner_cnt || !group->owner) ||
+	    !assert_pasid_dma_ownership(group))
 		return;
 
 	group->owner_cnt = 0;
-- 
2.34.1

[PATCH v2 3/3] iommu: Move pasid array from group to device

[Lu Baolu]

The PASID (Process Address Space ID) feature is a device feature that
allows a device driver to manage the PASID value and attach or detach
its domain to the pasid. The pasid array, which is used to store the
domain of each pasid, is currently stored in iommu group struct, but
it would be more natural to move it to the device so that the device
drivers don't need to understand the internal iommu group concept.

Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
---
 include/linux/iommu.h |  2 ++
 drivers/iommu/iommu.c | 80 +++++++++++++++----------------------------
 2 files changed, 29 insertions(+), 53 deletions(-)

diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index 411ce9b998dc..76c960741449 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -412,6 +412,7 @@ struct iommu_fault_param {
  * @iopf_param:	 I/O Page Fault queue and data
  * @fwspec:	 IOMMU fwspec data
  * @iommu_dev:	 IOMMU device this device is linked to
+ * @pasid_array: pasid-indexed array of domains attached to pasid
  * @priv:	 IOMMU Driver private data
  * @max_pasids:  number of PASIDs this device can consume
  * @attach_deferred: the dma domain attachment is deferred
@@ -427,6 +428,7 @@ struct dev_iommu {
 	struct iopf_device_param	*iopf_param;
 	struct iommu_fwspec		*fwspec;
 	struct iommu_device		*iommu_dev;
+	struct xarray			pasid_array;
 	void				*priv;
 	u32				max_pasids;
 	u32				attach_deferred:1;
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index d4a06a37ce39..35cccfb5c24a 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -51,7 +51,6 @@ struct iommu_group {
 	struct kobject kobj;
 	struct kobject *devices_kobj;
 	struct list_head devices;
-	struct xarray pasid_array;
 	struct mutex mutex;
 	void *iommu_data;
 	void (*iommu_data_release)(void *iommu_data);
@@ -310,6 +309,7 @@ static struct dev_iommu *dev_iommu_get(struct device *dev)
 		return NULL;
 
 	mutex_init(&param->lock);
+	xa_init(&param->pasid_array);
 	dev->iommu = param;
 	return param;
 }
@@ -919,7 +919,6 @@ struct iommu_group *iommu_group_alloc(void)
 	mutex_init(&group->mutex);
 	INIT_LIST_HEAD(&group->devices);
 	INIT_LIST_HEAD(&group->entry);
-	xa_init(&group->pasid_array);
 
 	ret = ida_alloc(&iommu_group_ida, GFP_KERNEL);
 	if (ret < 0) {
@@ -3135,9 +3134,15 @@ static bool iommu_is_default_domain(struct iommu_group *group)
  */
 static bool assert_pasid_dma_ownership(struct iommu_group *group)
 {
+	struct group_device *device;
+
 	lockdep_assert_held(&group->mutex);
+	for_each_group_device(group, device) {
+		if (WARN_ON(!xa_empty(&device->dev->iommu->pasid_array)))
+			return false;
+	}
 
-	return !WARN_ON(!xa_empty(&group->pasid_array));
+	return true;
 }
 
 /**
@@ -3371,33 +3376,6 @@ bool iommu_group_dma_owner_claimed(struct iommu_group *group)
 }
 EXPORT_SYMBOL_GPL(iommu_group_dma_owner_claimed);
 
-static int __iommu_set_group_pasid(struct iommu_domain *domain,
-				   struct iommu_group *group, ioasid_t pasid)
-{
-	struct group_device *device;
-	int ret = 0;
-
-	for_each_group_device(group, device) {
-		ret = domain->ops->set_dev_pasid(domain, device->dev, pasid);
-		if (ret)
-			break;
-	}
-
-	return ret;
-}
-
-static void __iommu_remove_group_pasid(struct iommu_group *group,
-				       ioasid_t pasid)
-{
-	struct group_device *device;
-	const struct iommu_ops *ops;
-
-	for_each_group_device(group, device) {
-		ops = dev_iommu_ops(device->dev);
-		ops->remove_dev_pasid(device->dev, pasid);
-	}
-}
-
 /*
  * iommu_attach_device_pasid() - Attach a domain to pasid of device
  * @domain: the iommu domain.
@@ -3411,6 +3389,7 @@ int iommu_attach_device_pasid(struct iommu_domain *domain,
 {
 	/* Caller must be a probed driver on dev */
 	struct iommu_group *group = dev->iommu_group;
+	struct dev_iommu *param = dev->iommu;
 	void *curr;
 	int ret;
 
@@ -3422,23 +3401,23 @@ int iommu_attach_device_pasid(struct iommu_domain *domain,
 
 	mutex_lock(&group->mutex);
 	if (list_count_nodes(&group->devices) != 1) {
-		ret = -EINVAL;
-		goto out_unlock;
+		mutex_unlock(&group->mutex);
+		return -EINVAL;
 	}
+	mutex_unlock(&group->mutex);
 
-	curr = xa_cmpxchg(&group->pasid_array, pasid, NULL, domain, GFP_KERNEL);
+	mutex_lock(&param->lock);
+	curr = xa_cmpxchg(&param->pasid_array, pasid, NULL, domain, GFP_KERNEL);
 	if (curr) {
 		ret = xa_err(curr) ? : -EBUSY;
 		goto out_unlock;
 	}
 
-	ret = __iommu_set_group_pasid(domain, group, pasid);
-	if (ret) {
-		__iommu_remove_group_pasid(group, pasid);
-		xa_erase(&group->pasid_array, pasid);
-	}
+	ret = domain->ops->set_dev_pasid(domain, dev, pasid);
+	if (ret)
+		xa_erase(&param->pasid_array, pasid);
 out_unlock:
-	mutex_unlock(&group->mutex);
+	mutex_unlock(&param->lock);
 	return ret;
 }
 EXPORT_SYMBOL_GPL(iommu_attach_device_pasid);
@@ -3455,13 +3434,13 @@ EXPORT_SYMBOL_GPL(iommu_attach_device_pasid);
 void iommu_detach_device_pasid(struct iommu_domain *domain, struct device *dev,
 			       ioasid_t pasid)
 {
-	/* Caller must be a probed driver on dev */
-	struct iommu_group *group = dev->iommu_group;
+	const struct iommu_ops *ops = dev_iommu_ops(dev);
+	struct dev_iommu *param = dev->iommu;
 
-	mutex_lock(&group->mutex);
-	__iommu_remove_group_pasid(group, pasid);
-	WARN_ON(xa_erase(&group->pasid_array, pasid) != domain);
-	mutex_unlock(&group->mutex);
+	mutex_lock(&param->lock);
+	ops->remove_dev_pasid(dev, pasid);
+	WARN_ON(xa_erase(&param->pasid_array, pasid) != domain);
+	mutex_unlock(&param->lock);
 }
 EXPORT_SYMBOL_GPL(iommu_detach_device_pasid);
 
@@ -3483,18 +3462,13 @@ struct iommu_domain *iommu_get_domain_for_dev_pasid(struct device *dev,
 						    ioasid_t pasid,
 						    unsigned int type)
 {
-	/* Caller must be a probed driver on dev */
-	struct iommu_group *group = dev->iommu_group;
 	struct iommu_domain *domain;
 
-	if (!group)
-		return NULL;
-
-	xa_lock(&group->pasid_array);
-	domain = xa_load(&group->pasid_array, pasid);
+	xa_lock(&dev->iommu->pasid_array);
+	domain = xa_load(&dev->iommu->pasid_array, pasid);
 	if (type && domain && domain->type != type)
 		domain = ERR_PTR(-EBUSY);
-	xa_unlock(&group->pasid_array);
+	xa_unlock(&dev->iommu->pasid_array);
 
 	return domain;
 }
-- 
2.34.1

RE: [PATCH v2 1/3] iommu: Make single-device group for PASID explicit

[Tian, Kevin]

> From: Lu Baolu <baolu.lu@linux.intel.com>
> Sent: Monday, August 14, 2023 9:18 AM
> 
> The PASID interfaces have always supported only single-device groups.
> This was first introduced in commit 26b25a2b98e45 ("iommu: Bind process
> address spaces to devices"), and has been kept consistent in subsequent
> commits.
> 
> However, the core code doesn't explicitly check for this requirement
> after commit 201007ef707a8 ("PCI: Enable PASID only when ACS RR & UF
> enabled on upstream path"), which made this requirement implicit.
> 
> Restore the check to make it explicit that the PASID interfaces only
> support devices belonging to single-device groups.
> 
> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
> ---
>  drivers/iommu/iommu.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index 71b9c41f2a9e..f1eba60e573f 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -3408,6 +3408,11 @@ int iommu_attach_device_pasid(struct
> iommu_domain *domain,
>  		return -ENODEV;
> 
>  	mutex_lock(&group->mutex);
> +	if (list_count_nodes(&group->devices) != 1) {
> +		ret = -EINVAL;
> +		goto out_unlock;
> +	}
> +

I wonder whether we should also block adding new device to this
group once the single-device has pasid enabled. Otherwise the
check alone at attach time doesn't mean this group won't be
expanded to have multiple devices later. 

Re: [PATCH v2 2/3] iommu: Consolidate pasid dma ownership check

[Nicolin Chen]

On Mon, Aug 14, 2023 at 09:17:58AM +0800, Lu Baolu wrote:

> When switching device DMA ownership, it is required that all the device's
> pasid DMA be disabled. This is done by checking if the pasid array of the
> group is empty. Consolidate all the open code into a single helper. No
> intentional functionality change.
> 
> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
> ---
>  drivers/iommu/iommu.c | 23 ++++++++++++++++++-----
>  1 file changed, 18 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index f1eba60e573f..d4a06a37ce39 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -3127,6 +3127,19 @@ static bool iommu_is_default_domain(struct iommu_group *group)
>         return false;
>  }
> 
> +/*
> + * Assert no PASID DMA when claiming or releasing group's DMA ownership.
             ^
	     |...

> + * The device pasid interfaces are only for device drivers that have
> + * claimed the DMA ownership. Return true if no pasid DMA setup, otherwise
> + * return false with a WARN().
> + */
> +static bool assert_pasid_dma_ownership(struct iommu_group *group)

... should it be assert_no_pasid_dma_ownership?

Nic

Re: [PATCH v2 1/3] iommu: Make single-device group for PASID explicit

[Baolu Lu]

On 2023/8/18 11:56, Tian, Kevin wrote:
>> From: Lu Baolu <baolu.lu@linux.intel.com>
>> Sent: Monday, August 14, 2023 9:18 AM
>>
>> The PASID interfaces have always supported only single-device groups.
>> This was first introduced in commit 26b25a2b98e45 ("iommu: Bind process
>> address spaces to devices"), and has been kept consistent in subsequent
>> commits.
>>
>> However, the core code doesn't explicitly check for this requirement
>> after commit 201007ef707a8 ("PCI: Enable PASID only when ACS RR & UF
>> enabled on upstream path"), which made this requirement implicit.
>>
>> Restore the check to make it explicit that the PASID interfaces only
>> support devices belonging to single-device groups.
>>
>> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
>> ---
>>   drivers/iommu/iommu.c | 5 +++++
>>   1 file changed, 5 insertions(+)
>>
>> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
>> index 71b9c41f2a9e..f1eba60e573f 100644
>> --- a/drivers/iommu/iommu.c
>> +++ b/drivers/iommu/iommu.c
>> @@ -3408,6 +3408,11 @@ int iommu_attach_device_pasid(struct
>> iommu_domain *domain,
>>   		return -ENODEV;
>>
>>   	mutex_lock(&group->mutex);
>> +	if (list_count_nodes(&group->devices) != 1) {
>> +		ret = -EINVAL;
>> +		goto out_unlock;
>> +	}
>> +
> 
> I wonder whether we should also block adding new device to this
> group once the single-device has pasid enabled. Otherwise the

This has been guaranteed by pci_enable_pasid():

         if (!pci_acs_path_enabled(pdev, NULL, PCI_ACS_RR | PCI_ACS_UF))
                 return -EINVAL;

> check alone at attach time doesn't mean this group won't be
> expanded to have multiple devices later.

Best regards,
baolu

Re: [PATCH v2 2/3] iommu: Consolidate pasid dma ownership check

[Baolu Lu]

On 2023/8/18 13:43, Nicolin Chen wrote:
> On Mon, Aug 14, 2023 at 09:17:58AM +0800, Lu Baolu wrote:
> 
>> When switching device DMA ownership, it is required that all the device's
>> pasid DMA be disabled. This is done by checking if the pasid array of the
>> group is empty. Consolidate all the open code into a single helper. No
>> intentional functionality change.
>>
>> Signed-off-by: Lu Baolu<baolu.lu@linux.intel.com>
>> ---
>>   drivers/iommu/iommu.c | 23 ++++++++++++++++++-----
>>   1 file changed, 18 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
>> index f1eba60e573f..d4a06a37ce39 100644
>> --- a/drivers/iommu/iommu.c
>> +++ b/drivers/iommu/iommu.c
>> @@ -3127,6 +3127,19 @@ static bool iommu_is_default_domain(struct iommu_group *group)
>>          return false;
>>   }
>>
>> +/*
>> + * Assert no PASID DMA when claiming or releasing group's DMA ownership.
>               ^
> 	     |...
> 
>> + * The device pasid interfaces are only for device drivers that have
>> + * claimed the DMA ownership. Return true if no pasid DMA setup, otherwise
>> + * return false with a WARN().
>> + */
>> +static bool assert_pasid_dma_ownership(struct iommu_group *group)
> ... should it be assert_no_pasid_dma_ownership?

Fair enough. Or, perhaps just assert_no_pasid_dma()?

Best regards,
baolu

RE: [PATCH v2 1/3] iommu: Make single-device group for PASID explicit

[Tian, Kevin]

> From: Baolu Lu <baolu.lu@linux.intel.com>
> Sent: Monday, August 21, 2023 1:45 PM
> 
> On 2023/8/18 11:56, Tian, Kevin wrote:
> >> From: Lu Baolu <baolu.lu@linux.intel.com>
> >> Sent: Monday, August 14, 2023 9:18 AM
> >>
> >> The PASID interfaces have always supported only single-device groups.
> >> This was first introduced in commit 26b25a2b98e45 ("iommu: Bind
> process
> >> address spaces to devices"), and has been kept consistent in subsequent
> >> commits.
> >>
> >> However, the core code doesn't explicitly check for this requirement
> >> after commit 201007ef707a8 ("PCI: Enable PASID only when ACS RR & UF
> >> enabled on upstream path"), which made this requirement implicit.
> >>
> >> Restore the check to make it explicit that the PASID interfaces only
> >> support devices belonging to single-device groups.
> >>
> >> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
> >> ---
> >>   drivers/iommu/iommu.c | 5 +++++
> >>   1 file changed, 5 insertions(+)
> >>
> >> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> >> index 71b9c41f2a9e..f1eba60e573f 100644
> >> --- a/drivers/iommu/iommu.c
> >> +++ b/drivers/iommu/iommu.c
> >> @@ -3408,6 +3408,11 @@ int iommu_attach_device_pasid(struct
> >> iommu_domain *domain,
> >>   		return -ENODEV;
> >>
> >>   	mutex_lock(&group->mutex);
> >> +	if (list_count_nodes(&group->devices) != 1) {
> >> +		ret = -EINVAL;
> >> +		goto out_unlock;
> >> +	}
> >> +
> >
> > I wonder whether we should also block adding new device to this
> > group once the single-device has pasid enabled. Otherwise the
> 
> This has been guaranteed by pci_enable_pasid():
> 
>          if (!pci_acs_path_enabled(pdev, NULL, PCI_ACS_RR | PCI_ACS_UF))
>                  return -EINVAL;
> 

well since you are adding generic core check then it's not good to
rely on the fact of a specific bus...

Re: [PATCH v2 1/3] iommu: Make single-device group for PASID explicit

[Baolu Lu]

On 2023/8/21 14:33, Tian, Kevin wrote:
>> From: Baolu Lu <baolu.lu@linux.intel.com>
>> Sent: Monday, August 21, 2023 1:45 PM
>>
>> On 2023/8/18 11:56, Tian, Kevin wrote:
>>>> From: Lu Baolu <baolu.lu@linux.intel.com>
>>>> Sent: Monday, August 14, 2023 9:18 AM
>>>>
>>>> The PASID interfaces have always supported only single-device groups.
>>>> This was first introduced in commit 26b25a2b98e45 ("iommu: Bind
>> process
>>>> address spaces to devices"), and has been kept consistent in subsequent
>>>> commits.
>>>>
>>>> However, the core code doesn't explicitly check for this requirement
>>>> after commit 201007ef707a8 ("PCI: Enable PASID only when ACS RR & UF
>>>> enabled on upstream path"), which made this requirement implicit.
>>>>
>>>> Restore the check to make it explicit that the PASID interfaces only
>>>> support devices belonging to single-device groups.
>>>>
>>>> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
>>>> ---
>>>>    drivers/iommu/iommu.c | 5 +++++
>>>>    1 file changed, 5 insertions(+)
>>>>
>>>> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
>>>> index 71b9c41f2a9e..f1eba60e573f 100644
>>>> --- a/drivers/iommu/iommu.c
>>>> +++ b/drivers/iommu/iommu.c
>>>> @@ -3408,6 +3408,11 @@ int iommu_attach_device_pasid(struct
>>>> iommu_domain *domain,
>>>>    		return -ENODEV;
>>>>
>>>>    	mutex_lock(&group->mutex);
>>>> +	if (list_count_nodes(&group->devices) != 1) {
>>>> +		ret = -EINVAL;
>>>> +		goto out_unlock;
>>>> +	}
>>>> +
>>>
>>> I wonder whether we should also block adding new device to this
>>> group once the single-device has pasid enabled. Otherwise the
>>
>> This has been guaranteed by pci_enable_pasid():
>>
>>           if (!pci_acs_path_enabled(pdev, NULL, PCI_ACS_RR | PCI_ACS_UF))
>>                   return -EINVAL;
>>
> 
> well since you are adding generic core check then it's not good to
> rely on the fact of a specific bus...

We attempted to do this in the patch linked below.

https://lore.kernel.org/linux-iommu/20220705050710.2887204-5-baolu.lu@linux.intel.com/

After long discussion, we decided to move it to the pci_enable_pasid()
interface. The non-static single device group is only relevant to PCI
fabrics that support hot-plugging without ACS support on the upstream
path.

Best regards,
baolu

RE: [PATCH v2 1/3] iommu: Make single-device group for PASID explicit

[Tian, Kevin]

> From: Baolu Lu <baolu.lu@linux.intel.com>
> Sent: Tuesday, August 22, 2023 2:37 PM
> 
> On 2023/8/21 14:33, Tian, Kevin wrote:
> >> From: Baolu Lu <baolu.lu@linux.intel.com>
> >> Sent: Monday, August 21, 2023 1:45 PM
> >>
> >> On 2023/8/18 11:56, Tian, Kevin wrote:
> >>>> From: Lu Baolu <baolu.lu@linux.intel.com>
> >>>> Sent: Monday, August 14, 2023 9:18 AM
> >>>>
> >>>> The PASID interfaces have always supported only single-device groups.
> >>>> This was first introduced in commit 26b25a2b98e45 ("iommu: Bind
> >> process
> >>>> address spaces to devices"), and has been kept consistent in
> subsequent
> >>>> commits.
> >>>>
> >>>> However, the core code doesn't explicitly check for this requirement
> >>>> after commit 201007ef707a8 ("PCI: Enable PASID only when ACS RR &
> UF
> >>>> enabled on upstream path"), which made this requirement implicit.
> >>>>
> >>>> Restore the check to make it explicit that the PASID interfaces only
> >>>> support devices belonging to single-device groups.
> >>>>
> >>>> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
> >>>> ---
> >>>>    drivers/iommu/iommu.c | 5 +++++
> >>>>    1 file changed, 5 insertions(+)
> >>>>
> >>>> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> >>>> index 71b9c41f2a9e..f1eba60e573f 100644
> >>>> --- a/drivers/iommu/iommu.c
> >>>> +++ b/drivers/iommu/iommu.c
> >>>> @@ -3408,6 +3408,11 @@ int iommu_attach_device_pasid(struct
> >>>> iommu_domain *domain,
> >>>>    		return -ENODEV;
> >>>>
> >>>>    	mutex_lock(&group->mutex);
> >>>> +	if (list_count_nodes(&group->devices) != 1) {
> >>>> +		ret = -EINVAL;
> >>>> +		goto out_unlock;
> >>>> +	}
> >>>> +
> >>>
> >>> I wonder whether we should also block adding new device to this
> >>> group once the single-device has pasid enabled. Otherwise the
> >>
> >> This has been guaranteed by pci_enable_pasid():
> >>
> >>           if (!pci_acs_path_enabled(pdev, NULL, PCI_ACS_RR | PCI_ACS_UF))
> >>                   return -EINVAL;
> >>
> >
> > well since you are adding generic core check then it's not good to
> > rely on the fact of a specific bus...
> 
> We attempted to do this in the patch linked below.
> 
> https://lore.kernel.org/linux-iommu/20220705050710.2887204-5-
> baolu.lu@linux.intel.com/
> 
> After long discussion, we decided to move it to the pci_enable_pasid()
> interface. The non-static single device group is only relevant to PCI
> fabrics that support hot-plugging without ACS support on the upstream
> path.
> 

If that's the case better add a comment to include this fact. So 
another one looking at this code won't fall into the same puzzle
wondering what about a group becoming non-singleton after
above check. 😊

Re: [PATCH v2 1/3] iommu: Make single-device group for PASID explicit

[Baolu Lu]

On 2023/8/22 16:24, Tian, Kevin wrote:
>> From: Baolu Lu <baolu.lu@linux.intel.com>
>> Sent: Tuesday, August 22, 2023 2:37 PM
>>
>> On 2023/8/21 14:33, Tian, Kevin wrote:
>>>> From: Baolu Lu <baolu.lu@linux.intel.com>
>>>> Sent: Monday, August 21, 2023 1:45 PM
>>>>
>>>> On 2023/8/18 11:56, Tian, Kevin wrote:
>>>>>> From: Lu Baolu <baolu.lu@linux.intel.com>
>>>>>> Sent: Monday, August 14, 2023 9:18 AM
>>>>>>
>>>>>> The PASID interfaces have always supported only single-device groups.
>>>>>> This was first introduced in commit 26b25a2b98e45 ("iommu: Bind
>>>> process
>>>>>> address spaces to devices"), and has been kept consistent in
>> subsequent
>>>>>> commits.
>>>>>>
>>>>>> However, the core code doesn't explicitly check for this requirement
>>>>>> after commit 201007ef707a8 ("PCI: Enable PASID only when ACS RR &
>> UF
>>>>>> enabled on upstream path"), which made this requirement implicit.
>>>>>>
>>>>>> Restore the check to make it explicit that the PASID interfaces only
>>>>>> support devices belonging to single-device groups.
>>>>>>
>>>>>> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
>>>>>> ---
>>>>>>     drivers/iommu/iommu.c | 5 +++++
>>>>>>     1 file changed, 5 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
>>>>>> index 71b9c41f2a9e..f1eba60e573f 100644
>>>>>> --- a/drivers/iommu/iommu.c
>>>>>> +++ b/drivers/iommu/iommu.c
>>>>>> @@ -3408,6 +3408,11 @@ int iommu_attach_device_pasid(struct
>>>>>> iommu_domain *domain,
>>>>>>     		return -ENODEV;
>>>>>>
>>>>>>     	mutex_lock(&group->mutex);
>>>>>> +	if (list_count_nodes(&group->devices) != 1) {
>>>>>> +		ret = -EINVAL;
>>>>>> +		goto out_unlock;
>>>>>> +	}
>>>>>> +
>>>>>
>>>>> I wonder whether we should also block adding new device to this
>>>>> group once the single-device has pasid enabled. Otherwise the
>>>>
>>>> This has been guaranteed by pci_enable_pasid():
>>>>
>>>>            if (!pci_acs_path_enabled(pdev, NULL, PCI_ACS_RR | PCI_ACS_UF))
>>>>                    return -EINVAL;
>>>>
>>>
>>> well since you are adding generic core check then it's not good to
>>> rely on the fact of a specific bus...
>>
>> We attempted to do this in the patch linked below.
>>
>> https://lore.kernel.org/linux-iommu/20220705050710.2887204-5-
>> baolu.lu@linux.intel.com/
>>
>> After long discussion, we decided to move it to the pci_enable_pasid()
>> interface. The non-static single device group is only relevant to PCI
>> fabrics that support hot-plugging without ACS support on the upstream
>> path.
>>
> 
> If that's the case better add a comment to include this fact. So
> another one looking at this code won't fall into the same puzzle
> wondering what about a group becoming non-singleton after
> above check. 😊

Yeah, fair enough.

Best regards,
baolu