Re: [PATCH 04/12] iommu/arm-smmu: Convert to iommu_capable() API function

Re: [PATCH 04/12] iommu/arm-smmu: Convert to iommu_capable() API function

[Will Deacon]

Hi Joerg,

[adding devicetree for the last paragraph]

On Fri, Sep 05, 2014 at 11:52:56AM +0100, Joerg Roedel wrote:
> From: Joerg Roedel <jroedel-l3A5Bk7waGM@public.gmane.org>
> 
> Cc: Will Deacon <will.deacon-5wv7dgnIgG8@public.gmane.org>
> Signed-off-by: Joerg Roedel <jroedel-l3A5Bk7waGM@public.gmane.org>
> ---
>  drivers/iommu/arm-smmu.c | 35 ++++++++++++++++++++++++++---------
>  1 file changed, 26 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
> index ca18d6d..47c2cb6 100644
> --- a/drivers/iommu/arm-smmu.c
> +++ b/drivers/iommu/arm-smmu.c
> @@ -1515,20 +1515,37 @@ static phys_addr_t arm_smmu_iova_to_phys(struct iommu_domain *domain,
>  	return __pfn_to_phys(pte_pfn(pte)) | (iova & ~PAGE_MASK);
>  }
>  
> -static int arm_smmu_domain_has_cap(struct iommu_domain *domain,
> -				   unsigned long cap)
> +static bool arm_smmu_capable(enum iommu_cap cap)
>  {
> -	struct arm_smmu_domain *smmu_domain = domain->priv;
> -	struct arm_smmu_device *smmu = smmu_domain->smmu;
> -	u32 features = smmu ? smmu->features : 0;
> +	struct arm_smmu_device *smmu;
> +	bool ret = false;
>  
>  	switch (cap) {
>  	case IOMMU_CAP_CACHE_COHERENCY:
> -		return features & ARM_SMMU_FEAT_COHERENT_WALK;
> +		/*
> +		 * Use ARM_SMMU_FEAT_COHERENT_WALK as an indicator on whether
> +		 * the SMMU can force coherency on the DMA transaction. If it
> +		 * supports COHERENT_WALK it must be behind a coherent
> +		 * interconnect.
> +		 * A domain can be attached to any SMMU, so to reliably support
> +		 * IOMMU_CAP_CACHE_COHERENCY all SMMUs in the system need to be
> +		 * behind a coherent interconnect.
> +		 */

I don't think we should rely on the SMMU's advertisement of the coherent
table walk to mean anything other than `the SMMU can emit cacheable page
table walks'. The actual walker is a separate observer, and could have a
different route to memory than transactions flowing through the SMMU.

In reality, all we can report here is what the SMMU (as opposed to the rest
of the system) is capable of. The SMMU can always emit cacheable
transactions for a master if a stage-1 translation is in use so, without
extending the device-tree binding, we should report true here
unconditionally.

An alternative is to extend the device-tree bindings to have something like
"dma-coherent" for the SMMU node, which would imply that the interconnect is
configured to honour snoops from the SMMU into the CPU caches. We might want
an additional property on top of that to indicate that the table walker is
also coherent (and we could check this against our ARM_SMMU_FEAT_COHERENT_WALK
flag)

Will

Re: [PATCH 04/12] iommu/arm-smmu: Convert to iommu_capable() API function

[Joerg Roedel]

Hi Will,

On Mon, Sep 08, 2014 at 05:51:36PM +0100, Will Deacon wrote:
> On Fri, Sep 05, 2014 at 11:52:56AM +0100, Joerg Roedel wrote:
> >  	switch (cap) {
> >  	case IOMMU_CAP_CACHE_COHERENCY:
> > -		return features & ARM_SMMU_FEAT_COHERENT_WALK;
> > +		/*
> > +		 * Use ARM_SMMU_FEAT_COHERENT_WALK as an indicator on whether
> > +		 * the SMMU can force coherency on the DMA transaction. If it
> > +		 * supports COHERENT_WALK it must be behind a coherent
> > +		 * interconnect.
> > +		 * A domain can be attached to any SMMU, so to reliably support
> > +		 * IOMMU_CAP_CACHE_COHERENCY all SMMUs in the system need to be
> > +		 * behind a coherent interconnect.
> > +		 */
> 
> I don't think we should rely on the SMMU's advertisement of the coherent
> table walk to mean anything other than `the SMMU can emit cacheable page
> table walks'. The actual walker is a separate observer, and could have a
> different route to memory than transactions flowing through the SMMU.

Okay, so this should be advertised via DT then.

> In reality, all we can report here is what the SMMU (as opposed to the rest
> of the system) is capable of. The SMMU can always emit cacheable
> transactions for a master if a stage-1 translation is in use so, without
> extending the device-tree binding, we should report true here
> unconditionally.

Sounds more like we should return false here unconditionally until we
have a reliable way to tell whether this feature is available, no? When
we return true the user of the IOMMU-API might rely on coherency that is
not available.

> An alternative is to extend the device-tree bindings to have something like
> "dma-coherent" for the SMMU node, which would imply that the interconnect is
> configured to honour snoops from the SMMU into the CPU caches. We might want
> an additional property on top of that to indicate that the table walker is
> also coherent (and we could check this against our ARM_SMMU_FEAT_COHERENT_WALK
> flag)

Sounds like a good idea, as long as there is no other way to detect
this.


	Joerg

[PATCH 04/13 v2] iommu/arm-smmu: Convert to iommu_capable() API function function

[Joerg Roedel]

Hi Will,

On Mon, Sep 08, 2014 at 05:51:36PM +0100, Will Deacon wrote:
> Hi Joerg,
> 
> [adding devicetree for the last paragraph]
> 
> On Fri, Sep 05, 2014 at 11:52:56AM +0100, Joerg Roedel wrote:
> > From: Joerg Roedel <jroedel-l3A5Bk7waGM@public.gmane.org>
> > 
> > Cc: Will Deacon <will.deacon-5wv7dgnIgG8@public.gmane.org>
> > Signed-off-by: Joerg Roedel <jroedel-l3A5Bk7waGM@public.gmane.org>
> > ---
> >  drivers/iommu/arm-smmu.c | 35 ++++++++++++++++++++++++++---------
> >  1 file changed, 26 insertions(+), 9 deletions(-)

Okay, so here is the updated patch:

>From b5d895980849ba1a46a5250cd4cc5f3f9f28235d Mon Sep 17 00:00:00 2001
From: Joerg Roedel <jroedel-l3A5Bk7waGM@public.gmane.org>
Date: Fri, 5 Sep 2014 10:49:34 +0200
Subject: [PATCH 04/13] iommu/arm-smmu: Convert to iommu_capable() API function

Cc: Will Deacon <will.deacon-5wv7dgnIgG8@public.gmane.org>
Signed-off-by: Joerg Roedel <jroedel-l3A5Bk7waGM@public.gmane.org>
---
 drivers/iommu/arm-smmu.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index a83cc2a..f5cacf4 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -1526,20 +1526,20 @@ static phys_addr_t arm_smmu_iova_to_phys(struct iommu_domain *domain,
 	return __pfn_to_phys(pte_pfn(pte)) | (iova & ~PAGE_MASK);
 }
 
-static int arm_smmu_domain_has_cap(struct iommu_domain *domain,
-				   unsigned long cap)
+static bool arm_smmu_capable(enum iommu_cap cap)
 {
-	struct arm_smmu_domain *smmu_domain = domain->priv;
-	struct arm_smmu_device *smmu = smmu_domain->smmu;
-	u32 features = smmu ? smmu->features : 0;
-
 	switch (cap) {
 	case IOMMU_CAP_CACHE_COHERENCY:
-		return features & ARM_SMMU_FEAT_COHERENT_WALK;
+		/*
+		 * Return false here until we have a way to find out whether the
+		 * SMMUs in the system a coherently connected and able to force
+		 * DMA coherency.
+		 */
+		return false;
 	case IOMMU_CAP_INTR_REMAP:
-		return 1; /* MSIs are just memory writes */
+		return true; /* MSIs are just memory writes */
 	default:
-		return 0;
+		return false;
 	}
 }
 
@@ -1609,6 +1609,7 @@ static void arm_smmu_remove_device(struct device *dev)
 }
 
 static const struct iommu_ops arm_smmu_ops = {
+	.capable	= arm_smmu_capable,
 	.domain_init	= arm_smmu_domain_init,
 	.domain_destroy	= arm_smmu_domain_destroy,
 	.attach_dev	= arm_smmu_attach_dev,
@@ -1616,7 +1617,6 @@ static const struct iommu_ops arm_smmu_ops = {
 	.map		= arm_smmu_map,
 	.unmap		= arm_smmu_unmap,
 	.iova_to_phys	= arm_smmu_iova_to_phys,
-	.domain_has_cap	= arm_smmu_domain_has_cap,
 	.add_device	= arm_smmu_add_device,
 	.remove_device	= arm_smmu_remove_device,
 	.pgsize_bitmap	= (SECTION_SIZE |
-- 
1.8.4.5

Re: [PATCH 04/13 v2] iommu/arm-smmu: Convert to iommu_capable() API function function

[Will Deacon]

On Wed, Sep 17, 2014 at 09:53:12AM +0100, Joerg Roedel wrote:
> Hi Will,

Hello Joerg,

> On Mon, Sep 08, 2014 at 05:51:36PM +0100, Will Deacon wrote:
> > On Fri, Sep 05, 2014 at 11:52:56AM +0100, Joerg Roedel wrote:
> > > From: Joerg Roedel <jroedel-l3A5Bk7waGM@public.gmane.org>
> > > 
> > > Cc: Will Deacon <will.deacon-5wv7dgnIgG8@public.gmane.org>
> > > Signed-off-by: Joerg Roedel <jroedel-l3A5Bk7waGM@public.gmane.org>
> > > ---
> > >  drivers/iommu/arm-smmu.c | 35 ++++++++++++++++++++++++++---------
> > >  1 file changed, 26 insertions(+), 9 deletions(-)
> 
> Okay, so here is the updated patch:

Thanks, comments inline.

> From b5d895980849ba1a46a5250cd4cc5f3f9f28235d Mon Sep 17 00:00:00 2001
> From: Joerg Roedel <jroedel-l3A5Bk7waGM@public.gmane.org>
> Date: Fri, 5 Sep 2014 10:49:34 +0200
> Subject: [PATCH 04/13] iommu/arm-smmu: Convert to iommu_capable() API function
> 
> Cc: Will Deacon <will.deacon-5wv7dgnIgG8@public.gmane.org>
> Signed-off-by: Joerg Roedel <jroedel-l3A5Bk7waGM@public.gmane.org>
> ---
>  drivers/iommu/arm-smmu.c | 20 ++++++++++----------
>  1 file changed, 10 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
> index a83cc2a..f5cacf4 100644
> --- a/drivers/iommu/arm-smmu.c
> +++ b/drivers/iommu/arm-smmu.c
> @@ -1526,20 +1526,20 @@ static phys_addr_t arm_smmu_iova_to_phys(struct iommu_domain *domain,
>  	return __pfn_to_phys(pte_pfn(pte)) | (iova & ~PAGE_MASK);
>  }
>  
> -static int arm_smmu_domain_has_cap(struct iommu_domain *domain,
> -				   unsigned long cap)
> +static bool arm_smmu_capable(enum iommu_cap cap)
>  {
> -	struct arm_smmu_domain *smmu_domain = domain->priv;
> -	struct arm_smmu_device *smmu = smmu_domain->smmu;
> -	u32 features = smmu ? smmu->features : 0;
> -
>  	switch (cap) {
>  	case IOMMU_CAP_CACHE_COHERENCY:
> -		return features & ARM_SMMU_FEAT_COHERENT_WALK;
> +		/*
> +		 * Return false here until we have a way to find out whether the
> +		 * SMMUs in the system a coherently connected and able to force
> +		 * DMA coherency.
> +		 */

s/a/are/

However, I thought about this a bit more and the coherency isn't necessarily
a global property of the SMMU. In reality, it is dependent on the IOTLBs in
use by the domain, so it's not going to be possible to report true here in
many cases.

That means we'd need a way to say `this device is dma coherent when its
downstream IOMMU is enabled with IOMMU_CACHE mappings'. For the moment,
people will probably just add `dma-coherent' to the endpoint and dma-mapping
will request IOMMU_CACHE mappings regardless of the features advertised by
the IOMMU. In that case, it might make more sense to return `true' here as
we can always generated cacheable transactions from the SMMU. The
dma-coherent property on the device would then indicate whether those
transactions will snoop the CPU caches.

Will

Re: [PATCH 04/13 v2] iommu/arm-smmu: Convert to iommu_capable() API function function

[Joerg Roedel]

Hi Will,

On Fri, Sep 19, 2014 at 05:42:20PM +0100, Will Deacon wrote:
> However, I thought about this a bit more and the coherency isn't necessarily
> a global property of the SMMU. In reality, it is dependent on the IOTLBs in
> use by the domain, so it's not going to be possible to report true here in
> many cases.
> 
> That means we'd need a way to say `this device is dma coherent when its
> downstream IOMMU is enabled with IOMMU_CACHE mappings'. For the moment,
> people will probably just add `dma-coherent' to the endpoint and dma-mapping
> will request IOMMU_CACHE mappings regardless of the features advertised by
> the IOMMU. In that case, it might make more sense to return `true' here as
> we can always generated cacheable transactions from the SMMU. The
> dma-coherent property on the device would then indicate whether those
> transactions will snoop the CPU caches.

Okay, when the SMMU can always generate cachable transactions it is
surely fine to return true here. I'll change that before I add the
patches to my tree.


	Joerg