diff mbox series

[v9,5/5] iommu/tegra-smmu: Support managed domains

Message ID 20220923123557.866972-6-thierry.reding@gmail.com
State Changes Requested
Headers show
Series iommu: Support mappings/reservations in reserved-memory regions | expand

Commit Message

Thierry Reding Sept. 23, 2022, 12:35 p.m. UTC 
From: Navneet Kumar <navneetk@nvidia.com>

Allow creating identity and DMA API compatible IOMMU domains. When
creating a DMA API compatible domain, make sure to also create the
required cookie.

Signed-off-by: Navneet Kumar <navneetk@nvidia.com>
Signed-off-by: Thierry Reding <treding@nvidia.com>
---
Changes in v5:
- remove DMA cookie initialization that's now no longer needed

 drivers/iommu/tegra-smmu.c | 38 +++++++++++++++++++++-----------------
 1 file changed, 21 insertions(+), 17 deletions(-)

Comments

Robin Murphy Oct. 7, 2022, 1:48 p.m. UTC | #1 
On 2022-09-23 13:35, Thierry Reding wrote:
> From: Navneet Kumar <navneetk@nvidia.com>
> 
> Allow creating identity and DMA API compatible IOMMU domains. When
> creating a DMA API compatible domain, make sure to also create the
> required cookie.

Nit: this description is now confusingly outdated.

> Signed-off-by: Navneet Kumar <navneetk@nvidia.com>
> Signed-off-by: Thierry Reding <treding@nvidia.com>
> ---
> Changes in v5:
> - remove DMA cookie initialization that's now no longer needed
> 
>   drivers/iommu/tegra-smmu.c | 38 +++++++++++++++++++++-----------------
>   1 file changed, 21 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
> index 57b4f2b37447..7ad993330634 100644
> --- a/drivers/iommu/tegra-smmu.c
> +++ b/drivers/iommu/tegra-smmu.c
> @@ -20,6 +20,8 @@
>   #include <soc/tegra/ahb.h>
>   #include <soc/tegra/mc.h>
>   
> +#include "dma-iommu.h"
> +
>   struct tegra_smmu_group {
>   	struct list_head list;
>   	struct tegra_smmu *smmu;
> @@ -277,7 +279,9 @@ static struct iommu_domain *tegra_smmu_domain_alloc(unsigned type)
>   {
>   	struct tegra_smmu_as *as;
>   
> -	if (type != IOMMU_DOMAIN_UNMANAGED)
> +	if (type != IOMMU_DOMAIN_UNMANAGED &&
> +	    type != IOMMU_DOMAIN_DMA &&
> +	    type != IOMMU_DOMAIN_IDENTITY)

Since there's apparently no actual handling of IOMMU_DOMAIN_IDENTITY 
being added anywhere, AFAICS it's still going to set up an address space 
for translation with nothing mapped in its pagetable, which is pretty 
much the opposite of what's required :/

>   		return NULL;
>   
>   	as = kzalloc(sizeof(*as), GFP_KERNEL);
> @@ -287,25 +291,16 @@ static struct iommu_domain *tegra_smmu_domain_alloc(unsigned type)
>   	as->attr = SMMU_PD_READABLE | SMMU_PD_WRITABLE | SMMU_PD_NONSECURE;
>   
>   	as->pd = alloc_page(GFP_KERNEL | __GFP_DMA | __GFP_ZERO);
> -	if (!as->pd) {
> -		kfree(as);
> -		return NULL;
> -	}
> +	if (!as->pd)
> +		goto free_as;
>   
>   	as->count = kcalloc(SMMU_NUM_PDE, sizeof(u32), GFP_KERNEL);
> -	if (!as->count) {
> -		__free_page(as->pd);
> -		kfree(as);
> -		return NULL;
> -	}
> +	if (!as->count)
> +		goto free_pd_range;
>   
>   	as->pts = kcalloc(SMMU_NUM_PDE, sizeof(*as->pts), GFP_KERNEL);
> -	if (!as->pts) {
> -		kfree(as->count);
> -		__free_page(as->pd);
> -		kfree(as);
> -		return NULL;
> -	}
> +	if (!as->pts)
> +		goto free_pts;

Nit: all this part is now just unrelated refactoring.

Thanks,
Robin.

>   
>   	spin_lock_init(&as->lock);
>   
> @@ -315,6 +310,15 @@ static struct iommu_domain *tegra_smmu_domain_alloc(unsigned type)
>   	as->domain.geometry.force_aperture = true;
>   
>   	return &as->domain;
> +
> +free_pts:
> +	kfree(as->pts);
> +free_pd_range:
> +	__free_page(as->pd);
> +free_as:
> +	kfree(as);
> +
> +	return NULL;
>   }
>   
>   static void tegra_smmu_domain_free(struct iommu_domain *domain)
> @@ -1012,7 +1016,7 @@ static const struct iommu_ops tegra_smmu_ops = {
>   	.probe_device = tegra_smmu_probe_device,
>   	.release_device = tegra_smmu_release_device,
>   	.device_group = tegra_smmu_device_group,
> -	.get_resv_regions = of_iommu_get_resv_regions,
> +	.get_resv_regions = iommu_dma_get_resv_regions,
>   	.of_xlate = tegra_smmu_of_xlate,
>   	.pgsize_bitmap = SZ_4K,
>   	.default_domain_ops = &(const struct iommu_domain_ops) {
Thierry Reding Oct. 7, 2022, 3:40 p.m. UTC | #2 
On Fri, Oct 07, 2022 at 02:48:19PM +0100, Robin Murphy wrote:
> On 2022-09-23 13:35, Thierry Reding wrote:
> > From: Navneet Kumar <navneetk@nvidia.com>
> > 
> > Allow creating identity and DMA API compatible IOMMU domains. When
> > creating a DMA API compatible domain, make sure to also create the
> > required cookie.
> 
> Nit: this description is now confusingly outdated.
> 
> > Signed-off-by: Navneet Kumar <navneetk@nvidia.com>
> > Signed-off-by: Thierry Reding <treding@nvidia.com>
> > ---
> > Changes in v5:
> > - remove DMA cookie initialization that's now no longer needed
> > 
> >   drivers/iommu/tegra-smmu.c | 38 +++++++++++++++++++++-----------------
> >   1 file changed, 21 insertions(+), 17 deletions(-)
> > 
> > diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
> > index 57b4f2b37447..7ad993330634 100644
> > --- a/drivers/iommu/tegra-smmu.c
> > +++ b/drivers/iommu/tegra-smmu.c
> > @@ -20,6 +20,8 @@
> >   #include <soc/tegra/ahb.h>
> >   #include <soc/tegra/mc.h>
> > +#include "dma-iommu.h"
> > +
> >   struct tegra_smmu_group {
> >   	struct list_head list;
> >   	struct tegra_smmu *smmu;
> > @@ -277,7 +279,9 @@ static struct iommu_domain *tegra_smmu_domain_alloc(unsigned type)
> >   {
> >   	struct tegra_smmu_as *as;
> > -	if (type != IOMMU_DOMAIN_UNMANAGED)
> > +	if (type != IOMMU_DOMAIN_UNMANAGED &&
> > +	    type != IOMMU_DOMAIN_DMA &&
> > +	    type != IOMMU_DOMAIN_IDENTITY)
> 
> Since there's apparently no actual handling of IOMMU_DOMAIN_IDENTITY being
> added anywhere, AFAICS it's still going to set up an address space for
> translation with nothing mapped in its pagetable, which is pretty much the
> opposite of what's required :/

Yeah, I think we can safely skip identity domains. I don't think I've
ever seen them get used on Tegra.

> 
> >   		return NULL;
> >   	as = kzalloc(sizeof(*as), GFP_KERNEL);
> > @@ -287,25 +291,16 @@ static struct iommu_domain *tegra_smmu_domain_alloc(unsigned type)
> >   	as->attr = SMMU_PD_READABLE | SMMU_PD_WRITABLE | SMMU_PD_NONSECURE;
> >   	as->pd = alloc_page(GFP_KERNEL | __GFP_DMA | __GFP_ZERO);
> > -	if (!as->pd) {
> > -		kfree(as);
> > -		return NULL;
> > -	}
> > +	if (!as->pd)
> > +		goto free_as;
> >   	as->count = kcalloc(SMMU_NUM_PDE, sizeof(u32), GFP_KERNEL);
> > -	if (!as->count) {
> > -		__free_page(as->pd);
> > -		kfree(as);
> > -		return NULL;
> > -	}
> > +	if (!as->count)
> > +		goto free_pd_range;
> >   	as->pts = kcalloc(SMMU_NUM_PDE, sizeof(*as->pts), GFP_KERNEL);
> > -	if (!as->pts) {
> > -		kfree(as->count);
> > -		__free_page(as->pd);
> > -		kfree(as);
> > -		return NULL;
> > -	}
> > +	if (!as->pts)
> > +		goto free_pts;
> 
> Nit: all this part is now just unrelated refactoring.

Okay, I'll see if I can pull this into a separate patch. Or perhaps just
drop it.

Frankly, at this point it's unlikely that all of this will ever be
deployed on Tegra210 (and earlier) that used this SMMU, so I've included
this particular patch here primarily because Tegra210 was originally
meant to be the primary target. This patch is now close to 4 years
old...

Thierry

> 
> Thanks,
> Robin.
> 
> >   	spin_lock_init(&as->lock);
> > @@ -315,6 +310,15 @@ static struct iommu_domain *tegra_smmu_domain_alloc(unsigned type)
> >   	as->domain.geometry.force_aperture = true;
> >   	return &as->domain;
> > +
> > +free_pts:
> > +	kfree(as->pts);
> > +free_pd_range:
> > +	__free_page(as->pd);
> > +free_as:
> > +	kfree(as);
> > +
> > +	return NULL;
> >   }
> >   static void tegra_smmu_domain_free(struct iommu_domain *domain)
> > @@ -1012,7 +1016,7 @@ static const struct iommu_ops tegra_smmu_ops = {
> >   	.probe_device = tegra_smmu_probe_device,
> >   	.release_device = tegra_smmu_release_device,
> >   	.device_group = tegra_smmu_device_group,
> > -	.get_resv_regions = of_iommu_get_resv_regions,
> > +	.get_resv_regions = iommu_dma_get_resv_regions,
> >   	.of_xlate = tegra_smmu_of_xlate,
> >   	.pgsize_bitmap = SZ_4K,
> >   	.default_domain_ops = &(const struct iommu_domain_ops) {
diff mbox series

Patch

diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
index 57b4f2b37447..7ad993330634 100644
--- a/drivers/iommu/tegra-smmu.c
+++ b/drivers/iommu/tegra-smmu.c
@@ -20,6 +20,8 @@ 
 #include <soc/tegra/ahb.h>
 #include <soc/tegra/mc.h>
 
+#include "dma-iommu.h"
+
 struct tegra_smmu_group {
 	struct list_head list;
 	struct tegra_smmu *smmu;
@@ -277,7 +279,9 @@  static struct iommu_domain *tegra_smmu_domain_alloc(unsigned type)
 {
 	struct tegra_smmu_as *as;
 
-	if (type != IOMMU_DOMAIN_UNMANAGED)
+	if (type != IOMMU_DOMAIN_UNMANAGED &&
+	    type != IOMMU_DOMAIN_DMA &&
+	    type != IOMMU_DOMAIN_IDENTITY)
 		return NULL;
 
 	as = kzalloc(sizeof(*as), GFP_KERNEL);
@@ -287,25 +291,16 @@  static struct iommu_domain *tegra_smmu_domain_alloc(unsigned type)
 	as->attr = SMMU_PD_READABLE | SMMU_PD_WRITABLE | SMMU_PD_NONSECURE;
 
 	as->pd = alloc_page(GFP_KERNEL | __GFP_DMA | __GFP_ZERO);
-	if (!as->pd) {
-		kfree(as);
-		return NULL;
-	}
+	if (!as->pd)
+		goto free_as;
 
 	as->count = kcalloc(SMMU_NUM_PDE, sizeof(u32), GFP_KERNEL);
-	if (!as->count) {
-		__free_page(as->pd);
-		kfree(as);
-		return NULL;
-	}
+	if (!as->count)
+		goto free_pd_range;
 
 	as->pts = kcalloc(SMMU_NUM_PDE, sizeof(*as->pts), GFP_KERNEL);
-	if (!as->pts) {
-		kfree(as->count);
-		__free_page(as->pd);
-		kfree(as);
-		return NULL;
-	}
+	if (!as->pts)
+		goto free_pts;
 
 	spin_lock_init(&as->lock);
 
@@ -315,6 +310,15 @@  static struct iommu_domain *tegra_smmu_domain_alloc(unsigned type)
 	as->domain.geometry.force_aperture = true;
 
 	return &as->domain;
+
+free_pts:
+	kfree(as->pts);
+free_pd_range:
+	__free_page(as->pd);
+free_as:
+	kfree(as);
+
+	return NULL;
 }
 
 static void tegra_smmu_domain_free(struct iommu_domain *domain)
@@ -1012,7 +1016,7 @@  static const struct iommu_ops tegra_smmu_ops = {
 	.probe_device = tegra_smmu_probe_device,
 	.release_device = tegra_smmu_release_device,
 	.device_group = tegra_smmu_device_group,
-	.get_resv_regions = of_iommu_get_resv_regions,
+	.get_resv_regions = iommu_dma_get_resv_regions,
 	.of_xlate = tegra_smmu_of_xlate,
 	.pgsize_bitmap = SZ_4K,
 	.default_domain_ops = &(const struct iommu_domain_ops) {