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

[PATCH 0/2] 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. No functional changes
are intended.

Please help review and suggest.

Lu Baolu (2):
  iommu: Consolidate pasid dma ownership check
  iommu: Move pasid array from group to device

 include/linux/iommu.h |   2 +
 drivers/iommu/iommu.c | 105 +++++++++++++++++-------------------------
 2 files changed, 43 insertions(+), 64 deletions(-)

-- 
2.34.1

[PATCH 1/2] 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 | 27 ++++++++++++++++++++-------
 1 file changed, 20 insertions(+), 7 deletions(-)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 4352a149a935..1a8fb30341e6 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -3034,6 +3034,17 @@ 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 iommu_xxx_device_pasid() interfaces are only for device drivers
+ * that have claimed the DMA ownership. Otherwise, it's a driver bug.
+ */
+static void assert_pasid_dma_ownership(struct iommu_group *group)
+{
+	lockdep_assert_held(&group->mutex);
+	WARN_ON(!xa_empty(&group->pasid_array));
+}
+
 /**
  * iommu_device_use_default_domain() - Device driver wants to handle device
  *                                     DMA through the kernel DMA API.
@@ -3052,14 +3063,14 @@ 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)) {
+		if (group->owner || !iommu_is_default_domain(group)) {
 			ret = -EBUSY;
 			goto unlock_out;
 		}
 	}
 
 	group->owner_cnt++;
+	assert_pasid_dma_ownership(group);
 
 unlock_out:
 	mutex_unlock(&group->mutex);
@@ -3084,7 +3095,8 @@ 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)))
+	assert_pasid_dma_ownership(group);
+	if (!WARN_ON(!group->owner_cnt))
 		group->owner_cnt--;
 
 	mutex_unlock(&group->mutex);
@@ -3118,8 +3130,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))
+	if (group->domain && group->domain != group->default_domain)
 		return -EBUSY;
 
 	ret = __iommu_group_alloc_blocking_domain(group);
@@ -3129,8 +3140,10 @@ static int __iommu_take_dma_ownership(struct iommu_group *group, void *owner)
 	if (ret)
 		return ret;
 
+	assert_pasid_dma_ownership(group);
 	group->owner = owner;
 	group->owner_cnt++;
+
 	return 0;
 }
 
@@ -3206,10 +3219,10 @@ 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))
 		return;
 
+	assert_pasid_dma_ownership(group);
 	group->owner_cnt = 0;
 	group->owner = NULL;
 	__iommu_group_set_domain_nofail(group, group->default_domain);
-- 
2.34.1

RE: [PATCH 1/2] iommu: Consolidate pasid dma ownership check

[Tian, Kevin]

> From: Lu Baolu <baolu.lu@linux.intel.com>
> Sent: Tuesday, August 1, 2023 2:31 PM
>
> 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.

...

>  /**
>   * iommu_device_use_default_domain() - Device driver wants to handle
> device
>   *                                     DMA through the kernel DMA API.
> @@ -3052,14 +3063,14 @@ 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)) {
> +		if (group->owner || !iommu_is_default_domain(group)) {
>  			ret = -EBUSY;
>  			goto unlock_out;
>  		}
>  	}
> 
>  	group->owner_cnt++;
> +	assert_pasid_dma_ownership(group);

Old code returns error if pasid_xrrary is not empty.

New code continues to take ownership with a warning.

this is a functional change. Is it intended or not?

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

[Baolu Lu]

On 2023/8/1 15:03, Tian, Kevin wrote:
>>   /**
>>    * iommu_device_use_default_domain() - Device driver wants to handle
>> device
>>    *                                     DMA through the kernel DMA API.
>> @@ -3052,14 +3063,14 @@ 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)) {
>> +		if (group->owner || !iommu_is_default_domain(group)) {
>>   			ret = -EBUSY;
>>   			goto unlock_out;
>>   		}
>>   	}
>>
>>   	group->owner_cnt++;
>> +	assert_pasid_dma_ownership(group);
> Old code returns error if pasid_xrrary is not empty.
> 
> New code continues to take ownership with a warning.
> 
> this is a functional change. Is it intended or not?

If iommu_device_use_default_domain() is called with pasid_array not
empty, there must be a bug somewhere in the device driver. We should
WARN it instead of returning an error. Probably this is a functional
change? If so, I can add this in the commit message.

Best regards,
baolu

RE: [PATCH 1/2] iommu: Consolidate pasid dma ownership check

[Tian, Kevin]

> From: Baolu Lu <baolu.lu@linux.intel.com>
> Sent: Tuesday, August 1, 2023 3:44 PM
> 
> On 2023/8/1 15:03, Tian, Kevin wrote:
> >>   /**
> >>    * iommu_device_use_default_domain() - Device driver wants to handle
> >> device
> >>    *                                     DMA through the kernel DMA API.
> >> @@ -3052,14 +3063,14 @@ 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)) {
> >> +		if (group->owner || !iommu_is_default_domain(group)) {
> >>   			ret = -EBUSY;
> >>   			goto unlock_out;
> >>   		}
> >>   	}
> >>
> >>   	group->owner_cnt++;
> >> +	assert_pasid_dma_ownership(group);
> > Old code returns error if pasid_xrrary is not empty.
> >
> > New code continues to take ownership with a warning.
> >
> > this is a functional change. Is it intended or not?
> 
> If iommu_device_use_default_domain() is called with pasid_array not
> empty, there must be a bug somewhere in the device driver. We should
> WARN it instead of returning an error. Probably this is a functional
> change? If so, I can add this in the commit message.
> 

IMHO we should WARN *and* return an error.

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

[Baolu Lu]

On 2023/8/2 9:39, Tian, Kevin wrote:
>> From: Baolu Lu<baolu.lu@linux.intel.com>
>> Sent: Tuesday, August 1, 2023 3:44 PM
>>
>> On 2023/8/1 15:03, Tian, Kevin wrote:
>>>>    /**
>>>>     * iommu_device_use_default_domain() - Device driver wants to handle
>>>> device
>>>>     *                                     DMA through the kernel DMA API.
>>>> @@ -3052,14 +3063,14 @@ 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)) {
>>>> +		if (group->owner || !iommu_is_default_domain(group)) {
>>>>    			ret = -EBUSY;
>>>>    			goto unlock_out;
>>>>    		}
>>>>    	}
>>>>
>>>>    	group->owner_cnt++;
>>>> +	assert_pasid_dma_ownership(group);
>>> Old code returns error if pasid_xrrary is not empty.
>>>
>>> New code continues to take ownership with a warning.
>>>
>>> this is a functional change. Is it intended or not?
>> If iommu_device_use_default_domain() is called with pasid_array not
>> empty, there must be a bug somewhere in the device driver. We should
>> WARN it instead of returning an error. Probably this is a functional
>> change? If so, I can add this in the commit message.
>>
> IMHO we should WARN*and*  return an error.

Okay, fine to me. Will make this in the next version.

Best regards,
baolu

[PATCH 2/2] 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, 24 insertions(+), 58 deletions(-)

diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index b1dcb1b9b170..90be3efd0e91 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -406,6 +406,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
@@ -420,6 +421,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 1a8fb30341e6..7fd53b9e9754 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -48,7 +48,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);
@@ -302,6 +301,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;
 }
@@ -891,7 +891,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) {
@@ -3041,8 +3040,11 @@ static bool iommu_is_default_domain(struct iommu_group *group)
  */
 static void assert_pasid_dma_ownership(struct iommu_group *group)
 {
+	struct group_device *device;
+
 	lockdep_assert_held(&group->mutex);
-	WARN_ON(!xa_empty(&group->pasid_array));
+	for_each_group_device(group, device)
+		WARN_ON(!xa_empty(&device->dev->iommu->pasid_array));
 }
 
 /**
@@ -3281,33 +3283,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.
@@ -3319,32 +3294,28 @@ static void __iommu_remove_group_pasid(struct iommu_group *group,
 int iommu_attach_device_pasid(struct iommu_domain *domain,
 			      struct device *dev, ioasid_t pasid)
 {
-	struct iommu_group *group;
+	struct dev_iommu *param = dev->iommu;
 	void *curr;
 	int ret;
 
 	if (!domain->ops->set_dev_pasid)
 		return -EOPNOTSUPP;
 
-	group = iommu_group_get(dev);
-	if (!group)
+	if (!param)
 		return -ENODEV;
 
-	mutex_lock(&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);
-	iommu_group_put(group);
+	mutex_unlock(&param->lock);
 
 	return ret;
 }
@@ -3362,14 +3333,13 @@ EXPORT_SYMBOL_GPL(iommu_attach_device_pasid);
 void iommu_detach_device_pasid(struct iommu_domain *domain, struct device *dev,
 			       ioasid_t pasid)
 {
-	struct iommu_group *group = iommu_group_get(dev);
+	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);
-
-	iommu_group_put(group);
+	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);
 
@@ -3392,18 +3362,12 @@ struct iommu_domain *iommu_get_domain_for_dev_pasid(struct device *dev,
 						    unsigned int type)
 {
 	struct iommu_domain *domain;
-	struct iommu_group *group;
 
-	group = iommu_group_get(dev);
-	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);
-	iommu_group_put(group);
+	xa_unlock(&dev->iommu->pasid_array);
 
 	return domain;
 }
-- 
2.34.1

Re: [PATCH 2/2] iommu: Move pasid array from group to device

[Baolu Lu]

On 2023/8/1 16:07, tina.zhang wrote:
> Hi Baolu,

Hi Tina,

> Although this patch moves the domain reference pointer from a per-group
> structure to a per-device structure, the domain life-cycle is still
> expected to be managed per-group (i.e., iommu_domain_free() is called in
> iommu_group_release()). Is this what we expect?

The lifecycle of an iommu domain is independent of the lifecycle of the
iommu group that it is attached to. The system domains, such as the
default domain and the blocking domain, are allocated and managed by the
iommu core. These domains are freed when the group is freed.

However, any device driver can allocate its own iommu domains. The
device driver can set/remove the domain to/from the RID or PASID of the
device.

Best regards,
baolu

Re: [PATCH 0/2] iommu: Make pasid array per device

[Jason Gunthorpe]

On Tue, Aug 01, 2023 at 02:31:23PM +0800, Lu Baolu wrote:
> 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. No functional changes
> are intended.

Is there a reason to do this?

*PCI* requires the ACS/etc because PCI kind of messed up how switches
handled PASID so PASID doesn't work otherwise.

But there is nothing that says other bus type can't have working
(non-PCI) PASID and still have device isolation issues.

So unless there is a really strong reason to do this we should keep
the PASID list in the group just like the domain.

Jason

RE: [PATCH 0/2] iommu: Make pasid array per device

[Tian, Kevin]

> From: Jason Gunthorpe <jgg@ziepe.ca>
> Sent: Wednesday, August 2, 2023 10:16 PM
> 
> On Tue, Aug 01, 2023 at 02:31:23PM +0800, Lu Baolu wrote:
> > 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. No functional changes
> > are intended.
> 
> Is there a reason to do this?
> 
> *PCI* requires the ACS/etc because PCI kind of messed up how switches
> handled PASID so PASID doesn't work otherwise.
> 
> But there is nothing that says other bus type can't have working
> (non-PCI) PASID and still have device isolation issues.
> 
> So unless there is a really strong reason to do this we should keep
> the PASID list in the group just like the domain.
> 

this comes from the consensus in [1].

[1] https://lore.kernel.org/linux-iommu/ZAcyEzN4102gPsWC@nvidia.com/

Re: [PATCH 0/2] iommu: Make pasid array per device

[Jason Gunthorpe]

On Thu, Aug 03, 2023 at 12:44:03AM +0000, Tian, Kevin wrote:
> > From: Jason Gunthorpe <jgg@ziepe.ca>
> > Sent: Wednesday, August 2, 2023 10:16 PM
> > 
> > On Tue, Aug 01, 2023 at 02:31:23PM +0800, Lu Baolu wrote:
> > > 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. No functional changes
> > > are intended.
> > 
> > Is there a reason to do this?
> > 
> > *PCI* requires the ACS/etc because PCI kind of messed up how switches
> > handled PASID so PASID doesn't work otherwise.
> > 
> > But there is nothing that says other bus type can't have working
> > (non-PCI) PASID and still have device isolation issues.
> > 
> > So unless there is a really strong reason to do this we should keep
> > the PASID list in the group just like the domain.
> > 
> 
> this comes from the consensus in [1].
> 
> [1] https://lore.kernel.org/linux-iommu/ZAcyEzN4102gPsWC@nvidia.com/

That consensus was that we don't have PASID support if there is
multi-device groups, at least in iommufd.. That makes sense. If we
want to change the core code to enforce this that also makes sense

But this series is just moving the array?

Jason

RE: [PATCH 0/2] iommu: Make pasid array per device

[Tian, Kevin]

> From: Jason Gunthorpe <jgg@ziepe.ca>
> Sent: Thursday, August 3, 2023 11:19 PM
> 
> On Thu, Aug 03, 2023 at 12:44:03AM +0000, Tian, Kevin wrote:
> > > From: Jason Gunthorpe <jgg@ziepe.ca>
> > > Sent: Wednesday, August 2, 2023 10:16 PM
> > >
> > > On Tue, Aug 01, 2023 at 02:31:23PM +0800, Lu Baolu wrote:
> > > > 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. No functional
> changes
> > > > are intended.
> > >
> > > Is there a reason to do this?
> > >
> > > *PCI* requires the ACS/etc because PCI kind of messed up how switches
> > > handled PASID so PASID doesn't work otherwise.
> > >
> > > But there is nothing that says other bus type can't have working
> > > (non-PCI) PASID and still have device isolation issues.
> > >
> > > So unless there is a really strong reason to do this we should keep
> > > the PASID list in the group just like the domain.
> > >
> >
> > this comes from the consensus in [1].
> >
> > [1] https://lore.kernel.org/linux-iommu/ZAcyEzN4102gPsWC@nvidia.com/
> 
> That consensus was that we don't have PASID support if there is
> multi-device groups, at least in iommufd.. That makes sense. If we
> want to change the core code to enforce this that also makes sense
> 
> But this series is just moving the array?
> 

This is a preparation series for supporting PASID in iommufd (will be
sent out probably after next version of the nesting series).

It only moves the array by taking only PCI into consideration. We could
add an explicit enforcement in iommu core for all types of devices.

Re: [PATCH 0/2] iommu: Make pasid array per device

[Baolu Lu]

On 2023/8/3 23:18, Jason Gunthorpe wrote:
> On Thu, Aug 03, 2023 at 12:44:03AM +0000, Tian, Kevin wrote:
>>> From: Jason Gunthorpe<jgg@ziepe.ca>
>>> Sent: Wednesday, August 2, 2023 10:16 PM
>>>
>>> On Tue, Aug 01, 2023 at 02:31:23PM +0800, Lu Baolu wrote:
>>>> 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. No functional changes
>>>> are intended.
>>> Is there a reason to do this?
>>>
>>> *PCI*  requires the ACS/etc because PCI kind of messed up how switches
>>> handled PASID so PASID doesn't work otherwise.
>>>
>>> But there is nothing that says other bus type can't have working
>>> (non-PCI) PASID and still have device isolation issues.
>>>
>>> So unless there is a really strong reason to do this we should keep
>>> the PASID list in the group just like the domain.
>>>
>> this comes from the consensus in [1].
>>
>> [1]https://lore.kernel.org/linux-iommu/ZAcyEzN4102gPsWC@nvidia.com/
> That consensus was that we don't have PASID support if there is
> multi-device groups, at least in iommufd.. That makes sense. If we
> want to change the core code to enforce this that also makes sense

In my initial plan, I had a third patch that would have enforced single-
device groups for PASID interfaces in the core. But I ultimately dropped
it because it is the fact for PCI devices, but I am not sure about other
buses although perhaps there is none.

> But this series is just moving the array?

So I took the first step by moving the pasid_array from iommu group to
the device. :-)

Best regards,
baolu

Re: [PATCH 0/2] iommu: Make pasid array per device

[Baolu Lu]

On 2023/8/4 10:20, Baolu Lu wrote:
> On 2023/8/3 23:18, Jason Gunthorpe wrote:
>> On Thu, Aug 03, 2023 at 12:44:03AM +0000, Tian, Kevin wrote:
>>>> From: Jason Gunthorpe<jgg@ziepe.ca>
>>>> Sent: Wednesday, August 2, 2023 10:16 PM
>>>>
>>>> On Tue, Aug 01, 2023 at 02:31:23PM +0800, Lu Baolu wrote:
>>>>> 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. No functional changes
>>>>> are intended.
>>>> Is there a reason to do this?
>>>>
>>>> *PCI*  requires the ACS/etc because PCI kind of messed up how switches
>>>> handled PASID so PASID doesn't work otherwise.
>>>>
>>>> But there is nothing that says other bus type can't have working
>>>> (non-PCI) PASID and still have device isolation issues.
>>>>
>>>> So unless there is a really strong reason to do this we should keep
>>>> the PASID list in the group just like the domain.
>>>>
>>> this comes from the consensus in [1].
>>>
>>> [1]https://lore.kernel.org/linux-iommu/ZAcyEzN4102gPsWC@nvidia.com/
>> That consensus was that we don't have PASID support if there is
>> multi-device groups, at least in iommufd.. That makes sense. If we
>> want to change the core code to enforce this that also makes sense
> 
> In my initial plan, I had a third patch that would have enforced single-
> device groups for PASID interfaces in the core. But I ultimately dropped
> it because it is the fact for PCI devices, but I am not sure about other
> buses although perhaps there is none.
> 
>> But this series is just moving the array?
> 
> So I took the first step by moving the pasid_array from iommu group to
> the device. 😄

In my mind, iommu_group was introduced to solve the PCI alias and P2P
transactions which bypass IOMMU translation. When we enter the PASID
world, the architecture should disallow these anymore. Hence, it's safe
to move pasid_array to device.

This was the motivation of this series.

Best regards,
baolu

Re: [PATCH 0/2] iommu: Make pasid array per device

[Jason Gunthorpe]

On Fri, Aug 04, 2023 at 10:30:12AM +0800, Baolu Lu wrote:
> On 2023/8/4 10:20, Baolu Lu wrote:
> > On 2023/8/3 23:18, Jason Gunthorpe wrote:
> > > On Thu, Aug 03, 2023 at 12:44:03AM +0000, Tian, Kevin wrote:
> > > > > From: Jason Gunthorpe<jgg@ziepe.ca>
> > > > > Sent: Wednesday, August 2, 2023 10:16 PM
> > > > > 
> > > > > On Tue, Aug 01, 2023 at 02:31:23PM +0800, Lu Baolu wrote:
> > > > > > 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. No functional changes
> > > > > > are intended.
> > > > > Is there a reason to do this?
> > > > > 
> > > > > *PCI*  requires the ACS/etc because PCI kind of messed up how switches
> > > > > handled PASID so PASID doesn't work otherwise.
> > > > > 
> > > > > But there is nothing that says other bus type can't have working
> > > > > (non-PCI) PASID and still have device isolation issues.
> > > > > 
> > > > > So unless there is a really strong reason to do this we should keep
> > > > > the PASID list in the group just like the domain.
> > > > > 
> > > > this comes from the consensus in [1].
> > > > 
> > > > [1]https://lore.kernel.org/linux-iommu/ZAcyEzN4102gPsWC@nvidia.com/
> > > That consensus was that we don't have PASID support if there is
> > > multi-device groups, at least in iommufd.. That makes sense. If we
> > > want to change the core code to enforce this that also makes sense
> > 
> > In my initial plan, I had a third patch that would have enforced single-
> > device groups for PASID interfaces in the core. But I ultimately dropped
> > it because it is the fact for PCI devices, but I am not sure about other
> > buses although perhaps there is none.
> > 
> > > But this series is just moving the array?
> > 
> > So I took the first step by moving the pasid_array from iommu group to
> > the device. 😄
> 
> In my mind, iommu_group was introduced to solve the PCI alias and P2P
> transactions which bypass IOMMU translation. When we enter the PASID
> world, the architecture should disallow these anymore. Hence, it's safe
> to move pasid_array to device.
> 
> This was the motivation of this series.

I think you should add a protection as well, directly prevent
multi-device groups being used with pasid.

Jason