[PATCH 1/1] iommu/vt-d: Enable PASID during iommu device probe

[PATCH 1/1] iommu/vt-d: Enable PASID during iommu device probe

[Lu Baolu]

Previously PASID supports on both IOMMU and PCI devices are enabled in the
iommu_dev_enable_feature(dev, IOMMU_DEV_FEAT_SVA) path. It's functionally
correct as the SVA is the only feature that requires PASID setup. However,
looking ahead, we will add more features that need to enable pasid (for
example, kernel DMA with PASID, SIOV, VM guest SVA, etc.). It makes more
sense to enable PASID during iommu probing device.

This enables PASID during iommu probing device and deprecates the
intel_iommu_enable_pasid() helper. This is safe because the IOMMU hardware
will block any PCI TLP with a PASID prefix if there is no IOMMU domain
attached to the PASID of the device.

Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
---
 drivers/iommu/intel/iommu.h |  1 -
 drivers/iommu/intel/iommu.c | 74 +++++++------------------------------
 drivers/iommu/intel/Kconfig |  4 +-
 3 files changed, 16 insertions(+), 63 deletions(-)

diff --git a/drivers/iommu/intel/iommu.h b/drivers/iommu/intel/iommu.h
index fae45bbb0c7f..ce5f343ca864 100644
--- a/drivers/iommu/intel/iommu.h
+++ b/drivers/iommu/intel/iommu.h
@@ -737,7 +737,6 @@ extern int dmar_ir_support(void);
 void *alloc_pgtable_page(int node);
 void free_pgtable_page(void *vaddr);
 void iommu_flush_write_buffer(struct intel_iommu *iommu);
-int intel_iommu_enable_pasid(struct intel_iommu *iommu, struct device *dev);
 struct intel_iommu *device_to_iommu(struct device *dev, u8 *bus, u8 *devfn);
 
 #ifdef CONFIG_INTEL_IOMMU_SVM
diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index 7cca030a508e..69357c7c8c39 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -231,6 +231,11 @@ static inline void context_set_domain_id(struct context_entry *context,
 	context->hi |= (value & ((1 << 16) - 1)) << 8;
 }
 
+static inline void context_set_pasid(struct context_entry *context)
+{
+	context->lo |= CONTEXT_PASIDE;
+}
+
 static inline int context_domain_id(struct context_entry *c)
 {
 	return((c->hi >> 8) & 0xffff);
@@ -1341,20 +1346,17 @@ static void __iommu_flush_iotlb(struct intel_iommu *iommu, u16 did,
 }
 
 static struct device_domain_info *
-iommu_support_dev_iotlb(struct dmar_domain *domain, struct intel_iommu *iommu,
-			u8 bus, u8 devfn)
+domain_lookup_dev_info(struct dmar_domain *domain,
+		       struct intel_iommu *iommu, u8 bus, u8 devfn)
 {
 	struct device_domain_info *info;
 
-	if (!iommu->qi)
-		return NULL;
-
 	spin_lock(&domain->lock);
 	list_for_each_entry(info, &domain->devices, link) {
 		if (info->iommu == iommu && info->bus == bus &&
 		    info->devfn == devfn) {
 			spin_unlock(&domain->lock);
-			return info->ats_supported ? info : NULL;
+			return info;
 		}
 	}
 	spin_unlock(&domain->lock);
@@ -1401,7 +1403,6 @@ static void iommu_enable_dev_iotlb(struct device_domain_info *info)
 		info->pfsid = pci_dev_id(pf_pdev);
 	}
 
-#ifdef CONFIG_INTEL_IOMMU_SVM
 	/* The PCIe spec, in its wisdom, declares that the behaviour of
 	   the device if you enable PASID support after ATS support is
 	   undefined. So always enable PASID support on devices which
@@ -1414,7 +1415,7 @@ static void iommu_enable_dev_iotlb(struct device_domain_info *info)
 	    (info->pasid_enabled ? pci_prg_resp_pasid_required(pdev) : 1)  &&
 	    !pci_reset_pri(pdev) && !pci_enable_pri(pdev, PRQ_DEPTH))
 		info->pri_enabled = 1;
-#endif
+
 	if (info->ats_supported && pci_ats_page_aligned(pdev) &&
 	    !pci_enable_ats(pdev, VTD_PAGE_SHIFT)) {
 		info->ats_enabled = 1;
@@ -1437,16 +1438,16 @@ static void iommu_disable_dev_iotlb(struct device_domain_info *info)
 		info->ats_enabled = 0;
 		domain_update_iotlb(info->domain);
 	}
-#ifdef CONFIG_INTEL_IOMMU_SVM
+
 	if (info->pri_enabled) {
 		pci_disable_pri(pdev);
 		info->pri_enabled = 0;
 	}
+
 	if (info->pasid_enabled) {
 		pci_disable_pasid(pdev);
 		info->pasid_enabled = 0;
 	}
-#endif
 }
 
 static void __iommu_flush_dev_iotlb(struct device_domain_info *info,
@@ -1890,7 +1891,7 @@ static int domain_context_mapping_one(struct dmar_domain *domain,
 				      u8 bus, u8 devfn)
 {
 	struct device_domain_info *info =
-			iommu_support_dev_iotlb(domain, iommu, bus, devfn);
+			domain_lookup_dev_info(domain, iommu, bus, devfn);
 	u16 did = domain_id_iommu(domain, iommu);
 	int translation = CONTEXT_TT_MULTI_LEVEL;
 	struct context_entry *context;
@@ -1961,6 +1962,8 @@ static int domain_context_mapping_one(struct dmar_domain *domain,
 			context_set_sm_dte(context);
 		if (info && info->pri_supported)
 			context_set_sm_pre(context);
+		if (info && info->pasid_supported)
+			context_set_pasid(context);
 	} else {
 		struct dma_pte *pgd = domain->pgd;
 		int agaw;
@@ -4566,52 +4569,6 @@ static void intel_iommu_get_resv_regions(struct device *device,
 	list_add_tail(&reg->list, head);
 }
 
-int intel_iommu_enable_pasid(struct intel_iommu *iommu, struct device *dev)
-{
-	struct device_domain_info *info = dev_iommu_priv_get(dev);
-	struct context_entry *context;
-	struct dmar_domain *domain;
-	u64 ctx_lo;
-	int ret;
-
-	domain = info->domain;
-	if (!domain)
-		return -EINVAL;
-
-	spin_lock(&iommu->lock);
-	ret = -EINVAL;
-	if (!info->pasid_supported)
-		goto out;
-
-	context = iommu_context_addr(iommu, info->bus, info->devfn, 0);
-	if (WARN_ON(!context))
-		goto out;
-
-	ctx_lo = context[0].lo;
-
-	if (!(ctx_lo & CONTEXT_PASIDE)) {
-		ctx_lo |= CONTEXT_PASIDE;
-		context[0].lo = ctx_lo;
-		wmb();
-		iommu->flush.flush_context(iommu,
-					   domain_id_iommu(domain, iommu),
-					   PCI_DEVID(info->bus, info->devfn),
-					   DMA_CCMD_MASK_NOBIT,
-					   DMA_CCMD_DEVICE_INVL);
-	}
-
-	/* Enable PASID support in the device, if it wasn't already */
-	if (!info->pasid_enabled)
-		iommu_enable_dev_iotlb(info);
-
-	ret = 0;
-
- out:
-	spin_unlock(&iommu->lock);
-
-	return ret;
-}
-
 static struct iommu_group *intel_iommu_device_group(struct device *dev)
 {
 	if (dev_is_pci(dev))
@@ -4635,9 +4592,6 @@ static int intel_iommu_enable_sva(struct device *dev)
 	if (!(iommu->flags & VTD_FLAG_SVM_CAPABLE))
 		return -ENODEV;
 
-	if (intel_iommu_enable_pasid(iommu, dev))
-		return -ENODEV;
-
 	if (!info->pasid_enabled || !info->pri_enabled || !info->ats_enabled)
 		return -EINVAL;
 
diff --git a/drivers/iommu/intel/Kconfig b/drivers/iommu/intel/Kconfig
index 39a06d245f12..b3f40375f214 100644
--- a/drivers/iommu/intel/Kconfig
+++ b/drivers/iommu/intel/Kconfig
@@ -21,6 +21,8 @@ config INTEL_IOMMU
 	select IOASID
 	select IOMMU_DMA
 	select PCI_ATS
+	select PCI_PRI
+	select PCI_PASID
 	help
 	  DMA remapping (DMAR) devices support enables independent address
 	  translations for Direct Memory Access (DMA) from devices.
@@ -48,8 +50,6 @@ config INTEL_IOMMU_DEBUGFS
 config INTEL_IOMMU_SVM
 	bool "Support for Shared Virtual Memory with Intel IOMMU"
 	depends on X86_64
-	select PCI_PASID
-	select PCI_PRI
 	select MMU_NOTIFIER
 	select IOASID
 	select IOMMU_SVA
-- 
2.25.1

RE: [PATCH 1/1] iommu/vt-d: Enable PASID during iommu device probe

[Tian, Kevin]

> From: Lu Baolu <baolu.lu@linux.intel.com>
> Sent: Monday, September 12, 2022 10:48 AM
> 
> Previously PASID supports on both IOMMU and PCI devices are enabled in
> the
> iommu_dev_enable_feature(dev, IOMMU_DEV_FEAT_SVA) path. It's
> functionally
> correct as the SVA is the only feature that requires PASID setup. However,
> looking ahead, we will add more features that need to enable pasid (for
> example, kernel DMA with PASID, SIOV, VM guest SVA, etc.). It makes more
> sense to enable PASID during iommu probing device.
> 
> This enables PASID during iommu probing device and deprecates the
> intel_iommu_enable_pasid() helper. This is safe because the IOMMU
> hardware
> will block any PCI TLP with a PASID prefix if there is no IOMMU domain
> attached to the PASID of the device.
> 

IMHO it's better to enable something only when it's actually required,
e.g. does it make more sense to have a IOMMU_DEV_FEAT_PASID
instead?

What this patch does has one problem. It's an intel-iommu driver
internal policy. How can a device driver reliably tell that the pasid
capability has been enabled by the iommu driver?

Thanks
Kevin

Re: [PATCH 1/1] iommu/vt-d: Enable PASID during iommu device probe

[Baolu Lu]

Hi Kevin,

On 2022/9/13 11:13, Tian, Kevin wrote:
>> From: Lu Baolu <baolu.lu@linux.intel.com>
>> Sent: Monday, September 12, 2022 10:48 AM
>>
>> Previously PASID supports on both IOMMU and PCI devices are enabled in
>> the
>> iommu_dev_enable_feature(dev, IOMMU_DEV_FEAT_SVA) path. It's
>> functionally
>> correct as the SVA is the only feature that requires PASID setup. However,
>> looking ahead, we will add more features that need to enable pasid (for
>> example, kernel DMA with PASID, SIOV, VM guest SVA, etc.). It makes more
>> sense to enable PASID during iommu probing device.
>>
>> This enables PASID during iommu probing device and deprecates the
>> intel_iommu_enable_pasid() helper. This is safe because the IOMMU
>> hardware
>> will block any PCI TLP with a PASID prefix if there is no IOMMU domain
>> attached to the PASID of the device.
>>
> 
> IMHO it's better to enable something only when it's actually required,
> e.g. does it make more sense to have a IOMMU_DEV_FEAT_PASID
> instead?

PASID is a capability (not a feature) of a device. Hence from my point
of view, the IOMMU driver could enable it by default as long as the
IOMMU can handle transactions with PASID. Currently other PCIe
capabilities like ATS and PRI are also handled in this way.

> 
> What this patch does has one problem. It's an intel-iommu driver
> internal policy. How can a device driver reliably tell that the pasid
> capability has been enabled by the iommu driver?

If *necessary*, I do not object to letting the device drivers enable or
disable PCI/PASID through an IOMMU interface. In that case, we may need
a reference counter, and explicitly tell the device driver that
disabling PASID will only take effect when the reference counter
becomes 0.

Best regards,
baolu

RE: [PATCH 1/1] iommu/vt-d: Enable PASID during iommu device probe

[Tian, Kevin]

> From: Baolu Lu <baolu.lu@linux.intel.com>
> Sent: Tuesday, September 13, 2022 2:01 PM
> 
> Hi Kevin,
> 
> On 2022/9/13 11:13, Tian, Kevin wrote:
> >> From: Lu Baolu <baolu.lu@linux.intel.com>
> >> Sent: Monday, September 12, 2022 10:48 AM
> >>
> >> Previously PASID supports on both IOMMU and PCI devices are enabled in
> >> the
> >> iommu_dev_enable_feature(dev, IOMMU_DEV_FEAT_SVA) path. It's
> >> functionally
> >> correct as the SVA is the only feature that requires PASID setup. However,
> >> looking ahead, we will add more features that need to enable pasid (for
> >> example, kernel DMA with PASID, SIOV, VM guest SVA, etc.). It makes
> more
> >> sense to enable PASID during iommu probing device.
> >>
> >> This enables PASID during iommu probing device and deprecates the
> >> intel_iommu_enable_pasid() helper. This is safe because the IOMMU
> >> hardware
> >> will block any PCI TLP with a PASID prefix if there is no IOMMU domain
> >> attached to the PASID of the device.
> >>
> >
> > IMHO it's better to enable something only when it's actually required,
> > e.g. does it make more sense to have a IOMMU_DEV_FEAT_PASID
> > instead?
> 
> PASID is a capability (not a feature) of a device. Hence from my point
> of view, the IOMMU driver could enable it by default as long as the
> IOMMU can handle transactions with PASID. Currently other PCIe
> capabilities like ATS and PRI are also handled in this way.
> 

OK, then it makes some sense. and later the PASID capability can
be reported so a driver can query to enable certain usage (e.g. SIOV)
based on it.

Re: [PATCH 1/1] iommu/vt-d: Enable PASID during iommu device probe

[Ethan Zhao]

Baolu,

在 2022/9/12 10:48, Lu Baolu 写道:
> Previously PASID supports on both IOMMU and PCI devices are enabled in the
> iommu_dev_enable_feature(dev, IOMMU_DEV_FEAT_SVA) path. It's functionally
> correct as the SVA is the only feature that requires PASID setup. However,
> looking ahead, we will add more features that need to enable pasid (for
> example, kernel DMA with PASID, SIOV, VM guest SVA, etc.). It makes more
> sense to enable PASID during iommu probing device.
>
> This enables PASID during iommu probing device and deprecates the
> intel_iommu_enable_pasid() helper. This is safe because the IOMMU hardware
> will block any PCI TLP with a PASID prefix if there is no IOMMU domain
> attached to the PASID of the device.

What the error path would be if this code runs on some old platforms don't

support PASID, would you print out "this platform doesn't suppor PASID" and

give users an interface function to query if the PASID cap of iommu is 
enabled

and if not why ?


Thanks,

Ethan

> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
> ---
>   drivers/iommu/intel/iommu.h |  1 -
>   drivers/iommu/intel/iommu.c | 74 +++++++------------------------------
>   drivers/iommu/intel/Kconfig |  4 +-
>   3 files changed, 16 insertions(+), 63 deletions(-)
>
> diff --git a/drivers/iommu/intel/iommu.h b/drivers/iommu/intel/iommu.h
> index fae45bbb0c7f..ce5f343ca864 100644
> --- a/drivers/iommu/intel/iommu.h
> +++ b/drivers/iommu/intel/iommu.h
> @@ -737,7 +737,6 @@ extern int dmar_ir_support(void);
>   void *alloc_pgtable_page(int node);
>   void free_pgtable_page(void *vaddr);
>   void iommu_flush_write_buffer(struct intel_iommu *iommu);
> -int intel_iommu_enable_pasid(struct intel_iommu *iommu, struct device *dev);
>   struct intel_iommu *device_to_iommu(struct device *dev, u8 *bus, u8 *devfn);
>   
>   #ifdef CONFIG_INTEL_IOMMU_SVM
> diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
> index 7cca030a508e..69357c7c8c39 100644
> --- a/drivers/iommu/intel/iommu.c
> +++ b/drivers/iommu/intel/iommu.c
> @@ -231,6 +231,11 @@ static inline void context_set_domain_id(struct context_entry *context,
>   	context->hi |= (value & ((1 << 16) - 1)) << 8;
>   }
>   
> +static inline void context_set_pasid(struct context_entry *context)
> +{
> +	context->lo |= CONTEXT_PASIDE;
> +}
> +
>   static inline int context_domain_id(struct context_entry *c)
>   {
>   	return((c->hi >> 8) & 0xffff);
> @@ -1341,20 +1346,17 @@ static void __iommu_flush_iotlb(struct intel_iommu *iommu, u16 did,
>   }
>   
>   static struct device_domain_info *
> -iommu_support_dev_iotlb(struct dmar_domain *domain, struct intel_iommu *iommu,
> -			u8 bus, u8 devfn)
> +domain_lookup_dev_info(struct dmar_domain *domain,
> +		       struct intel_iommu *iommu, u8 bus, u8 devfn)
>   {
>   	struct device_domain_info *info;
>   
> -	if (!iommu->qi)
> -		return NULL;
> -
>   	spin_lock(&domain->lock);
>   	list_for_each_entry(info, &domain->devices, link) {
>   		if (info->iommu == iommu && info->bus == bus &&
>   		    info->devfn == devfn) {
>   			spin_unlock(&domain->lock);
> -			return info->ats_supported ? info : NULL;
> +			return info;
>   		}
>   	}
>   	spin_unlock(&domain->lock);
> @@ -1401,7 +1403,6 @@ static void iommu_enable_dev_iotlb(struct device_domain_info *info)
>   		info->pfsid = pci_dev_id(pf_pdev);
>   	}
>   
> -#ifdef CONFIG_INTEL_IOMMU_SVM
>   	/* The PCIe spec, in its wisdom, declares that the behaviour of
>   	   the device if you enable PASID support after ATS support is
>   	   undefined. So always enable PASID support on devices which
> @@ -1414,7 +1415,7 @@ static void iommu_enable_dev_iotlb(struct device_domain_info *info)
>   	    (info->pasid_enabled ? pci_prg_resp_pasid_required(pdev) : 1)  &&
>   	    !pci_reset_pri(pdev) && !pci_enable_pri(pdev, PRQ_DEPTH))
>   		info->pri_enabled = 1;
> -#endif
> +
>   	if (info->ats_supported && pci_ats_page_aligned(pdev) &&
>   	    !pci_enable_ats(pdev, VTD_PAGE_SHIFT)) {
>   		info->ats_enabled = 1;
> @@ -1437,16 +1438,16 @@ static void iommu_disable_dev_iotlb(struct device_domain_info *info)
>   		info->ats_enabled = 0;
>   		domain_update_iotlb(info->domain);
>   	}
> -#ifdef CONFIG_INTEL_IOMMU_SVM
> +
>   	if (info->pri_enabled) {
>   		pci_disable_pri(pdev);
>   		info->pri_enabled = 0;
>   	}
> +
>   	if (info->pasid_enabled) {
>   		pci_disable_pasid(pdev);
>   		info->pasid_enabled = 0;
>   	}
> -#endif
>   }
>   
>   static void __iommu_flush_dev_iotlb(struct device_domain_info *info,
> @@ -1890,7 +1891,7 @@ static int domain_context_mapping_one(struct dmar_domain *domain,
>   				      u8 bus, u8 devfn)
>   {
>   	struct device_domain_info *info =
> -			iommu_support_dev_iotlb(domain, iommu, bus, devfn);
> +			domain_lookup_dev_info(domain, iommu, bus, devfn);
>   	u16 did = domain_id_iommu(domain, iommu);
>   	int translation = CONTEXT_TT_MULTI_LEVEL;
>   	struct context_entry *context;
> @@ -1961,6 +1962,8 @@ static int domain_context_mapping_one(struct dmar_domain *domain,
>   			context_set_sm_dte(context);
>   		if (info && info->pri_supported)
>   			context_set_sm_pre(context);
> +		if (info && info->pasid_supported)
> +			context_set_pasid(context);
>   	} else {
>   		struct dma_pte *pgd = domain->pgd;
>   		int agaw;
> @@ -4566,52 +4569,6 @@ static void intel_iommu_get_resv_regions(struct device *device,
>   	list_add_tail(&reg->list, head);
>   }
>   
> -int intel_iommu_enable_pasid(struct intel_iommu *iommu, struct device *dev)
> -{
> -	struct device_domain_info *info = dev_iommu_priv_get(dev);
> -	struct context_entry *context;
> -	struct dmar_domain *domain;
> -	u64 ctx_lo;
> -	int ret;
> -
> -	domain = info->domain;
> -	if (!domain)
> -		return -EINVAL;
> -
> -	spin_lock(&iommu->lock);
> -	ret = -EINVAL;
> -	if (!info->pasid_supported)
> -		goto out;
> -
> -	context = iommu_context_addr(iommu, info->bus, info->devfn, 0);
> -	if (WARN_ON(!context))
> -		goto out;
> -
> -	ctx_lo = context[0].lo;
> -
> -	if (!(ctx_lo & CONTEXT_PASIDE)) {
> -		ctx_lo |= CONTEXT_PASIDE;
> -		context[0].lo = ctx_lo;
> -		wmb();
> -		iommu->flush.flush_context(iommu,
> -					   domain_id_iommu(domain, iommu),
> -					   PCI_DEVID(info->bus, info->devfn),
> -					   DMA_CCMD_MASK_NOBIT,
> -					   DMA_CCMD_DEVICE_INVL);
> -	}
> -
> -	/* Enable PASID support in the device, if it wasn't already */
> -	if (!info->pasid_enabled)
> -		iommu_enable_dev_iotlb(info);
> -
> -	ret = 0;
> -
> - out:
> -	spin_unlock(&iommu->lock);
> -
> -	return ret;
> -}
> -
>   static struct iommu_group *intel_iommu_device_group(struct device *dev)
>   {
>   	if (dev_is_pci(dev))
> @@ -4635,9 +4592,6 @@ static int intel_iommu_enable_sva(struct device *dev)
>   	if (!(iommu->flags & VTD_FLAG_SVM_CAPABLE))
>   		return -ENODEV;
>   
> -	if (intel_iommu_enable_pasid(iommu, dev))
> -		return -ENODEV;
> -
>   	if (!info->pasid_enabled || !info->pri_enabled || !info->ats_enabled)
>   		return -EINVAL;
>   
> diff --git a/drivers/iommu/intel/Kconfig b/drivers/iommu/intel/Kconfig
> index 39a06d245f12..b3f40375f214 100644
> --- a/drivers/iommu/intel/Kconfig
> +++ b/drivers/iommu/intel/Kconfig
> @@ -21,6 +21,8 @@ config INTEL_IOMMU
>   	select IOASID
>   	select IOMMU_DMA
>   	select PCI_ATS
> +	select PCI_PRI
> +	select PCI_PASID
>   	help
>   	  DMA remapping (DMAR) devices support enables independent address
>   	  translations for Direct Memory Access (DMA) from devices.
> @@ -48,8 +50,6 @@ config INTEL_IOMMU_DEBUGFS
>   config INTEL_IOMMU_SVM
>   	bool "Support for Shared Virtual Memory with Intel IOMMU"
>   	depends on X86_64
> -	select PCI_PASID
> -	select PCI_PRI
>   	select MMU_NOTIFIER
>   	select IOASID
>   	select IOMMU_SVA

-- 
"firm, enduring, strong, and long-lived"

RE: [PATCH 1/1] iommu/vt-d: Enable PASID during iommu device probe

[Tian, Kevin]

> From: Lu Baolu <baolu.lu@linux.intel.com>
> Sent: Monday, September 12, 2022 10:48 AM
>
> @@ -1401,7 +1403,6 @@ static void iommu_enable_dev_iotlb(struct
> device_domain_info *info)

This is not the right name now as dev_iotlb is only related to ATS.

>  		info->pfsid = pci_dev_id(pf_pdev);
>  	}
> 
> -#ifdef CONFIG_INTEL_IOMMU_SVM
>  	/* The PCIe spec, in its wisdom, declares that the behaviour of
>  	   the device if you enable PASID support after ATS support is
>  	   undefined. So always enable PASID support on devices which
> @@ -1414,7 +1415,7 @@ static void iommu_enable_dev_iotlb(struct
> device_domain_info *info)
>  	    (info->pasid_enabled ? pci_prg_resp_pasid_required(pdev) : 1)
> &&
>  	    !pci_reset_pri(pdev) && !pci_enable_pri(pdev, PRQ_DEPTH))
>  		info->pri_enabled = 1;
> -#endif
> +
>  	if (info->ats_supported && pci_ats_page_aligned(pdev) &&
>  	    !pci_enable_ats(pdev, VTD_PAGE_SHIFT)) {
>  		info->ats_enabled = 1;

iommu_enable_dev_iotlb() is currently called both when the device is probed
and when sva is enabled (which is actually useless). From this angle the commit
msg is inaccurate. 

> diff --git a/drivers/iommu/intel/Kconfig b/drivers/iommu/intel/Kconfig
> index 39a06d245f12..b3f40375f214 100644
> --- a/drivers/iommu/intel/Kconfig
> +++ b/drivers/iommu/intel/Kconfig
> @@ -21,6 +21,8 @@ config INTEL_IOMMU
>  	select IOASID
>  	select IOMMU_DMA
>  	select PCI_ATS
> +	select PCI_PRI
> +	select PCI_PASID
>  	help
>  	  DMA remapping (DMAR) devices support enables independent
> address
>  	  translations for Direct Memory Access (DMA) from devices.
> @@ -48,8 +50,6 @@ config INTEL_IOMMU_DEBUGFS
>  config INTEL_IOMMU_SVM
>  	bool "Support for Shared Virtual Memory with Intel IOMMU"
>  	depends on X86_64
> -	select PCI_PASID
> -	select PCI_PRI
>  	select MMU_NOTIFIER
>  	select IOASID

this is already selected by CONFIG_INTEL_IOMMU

>  	select IOMMU_SVA
> --
> 2.25.1

Re: [PATCH 1/1] iommu/vt-d: Enable PASID during iommu device probe

[Baolu Lu]

On 2022/9/13 16:01, Tian, Kevin wrote:
>> From: Lu Baolu <baolu.lu@linux.intel.com>
>> Sent: Monday, September 12, 2022 10:48 AM
>>
>> @@ -1401,7 +1403,6 @@ static void iommu_enable_dev_iotlb(struct
>> device_domain_info *info)
> 
> This is not the right name now as dev_iotlb is only related to ATS.

Yes. This name is confusing. Perhaps we can split it into some specific
helpers,

- intel_iommu_enable_pci_ats()
- intel_iommu_enabel_pci_pri()
- intel_iommu_enable_pci_pasid()
?

> 
>>   		info->pfsid = pci_dev_id(pf_pdev);
>>   	}
>>
>> -#ifdef CONFIG_INTEL_IOMMU_SVM
>>   	/* The PCIe spec, in its wisdom, declares that the behaviour of
>>   	   the device if you enable PASID support after ATS support is
>>   	   undefined. So always enable PASID support on devices which
>> @@ -1414,7 +1415,7 @@ static void iommu_enable_dev_iotlb(struct
>> device_domain_info *info)
>>   	    (info->pasid_enabled ? pci_prg_resp_pasid_required(pdev) : 1)
>> &&
>>   	    !pci_reset_pri(pdev) && !pci_enable_pri(pdev, PRQ_DEPTH))
>>   		info->pri_enabled = 1;
>> -#endif
>> +
>>   	if (info->ats_supported && pci_ats_page_aligned(pdev) &&
>>   	    !pci_enable_ats(pdev, VTD_PAGE_SHIFT)) {
>>   		info->ats_enabled = 1;
> 
> iommu_enable_dev_iotlb() is currently called both when the device is probed
> and when sva is enabled (which is actually useless). From this angle the commit
> msg is inaccurate.

The logic is a bit tricky. iommu_support_dev_iotlb() only returns a
devinfo pointer when ATS is supported on the device. So, you are right
if device supports both ATS and PASID; otherwise PASID will not be
enabled.

> 
>> diff --git a/drivers/iommu/intel/Kconfig b/drivers/iommu/intel/Kconfig
>> index 39a06d245f12..b3f40375f214 100644
>> --- a/drivers/iommu/intel/Kconfig
>> +++ b/drivers/iommu/intel/Kconfig
>> @@ -21,6 +21,8 @@ config INTEL_IOMMU
>>   	select IOASID
>>   	select IOMMU_DMA
>>   	select PCI_ATS
>> +	select PCI_PRI
>> +	select PCI_PASID
>>   	help
>>   	  DMA remapping (DMAR) devices support enables independent
>> address
>>   	  translations for Direct Memory Access (DMA) from devices.
>> @@ -48,8 +50,6 @@ config INTEL_IOMMU_DEBUGFS
>>   config INTEL_IOMMU_SVM
>>   	bool "Support for Shared Virtual Memory with Intel IOMMU"
>>   	depends on X86_64
>> -	select PCI_PASID
>> -	select PCI_PRI
>>   	select MMU_NOTIFIER
>>   	select IOASID
> 
> this is already selected by CONFIG_INTEL_IOMMU

Yes. Should be removed.

> 
>>   	select IOMMU_SVA
>> --
>> 2.25.1
> 
> 

Best regards,
baolu

Re: [PATCH 1/1] iommu/vt-d: Enable PASID during iommu device probe

[Baolu Lu]

Hi Ethan,

On 2022/9/13 15:46, Ethan Zhao wrote:
> Baolu,
> 
> 在 2022/9/12 10:48, Lu Baolu 写道:
>> Previously PASID supports on both IOMMU and PCI devices are enabled in 
>> the
>> iommu_dev_enable_feature(dev, IOMMU_DEV_FEAT_SVA) path. It's functionally
>> correct as the SVA is the only feature that requires PASID setup. 
>> However,
>> looking ahead, we will add more features that need to enable pasid (for
>> example, kernel DMA with PASID, SIOV, VM guest SVA, etc.). It makes more
>> sense to enable PASID during iommu probing device.
>>
>> This enables PASID during iommu probing device and deprecates the
>> intel_iommu_enable_pasid() helper. This is safe because the IOMMU 
>> hardware
>> will block any PCI TLP with a PASID prefix if there is no IOMMU domain
>> attached to the PASID of the device.
> 
> What the error path would be if this code runs on some old platforms don't
> 
> support PASID, would you print out "this platform doesn't suppor PASID" and
> 
> give users an interface function to query if the PASID cap of iommu is 
> enabled
> 
> and if not why ?

It's not an error case if the IOMMU doesn't support PASID. But it's an
error case if any device drivers call PASID related IOMMU interfaces
(for example, iommu_domain_attach/detach_dev_pasid()). The corresponding
error code will be returned to the drivers.

Best regards,
baolu

Re: [PATCH 1/1] iommu/vt-d: Enable PASID during iommu device probe

[Baolu Lu]

On 9/14/22 4:59 PM, Ethan Zhao wrote:
>>> What the error path would be if this code runs on some old platforms 
>>> don't
>>>
>>> support PASID, would you print out "this platform doesn't suppor 
>>> PASID" and
>>>
>>> give users an interface function to query if the PASID cap of iommu 
>>> is enabled
>>>
>>> and if not why ?
>>
>> It's not an error case if the IOMMU doesn't support PASID. But it's an
>> error case if any device drivers call PASID related IOMMU interfaces
>> (for example, iommu_domain_attach/detach_dev_pasid()). The corresponding
>> error code will be returned to the drivers.
>>
> So iommu driver withdraws the flexibility/rights from device driver 
> about the
> 
> ability to enable PASID, looks much more *integrated* iommu works as 
> relation

No. This patch doesn't withdraw anything. It's just a code refactoring.

> 
> controller in device-iommu-domain by enabling PASID in iommu probe stage
> 
> by default -- iommu decides to enable PASID or not though device-iommu-
> 
> domain might not work ? or they should work because they are hard-wired
> 
> together (device - iommu) even device is hotpluged later.
> 

I may not get you exactly. :-) Some IOMMU features reply on PASID
capabilities on both IOMMU and device. The IOMMU drivers enumerate the
capabilities and enable them if they are supported.

Best regards,
baolu

RE: [PATCH 1/1] iommu/vt-d: Enable PASID during iommu device probe

[Tian, Kevin]

> From: Baolu Lu <baolu.lu@linux.intel.com>
> Sent: Tuesday, September 13, 2022 5:25 PM
> 
> On 2022/9/13 16:01, Tian, Kevin wrote:
> >> From: Lu Baolu <baolu.lu@linux.intel.com>
> >> Sent: Monday, September 12, 2022 10:48 AM
> >>
> >> @@ -1401,7 +1403,6 @@ static void iommu_enable_dev_iotlb(struct
> >> device_domain_info *info)
> >
> > This is not the right name now as dev_iotlb is only related to ATS.
> 
> Yes. This name is confusing. Perhaps we can split it into some specific
> helpers,
> 
> - intel_iommu_enable_pci_ats()
> - intel_iommu_enabel_pci_pri()
> - intel_iommu_enable_pci_pasid()
> ?

Probably intel_iommu_enable_pci_caps()

> 
> >
> >>   		info->pfsid = pci_dev_id(pf_pdev);
> >>   	}
> >>
> >> -#ifdef CONFIG_INTEL_IOMMU_SVM
> >>   	/* The PCIe spec, in its wisdom, declares that the behaviour of
> >>   	   the device if you enable PASID support after ATS support is
> >>   	   undefined. So always enable PASID support on devices which
> >> @@ -1414,7 +1415,7 @@ static void iommu_enable_dev_iotlb(struct
> >> device_domain_info *info)
> >>   	    (info->pasid_enabled ? pci_prg_resp_pasid_required(pdev) : 1)
> >> &&
> >>   	    !pci_reset_pri(pdev) && !pci_enable_pri(pdev, PRQ_DEPTH))
> >>   		info->pri_enabled = 1;
> >> -#endif
> >> +
> >>   	if (info->ats_supported && pci_ats_page_aligned(pdev) &&
> >>   	    !pci_enable_ats(pdev, VTD_PAGE_SHIFT)) {
> >>   		info->ats_enabled = 1;
> >
> > iommu_enable_dev_iotlb() is currently called both when the device is
> probed
> > and when sva is enabled (which is actually useless). From this angle the
> commit
> > msg is inaccurate.
> 
> The logic is a bit tricky. iommu_support_dev_iotlb() only returns a
> devinfo pointer when ATS is supported on the device. So, you are right
> if device supports both ATS and PASID; otherwise PASID will not be
> enabled.

Yes, that is what the first part of this patch fixes.

But my point is about the message that previously PASID was enabled
only when FEAT_SVA is enabled and now the patch moves it to the
probe time. 

My point is that even in old way iommu_enable_dev_iotlb() was called
twice: one at probe time and the other at FEAT_SVA. If ATS exists
then PASID is enabled at probe time already. If ATS doesn't exist then
PASID is always disabled.

So this patch is really to decouple PASID enabling from ATS and remove
the unnecessary/duplicated call of iommu_enable_dev_iotlb() in
intel_iommu_enable_sva().

Thanks
Kevin

Re: [PATCH 1/1] iommu/vt-d: Enable PASID during iommu device probe

[Baolu Lu]

On 2022/9/15 11:22, Tian, Kevin wrote:
>> From: Baolu Lu <baolu.lu@linux.intel.com>
>> Sent: Tuesday, September 13, 2022 5:25 PM
>>
>> On 2022/9/13 16:01, Tian, Kevin wrote:
>>>> From: Lu Baolu <baolu.lu@linux.intel.com>
>>>> Sent: Monday, September 12, 2022 10:48 AM
>>>>
>>>> @@ -1401,7 +1403,6 @@ static void iommu_enable_dev_iotlb(struct
>>>> device_domain_info *info)
>>>
>>> This is not the right name now as dev_iotlb is only related to ATS.
>>
>> Yes. This name is confusing. Perhaps we can split it into some specific
>> helpers,
>>
>> - intel_iommu_enable_pci_ats()
>> - intel_iommu_enabel_pci_pri()
>> - intel_iommu_enable_pci_pasid()
>> ?
> 
> Probably intel_iommu_enable_pci_caps()

It's better.

> 
>>
>>>
>>>>    		info->pfsid = pci_dev_id(pf_pdev);
>>>>    	}
>>>>
>>>> -#ifdef CONFIG_INTEL_IOMMU_SVM
>>>>    	/* The PCIe spec, in its wisdom, declares that the behaviour of
>>>>    	   the device if you enable PASID support after ATS support is
>>>>    	   undefined. So always enable PASID support on devices which
>>>> @@ -1414,7 +1415,7 @@ static void iommu_enable_dev_iotlb(struct
>>>> device_domain_info *info)
>>>>    	    (info->pasid_enabled ? pci_prg_resp_pasid_required(pdev) : 1)
>>>> &&
>>>>    	    !pci_reset_pri(pdev) && !pci_enable_pri(pdev, PRQ_DEPTH))
>>>>    		info->pri_enabled = 1;
>>>> -#endif
>>>> +
>>>>    	if (info->ats_supported && pci_ats_page_aligned(pdev) &&
>>>>    	    !pci_enable_ats(pdev, VTD_PAGE_SHIFT)) {
>>>>    		info->ats_enabled = 1;
>>>
>>> iommu_enable_dev_iotlb() is currently called both when the device is
>> probed
>>> and when sva is enabled (which is actually useless). From this angle the
>> commit
>>> msg is inaccurate.
>>
>> The logic is a bit tricky. iommu_support_dev_iotlb() only returns a
>> devinfo pointer when ATS is supported on the device. So, you are right
>> if device supports both ATS and PASID; otherwise PASID will not be
>> enabled.
> 
> Yes, that is what the first part of this patch fixes.
> 
> But my point is about the message that previously PASID was enabled
> only when FEAT_SVA is enabled and now the patch moves it to the
> probe time.
> 
> My point is that even in old way iommu_enable_dev_iotlb() was called
> twice: one at probe time and the other at FEAT_SVA. If ATS exists
> then PASID is enabled at probe time already. If ATS doesn't exist then
> PASID is always disabled.
> 
> So this patch is really to decouple PASID enabling from ATS and remove
> the unnecessary/duplicated call of iommu_enable_dev_iotlb() in
> intel_iommu_enable_sva().

Yes. Exactly. I will rephrase the commit message and send a v2.

Best regards,
baolu

Re: [PATCH 1/1] iommu/vt-d: Enable PASID during iommu device probe

[Ethan Zhao]

在 2022/9/15 11:00, Baolu Lu 写道:
>
>
> On 9/14/22 4:59 PM, Ethan Zhao wrote:
>>>> What the error path would be if this code runs on some old 
>>>> platforms don't
>>>>
>>>> support PASID, would you print out "this platform doesn't suppor 
>>>> PASID" and
>>>>
>>>> give users an interface function to query if the PASID cap of iommu 
>>>> is enabled
>>>>
>>>> and if not why ?
>>>
>>> It's not an error case if the IOMMU doesn't support PASID. But it's an
>>> error case if any device drivers call PASID related IOMMU interfaces
>>> (for example, iommu_domain_attach/detach_dev_pasid()). The 
>>> corresponding
>>> error code will be returned to the drivers.
>>>
>> So iommu driver withdraws the flexibility/rights from device driver 
>> about the
>>
>> ability to enable PASID, looks much more *integrated* iommu works as 
>> relation
>
> No. This patch doesn't withdraw anything. It's just a code refactoring.
>
>>
>> controller in device-iommu-domain by enabling PASID in iommu probe stage
>>
>> by default -- iommu decides to enable PASID or not though device-iommu-
>>
>> domain might not work ? or they should work because they are hard-wired
>>
>> together (device - iommu) even device is hotpluged later.
>>
>
> I may not get you exactly. :-) Some IOMMU features reply on PASID
> capabilities on both IOMMU and device. The IOMMU drivers enumerate the
> capabilities and enable them if they are supported.
I might not express it straightforward,  I mean with this patch iommu 
deals with

the complexity of enabling PASID (globally?)  or not at probing stage , 
instead

of other device driver side decision to request IOMMU PASID enabling during

their setup state.  if so you move the decision to iommu probe stage. 
hmmm...

Pros,  iommu driver controls everything about PASID enabling.

Cons,  iommu driver handles all possible complexity about capability 
matching

etc.


Thanks,

Ethan

>
> Best regards,
> baolu

-- 
"firm, enduring, strong, and long-lived"

Re: [PATCH 1/1] iommu/vt-d: Enable PASID during iommu device probe

[Baolu Lu]

On 2022/9/16 10:40, Ethan Zhao wrote:
>>
>> I may not get you exactly. 😄 Some IOMMU features reply on PASID
>> capabilities on both IOMMU and device. The IOMMU drivers enumerate the
>> capabilities and enable them if they are supported.
> I might not express it straightforward,  I mean with this patch iommu 
> deals with
> 
> the complexity of enabling PASID (globally?)  or not at probing stage , 
> instead
> 
> of other device driver side decision to request IOMMU PASID enabling during
> 
> their setup state.  if so you move the decision to iommu probe stage. 
> hmmm...

I am sorry that the commit message was a bit confusing. Actually we
always enable PASID at iommu probe path w/ or w/o this patch.

> 
> Pros,  iommu driver controls everything about PASID enabling.
> 
> Cons,  iommu driver handles all possible complexity about capability 
> matching

Do device drivers need to configure PCI PASID without IOMMU? I searched
the tree and found nothing.

Best regards,
baolu

Re: [PATCH 1/1] iommu/vt-d: Enable PASID during iommu device probe

[Ethan Zhao]

在 2022/9/16 11:05, Baolu Lu 写道:
> On 2022/9/16 10:40, Ethan Zhao wrote:
>>>
>>> I may not get you exactly. 😄 Some IOMMU features reply on PASID
>>> capabilities on both IOMMU and device. The IOMMU drivers enumerate the
>>> capabilities and enable them if they are supported.
>> I might not express it straightforward,  I mean with this patch iommu 
>> deals with
>>
>> the complexity of enabling PASID (globally?)  or not at probing stage 
>> , instead
>>
>> of other device driver side decision to request IOMMU PASID enabling 
>> during
>>
>> their setup state.  if so you move the decision to iommu probe stage. 
>> hmmm...
>
> I am sorry that the commit message was a bit confusing. Actually we
> always enable PASID at iommu probe path w/ or w/o this patch.
>
Really ?  the commit message is quit clear to me ~~@
>>
>> Pros,  iommu driver controls everything about PASID enabling.
>>
>> Cons,  iommu driver handles all possible complexity about capability 
>> matching
>
> Do device drivers need to configure PCI PASID without IOMMU? I searched
> the tree and found nothing.

Device knows if it has PCI PASID cap and its driver also could determine 
to request

iommu to enable PASID or not by invoking

intel_iommu_enable_sva()->*intel_iommu_enable_pasid()*

*that is the old style without this patch.
*

*Iommu driver of course  also knows if devices in its group related the 
iommu
*

*have PASID cap support or not by enumerating them from the ACPI DMAR.
*

This is my understanding, correct me if wrong.

While configuring device PCI PASID cap is another thing, from kernel or 
userland,

compatible with the iommu cap, works, or not work.  Could you prevent anyone

from doing that ?


Thanks,

Ethan
>
> Best regards,
> baolu

-- 
"firm, enduring, strong, and long-lived"

Re: [PATCH 1/1] iommu/vt-d: Enable PASID during iommu device probe

[Baolu Lu]

On 2022/9/16 11:35, Ethan Zhao wrote:
> Baolu,
> 
> 在 2022/9/16 11:05, Baolu Lu 写道:
>> On 2022/9/16 10:40, Ethan Zhao wrote:
>>>>
>>>> I may not get you exactly. 😄 Some IOMMU features reply on PASID
>>>> capabilities on both IOMMU and device. The IOMMU drivers enumerate the
>>>> capabilities and enable them if they are supported.
>>> I might not express it straightforward,  I mean with this patch iommu 
>>> deals with
>>>
>>> the complexity of enabling PASID (globally?)  or not at probing stage 
>>> , instead
>>>
>>> of other device driver side decision to request IOMMU PASID enabling 
>>> during
>>>
>>> their setup state.  if so you move the decision to iommu probe stage. 
>>> hmmm...
>>
>> I am sorry that the commit message was a bit confusing. Actually we
>> always enable PASID at iommu probe path w/ or w/o this patch.
> Really ?  the commit message is quit clear to me ~~@
>>
>>>
>>> Pros,  iommu driver controls everything about PASID enabling.
>>>
>>> Cons,  iommu driver handles all possible complexity about capability 
>>> matching
>>
>> Do device drivers need to configure PCI PASID without IOMMU? I searched
>> the tree and found nothing.
> 
> Device knows if it has PCI PASID cap and its driver also could determine 
> to request
> 
> iommu to enable PASID or not by invoking
> 
> intel_iommu_enable_sva()->*intel_iommu_enable_pasid()*

PASID is a PCIe capability. Though SVA is built on it,it's not only
for SVA. Thus, the purpose of intel_iommu_enable_sva() is not for
enabling PASID.

> 
> *that is the old style without this patch.

No. Without this patch, PASID is also enabled in probe path. Calling
intel_iommu_enable_pasid() in enabling SVA path is actually duplicate.

The commit message for this patch is not correct. It's my bad. :-)

> *
> 
> *Iommu driver of course  also knows if devices in its group related the 
> iommu
> *
> 
> *have PASID cap support or not by enumerating them from the ACPI DMAR.
> *
> 
> This is my understanding, correct me if wrong.
> 
> While configuring device PCI PASID cap is another thing, from kernel or 
> userland,
> 
> compatible with the iommu cap, works, or not work.  Could you prevent anyone
> 
> from doing that ?

I do not object to adding a common interface to enable/disable PASID if
any device driver needs to manage PASID by itself. Before that, there
is no need to add complexity in IOMMU subsystem for a non-existent
requirement.

Best regards,
baolu