On VT-d platforms, legacy DMA requests without PASID use device’s
default domain, where RID_PASID is always attached. Device drivers
can then use the DMA API for all in-kernel DMA on the RID.

Ideally, devices capable of using ENQCMDS can also transparently use the
default domain, consequently DMA API. However, VT-d architecture
dictates that the PASID used by ENQCMDS must be different from the
RID_PASID value.

To provide support for transparent use of DMA API with non-RID_PASID
value, this patch implements the set_dev_pasid() function for the
default domain. The idea is that device drivers wishing to use ENQCMDS
to submit work on buffers mapped by DMA API will call
iommu_attach_device_pasid() beforehand.

Signed-off-by: Jacob Pan <jacob.jun.pan@linux.intel.com>
---
 drivers/iommu/intel/iommu.c | 32 ++++++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index 10f657828d3a..a0cb3bc851ac 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -4665,6 +4665,10 @@ static void intel_iommu_remove_dev_pasid(struct device *dev, ioasid_t pasid)
 		case IOMMU_DOMAIN_SVA:
 			intel_svm_remove_dev_pasid(dev, pasid);
 			break;
+		case IOMMU_DOMAIN_DMA:
+		case IOMMU_DOMAIN_DMA_FQ:
+		case IOMMU_DOMAIN_IDENTITY:
+			break;
 		default:
 			/* should never reach here */
 			WARN_ON(1);
@@ -4675,6 +4679,33 @@ static void intel_iommu_remove_dev_pasid(struct device *dev, ioasid_t pasid)
 	intel_pasid_tear_down_entry(iommu, dev, pasid, false);
 }
 
+static int intel_iommu_set_dev_pasid(struct iommu_domain *domain,
+				   struct device *dev, ioasid_t pasid)
+{
+	struct device_domain_info *info = dev_iommu_priv_get(dev);
+	struct dmar_domain *dmar_domain = to_dmar_domain(domain);
+	struct intel_iommu *iommu = info->iommu;
+	int ret = 0;
+
+	if (!sm_supported(iommu) || !info)
+		return -ENODEV;
+
+	if (WARN_ON(pasid == PASID_RID2PASID))
+		return -EINVAL;
+
+	if (hw_pass_through && domain_type_is_si(dmar_domain))
+		ret = intel_pasid_setup_pass_through(iommu, dmar_domain,
+						     dev, pasid);
+	else if (dmar_domain->use_first_level)
+		ret = domain_setup_first_level(iommu, dmar_domain,
+					       dev, pasid);
+	else
+		ret = intel_pasid_setup_second_level(iommu, dmar_domain,
+						     dev, pasid);
+
+	return ret;
+}
+
 const struct iommu_ops intel_iommu_ops = {
 	.capable		= intel_iommu_capable,
 	.domain_alloc		= intel_iommu_domain_alloc,
@@ -4702,6 +4733,7 @@ const struct iommu_ops intel_iommu_ops = {
 		.iova_to_phys		= intel_iommu_iova_to_phys,
 		.free			= intel_iommu_domain_free,
 		.enforce_cache_coherency = intel_iommu_enforce_cache_coherency,
+		.set_dev_pasid		= intel_iommu_set_dev_pasid,
 	}
 };
 
-- 
2.25.1

On Wed, Mar 01, 2023 at 04:59:56PM -0800, Jacob Pan wrote:
> On VT-d platforms, legacy DMA requests without PASID use device’s
> default domain, where RID_PASID is always attached. Device drivers
> can then use the DMA API for all in-kernel DMA on the RID.
> 
> Ideally, devices capable of using ENQCMDS can also transparently use the
> default domain, consequently DMA API. However, VT-d architecture
> dictates that the PASID used by ENQCMDS must be different from the
> RID_PASID value.
> 
> To provide support for transparent use of DMA API with non-RID_PASID
> value, this patch implements the set_dev_pasid() function for the
> default domain. The idea is that device drivers wishing to use ENQCMDS
> to submit work on buffers mapped by DMA API will call
> iommu_attach_device_pasid() beforehand.
> 
> Signed-off-by: Jacob Pan <jacob.jun.pan@linux.intel.com>
> ---
>  drivers/iommu/intel/iommu.c | 32 ++++++++++++++++++++++++++++++++
>  1 file changed, 32 insertions(+)
> 
> diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
> index 10f657828d3a..a0cb3bc851ac 100644
> --- a/drivers/iommu/intel/iommu.c
> +++ b/drivers/iommu/intel/iommu.c
> @@ -4665,6 +4665,10 @@ static void intel_iommu_remove_dev_pasid(struct device *dev, ioasid_t pasid)
>  		case IOMMU_DOMAIN_SVA:
>  			intel_svm_remove_dev_pasid(dev, pasid);
>  			break;
> +		case IOMMU_DOMAIN_DMA:
> +		case IOMMU_DOMAIN_DMA_FQ:
> +		case IOMMU_DOMAIN_IDENTITY:

Why do we need this switch statement anyhow? Something seems to have
gone wrong here.. SVM shouldn't be special, and why does this call
intel_pasid_tear_down_entry() twice on the SVA path?

It seems like all this is doing is flushing the PRI queue.

A domain should have a dedicated flag unrelated to the type if it is
using PRI and all PRI using domains should have the PRI queue flushed
here, using the same code as flushing the PRI for a RID attachment.

Jason

Hi Jason,

On Mon, 6 Mar 2023 08:57:57 -0400, Jason Gunthorpe <jgg@nvidia.com> wrote:

> On Wed, Mar 01, 2023 at 04:59:56PM -0800, Jacob Pan wrote:
> > On VT-d platforms, legacy DMA requests without PASID use device’s
> > default domain, where RID_PASID is always attached. Device drivers
> > can then use the DMA API for all in-kernel DMA on the RID.
> > 
> > Ideally, devices capable of using ENQCMDS can also transparently use the
> > default domain, consequently DMA API. However, VT-d architecture
> > dictates that the PASID used by ENQCMDS must be different from the
> > RID_PASID value.
> > 
> > To provide support for transparent use of DMA API with non-RID_PASID
> > value, this patch implements the set_dev_pasid() function for the
> > default domain. The idea is that device drivers wishing to use ENQCMDS
> > to submit work on buffers mapped by DMA API will call
> > iommu_attach_device_pasid() beforehand.
> > 
> > Signed-off-by: Jacob Pan <jacob.jun.pan@linux.intel.com>
> > ---
> >  drivers/iommu/intel/iommu.c | 32 ++++++++++++++++++++++++++++++++
> >  1 file changed, 32 insertions(+)
> > 
> > diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
> > index 10f657828d3a..a0cb3bc851ac 100644
> > --- a/drivers/iommu/intel/iommu.c
> > +++ b/drivers/iommu/intel/iommu.c
> > @@ -4665,6 +4665,10 @@ static void intel_iommu_remove_dev_pasid(struct
> > device *dev, ioasid_t pasid) case IOMMU_DOMAIN_SVA:
> >  			intel_svm_remove_dev_pasid(dev, pasid);
> >  			break;
> > +		case IOMMU_DOMAIN_DMA:
> > +		case IOMMU_DOMAIN_DMA_FQ:
> > +		case IOMMU_DOMAIN_IDENTITY:  
> 
> Why do we need this switch statement anyhow?
For DMA API pasid, there is nothing special just let it fall through and
call 
	intel_pasid_tear_down_entry(iommu, dev, pasid, false);

> Something seems to have
> gone wrong here.. SVM shouldn't be special, 
I think all the trouble is caused by  the asymmetrical setup of
iommu_op.remove_dev_pasid() and iommu_domain_ops.set_dev_pasid()
Perhaps, we should "demote" remove_dev_pasid to iommu_domain_ops then we
don't have to check SVA specific things.

> and why does this call intel_pasid_tear_down_entry() twice on the SVA
> path?
Good catch, that seems to be unnecessary.

> It seems like all this is doing is flushing the PRI queue.
> A domain should have a dedicated flag unrelated to the type if it is
> using PRI and all PRI using domains should have the PRI queue flushed
> here, using the same code as flushing the PRI for a RID attachment.
Yes, or if the teardown op is domain-specific, then it works too?


Thanks,

Jacob

On Mon, Mar 06, 2023 at 09:36:26AM -0800, Jacob Pan wrote:

> > It seems like all this is doing is flushing the PRI queue.
> > A domain should have a dedicated flag unrelated to the type if it is
> > using PRI and all PRI using domains should have the PRI queue flushed
> > here, using the same code as flushing the PRI for a RID attachment.
> Yes, or if the teardown op is domain-specific, then it works too?

That could only sense if we end up creating a PRI domain type too..

Right now PRI is messy because it doesn't really work with unmanaged
domains and that is something that will have to get fixed sort of
soonish

Once PRI is a proper thing then "SVA" is just a PRI domain that has a
non-managed from-the-mm page table pointer.

Most of the driver code marked svm should entirely disappear.

Jason

On 3/7/23 1:41 AM, Jason Gunthorpe wrote:
> On Mon, Mar 06, 2023 at 09:36:26AM -0800, Jacob Pan wrote:
> 
>>> It seems like all this is doing is flushing the PRI queue.
>>> A domain should have a dedicated flag unrelated to the type if it is
>>> using PRI and all PRI using domains should have the PRI queue flushed
>>> here, using the same code as flushing the PRI for a RID attachment.
>> Yes, or if the teardown op is domain-specific, then it works too?
> That could only sense if we end up creating a PRI domain type too..
> 
> Right now PRI is messy because it doesn't really work with unmanaged
> domains and that is something that will have to get fixed sort of
> soonish
> 
> Once PRI is a proper thing then "SVA" is just a PRI domain that has a
> non-managed from-the-mm page table pointer.
> 
> Most of the driver code marked svm should entirely disappear.

Agreed.

There is also another code consolidation we discussed earlier, that is
moving mmu_notifier_register/unregister to drivers/iommu/iommu-sva.c.
It's common for all SVA-capable iommu drivers.

With PRI domain and mm_notifier addressed, we could safely remove the
switch here. SVA domain will be nothing special. :-)

Best regards,
baolu

On 2023/3/2 8:59, Jacob Pan wrote:
> On VT-d platforms, legacy DMA requests without PASID use device’s
> default domain, where RID_PASID is always attached. Device drivers
> can then use the DMA API for all in-kernel DMA on the RID.
> 
> Ideally, devices capable of using ENQCMDS can also transparently use the
> default domain, consequently DMA API. However, VT-d architecture
> dictates that the PASID used by ENQCMDS must be different from the
> RID_PASID value.
> 
> To provide support for transparent use of DMA API with non-RID_PASID
> value, this patch implements the set_dev_pasid() function for the
> default domain. The idea is that device drivers wishing to use ENQCMDS
> to submit work on buffers mapped by DMA API will call
> iommu_attach_device_pasid() beforehand.
> 
> Signed-off-by: Jacob Pan <jacob.jun.pan@linux.intel.com>
> ---
>   drivers/iommu/intel/iommu.c | 32 ++++++++++++++++++++++++++++++++
>   1 file changed, 32 insertions(+)
> 
> diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
> index 10f657828d3a..a0cb3bc851ac 100644
> --- a/drivers/iommu/intel/iommu.c
> +++ b/drivers/iommu/intel/iommu.c
> @@ -4665,6 +4665,10 @@ static void intel_iommu_remove_dev_pasid(struct device *dev, ioasid_t pasid)
>   		case IOMMU_DOMAIN_SVA:
>   			intel_svm_remove_dev_pasid(dev, pasid);
>   			break;
> +		case IOMMU_DOMAIN_DMA:
> +		case IOMMU_DOMAIN_DMA_FQ:
> +		case IOMMU_DOMAIN_IDENTITY:
> +			break;
>   		default:
>   			/* should never reach here */
>   			WARN_ON(1);
> @@ -4675,6 +4679,33 @@ static void intel_iommu_remove_dev_pasid(struct device *dev, ioasid_t pasid)
>   	intel_pasid_tear_down_entry(iommu, dev, pasid, false);
>   }
>   
> +static int intel_iommu_set_dev_pasid(struct iommu_domain *domain,
> +				   struct device *dev, ioasid_t pasid)
> +{
> +	struct device_domain_info *info = dev_iommu_priv_get(dev);
> +	struct dmar_domain *dmar_domain = to_dmar_domain(domain);
> +	struct intel_iommu *iommu = info->iommu;
> +	int ret = 0;
> +
> +	if (!sm_supported(iommu) || !info)

@info has been referenced. !info check makes no sense.

Add pasid_supported(iommu).

Do you need to check whether the domain is compatible for this rid
pasid?

> +		return -ENODEV;
> +
> +	if (WARN_ON(pasid == PASID_RID2PASID))
> +		return -EINVAL;

Add a call to domain_attach_iommu() here to get a refcount of the domain
ID. And call domain_detach_iommu() in intel_iommu_remove_dev_pasid().

> +
> +	if (hw_pass_through && domain_type_is_si(dmar_domain))
> +		ret = intel_pasid_setup_pass_through(iommu, dmar_domain,
> +						     dev, pasid);
> +	else if (dmar_domain->use_first_level)
> +		ret = domain_setup_first_level(iommu, dmar_domain,
> +					       dev, pasid);
> +	else
> +		ret = intel_pasid_setup_second_level(iommu, dmar_domain,
> +						     dev, pasid);
> +
> +	return ret;
> +}

Do you need to consider pasid cache invalidation?

> +
>   const struct iommu_ops intel_iommu_ops = {
>   	.capable		= intel_iommu_capable,
>   	.domain_alloc		= intel_iommu_domain_alloc,
> @@ -4702,6 +4733,7 @@ const struct iommu_ops intel_iommu_ops = {
>   		.iova_to_phys		= intel_iommu_iova_to_phys,
>   		.free			= intel_iommu_domain_free,
>   		.enforce_cache_coherency = intel_iommu_enforce_cache_coherency,
> +		.set_dev_pasid		= intel_iommu_set_dev_pasid,
>   	}
>   };
>   

Best regards,
baolu

> From: Baolu Lu <baolu.lu@linux.intel.com>
> Sent: Thursday, March 2, 2023 10:07 PM
> 
> > +
> > +	if (hw_pass_through && domain_type_is_si(dmar_domain))
> > +		ret = intel_pasid_setup_pass_through(iommu, dmar_domain,
> > +						     dev, pasid);
> > +	else if (dmar_domain->use_first_level)
> > +		ret = domain_setup_first_level(iommu, dmar_domain,
> > +					       dev, pasid);
> > +	else
> > +		ret = intel_pasid_setup_second_level(iommu, dmar_domain,
> > +						     dev, pasid);
> > +
> > +	return ret;
> > +}
> 
> Do you need to consider pasid cache invalidation?
> 

To avoid confusion this is not about invalidation of pasid cache itself
which should be covered by above setup functions already.

Here actually means per-PASID invalidation in iotlb and devtlb. Today
only RID is tracked per domain for invalidation. it needs extension to
walk attached pasid too.

Hi Kevin,

On Fri, 3 Mar 2023 05:38:19 +0000, "Tian, Kevin" <kevin.tian@intel.com>
wrote:

> > From: Baolu Lu <baolu.lu@linux.intel.com>
> > Sent: Thursday, March 2, 2023 10:07 PM
> >   
> > > +
> > > +	if (hw_pass_through && domain_type_is_si(dmar_domain))
> > > +		ret = intel_pasid_setup_pass_through(iommu,
> > > dmar_domain,
> > > +						     dev, pasid);
> > > +	else if (dmar_domain->use_first_level)
> > > +		ret = domain_setup_first_level(iommu, dmar_domain,
> > > +					       dev, pasid);
> > > +	else
> > > +		ret = intel_pasid_setup_second_level(iommu,
> > > dmar_domain,
> > > +						     dev, pasid);
> > > +
> > > +	return ret;
> > > +}  
> > 
> > Do you need to consider pasid cache invalidation?
> >   
> 
> To avoid confusion this is not about invalidation of pasid cache itself
> which should be covered by above setup functions already.
> 
> Here actually means per-PASID invalidation in iotlb and devtlb. Today
> only RID is tracked per domain for invalidation. it needs extension to
> walk attached pasid too.

Yes, will add.

For the set up path, there is no need to flush IOTLBs,  because we're going
from non present to present.

On the remove path, IOTLB flush should be covered when device driver
calls iommu_detach_device_pasid(). Covered with this patch.


Thanks,

Jacob

On 3/4/23 12:35 AM, Jacob Pan wrote:
>>> From: Baolu Lu<baolu.lu@linux.intel.com>
>>> Sent: Thursday, March 2, 2023 10:07 PM
>>>    
>>>> +
>>>> +	if (hw_pass_through && domain_type_is_si(dmar_domain))
>>>> +		ret = intel_pasid_setup_pass_through(iommu,
>>>> dmar_domain,
>>>> +						     dev, pasid);
>>>> +	else if (dmar_domain->use_first_level)
>>>> +		ret = domain_setup_first_level(iommu, dmar_domain,
>>>> +					       dev, pasid);
>>>> +	else
>>>> +		ret = intel_pasid_setup_second_level(iommu,
>>>> dmar_domain,
>>>> +						     dev, pasid);
>>>> +
>>>> +	return ret;
>>>> +}
>>> Do you need to consider pasid cache invalidation?
>>>    
>> To avoid confusion this is not about invalidation of pasid cache itself
>> which should be covered by above setup functions already.
>>
>> Here actually means per-PASID invalidation in iotlb and devtlb. Today
>> only RID is tracked per domain for invalidation. it needs extension to
>> walk attached pasid too.
> Yes, will add.
> 
> For the set up path, there is no need to flush IOTLBs,  because we're going
> from non present to present.
> 
> On the remove path, IOTLB flush should be covered when device driver
> calls iommu_detach_device_pasid(). Covered with this patch.

It's not only for the PASID teardown path, but also for unmap(). As the
device has issued DMA requests with PASID, the IOMMU probably will cache
the DMA translation with PASID tagged. Hence, we need to invalidate the
PASID-specific IOTLB and device TLB in the unmap() path.

I once had a patch for this:

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

Probably you can use it as a starting point.

Best regards,
baolu

Hi Baolu,

On Sun, 5 Mar 2023 11:05:50 +0800, Baolu Lu <baolu.lu@linux.intel.com>
wrote:

> On 3/4/23 12:35 AM, Jacob Pan wrote:
> >>> From: Baolu Lu<baolu.lu@linux.intel.com>
> >>> Sent: Thursday, March 2, 2023 10:07 PM
> >>>      
> >>>> +
> >>>> +	if (hw_pass_through && domain_type_is_si(dmar_domain))
> >>>> +		ret = intel_pasid_setup_pass_through(iommu,
> >>>> dmar_domain,
> >>>> +						     dev, pasid);
> >>>> +	else if (dmar_domain->use_first_level)
> >>>> +		ret = domain_setup_first_level(iommu, dmar_domain,
> >>>> +					       dev, pasid);
> >>>> +	else
> >>>> +		ret = intel_pasid_setup_second_level(iommu,
> >>>> dmar_domain,
> >>>> +						     dev, pasid);
> >>>> +
> >>>> +	return ret;
> >>>> +}  
> >>> Do you need to consider pasid cache invalidation?
> >>>      
> >> To avoid confusion this is not about invalidation of pasid cache itself
> >> which should be covered by above setup functions already.
> >>
> >> Here actually means per-PASID invalidation in iotlb and devtlb. Today
> >> only RID is tracked per domain for invalidation. it needs extension to
> >> walk attached pasid too.  
> > Yes, will add.
> > 
> > For the set up path, there is no need to flush IOTLBs,  because we're
> > going from non present to present.
> > 
> > On the remove path, IOTLB flush should be covered when device driver
> > calls iommu_detach_device_pasid(). Covered with this patch.  
> 
> It's not only for the PASID teardown path, but also for unmap(). As the
> device has issued DMA requests with PASID, the IOMMU probably will cache
> the DMA translation with PASID tagged. Hence, we need to invalidate the
> PASID-specific IOTLB and device TLB in the unmap() path.
> 
> I once had a patch for this:
> 
> https://lore.kernel.org/linux-iommu/20220614034411.1634238-1-baolu.lu@linux.intel.com/
> 
> Probably you can use it as a starting point.
understood, actually my previous version had unmap flush, based on yours as
well.
https://lore.kernel.org/lkml/20220518182120.1136715-1-jacob.jun.pan@linux.intel.com/T/
> 
> Best regards,
> baolu


Thanks,

Jacob

> From: Baolu Lu <baolu.lu@linux.intel.com>
> Sent: Sunday, March 5, 2023 11:06 AM
> 
> On 3/4/23 12:35 AM, Jacob Pan wrote:
> >>> From: Baolu Lu<baolu.lu@linux.intel.com>
> >>> Sent: Thursday, March 2, 2023 10:07 PM
> >>>
> >>>> +
> >>>> +	if (hw_pass_through && domain_type_is_si(dmar_domain))
> >>>> +		ret = intel_pasid_setup_pass_through(iommu,
> >>>> dmar_domain,
> >>>> +						     dev, pasid);
> >>>> +	else if (dmar_domain->use_first_level)
> >>>> +		ret = domain_setup_first_level(iommu, dmar_domain,
> >>>> +					       dev, pasid);
> >>>> +	else
> >>>> +		ret = intel_pasid_setup_second_level(iommu,
> >>>> dmar_domain,
> >>>> +						     dev, pasid);
> >>>> +
> >>>> +	return ret;
> >>>> +}
> >>> Do you need to consider pasid cache invalidation?
> >>>
> >> To avoid confusion this is not about invalidation of pasid cache itself
> >> which should be covered by above setup functions already.
> >>
> >> Here actually means per-PASID invalidation in iotlb and devtlb. Today
> >> only RID is tracked per domain for invalidation. it needs extension to
> >> walk attached pasid too.
> > Yes, will add.
> >
> > For the set up path, there is no need to flush IOTLBs,  because we're going
> > from non present to present.
> >
> > On the remove path, IOTLB flush should be covered when device driver
> > calls iommu_detach_device_pasid(). Covered with this patch.
> 
> It's not only for the PASID teardown path, but also for unmap(). As the
> device has issued DMA requests with PASID, the IOMMU probably will cache
> the DMA translation with PASID tagged. Hence, we need to invalidate the
> PASID-specific IOTLB and device TLB in the unmap() path.
> 
> I once had a patch for this:
> 
> https://lore.kernel.org/linux-iommu/20220614034411.1634238-1-
> baolu.lu@linux.intel.com/
> 
> Probably you can use it as a starting point.
> 

just that we should not have a sub-device term there. Just name
the tracking information per pasid.

Hi Kevin,

On Mon, 6 Mar 2023 08:18:37 +0000, "Tian, Kevin" <kevin.tian@intel.com>
wrote:

> > From: Baolu Lu <baolu.lu@linux.intel.com>
> > Sent: Sunday, March 5, 2023 11:06 AM
> > 
> > On 3/4/23 12:35 AM, Jacob Pan wrote:  
> > >>> From: Baolu Lu<baolu.lu@linux.intel.com>
> > >>> Sent: Thursday, March 2, 2023 10:07 PM
> > >>>  
> > >>>> +
> > >>>> +	if (hw_pass_through && domain_type_is_si(dmar_domain))
> > >>>> +		ret = intel_pasid_setup_pass_through(iommu,
> > >>>> dmar_domain,
> > >>>> +						     dev, pasid);
> > >>>> +	else if (dmar_domain->use_first_level)
> > >>>> +		ret = domain_setup_first_level(iommu, dmar_domain,
> > >>>> +					       dev, pasid);
> > >>>> +	else
> > >>>> +		ret = intel_pasid_setup_second_level(iommu,
> > >>>> dmar_domain,
> > >>>> +						     dev, pasid);
> > >>>> +
> > >>>> +	return ret;
> > >>>> +}  
> > >>> Do you need to consider pasid cache invalidation?
> > >>>  
> > >> To avoid confusion this is not about invalidation of pasid cache
> > >> itself which should be covered by above setup functions already.
> > >>
> > >> Here actually means per-PASID invalidation in iotlb and devtlb. Today
> > >> only RID is tracked per domain for invalidation. it needs extension
> > >> to walk attached pasid too.  
> > > Yes, will add.
> > >
> > > For the set up path, there is no need to flush IOTLBs,  because we're
> > > going from non present to present.
> > >
> > > On the remove path, IOTLB flush should be covered when device driver
> > > calls iommu_detach_device_pasid(). Covered with this patch.  
> > 
> > It's not only for the PASID teardown path, but also for unmap(). As the
> > device has issued DMA requests with PASID, the IOMMU probably will cache
> > the DMA translation with PASID tagged. Hence, we need to invalidate the
> > PASID-specific IOTLB and device TLB in the unmap() path.
> > 
> > I once had a patch for this:
> > 
> > https://lore.kernel.org/linux-iommu/20220614034411.1634238-1-
> > baolu.lu@linux.intel.com/
> > 
> > Probably you can use it as a starting point.
> >   
> 
> just that we should not have a sub-device term there. Just name
> the tracking information per pasid.
Sounds good, I should be enable to use Baolu's patch for the most part.

Thanks,

Jacob

> From: Baolu Lu <baolu.lu@linux.intel.com>
> Sent: Thursday, March 2, 2023 10:07 PM
> > +
> > +	if (!sm_supported(iommu) || !info)
> 
> @info has been referenced. !info check makes no sense.
> 
> Add pasid_supported(iommu).
> 
> Do you need to check whether the domain is compatible for this rid
> pasid?

what kind of compatibility is concerned here? In concept a pasid
can be attached to any domain if it has been successfully attached
to rid. Probably we can add a check here that RID2PASID must
point to the domain already.

> 
> > +		return -ENODEV;
> > +
> > +	if (WARN_ON(pasid == PASID_RID2PASID))
> > +		return -EINVAL;
> 
> Add a call to domain_attach_iommu() here to get a refcount of the domain
> ID. And call domain_detach_iommu() in intel_iommu_remove_dev_pasid().
> 

Is it necessary? iommu core doesn't allow taking ownership
if !xa_empty(&group->pasid_array) so if this pasid attach succeeds
this device cannot be attached to another domain before pasid
detach is done on the current domain.

On 3/3/23 10:36 AM, Tian, Kevin wrote:
>> From: Baolu Lu <baolu.lu@linux.intel.com>
>> Sent: Thursday, March 2, 2023 10:07 PM
>>> +
>>> +	if (!sm_supported(iommu) || !info)
>>
>> @info has been referenced. !info check makes no sense.
>>
>> Add pasid_supported(iommu).
>>
>> Do you need to check whether the domain is compatible for this rid
>> pasid?
> 
> what kind of compatibility is concerned here? In concept a pasid
> can be attached to any domain if it has been successfully attached
> to rid. Probably we can add a check here that RID2PASID must
> point to the domain already.

"...if it has been successfully attached to rid..."

We should not have this assumption in iommu driver's callback. The iommu
driver has no (and should not have) knowledge about the history of any
domain.

> 
>>
>>> +		return -ENODEV;
>>> +
>>> +	if (WARN_ON(pasid == PASID_RID2PASID))
>>> +		return -EINVAL;
>>
>> Add a call to domain_attach_iommu() here to get a refcount of the domain
>> ID. And call domain_detach_iommu() in intel_iommu_remove_dev_pasid().
>>
> 
> Is it necessary? iommu core doesn't allow taking ownership
> if !xa_empty(&group->pasid_array) so if this pasid attach succeeds
> this device cannot be attached to another domain before pasid
> detach is done on the current domain.

It's not about the pasid, but the domain id.

This domain's id will be set to a field of the device's pasid entry. It
must get a refcount of that domain id to avoid use after free.

Best regards,
baolu

> From: Baolu Lu <baolu.lu@linux.intel.com>
> Sent: Friday, March 3, 2023 10:49 AM
> 
> On 3/3/23 10:36 AM, Tian, Kevin wrote:
> >> From: Baolu Lu <baolu.lu@linux.intel.com>
> >> Sent: Thursday, March 2, 2023 10:07 PM
> >>> +
> >>> +	if (!sm_supported(iommu) || !info)
> >>
> >> @info has been referenced. !info check makes no sense.
> >>
> >> Add pasid_supported(iommu).
> >>
> >> Do you need to check whether the domain is compatible for this rid
> >> pasid?
> >
> > what kind of compatibility is concerned here? In concept a pasid
> > can be attached to any domain if it has been successfully attached
> > to rid. Probably we can add a check here that RID2PASID must
> > point to the domain already.
> 
> "...if it has been successfully attached to rid..."
> 
> We should not have this assumption in iommu driver's callback. The iommu
> driver has no (and should not have) knowledge about the history of any
> domain.

but this is an op for default domain which must have been attached
to RID2PASID and any compatibility check between this domain and device
should be passed.

We can have another set_pasid for unmanaged which then need similar
check as prepare_domain_attach_device() does.

> 
> >
> >>
> >>> +		return -ENODEV;
> >>> +
> >>> +	if (WARN_ON(pasid == PASID_RID2PASID))
> >>> +		return -EINVAL;
> >>
> >> Add a call to domain_attach_iommu() here to get a refcount of the
> domain
> >> ID. And call domain_detach_iommu() in
> intel_iommu_remove_dev_pasid().
> >>
> >
> > Is it necessary? iommu core doesn't allow taking ownership
> > if !xa_empty(&group->pasid_array) so if this pasid attach succeeds
> > this device cannot be attached to another domain before pasid
> > detach is done on the current domain.
> 
> It's not about the pasid, but the domain id.
> 
> This domain's id will be set to a field of the device's pasid entry. It
> must get a refcount of that domain id to avoid use after free.
> 

If the domain still has attached device (due to this pasid usage) how could
domain id be freed?

On 3/3/23 11:02 AM, Tian, Kevin wrote:
>> From: Baolu Lu <baolu.lu@linux.intel.com>
>> Sent: Friday, March 3, 2023 10:49 AM
>>
>> On 3/3/23 10:36 AM, Tian, Kevin wrote:
>>>> From: Baolu Lu <baolu.lu@linux.intel.com>
>>>> Sent: Thursday, March 2, 2023 10:07 PM
>>>>> +
>>>>> +	if (!sm_supported(iommu) || !info)
>>>>
>>>> @info has been referenced. !info check makes no sense.
>>>>
>>>> Add pasid_supported(iommu).
>>>>
>>>> Do you need to check whether the domain is compatible for this rid
>>>> pasid?
>>>
>>> what kind of compatibility is concerned here? In concept a pasid
>>> can be attached to any domain if it has been successfully attached
>>> to rid. Probably we can add a check here that RID2PASID must
>>> point to the domain already.
>>
>> "...if it has been successfully attached to rid..."
>>
>> We should not have this assumption in iommu driver's callback. The iommu
>> driver has no (and should not have) knowledge about the history of any
>> domain.
> 
> but this is an op for default domain which must have been attached
> to RID2PASID and any compatibility check between this domain and device
> should be passed.

This is an op for DMA, DMA-FQ and UNMANAGED domain. The IOMMU driver
doesn't need to interpret the default domain concept. :-)

> 
> We can have another set_pasid for unmanaged which then need similar
> check as prepare_domain_attach_device() does.

 From the perspective of the iommu driver, there's no essential
difference between DMA and UNMANAGED domains. So almost all IOMMU
drivers maintain a single set of domain ops for them.

>>
>>>
>>>>
>>>>> +		return -ENODEV;
>>>>> +
>>>>> +	if (WARN_ON(pasid == PASID_RID2PASID))
>>>>> +		return -EINVAL;
>>>>
>>>> Add a call to domain_attach_iommu() here to get a refcount of the
>> domain
>>>> ID. And call domain_detach_iommu() in
>> intel_iommu_remove_dev_pasid().
>>>>
>>>
>>> Is it necessary? iommu core doesn't allow taking ownership
>>> if !xa_empty(&group->pasid_array) so if this pasid attach succeeds
>>> this device cannot be attached to another domain before pasid
>>> detach is done on the current domain.
>>
>> It's not about the pasid, but the domain id.
>>
>> This domain's id will be set to a field of the device's pasid entry. It
>> must get a refcount of that domain id to avoid use after free.
>>
> 
> If the domain still has attached device (due to this pasid usage) how could
> domain id be freed?

The Intel IOMMU driver uses a user counter to determine when the domain
id could be freed.

Best regards,
baolu

> From: Baolu Lu <baolu.lu@linux.intel.com>
> Sent: Friday, March 3, 2023 12:38 PM
> 
> On 3/3/23 11:02 AM, Tian, Kevin wrote:
> >> From: Baolu Lu <baolu.lu@linux.intel.com>
> >> Sent: Friday, March 3, 2023 10:49 AM
> >>
> >> On 3/3/23 10:36 AM, Tian, Kevin wrote:
> >>>> From: Baolu Lu <baolu.lu@linux.intel.com>
> >>>> Sent: Thursday, March 2, 2023 10:07 PM
> >>>>> +
> >>>>> +	if (!sm_supported(iommu) || !info)
> >>>>
> >>>> @info has been referenced. !info check makes no sense.
> >>>>
> >>>> Add pasid_supported(iommu).
> >>>>
> >>>> Do you need to check whether the domain is compatible for this rid
> >>>> pasid?
> >>>
> >>> what kind of compatibility is concerned here? In concept a pasid
> >>> can be attached to any domain if it has been successfully attached
> >>> to rid. Probably we can add a check here that RID2PASID must
> >>> point to the domain already.
> >>
> >> "...if it has been successfully attached to rid..."
> >>
> >> We should not have this assumption in iommu driver's callback. The
> iommu
> >> driver has no (and should not have) knowledge about the history of any
> >> domain.
> >
> > but this is an op for default domain which must have been attached
> > to RID2PASID and any compatibility check between this domain and device
> > should be passed.
> 
> This is an op for DMA, DMA-FQ and UNMANAGED domain. The IOMMU
> driver
> doesn't need to interpret the default domain concept. :-)
> 

yes if we target a general callback for all those domain types.

and probably this is the right thing to do as in the end DMA type will
be removed with Jason's cleanup

Hi Kevin,

On Fri, 3 Mar 2023 05:35:58 +0000, "Tian, Kevin" <kevin.tian@intel.com>
wrote:

> > From: Baolu Lu <baolu.lu@linux.intel.com>
> > Sent: Friday, March 3, 2023 12:38 PM
> > 
> > On 3/3/23 11:02 AM, Tian, Kevin wrote:  
> > >> From: Baolu Lu <baolu.lu@linux.intel.com>
> > >> Sent: Friday, March 3, 2023 10:49 AM
> > >>
> > >> On 3/3/23 10:36 AM, Tian, Kevin wrote:  
> > >>>> From: Baolu Lu <baolu.lu@linux.intel.com>
> > >>>> Sent: Thursday, March 2, 2023 10:07 PM  
> > >>>>> +
> > >>>>> +	if (!sm_supported(iommu) || !info)  
> > >>>>
> > >>>> @info has been referenced. !info check makes no sense.
> > >>>>
> > >>>> Add pasid_supported(iommu).
> > >>>>
> > >>>> Do you need to check whether the domain is compatible for this rid
> > >>>> pasid?  
> > >>>
> > >>> what kind of compatibility is concerned here? In concept a pasid
> > >>> can be attached to any domain if it has been successfully attached
> > >>> to rid. Probably we can add a check here that RID2PASID must
> > >>> point to the domain already.  
> > >>
> > >> "...if it has been successfully attached to rid..."
> > >>
> > >> We should not have this assumption in iommu driver's callback. The  
> > iommu  
> > >> driver has no (and should not have) knowledge about the history of
> > >> any domain.  
> > >
> > > but this is an op for default domain which must have been attached
> > > to RID2PASID and any compatibility check between this domain and
> > > device should be passed.  
> > 
> > This is an op for DMA, DMA-FQ and UNMANAGED domain. The IOMMU
> > driver
> > doesn't need to interpret the default domain concept. :-)
> >   
> 
> yes if we target a general callback for all those domain types.
> 
> and probably this is the right thing to do as in the end DMA type will
> be removed with Jason's cleanup
so, let me recap. set_dev_pasid() should make no assumptions of
ordering, i.e. it is equal to iommu_domain_ops.attach_dev().
It will be kind of the same as Baolu's old patch
https://lore.kernel.org/linux-iommu/20220614034411.1634238-1-baolu.lu@linux.intel.com/


Thanks,

Jacob

On Mon, Mar 06, 2023 at 11:04:43AM -0800, Jacob Pan wrote:

> > and probably this is the right thing to do as in the end DMA type will
> > be removed with Jason's cleanup
>
> so, let me recap. set_dev_pasid() should make no assumptions of
> ordering, i.e. it is equal to iommu_domain_ops.attach_dev().

Absolutely yes.

You should factor out all the "prepare the domain to be used" code and
call it in both places.

Jason

Hi Jason,

On Mon, 6 Mar 2023 15:02:37 -0400, Jason Gunthorpe <jgg@nvidia.com> wrote:

> On Mon, Mar 06, 2023 at 11:04:43AM -0800, Jacob Pan wrote:
> 
> > > and probably this is the right thing to do as in the end DMA type will
> > > be removed with Jason's cleanup  
> >
> > so, let me recap. set_dev_pasid() should make no assumptions of
> > ordering, i.e. it is equal to iommu_domain_ops.attach_dev().  
> 
> Absolutely yes.
> 
> You should factor out all the "prepare the domain to be used" code and
> call it in both places.
> 
I think this has been done by Baolu
https://lore.kernel.org/linux-iommu/20190325013036.18400-1-baolu.lu@linux.intel.com/T/#m8c980357a39dc75dc360ff0f71dc7ebe1e49f9a6
iommu/vt-d: Move common code out of iommu_attch_device()
    
    This part of code could be used by both normal and aux
    domain specific attach entries. Hence move them into a
    common function to avoid duplication.

set_dev_pasid() will call prepare_domain_attach_device() as well.

Thanks,

Jacob

Hi Jacob,

On Mon, 6 Mar 2023 15:45:04 -0800, Jacob Pan
<jacob.jun.pan@linux.intel.com> wrote:

> Hi Jason,
> 
> On Mon, 6 Mar 2023 15:02:37 -0400, Jason Gunthorpe <jgg@nvidia.com> wrote:
> 
> > On Mon, Mar 06, 2023 at 11:04:43AM -0800, Jacob Pan wrote:
> >   
> > > > and probably this is the right thing to do as in the end DMA type
> > > > will be removed with Jason's cleanup    
> > >
> > > so, let me recap. set_dev_pasid() should make no assumptions of
> > > ordering, i.e. it is equal to iommu_domain_ops.attach_dev().    
> > 
> > Absolutely yes.
> > 
> > You should factor out all the "prepare the domain to be used" code and
> > call it in both places.
> >   
> I think this has been done by Baolu
> https://lore.kernel.org/linux-iommu/20190325013036.18400-1-baolu.lu@linux.intel.com/T/#m8c980357a39dc75dc360ff0f71dc7ebe1e49f9a6
> iommu/vt-d: Move common code out of iommu_attch_device()
>     
>     This part of code could be used by both normal and aux
>     domain specific attach entries. Hence move them into a
>     common function to avoid duplication.
> 
> set_dev_pasid() will call prepare_domain_attach_device() as well.
Actually, there are more to be factored to common code if we take that
assumption away. attach_dev() can be viewed as a special case for
set_dev_pasid() except the PASID is RID_PASID.

Thanks,

Jacob

> From: Jacob Pan <jacob.jun.pan@linux.intel.com>
> Sent: Thursday, March 2, 2023 9:00 AM
> 
> @@ -4675,6 +4679,33 @@ static void
> intel_iommu_remove_dev_pasid(struct device *dev, ioasid_t pasid)
>  	intel_pasid_tear_down_entry(iommu, dev, pasid, false);
>  }
> 
> +static int intel_iommu_set_dev_pasid(struct iommu_domain *domain,
> +				   struct device *dev, ioasid_t pasid)
> +{
> +	struct device_domain_info *info = dev_iommu_priv_get(dev);
> +	struct dmar_domain *dmar_domain = to_dmar_domain(domain);
> +	struct intel_iommu *iommu = info->iommu;
> +	int ret = 0;
> +
> +	if (!sm_supported(iommu) || !info)
> +		return -ENODEV;
> +
> +	if (WARN_ON(pasid == PASID_RID2PASID))
> +		return -EINVAL;
> +
> +	if (hw_pass_through && domain_type_is_si(dmar_domain))
> +		ret = intel_pasid_setup_pass_through(iommu, dmar_domain,
> +						     dev, pasid);
> +	else if (dmar_domain->use_first_level)
> +		ret = domain_setup_first_level(iommu, dmar_domain,
> +					       dev, pasid);
> +	else
> +		ret = intel_pasid_setup_second_level(iommu, dmar_domain,
> +						     dev, pasid);
> +

check whether pasid capability has been enabled on this device.

it's unlike SVA which has separate capability to tell.

Hi Kevin,

On Thu, 2 Mar 2023 09:37:30 +0000, "Tian, Kevin" <kevin.tian@intel.com>
wrote:

> > From: Jacob Pan <jacob.jun.pan@linux.intel.com>
> > Sent: Thursday, March 2, 2023 9:00 AM
> > 
> > @@ -4675,6 +4679,33 @@ static void
> > intel_iommu_remove_dev_pasid(struct device *dev, ioasid_t pasid)
> >  	intel_pasid_tear_down_entry(iommu, dev, pasid, false);
> >  }
> > 
> > +static int intel_iommu_set_dev_pasid(struct iommu_domain *domain,
> > +				   struct device *dev, ioasid_t pasid)
> > +{
> > +	struct device_domain_info *info = dev_iommu_priv_get(dev);
> > +	struct dmar_domain *dmar_domain = to_dmar_domain(domain);
> > +	struct intel_iommu *iommu = info->iommu;
> > +	int ret = 0;
> > +
> > +	if (!sm_supported(iommu) || !info)
> > +		return -ENODEV;
> > +
> > +	if (WARN_ON(pasid == PASID_RID2PASID))
> > +		return -EINVAL;
> > +
> > +	if (hw_pass_through && domain_type_is_si(dmar_domain))
> > +		ret = intel_pasid_setup_pass_through(iommu,
> > dmar_domain,
> > +						     dev, pasid);
> > +	else if (dmar_domain->use_first_level)
> > +		ret = domain_setup_first_level(iommu, dmar_domain,
> > +					       dev, pasid);
> > +	else
> > +		ret = intel_pasid_setup_second_level(iommu,
> > dmar_domain,
> > +						     dev, pasid);
> > +  
> 
> check whether pasid capability has been enabled on this device.
> 
> it's unlike SVA which has separate capability to tell.
yes, it is a little tricky in that enabling PASID should be done before ATS
but for now anything uses ENQCMDS should support ATS. I will add
	/*
	 * PASID should be enabled as part of PCI caps enabling where
	 * ordering is required relative to ATS.
	 */
	if (WARN_ON(!pdev->pasid_enabled))
		return -ENODEV;

Thanks,

Jacob