All the mail mirrored from lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 1/5] iommu/tegra-smmu: Fix domain_alloc
@ 2019-01-16 20:50 Navneet Kumar
       [not found] ` <1547671814-30088-1-git-send-email-navneetk-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
  0 siblings, 1 reply; 13+ messages in thread
From: Navneet Kumar @ 2019-01-16 20:50 UTC (permalink / raw
  To: iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA,
	joro-zLv9SwRftAIdnm+yROfE0A
  Cc: linux-tegra-u79uwXL29TY76Z2rM5mHXA,
	navneetk-DDmLM1+adcrQT0dZR+AlfA,
	thierry.reding-Re5JQEeQqe8AvxtiuMwx3w,
	jonathanh-DDmLM1+adcrQT0dZR+AlfA

* Allocate dma iova cookie for a domain while adding dma iommu
devices.
* Perform a stricter check for domain type parameter.

Signed-off-by: Navneet Kumar <navneetk-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 drivers/iommu/tegra-smmu.c | 43 +++++++++++++++++++++++++++----------------
 1 file changed, 27 insertions(+), 16 deletions(-)

diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
index 543f7c9..ee4d8a8 100644
--- a/drivers/iommu/tegra-smmu.c
+++ b/drivers/iommu/tegra-smmu.c
@@ -16,6 +16,7 @@
 #include <linux/platform_device.h>
 #include <linux/slab.h>
 #include <linux/dma-mapping.h>
+#include <linux/dma-iommu.h>
 
 #include <soc/tegra/ahb.h>
 #include <soc/tegra/mc.h>
@@ -271,8 +272,10 @@ static bool tegra_smmu_capable(enum iommu_cap cap)
 static struct iommu_domain *tegra_smmu_domain_alloc(unsigned type)
 {
 	struct tegra_smmu_as *as;
+	int ret;
 
-	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);
@@ -281,26 +284,22 @@ static struct iommu_domain *tegra_smmu_domain_alloc(unsigned type)
 
 	as->attr = SMMU_PD_READABLE | SMMU_PD_WRITABLE | SMMU_PD_NONSECURE;
 
+	ret = (type == IOMMU_DOMAIN_DMA) ? iommu_get_dma_cookie(&as->domain) :
+		-ENODEV;
+	if (ret)
+		goto free_as;
+
 	as->pd = alloc_page(GFP_KERNEL | __GFP_DMA | __GFP_ZERO);
-	if (!as->pd) {
-		kfree(as);
-		return NULL;
-	}
+	if (!as->pd)
+		goto put_dma_cookie;
 
 	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;
 
 	/* setup aperture */
 	as->domain.geometry.aperture_start = 0;
@@ -308,6 +307,18 @@ 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);
+put_dma_cookie:
+	if (type == IOMMU_DOMAIN_DMA)
+		iommu_put_dma_cookie(&as->domain);
+free_as:
+	kfree(as);
+
+	return NULL;
 }
 
 static void tegra_smmu_domain_free(struct iommu_domain *domain)
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH 2/5] iommu/tegra-smmu: Use non-secure register for flushing
       [not found] ` <1547671814-30088-1-git-send-email-navneetk-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
@ 2019-01-16 20:50   ` Navneet Kumar
       [not found]     ` <1547671814-30088-2-git-send-email-navneetk-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
  2019-01-16 20:50   ` [PATCH 3/5] iommu/tegra-smmu: Fix client enablement order Navneet Kumar
                     ` (4 subsequent siblings)
  5 siblings, 1 reply; 13+ messages in thread
From: Navneet Kumar @ 2019-01-16 20:50 UTC (permalink / raw
  To: iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA,
	joro-zLv9SwRftAIdnm+yROfE0A
  Cc: linux-tegra-u79uwXL29TY76Z2rM5mHXA,
	navneetk-DDmLM1+adcrQT0dZR+AlfA,
	thierry.reding-Re5JQEeQqe8AvxtiuMwx3w,
	jonathanh-DDmLM1+adcrQT0dZR+AlfA

Use PTB_ASID instead of SMMU_CONFIG to flush smmu.
PTB_ASID can be accessed from non-secure mode, SMMU_CONFIG cannot be.
Using SMMU_CONFIG could pose a problem when kernel doesn't have secure
mode access enabled from boot.

Signed-off-by: Navneet Kumar <navneetk-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 drivers/iommu/tegra-smmu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
index ee4d8a8..fa175d9 100644
--- a/drivers/iommu/tegra-smmu.c
+++ b/drivers/iommu/tegra-smmu.c
@@ -235,7 +235,7 @@ static inline void smmu_flush_tlb_group(struct tegra_smmu *smmu,
 
 static inline void smmu_flush(struct tegra_smmu *smmu)
 {
-	smmu_readl(smmu, SMMU_CONFIG);
+	smmu_readl(smmu, SMMU_PTB_ASID);
 }
 
 static int tegra_smmu_alloc_asid(struct tegra_smmu *smmu, unsigned int *idp)
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH 3/5] iommu/tegra-smmu: Fix client enablement order
       [not found] ` <1547671814-30088-1-git-send-email-navneetk-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
  2019-01-16 20:50   ` [PATCH 2/5] iommu/tegra-smmu: Use non-secure register for flushing Navneet Kumar
@ 2019-01-16 20:50   ` Navneet Kumar
  2019-01-16 20:50   ` [PATCH 4/5] iommu/tegra-smmu: Add PCI support Navneet Kumar
                     ` (3 subsequent siblings)
  5 siblings, 0 replies; 13+ messages in thread
From: Navneet Kumar @ 2019-01-16 20:50 UTC (permalink / raw
  To: iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA,
	joro-zLv9SwRftAIdnm+yROfE0A
  Cc: linux-tegra-u79uwXL29TY76Z2rM5mHXA,
	navneetk-DDmLM1+adcrQT0dZR+AlfA,
	thierry.reding-Re5JQEeQqe8AvxtiuMwx3w,
	jonathanh-DDmLM1+adcrQT0dZR+AlfA

Enable clients' translation only after setting up the swgroups.

Signed-off-by: Navneet Kumar <navneetk-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 drivers/iommu/tegra-smmu.c | 23 ++++++++++++++---------
 1 file changed, 14 insertions(+), 9 deletions(-)

diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
index fa175d9..1a44cf6 100644
--- a/drivers/iommu/tegra-smmu.c
+++ b/drivers/iommu/tegra-smmu.c
@@ -353,6 +353,20 @@ static void tegra_smmu_enable(struct tegra_smmu *smmu, unsigned int swgroup,
 	unsigned int i;
 	u32 value;
 
+	group = tegra_smmu_find_swgroup(smmu, swgroup);
+	if (group) {
+		value = smmu_readl(smmu, group->reg);
+		value &= ~SMMU_ASID_MASK;
+		value |= SMMU_ASID_VALUE(asid);
+		value |= SMMU_ASID_ENABLE;
+		smmu_writel(smmu, value, group->reg);
+	} else {
+		pr_warn("%s group from swgroup %u not found\n", __func__,
+				swgroup);
+		/* No point moving ahead if group was not found */
+		return;
+	}
+
 	for (i = 0; i < smmu->soc->num_clients; i++) {
 		const struct tegra_mc_client *client = &smmu->soc->clients[i];
 
@@ -363,15 +377,6 @@ static void tegra_smmu_enable(struct tegra_smmu *smmu, unsigned int swgroup,
 		value |= BIT(client->smmu.bit);
 		smmu_writel(smmu, value, client->smmu.reg);
 	}
-
-	group = tegra_smmu_find_swgroup(smmu, swgroup);
-	if (group) {
-		value = smmu_readl(smmu, group->reg);
-		value &= ~SMMU_ASID_MASK;
-		value |= SMMU_ASID_VALUE(asid);
-		value |= SMMU_ASID_ENABLE;
-		smmu_writel(smmu, value, group->reg);
-	}
 }
 
 static void tegra_smmu_disable(struct tegra_smmu *smmu, unsigned int swgroup,
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH 4/5] iommu/tegra-smmu: Add PCI support
       [not found] ` <1547671814-30088-1-git-send-email-navneetk-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
  2019-01-16 20:50   ` [PATCH 2/5] iommu/tegra-smmu: Use non-secure register for flushing Navneet Kumar
  2019-01-16 20:50   ` [PATCH 3/5] iommu/tegra-smmu: Fix client enablement order Navneet Kumar
@ 2019-01-16 20:50   ` Navneet Kumar
  2019-01-16 20:50   ` [PATCH 5/5] iommu/tegra-smmu: Add resv_regions ops Navneet Kumar
                     ` (2 subsequent siblings)
  5 siblings, 0 replies; 13+ messages in thread
From: Navneet Kumar @ 2019-01-16 20:50 UTC (permalink / raw
  To: iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA,
	joro-zLv9SwRftAIdnm+yROfE0A
  Cc: linux-tegra-u79uwXL29TY76Z2rM5mHXA,
	navneetk-DDmLM1+adcrQT0dZR+AlfA,
	thierry.reding-Re5JQEeQqe8AvxtiuMwx3w,
	jonathanh-DDmLM1+adcrQT0dZR+AlfA

Add support for PCI devices.

Signed-off-by: Navneet Kumar <navneetk-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 drivers/iommu/tegra-smmu.c | 47 +++++++++++++++++++++++++++++++++-------------
 1 file changed, 34 insertions(+), 13 deletions(-)

diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
index 1a44cf6..4b43c63 100644
--- a/drivers/iommu/tegra-smmu.c
+++ b/drivers/iommu/tegra-smmu.c
@@ -17,6 +17,7 @@
 #include <linux/slab.h>
 #include <linux/dma-mapping.h>
 #include <linux/dma-iommu.h>
+#include <linux/pci.h>
 
 #include <soc/tegra/ahb.h>
 #include <soc/tegra/mc.h>
@@ -820,7 +821,8 @@ tegra_smmu_find_group(struct tegra_smmu *smmu, unsigned int swgroup)
 	return NULL;
 }
 
-static struct iommu_group *tegra_smmu_group_get(struct tegra_smmu *smmu,
+static struct iommu_group *tegra_smmu_group_get(struct device *dev,
+						struct tegra_smmu *smmu,
 						unsigned int swgroup)
 {
 	const struct tegra_smmu_group_soc *soc;
@@ -847,7 +849,11 @@ static struct iommu_group *tegra_smmu_group_get(struct tegra_smmu *smmu,
 	INIT_LIST_HEAD(&group->list);
 	group->soc = soc;
 
-	group->group = iommu_group_alloc();
+	if (dev_is_pci(dev))
+		group->group = pci_device_group(dev);
+	else
+		group->group = generic_device_group(dev);
+
 	if (IS_ERR(group->group)) {
 		devm_kfree(smmu->dev, group);
 		mutex_unlock(&smmu->lock);
@@ -864,13 +870,8 @@ static struct iommu_group *tegra_smmu_device_group(struct device *dev)
 {
 	struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
 	struct tegra_smmu *smmu = dev->archdata.iommu;
-	struct iommu_group *group;
 
-	group = tegra_smmu_group_get(smmu, fwspec->ids[0]);
-	if (!group)
-		group = generic_device_group(dev);
-
-	return group;
+	return tegra_smmu_group_get(dev, smmu, fwspec->ids[0]);
 }
 
 static int tegra_smmu_of_xlate(struct device *dev,
@@ -989,6 +990,28 @@ static void tegra_smmu_debugfs_exit(struct tegra_smmu *smmu)
 	debugfs_remove_recursive(smmu->debugfs);
 }
 
+static int tegra_smmu_iommu_bus_init(struct tegra_smmu *smmu)
+{
+	int err;
+
+	err = bus_set_iommu(&platform_bus_type, &tegra_smmu_ops);
+	if (err < 0) {
+		iommu_device_unregister(&smmu->iommu);
+		iommu_device_sysfs_remove(&smmu->iommu);
+		return err;
+	}
+
+#ifdef CONFIG_PCI
+	if (!iommu_present(&pci_bus_type)) {
+		pci_request_acs();
+		err = bus_set_iommu(&pci_bus_type, &tegra_smmu_ops);
+		if (err < 0)
+			return err;
+	}
+#endif
+	return 0;
+}
+
 struct tegra_smmu *tegra_smmu_probe(struct device *dev,
 				    const struct tegra_smmu_soc *soc,
 				    struct tegra_mc *mc)
@@ -1072,12 +1095,10 @@ struct tegra_smmu *tegra_smmu_probe(struct device *dev,
 		return ERR_PTR(err);
 	}
 
-	err = bus_set_iommu(&platform_bus_type, &tegra_smmu_ops);
-	if (err < 0) {
-		iommu_device_unregister(&smmu->iommu);
-		iommu_device_sysfs_remove(&smmu->iommu);
+	err = tegra_smmu_iommu_bus_init(smmu);
+	if (err)
 		return ERR_PTR(err);
-	}
+	err = bus_set_iommu(&platform_bus_type, &tegra_smmu_ops);
 
 	if (IS_ENABLED(CONFIG_DEBUG_FS))
 		tegra_smmu_debugfs_init(smmu);
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH 5/5] iommu/tegra-smmu: Add resv_regions ops
       [not found] ` <1547671814-30088-1-git-send-email-navneetk-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
                     ` (2 preceding siblings ...)
  2019-01-16 20:50   ` [PATCH 4/5] iommu/tegra-smmu: Add PCI support Navneet Kumar
@ 2019-01-16 20:50   ` Navneet Kumar
       [not found]     ` <1547671814-30088-5-git-send-email-navneetk-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
  2019-01-17 15:13   ` [PATCH 1/5] iommu/tegra-smmu: Fix domain_alloc Dmitry Osipenko
  2019-02-14 11:12   ` Thierry Reding
  5 siblings, 1 reply; 13+ messages in thread
From: Navneet Kumar @ 2019-01-16 20:50 UTC (permalink / raw
  To: iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA,
	joro-zLv9SwRftAIdnm+yROfE0A
  Cc: linux-tegra-u79uwXL29TY76Z2rM5mHXA,
	navneetk-DDmLM1+adcrQT0dZR+AlfA,
	thierry.reding-Re5JQEeQqe8AvxtiuMwx3w,
	jonathanh-DDmLM1+adcrQT0dZR+AlfA

Add support for get/put reserved regions.

Signed-off-by: Navneet Kumar <navneetk-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 drivers/iommu/tegra-smmu.c | 31 +++++++++++++++++++++++++++++++
 1 file changed, 31 insertions(+)

diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
index 4b43c63..e848a45 100644
--- a/drivers/iommu/tegra-smmu.c
+++ b/drivers/iommu/tegra-smmu.c
@@ -22,6 +22,9 @@
 #include <soc/tegra/ahb.h>
 #include <soc/tegra/mc.h>
 
+#define MSI_IOVA_BASE	0x8000000
+#define MSI_IOVA_LENGTH	0x100000
+
 struct tegra_smmu_group {
 	struct list_head list;
 	const struct tegra_smmu_group_soc *soc;
@@ -882,6 +885,31 @@ static int tegra_smmu_of_xlate(struct device *dev,
 	return iommu_fwspec_add_ids(dev, &id, 1);
 }
 
+static void tegra_smmu_get_resv_regions(struct device *dev,
+		struct list_head *head)
+{
+	struct iommu_resv_region *region;
+	int prot = IOMMU_WRITE | IOMMU_NOEXEC | IOMMU_MMIO;
+
+	region = iommu_alloc_resv_region(MSI_IOVA_BASE, MSI_IOVA_LENGTH,
+					prot, IOMMU_RESV_SW_MSI);
+	if (!region)
+		return;
+
+	list_add_tail(&region->list, head);
+
+	iommu_dma_get_resv_regions(dev, head);
+}
+
+static void tegra_smmu_put_resv_regions(struct device *dev,
+		struct list_head *head)
+{
+	struct iommu_resv_region *entry, *next;
+
+	list_for_each_entry_safe(entry, next, head, list)
+		kfree(entry);
+}
+
 static const struct iommu_ops tegra_smmu_ops = {
 	.capable = tegra_smmu_capable,
 	.domain_alloc = tegra_smmu_domain_alloc,
@@ -896,6 +924,9 @@ static const struct iommu_ops tegra_smmu_ops = {
 	.iova_to_phys = tegra_smmu_iova_to_phys,
 	.of_xlate = tegra_smmu_of_xlate,
 	.pgsize_bitmap = SZ_4K,
+
+	.get_resv_regions = tegra_smmu_get_resv_regions,
+	.put_resv_regions = tegra_smmu_put_resv_regions,
 };
 
 static void tegra_smmu_ahb_enable(void)
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [PATCH 1/5] iommu/tegra-smmu: Fix domain_alloc
       [not found] ` <1547671814-30088-1-git-send-email-navneetk-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
                     ` (3 preceding siblings ...)
  2019-01-16 20:50   ` [PATCH 5/5] iommu/tegra-smmu: Add resv_regions ops Navneet Kumar
@ 2019-01-17 15:13   ` Dmitry Osipenko
       [not found]     ` <e55f408d-d518-f12d-4233-1b70263400f4-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
  2019-02-14 11:12   ` Thierry Reding
  5 siblings, 1 reply; 13+ messages in thread
From: Dmitry Osipenko @ 2019-01-17 15:13 UTC (permalink / raw
  To: Navneet Kumar, iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA,
	joro-zLv9SwRftAIdnm+yROfE0A
  Cc: linux-tegra-u79uwXL29TY76Z2rM5mHXA,
	thierry.reding-Re5JQEeQqe8AvxtiuMwx3w,
	jonathanh-DDmLM1+adcrQT0dZR+AlfA

16.01.2019 23:50, Navneet Kumar пишет:
> * Allocate dma iova cookie for a domain while adding dma iommu
> devices.
> * Perform a stricter check for domain type parameter.
> 

Commit message should tell what exactly is getting "fixed". Apparently you're trying to support T132 ARM64 here.

> Signed-off-by: Navneet Kumar <navneetk@nvidia.com>
> ---
>  drivers/iommu/tegra-smmu.c | 43 +++++++++++++++++++++++++++----------------
>  1 file changed, 27 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
> index 543f7c9..ee4d8a8 100644
> --- a/drivers/iommu/tegra-smmu.c
> +++ b/drivers/iommu/tegra-smmu.c
> @@ -16,6 +16,7 @@
>  #include <linux/platform_device.h>
>  #include <linux/slab.h>
>  #include <linux/dma-mapping.h>
> +#include <linux/dma-iommu.h>
>  
>  #include <soc/tegra/ahb.h>
>  #include <soc/tegra/mc.h>
> @@ -271,8 +272,10 @@ static bool tegra_smmu_capable(enum iommu_cap cap)
>  static struct iommu_domain *tegra_smmu_domain_alloc(unsigned type)
>  {
>  	struct tegra_smmu_as *as;
> +	int ret;
>  
> -	if (type != IOMMU_DOMAIN_UNMANAGED)
> +	if (type != IOMMU_DOMAIN_UNMANAGED && type != IOMMU_DOMAIN_DMA &&
> +			type != IOMMU_DOMAIN_IDENTITY)
>  		return NULL;

Should be better to write these lines like this for the sake of readability:

	if (type != IOMMU_DOMAIN_UNMANAGED && 
	    type != IOMMU_DOMAIN_DMA &&
	    type != IOMMU_DOMAIN_IDENTITY)
		return NULL;


>  
>  	as = kzalloc(sizeof(*as), GFP_KERNEL);
> @@ -281,26 +284,22 @@ static struct iommu_domain *tegra_smmu_domain_alloc(unsigned type)
>  
>  	as->attr = SMMU_PD_READABLE | SMMU_PD_WRITABLE | SMMU_PD_NONSECURE;
>  
> +	ret = (type == IOMMU_DOMAIN_DMA) ? iommu_get_dma_cookie(&as->domain) :
> +		-ENODEV;

This makes to fail allocation of domain that has type other than IOMMU_DOMAIN_DMA.

Should be:

	if (type == IOMMU_DOMAIN_DMA) {
		err = iommu_get_dma_cookie(&as->domain);
		if (err)
			goto free_as;
	}


> +	if (ret)
> +		goto free_as;
> +
>  	as->pd = alloc_page(GFP_KERNEL | __GFP_DMA | __GFP_ZERO);
> -	if (!as->pd) {
> -		kfree(as);
> -		return NULL;
> -	}
> +	if (!as->pd)
> +		goto put_dma_cookie;
>  
>  	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;
>  
>  	/* setup aperture */
>  	as->domain.geometry.aperture_start = 0;
> @@ -308,6 +307,18 @@ 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);
> +put_dma_cookie:
> +	if (type == IOMMU_DOMAIN_DMA)
> +		iommu_put_dma_cookie(&as->domain);
> +free_as:
> +	kfree(as);
> +
> +	return NULL;
>  }
>  
>  static void tegra_smmu_domain_free(struct iommu_domain *domain)
> 

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 2/5] iommu/tegra-smmu: Use non-secure register for flushing
       [not found]     ` <1547671814-30088-2-git-send-email-navneetk-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
@ 2019-01-17 15:25       ` Dmitry Osipenko
       [not found]         ` <340ed36a-297f-f1e6-b863-651454cf39d8-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
  0 siblings, 1 reply; 13+ messages in thread
From: Dmitry Osipenko @ 2019-01-17 15:25 UTC (permalink / raw
  To: Navneet Kumar, iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA,
	joro-zLv9SwRftAIdnm+yROfE0A
  Cc: linux-tegra-u79uwXL29TY76Z2rM5mHXA,
	thierry.reding-Re5JQEeQqe8AvxtiuMwx3w,
	jonathanh-DDmLM1+adcrQT0dZR+AlfA

16.01.2019 23:50, Navneet Kumar пишет:
> Use PTB_ASID instead of SMMU_CONFIG to flush smmu.
> PTB_ASID can be accessed from non-secure mode, SMMU_CONFIG cannot be.
> Using SMMU_CONFIG could pose a problem when kernel doesn't have secure
> mode access enabled from boot.
> 
> Signed-off-by: Navneet Kumar <navneetk@nvidia.com>
> ---
>  drivers/iommu/tegra-smmu.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
> index ee4d8a8..fa175d9 100644
> --- a/drivers/iommu/tegra-smmu.c
> +++ b/drivers/iommu/tegra-smmu.c
> @@ -235,7 +235,7 @@ static inline void smmu_flush_tlb_group(struct tegra_smmu *smmu,
>  
>  static inline void smmu_flush(struct tegra_smmu *smmu)
>  {
> -	smmu_readl(smmu, SMMU_CONFIG);
> +	smmu_readl(smmu, SMMU_PTB_ASID);
>  }
>  
>  static int tegra_smmu_alloc_asid(struct tegra_smmu *smmu, unsigned int *idp)
> 

Driver writes to SMMU_CONFIG during of the probing. Looks like it's better to drop this patch for now and make it part of a complete solution that will add proper support for a stricter insecure-mode platforms.
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 1/5] iommu/tegra-smmu: Fix domain_alloc
       [not found]     ` <e55f408d-d518-f12d-4233-1b70263400f4-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
@ 2019-01-17 16:50       ` Robin Murphy
       [not found]         ` <4f3104b7-120e-d71b-e7ea-9790ed2a3c97-5wv7dgnIgG8@public.gmane.org>
  0 siblings, 1 reply; 13+ messages in thread
From: Robin Murphy @ 2019-01-17 16:50 UTC (permalink / raw
  To: Dmitry Osipenko, Navneet Kumar,
	iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA,
	joro-zLv9SwRftAIdnm+yROfE0A
  Cc: linux-tegra-u79uwXL29TY76Z2rM5mHXA,
	thierry.reding-Re5JQEeQqe8AvxtiuMwx3w,
	jonathanh-DDmLM1+adcrQT0dZR+AlfA

On 17/01/2019 15:13, Dmitry Osipenko wrote:
> 16.01.2019 23:50, Navneet Kumar пишет:
>> * Allocate dma iova cookie for a domain while adding dma iommu
>> devices.
>> * Perform a stricter check for domain type parameter.
>>
> 
> Commit message should tell what exactly is getting "fixed". Apparently you're trying to support T132 ARM64 here.
> 
>> Signed-off-by: Navneet Kumar <navneetk@nvidia.com>
>> ---
>>   drivers/iommu/tegra-smmu.c | 43 +++++++++++++++++++++++++++----------------
>>   1 file changed, 27 insertions(+), 16 deletions(-)
>>
>> diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
>> index 543f7c9..ee4d8a8 100644
>> --- a/drivers/iommu/tegra-smmu.c
>> +++ b/drivers/iommu/tegra-smmu.c
>> @@ -16,6 +16,7 @@
>>   #include <linux/platform_device.h>
>>   #include <linux/slab.h>
>>   #include <linux/dma-mapping.h>
>> +#include <linux/dma-iommu.h>
>>   
>>   #include <soc/tegra/ahb.h>
>>   #include <soc/tegra/mc.h>
>> @@ -271,8 +272,10 @@ static bool tegra_smmu_capable(enum iommu_cap cap)
>>   static struct iommu_domain *tegra_smmu_domain_alloc(unsigned type)
>>   {
>>   	struct tegra_smmu_as *as;
>> +	int ret;
>>   
>> -	if (type != IOMMU_DOMAIN_UNMANAGED)
>> +	if (type != IOMMU_DOMAIN_UNMANAGED && type != IOMMU_DOMAIN_DMA &&
>> +			type != IOMMU_DOMAIN_IDENTITY)
>>   		return NULL;
> 
> Should be better to write these lines like this for the sake of readability:
> 
> 	if (type != IOMMU_DOMAIN_UNMANAGED &&
> 	    type != IOMMU_DOMAIN_DMA &&
> 	    type != IOMMU_DOMAIN_IDENTITY)
> 		return NULL;

Furthermore, AFAICS there's no special handling being added for identity 
domains - don't pretend to support them by giving back a regular 
translation domain, that's just misleading and broken.

> 
> 
>>   
>>   	as = kzalloc(sizeof(*as), GFP_KERNEL);
>> @@ -281,26 +284,22 @@ static struct iommu_domain *tegra_smmu_domain_alloc(unsigned type)
>>   
>>   	as->attr = SMMU_PD_READABLE | SMMU_PD_WRITABLE | SMMU_PD_NONSECURE;
>>   
>> +	ret = (type == IOMMU_DOMAIN_DMA) ? iommu_get_dma_cookie(&as->domain) :
>> +		-ENODEV;
> 
> This makes to fail allocation of domain that has type other than IOMMU_DOMAIN_DMA.
> 
> Should be:
> 
> 	if (type == IOMMU_DOMAIN_DMA) {
> 		err = iommu_get_dma_cookie(&as->domain);
> 		if (err)
> 			goto free_as;
> 	}
> 
> 
>> +	if (ret)
>> +		goto free_as;
>> +
>>   	as->pd = alloc_page(GFP_KERNEL | __GFP_DMA | __GFP_ZERO);
>> -	if (!as->pd) {
>> -		kfree(as);
>> -		return NULL;
>> -	}
>> +	if (!as->pd)
>> +		goto put_dma_cookie;
>>   
>>   	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;
>>   
>>   	/* setup aperture */
>>   	as->domain.geometry.aperture_start = 0;
>> @@ -308,6 +307,18 @@ 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);
>> +put_dma_cookie:
>> +	if (type == IOMMU_DOMAIN_DMA)

FWIW you don't strictly need that check - since domain->iova_cookie 
won't be set for other domain types anyway, iommu_put_dma_cookie() will 
simply ignore them.

>> +		iommu_put_dma_cookie(&as->domain);
>> +free_as:
>> +	kfree(as);
>> +
>> +	return NULL;
>>   }
>>   
>>   static void tegra_smmu_domain_free(struct iommu_domain *domain)

How about not leaking the cookie when you free a DMA ops domain?

Robin.
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 5/5] iommu/tegra-smmu: Add resv_regions ops
       [not found]     ` <1547671814-30088-5-git-send-email-navneetk-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
@ 2019-01-17 17:06       ` Robin Murphy
  0 siblings, 0 replies; 13+ messages in thread
From: Robin Murphy @ 2019-01-17 17:06 UTC (permalink / raw
  To: Navneet Kumar, iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA,
	joro-zLv9SwRftAIdnm+yROfE0A
  Cc: linux-tegra-u79uwXL29TY76Z2rM5mHXA,
	thierry.reding-Re5JQEeQqe8AvxtiuMwx3w,
	jonathanh-DDmLM1+adcrQT0dZR+AlfA

On 16/01/2019 20:50, Navneet Kumar wrote:
> Add support for get/put reserved regions.

For any particular reason? If you're aiming to get 
VFIO-passthrough-with-MSIs working on Tegra, this probably won't be 
correct anyway...

> Signed-off-by: Navneet Kumar <navneetk-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> ---
>   drivers/iommu/tegra-smmu.c | 31 +++++++++++++++++++++++++++++++
>   1 file changed, 31 insertions(+)
> 
> diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
> index 4b43c63..e848a45 100644
> --- a/drivers/iommu/tegra-smmu.c
> +++ b/drivers/iommu/tegra-smmu.c
> @@ -22,6 +22,9 @@
>   #include <soc/tegra/ahb.h>
>   #include <soc/tegra/mc.h>
>   
> +#define MSI_IOVA_BASE	0x8000000

...because this arbitrary IOVA relies on writes to the MSI doorbell 
undergoing address translation at the IOMMU just like any other access. 
Looking at Tegra 134, that seems to implement an MSI doorbell internal 
to the PCIe root complex, far upstream of translation, therefore at the 
very least you'd need to reserve whatever IOVA region is shadowed by 
that doorbell in PCI memory space...

> +#define MSI_IOVA_LENGTH	0x100000
> +
>   struct tegra_smmu_group {
>   	struct list_head list;
>   	const struct tegra_smmu_group_soc *soc;
> @@ -882,6 +885,31 @@ static int tegra_smmu_of_xlate(struct device *dev,
>   	return iommu_fwspec_add_ids(dev, &id, 1);
>   }
>   
> +static void tegra_smmu_get_resv_regions(struct device *dev,
> +		struct list_head *head)
> +{
> +	struct iommu_resv_region *region;
> +	int prot = IOMMU_WRITE | IOMMU_NOEXEC | IOMMU_MMIO;
> +
> +	region = iommu_alloc_resv_region(MSI_IOVA_BASE, MSI_IOVA_LENGTH,
> +					prot, IOMMU_RESV_SW_MSI);

...and as an untranslatable hardware MSI region, not a software-managed one.

Robin.

> +	if (!region)
> +		return;
> +
> +	list_add_tail(&region->list, head);
> +
> +	iommu_dma_get_resv_regions(dev, head);
> +}
> +
> +static void tegra_smmu_put_resv_regions(struct device *dev,
> +		struct list_head *head)
> +{
> +	struct iommu_resv_region *entry, *next;
> +
> +	list_for_each_entry_safe(entry, next, head, list)
> +		kfree(entry);
> +}
> +
>   static const struct iommu_ops tegra_smmu_ops = {
>   	.capable = tegra_smmu_capable,
>   	.domain_alloc = tegra_smmu_domain_alloc,
> @@ -896,6 +924,9 @@ static const struct iommu_ops tegra_smmu_ops = {
>   	.iova_to_phys = tegra_smmu_iova_to_phys,
>   	.of_xlate = tegra_smmu_of_xlate,
>   	.pgsize_bitmap = SZ_4K,
> +
> +	.get_resv_regions = tegra_smmu_get_resv_regions,
> +	.put_resv_regions = tegra_smmu_put_resv_regions,
>   };
>   
>   static void tegra_smmu_ahb_enable(void)
> 

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 2/5] iommu/tegra-smmu: Use non-secure register for flushing
       [not found]         ` <340ed36a-297f-f1e6-b863-651454cf39d8-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
@ 2019-01-24 18:29           ` navneet kumar
       [not found]             ` <ece55c12-2a6f-e12f-597c-ef15001e66a3-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
  0 siblings, 1 reply; 13+ messages in thread
From: navneet kumar @ 2019-01-24 18:29 UTC (permalink / raw
  To: Dmitry Osipenko,
	iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA,
	joro-zLv9SwRftAIdnm+yROfE0A
  Cc: linux-tegra-u79uwXL29TY76Z2rM5mHXA,
	thierry.reding-Re5JQEeQqe8AvxtiuMwx3w,
	jonathanh-DDmLM1+adcrQT0dZR+AlfA

On 1/17/19 7:25 AM, Dmitry Osipenko wrote:
> 16.01.2019 23:50, Navneet Kumar пишет:
>> Use PTB_ASID instead of SMMU_CONFIG to flush smmu.
>> PTB_ASID can be accessed from non-secure mode, SMMU_CONFIG cannot be.
>> Using SMMU_CONFIG could pose a problem when kernel doesn't have secure
>> mode access enabled from boot.
>>
>> Signed-off-by: Navneet Kumar <navneetk@nvidia.com>
>> ---
>>  drivers/iommu/tegra-smmu.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
>> index ee4d8a8..fa175d9 100644
>> --- a/drivers/iommu/tegra-smmu.c
>> +++ b/drivers/iommu/tegra-smmu.c
>> @@ -235,7 +235,7 @@ static inline void smmu_flush_tlb_group(struct tegra_smmu *smmu,
>>  
>>  static inline void smmu_flush(struct tegra_smmu *smmu)
>>  {
>> -	smmu_readl(smmu, SMMU_CONFIG);
>> +	smmu_readl(smmu, SMMU_PTB_ASID);
>>  }
>>  
>>  static int tegra_smmu_alloc_asid(struct tegra_smmu *smmu, unsigned int *idp)
>>
> 
> Driver writes to SMMU_CONFIG during of the probing. Looks like it's better to drop this patch for now and make it part of a complete solution that will add proper support for a stricter insecure-mode platforms.
> 
Thanks for the comment Dmitry. On secure platforms, writing to SMMU_CONFIG will be a no-op and
will pose no harm. Having this patch is important because it fixes the flushing behavior on
such platforms, which is critical.

I propose to keep this patch as is, however, i can add more explanation to the commit message,
stating the case of probe and how it will not have any ill effects. Pls ACK/NACK, and i shall post
a V2.

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 2/5] iommu/tegra-smmu: Use non-secure register for flushing
       [not found]             ` <ece55c12-2a6f-e12f-597c-ef15001e66a3-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
@ 2019-01-24 21:27               ` Dmitry Osipenko
  0 siblings, 0 replies; 13+ messages in thread
From: Dmitry Osipenko @ 2019-01-24 21:27 UTC (permalink / raw
  To: navneet kumar, iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA,
	joro-zLv9SwRftAIdnm+yROfE0A
  Cc: linux-tegra-u79uwXL29TY76Z2rM5mHXA,
	thierry.reding-Re5JQEeQqe8AvxtiuMwx3w,
	jonathanh-DDmLM1+adcrQT0dZR+AlfA

24.01.2019 21:29, navneet kumar пишет:
> On 1/17/19 7:25 AM, Dmitry Osipenko wrote:
>> 16.01.2019 23:50, Navneet Kumar пишет:
>>> Use PTB_ASID instead of SMMU_CONFIG to flush smmu.
>>> PTB_ASID can be accessed from non-secure mode, SMMU_CONFIG cannot be.
>>> Using SMMU_CONFIG could pose a problem when kernel doesn't have secure
>>> mode access enabled from boot.
>>>
>>> Signed-off-by: Navneet Kumar <navneetk@nvidia.com>
>>> ---
>>>  drivers/iommu/tegra-smmu.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
>>> index ee4d8a8..fa175d9 100644
>>> --- a/drivers/iommu/tegra-smmu.c
>>> +++ b/drivers/iommu/tegra-smmu.c
>>> @@ -235,7 +235,7 @@ static inline void smmu_flush_tlb_group(struct tegra_smmu *smmu,
>>>  
>>>  static inline void smmu_flush(struct tegra_smmu *smmu)
>>>  {
>>> -	smmu_readl(smmu, SMMU_CONFIG);
>>> +	smmu_readl(smmu, SMMU_PTB_ASID);
>>>  }
>>>  
>>>  static int tegra_smmu_alloc_asid(struct tegra_smmu *smmu, unsigned int *idp)
>>>
>>
>> Driver writes to SMMU_CONFIG during of the probing. Looks like it's better to drop this patch for now and make it part of a complete solution that will add proper support for a stricter insecure-mode platforms.
>>
> Thanks for the comment Dmitry. On secure platforms, writing to SMMU_CONFIG will be a no-op and
> will pose no harm. Having this patch is important because it fixes the flushing behavior on
> such platforms, which is critical.
> 
> I propose to keep this patch as is, however, i can add more explanation to the commit message,
> stating the case of probe and how it will not have any ill effects. Pls ACK/NACK, and i shall post
> a V2.
> 

Nothing breaks with this change at least for T30. Please extend the commit message and add a clarifying comment to the code, thanks.

With the clarifying message being addressed:

Reviewed-by: Dmitry Osipenko <digetx@gmail.com>
Tested-by: Dmitry Osipenko <digetx@gmail.com>
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 1/5] iommu/tegra-smmu: Fix domain_alloc
       [not found]         ` <4f3104b7-120e-d71b-e7ea-9790ed2a3c97-5wv7dgnIgG8@public.gmane.org>
@ 2019-01-24 22:15           ` navneet kumar
  0 siblings, 0 replies; 13+ messages in thread
From: navneet kumar @ 2019-01-24 22:15 UTC (permalink / raw
  To: Robin Murphy, Dmitry Osipenko,
	iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA,
	joro-zLv9SwRftAIdnm+yROfE0A
  Cc: linux-tegra-u79uwXL29TY76Z2rM5mHXA,
	thierry.reding-Re5JQEeQqe8AvxtiuMwx3w,
	jonathanh-DDmLM1+adcrQT0dZR+AlfA

Thanks for the feedback, Robin, and, Dmitry.
I mostly agree with all your comments, pls see my responses inline.
I'll make these fixes in V2.

On 1/17/19 8:50 AM, Robin Murphy wrote:
> On 17/01/2019 15:13, Dmitry Osipenko wrote:
>> 16.01.2019 23:50, Navneet Kumar пишет:
>>> * Allocate dma iova cookie for a domain while adding dma iommu
>>> devices.
>>> * Perform a stricter check for domain type parameter.
>>>
>>
>> Commit message should tell what exactly is getting "fixed". Apparently you're trying to support T132 ARM64 here.

I'll fix it.

>>
>>> Signed-off-by: Navneet Kumar <navneetk@nvidia.com>
>>> ---
>>>   drivers/iommu/tegra-smmu.c | 43 +++++++++++++++++++++++++++----------------
>>>   1 file changed, 27 insertions(+), 16 deletions(-)
>>>
>>> diff --git a/drivers/iommu/tegra-smmu.c b/drivers/iommu/tegra-smmu.c
>>> index 543f7c9..ee4d8a8 100644
>>> --- a/drivers/iommu/tegra-smmu.c
>>> +++ b/drivers/iommu/tegra-smmu.c
>>> @@ -16,6 +16,7 @@
>>>   #include <linux/platform_device.h>
>>>   #include <linux/slab.h>
>>>   #include <linux/dma-mapping.h>
>>> +#include <linux/dma-iommu.h>
>>>     #include <soc/tegra/ahb.h>
>>>   #include <soc/tegra/mc.h>
>>> @@ -271,8 +272,10 @@ static bool tegra_smmu_capable(enum iommu_cap cap)
>>>   static struct iommu_domain *tegra_smmu_domain_alloc(unsigned type)
>>>   {
>>>       struct tegra_smmu_as *as;
>>> +    int ret;
>>>   -    if (type != IOMMU_DOMAIN_UNMANAGED)
>>> +    if (type != IOMMU_DOMAIN_UNMANAGED && type != IOMMU_DOMAIN_DMA &&
>>> +            type != IOMMU_DOMAIN_IDENTITY)
>>>           return NULL;
>>
>> Should be better to write these lines like this for the sake of readability:
>>
>>     if (type != IOMMU_DOMAIN_UNMANAGED &&
>>         type != IOMMU_DOMAIN_DMA &&
>>         type != IOMMU_DOMAIN_IDENTITY)
>>         return NULL;
> 
> Furthermore, AFAICS there's no special handling being added for identity domains - don't pretend to support them by giving back a regular translation domain, that's just misleading and broken.

Agreed.

> 
>>
>>
>>>         as = kzalloc(sizeof(*as), GFP_KERNEL);
>>> @@ -281,26 +284,22 @@ static struct iommu_domain *tegra_smmu_domain_alloc(unsigned type)
>>>         as->attr = SMMU_PD_READABLE | SMMU_PD_WRITABLE | SMMU_PD_NONSECURE;
>>>   +    ret = (type == IOMMU_DOMAIN_DMA) ? iommu_get_dma_cookie(&as->domain) :
>>> +        -ENODEV;
>>
>> This makes to fail allocation of domain that has type other than IOMMU_DOMAIN_DMA.
>>
>> Should be:
>>
>>     if (type == IOMMU_DOMAIN_DMA) {
>>         err = iommu_get_dma_cookie(&as->domain);
>>         if (err)
>>             goto free_as;
>>     }
>>

Agreed.

>>
>>> +    if (ret)
>>> +        goto free_as;
>>> +
>>>       as->pd = alloc_page(GFP_KERNEL | __GFP_DMA | __GFP_ZERO);
>>> -    if (!as->pd) {
>>> -        kfree(as);
>>> -        return NULL;
>>> -    }
>>> +    if (!as->pd)
>>> +        goto put_dma_cookie;
>>>         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;
>>>         /* setup aperture */
>>>       as->domain.geometry.aperture_start = 0;
>>> @@ -308,6 +307,18 @@ 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);
>>> +put_dma_cookie:
>>> +    if (type == IOMMU_DOMAIN_DMA)
> 
> FWIW you don't strictly need that check - since domain->iova_cookie won't be set for other domain types anyway, iommu_put_dma_cookie() will simply ignore them.
> 

I'll still keep that check and not rely solely on the put_dma_cookie API to handle this case.
This will keep the code robust even if the put_dma_cookie API changes in future (for whatever reason).
It also looks like the canonical usage in other drivers.

>>> +        iommu_put_dma_cookie(&as->domain);
>>> +free_as:
>>> +    kfree(as);
>>> +
>>> +    return NULL;
>>>   }
>>>     static void tegra_smmu_domain_free(struct iommu_domain *domain)
> 
> How about not leaking the cookie when you free a DMA ops domain?
> 

Agreed.

> Robin.
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 1/5] iommu/tegra-smmu: Fix domain_alloc
       [not found] ` <1547671814-30088-1-git-send-email-navneetk-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
                     ` (4 preceding siblings ...)
  2019-01-17 15:13   ` [PATCH 1/5] iommu/tegra-smmu: Fix domain_alloc Dmitry Osipenko
@ 2019-02-14 11:12   ` Thierry Reding
  5 siblings, 0 replies; 13+ messages in thread
From: Thierry Reding @ 2019-02-14 11:12 UTC (permalink / raw
  To: Navneet Kumar
  Cc: linux-tegra-u79uwXL29TY76Z2rM5mHXA,
	iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA,
	jonathanh-DDmLM1+adcrQT0dZR+AlfA


[-- Attachment #1.1: Type: text/plain, Size: 2014 bytes --]

On Wed, Jan 16, 2019 at 12:50:10PM -0800, Navneet Kumar wrote:
> * Allocate dma iova cookie for a domain while adding dma iommu
> devices.
> * Perform a stricter check for domain type parameter.
> 
> Signed-off-by: Navneet Kumar <navneetk-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> ---
>  drivers/iommu/tegra-smmu.c | 43 +++++++++++++++++++++++++++----------------
>  1 file changed, 27 insertions(+), 16 deletions(-)

I just gave this a quick spin because I was investigating how we could
potentially make use of the DMA API instead of the IOMMU API directly in
Tegra DRM. We currently rely on the fact that the Tegra SMMU driver only
supports unmanaged domains. Once we start supporting DMA domains all the
automatic machinery kicks in and there's lots of SMMU faults. I think at
least partially those faults point out bugs we currently have in the
code. From the looks of it, the display controller is running during
boot and happily fetching from whatever address it was configured with
in the bootloader, and when we enable the ASID for the display
controller as part of the DMA/IOMMU setup, the fetches from the display
controller will be accessing IOV addresses that don't have a mapping.

One one hand that's a good thing because it points out existing
weaknesses, but then it also means that we can't merge this series
because it causes bad regressions.

I also see failures from the GPU with this applied, which I think stem
from the fact that we're now transparently mapping allocations through
the SMMU without the Nouveau driver knowing that and setting the
appropriate bit when addressing memory. Or it could come from the SMMU
code in Nouveau trying to map an already mapped buffer, so effectively
creating an IOVA mapping to an address that is already a IOV address
rather than a physical address.

So I think before we can go ahead with this series we have a lot of
janitorial work to do first so that this won't cause any regressions
when applied.

Thierry

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]



^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2019-02-14 11:12 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2019-01-16 20:50 [PATCH 1/5] iommu/tegra-smmu: Fix domain_alloc Navneet Kumar
     [not found] ` <1547671814-30088-1-git-send-email-navneetk-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
2019-01-16 20:50   ` [PATCH 2/5] iommu/tegra-smmu: Use non-secure register for flushing Navneet Kumar
     [not found]     ` <1547671814-30088-2-git-send-email-navneetk-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
2019-01-17 15:25       ` Dmitry Osipenko
     [not found]         ` <340ed36a-297f-f1e6-b863-651454cf39d8-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
2019-01-24 18:29           ` navneet kumar
     [not found]             ` <ece55c12-2a6f-e12f-597c-ef15001e66a3-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
2019-01-24 21:27               ` Dmitry Osipenko
2019-01-16 20:50   ` [PATCH 3/5] iommu/tegra-smmu: Fix client enablement order Navneet Kumar
2019-01-16 20:50   ` [PATCH 4/5] iommu/tegra-smmu: Add PCI support Navneet Kumar
2019-01-16 20:50   ` [PATCH 5/5] iommu/tegra-smmu: Add resv_regions ops Navneet Kumar
     [not found]     ` <1547671814-30088-5-git-send-email-navneetk-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
2019-01-17 17:06       ` Robin Murphy
2019-01-17 15:13   ` [PATCH 1/5] iommu/tegra-smmu: Fix domain_alloc Dmitry Osipenko
     [not found]     ` <e55f408d-d518-f12d-4233-1b70263400f4-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
2019-01-17 16:50       ` Robin Murphy
     [not found]         ` <4f3104b7-120e-d71b-e7ea-9790ed2a3c97-5wv7dgnIgG8@public.gmane.org>
2019-01-24 22:15           ` navneet kumar
2019-02-14 11:12   ` Thierry Reding

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.