[PATCH v2 0/5] Convert SMMU to domain_alloc_paging()

[PATCH v2 0/5] Convert SMMU to domain_alloc_paging()

[Jason Gunthorpe]

Add the global statics for IDENTITY and BLOCKED to the SMMU driver and
change to use domain_alloc_paging(). This allows SMMU to finalize the
domain during allocation.

This relies on the first few patches of the dart conversion series:

https://lore.kernel.org/r/0-v2-bff223cf6409+282-dart_paging_jgg@nvidia.com

To enable the BLOCKED global static.

v2:
 - Remove the disable_bypass fixing, Robin says to just leave it broken
   for ARM32
 - Rename arm_smmu_domain_add_master() to arm_smmu_master_install_s2crs()
   and remove smmu as a parameter
 - Fix missed use of dev in arm_smmu_context_fault
 - Use a local variable for domain in arm_smmu_init_domain_context()
v1: https://lore.kernel.org/r/0-v1-cf5846854f51+6db3f-smmu_newapi_jgg@nvidia.com

Jason Gunthorpe (5):
  iommu/arm-smmu: Reorganize arm_smmu_domain_add_master()
  iommu/arm-smmu: Convert to a global static identity domain
  iommu/arm-smmu: Implement IOMMU_DOMAIN_BLOCKED
  iommu/arm-smmu: Pass arm_smmu_domain to internal functions
  iommu/arm-smmu: Convert to domain_alloc_paging()

 drivers/iommu/arm/arm-smmu/arm-smmu.c | 126 ++++++++++++++++++--------
 drivers/iommu/arm/arm-smmu/arm-smmu.h |   1 -
 2 files changed, 88 insertions(+), 39 deletions(-)


base-commit: 853eb2d43e1be6e947fdd11c3341ffc2275fb6c7
-- 
2.42.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 1/5] iommu/arm-smmu: Reorganize arm_smmu_domain_add_master()

[Jason Gunthorpe]

Make arm_smmu_domain_add_master() not use the smmu_domain to detect the
s2cr configuration, instead pass it in as a parameter. It always returns
zero so make it return void.

Since it no longer really does anything to do with a domain call it
arm_smmu_master_install_s2crs().

This is done to make the next two patches able to re-use this code without
forcing the creation of a struct arm_smmu_domain.

Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
 drivers/iommu/arm/arm-smmu/arm-smmu.c | 23 ++++++++++-------------
 1 file changed, 10 insertions(+), 13 deletions(-)

diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu.c b/drivers/iommu/arm/arm-smmu/arm-smmu.c
index d6d1a2a55cc069..e2ec1fe14ed40b 100644
--- a/drivers/iommu/arm/arm-smmu/arm-smmu.c
+++ b/drivers/iommu/arm/arm-smmu/arm-smmu.c
@@ -1081,21 +1081,14 @@ static void arm_smmu_master_free_smes(struct arm_smmu_master_cfg *cfg,
 	mutex_unlock(&smmu->stream_map_mutex);
 }
 
-static int arm_smmu_domain_add_master(struct arm_smmu_domain *smmu_domain,
-				      struct arm_smmu_master_cfg *cfg,
-				      struct iommu_fwspec *fwspec)
+static void arm_smmu_master_install_s2crs(struct arm_smmu_master_cfg *cfg,
+					  enum arm_smmu_s2cr_type type,
+					  u8 cbndx, struct iommu_fwspec *fwspec)
 {
-	struct arm_smmu_device *smmu = smmu_domain->smmu;
+	struct arm_smmu_device *smmu = cfg->smmu;
 	struct arm_smmu_s2cr *s2cr = smmu->s2crs;
-	u8 cbndx = smmu_domain->cfg.cbndx;
-	enum arm_smmu_s2cr_type type;
 	int i, idx;
 
-	if (smmu_domain->stage == ARM_SMMU_DOMAIN_BYPASS)
-		type = S2CR_TYPE_BYPASS;
-	else
-		type = S2CR_TYPE_TRANS;
-
 	for_each_cfg_sme(cfg, fwspec, i, idx) {
 		if (type == s2cr[idx].type && cbndx == s2cr[idx].cbndx)
 			continue;
@@ -1105,7 +1098,6 @@ static int arm_smmu_domain_add_master(struct arm_smmu_domain *smmu_domain,
 		s2cr[idx].cbndx = cbndx;
 		arm_smmu_write_s2cr(smmu, idx);
 	}
-	return 0;
 }
 
 static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
@@ -1153,7 +1145,12 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
 	}
 
 	/* Looks ok, so add the device to the domain */
-	ret = arm_smmu_domain_add_master(smmu_domain, cfg, fwspec);
+	arm_smmu_master_install_s2crs(cfg,
+				      smmu_domain->stage ==
+						      ARM_SMMU_DOMAIN_BYPASS ?
+					      S2CR_TYPE_BYPASS :
+					      S2CR_TYPE_TRANS,
+				      smmu_domain->cfg.cbndx, fwspec);
 
 	/*
 	 * Setup an autosuspend delay to avoid bouncing runpm state.
-- 
2.42.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 2/5] iommu/arm-smmu: Convert to a global static identity domain

[Jason Gunthorpe]

Create a global static identity domain with it's own
arm_smmu_attach_dev_identity() that simply calls
arm_smmu_master_install_s2crs() with the identity parameters.

This is done by giving the attach path for identity its own unique
implementation that simply calls arm_smmu_master_install_s2crs().

Remove ARM_SMMU_DOMAIN_BYPASS and all checks of IOMMU_DOMAIN_IDENTITY.

Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
 drivers/iommu/arm/arm-smmu/arm-smmu.c | 50 ++++++++++++++++++++-------
 drivers/iommu/arm/arm-smmu/arm-smmu.h |  1 -
 2 files changed, 37 insertions(+), 14 deletions(-)

diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu.c b/drivers/iommu/arm/arm-smmu/arm-smmu.c
index e2ec1fe14ed40b..dde912f8ef35be 100644
--- a/drivers/iommu/arm/arm-smmu/arm-smmu.c
+++ b/drivers/iommu/arm/arm-smmu/arm-smmu.c
@@ -624,12 +624,6 @@ static int arm_smmu_init_domain_context(struct iommu_domain *domain,
 	if (smmu_domain->smmu)
 		goto out_unlock;
 
-	if (domain->type == IOMMU_DOMAIN_IDENTITY) {
-		smmu_domain->stage = ARM_SMMU_DOMAIN_BYPASS;
-		smmu_domain->smmu = smmu;
-		goto out_unlock;
-	}
-
 	/*
 	 * Mapping the requested stage onto what we support is surprisingly
 	 * complicated, mainly because the spec allows S1+S2 SMMUs without
@@ -825,7 +819,7 @@ static void arm_smmu_destroy_domain_context(struct iommu_domain *domain)
 	struct arm_smmu_cfg *cfg = &smmu_domain->cfg;
 	int ret, irq;
 
-	if (!smmu || domain->type == IOMMU_DOMAIN_IDENTITY)
+	if (!smmu)
 		return;
 
 	ret = arm_smmu_rpm_get(smmu);
@@ -854,7 +848,7 @@ static struct iommu_domain *arm_smmu_domain_alloc(unsigned type)
 {
 	struct arm_smmu_domain *smmu_domain;
 
-	if (type != IOMMU_DOMAIN_UNMANAGED && type != IOMMU_DOMAIN_IDENTITY) {
+	if (type != IOMMU_DOMAIN_UNMANAGED) {
 		if (using_legacy_binding || type != IOMMU_DOMAIN_DMA)
 			return NULL;
 	}
@@ -1145,11 +1139,7 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
 	}
 
 	/* Looks ok, so add the device to the domain */
-	arm_smmu_master_install_s2crs(cfg,
-				      smmu_domain->stage ==
-						      ARM_SMMU_DOMAIN_BYPASS ?
-					      S2CR_TYPE_BYPASS :
-					      S2CR_TYPE_TRANS,
+	arm_smmu_master_install_s2crs(cfg, S2CR_TYPE_TRANS,
 				      smmu_domain->cfg.cbndx, fwspec);
 
 	/*
@@ -1171,6 +1161,39 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
 	return ret;
 }
 
+static int arm_smmu_attach_dev_identity(struct iommu_domain *domain,
+					struct device *dev)
+{
+	struct arm_smmu_master_cfg *cfg = dev_iommu_priv_get(dev);
+	struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
+	struct arm_smmu_device *smmu;
+	int ret;
+
+	if (!cfg)
+		return -ENODEV;
+	smmu = cfg->smmu;
+
+	ret = arm_smmu_rpm_get(smmu);
+	if (ret < 0)
+		return ret;
+
+	arm_smmu_master_install_s2crs(cfg, S2CR_TYPE_BYPASS, 0, fwspec);
+
+	pm_runtime_set_autosuspend_delay(smmu->dev, 20);
+	pm_runtime_use_autosuspend(smmu->dev);
+	arm_smmu_rpm_put(smmu);
+	return 0;
+}
+
+static const struct iommu_domain_ops arm_smmu_identity_ops = {
+	.attach_dev = arm_smmu_attach_dev_identity,
+};
+
+static struct iommu_domain arm_smmu_identity_domain = {
+	.type = IOMMU_DOMAIN_IDENTITY,
+	.ops = &arm_smmu_identity_ops,
+};
+
 static int arm_smmu_map_pages(struct iommu_domain *domain, unsigned long iova,
 			      phys_addr_t paddr, size_t pgsize, size_t pgcount,
 			      int prot, gfp_t gfp, size_t *mapped)
@@ -1557,6 +1580,7 @@ static int arm_smmu_def_domain_type(struct device *dev)
 }
 
 static struct iommu_ops arm_smmu_ops = {
+	.identity_domain	= &arm_smmu_identity_domain,
 	.capable		= arm_smmu_capable,
 	.domain_alloc		= arm_smmu_domain_alloc,
 	.probe_device		= arm_smmu_probe_device,
diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu.h b/drivers/iommu/arm/arm-smmu/arm-smmu.h
index 703fd5817ec11f..836ed6799a801f 100644
--- a/drivers/iommu/arm/arm-smmu/arm-smmu.h
+++ b/drivers/iommu/arm/arm-smmu/arm-smmu.h
@@ -361,7 +361,6 @@ enum arm_smmu_domain_stage {
 	ARM_SMMU_DOMAIN_S1 = 0,
 	ARM_SMMU_DOMAIN_S2,
 	ARM_SMMU_DOMAIN_NESTED,
-	ARM_SMMU_DOMAIN_BYPASS,
 };
 
 struct arm_smmu_domain {
-- 
2.42.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 2/5] iommu/arm-smmu: Convert to a global static identity domain

[Will Deacon]

On Tue, Oct 17, 2023 at 03:11:41PM -0300, Jason Gunthorpe wrote:
> Create a global static identity domain with it's own
> arm_smmu_attach_dev_identity() that simply calls
> arm_smmu_master_install_s2crs() with the identity parameters.
> 
> This is done by giving the attach path for identity its own unique
> implementation that simply calls arm_smmu_master_install_s2crs().
> 
> Remove ARM_SMMU_DOMAIN_BYPASS and all checks of IOMMU_DOMAIN_IDENTITY.
> 
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> ---
>  drivers/iommu/arm/arm-smmu/arm-smmu.c | 50 ++++++++++++++++++++-------
>  drivers/iommu/arm/arm-smmu/arm-smmu.h |  1 -
>  2 files changed, 37 insertions(+), 14 deletions(-)

[...]

> +static int arm_smmu_attach_dev_identity(struct iommu_domain *domain,
> +					struct device *dev)
> +{
> +	struct arm_smmu_master_cfg *cfg = dev_iommu_priv_get(dev);
> +	struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
> +	struct arm_smmu_device *smmu;
> +	int ret;
> +
> +	if (!cfg)
> +		return -ENODEV;
> +	smmu = cfg->smmu;
> +
> +	ret = arm_smmu_rpm_get(smmu);
> +	if (ret < 0)
> +		return ret;
> +
> +	arm_smmu_master_install_s2crs(cfg, S2CR_TYPE_BYPASS, 0, fwspec);
> +
> +	pm_runtime_set_autosuspend_delay(smmu->dev, 20);
> +	pm_runtime_use_autosuspend(smmu->dev);

This is cargo-culted from arm_smmu_attach_dev() with the comments dropped
and it's not clear at all to me that the autosuspend delay makes any sense
for the identity domain.

So I think it would be better to either drop the call to
pm_runtime_set_autosuspend_delay() or move the two autosuspend calls into
a helper which is used by both of the attach routines so that the comment
is maintained.

Will

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 2/5] iommu/arm-smmu: Convert to a global static identity domain

[Jason Gunthorpe]

On Tue, Dec 12, 2023 at 01:27:08PM +0000, Will Deacon wrote:
> > +static int arm_smmu_attach_dev_identity(struct iommu_domain *domain,
> > +					struct device *dev)
> > +{
> > +	struct arm_smmu_master_cfg *cfg = dev_iommu_priv_get(dev);
> > +	struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
> > +	struct arm_smmu_device *smmu;
> > +	int ret;
> > +
> > +	if (!cfg)
> > +		return -ENODEV;
> > +	smmu = cfg->smmu;
> > +
> > +	ret = arm_smmu_rpm_get(smmu);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	arm_smmu_master_install_s2crs(cfg, S2CR_TYPE_BYPASS, 0, fwspec);
> > +
> > +	pm_runtime_set_autosuspend_delay(smmu->dev, 20);
> > +	pm_runtime_use_autosuspend(smmu->dev);
> 
> This is cargo-culted from arm_smmu_attach_dev() with the comments dropped
> and it's not clear at all to me that the autosuspend delay makes any sense
> for the identity domain.

Indeed, however it was how it worked before this split up.

> So I think it would be better to either drop the call to
> pm_runtime_set_autosuspend_delay() 

I looked through it more carefully and this does seem like the right
thing, so I will drop them.

Thanks,
Jason

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 2/5] iommu/arm-smmu: Convert to a global static identity domain

[Will Deacon]

On Tue, Dec 12, 2023 at 10:15:16AM -0400, Jason Gunthorpe wrote:
> On Tue, Dec 12, 2023 at 01:27:08PM +0000, Will Deacon wrote:
> > > +static int arm_smmu_attach_dev_identity(struct iommu_domain *domain,
> > > +					struct device *dev)
> > > +{
> > > +	struct arm_smmu_master_cfg *cfg = dev_iommu_priv_get(dev);
> > > +	struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
> > > +	struct arm_smmu_device *smmu;
> > > +	int ret;
> > > +
> > > +	if (!cfg)
> > > +		return -ENODEV;
> > > +	smmu = cfg->smmu;
> > > +
> > > +	ret = arm_smmu_rpm_get(smmu);
> > > +	if (ret < 0)
> > > +		return ret;
> > > +
> > > +	arm_smmu_master_install_s2crs(cfg, S2CR_TYPE_BYPASS, 0, fwspec);
> > > +
> > > +	pm_runtime_set_autosuspend_delay(smmu->dev, 20);
> > > +	pm_runtime_use_autosuspend(smmu->dev);
> > 
> > This is cargo-culted from arm_smmu_attach_dev() with the comments dropped
> > and it's not clear at all to me that the autosuspend delay makes any sense
> > for the identity domain.
> 
> Indeed, however it was how it worked before this split up.

Yeah, and I suppose given that I certainly don't have an easy way to test
this, there's a lot to be said for preserving the current behaviour.

> > So I think it would be better to either drop the call to
> > pm_runtime_set_autosuspend_delay() 
> 
> I looked through it more carefully and this does seem like the right
> thing, so I will drop them.

In the interests of getting this queued, I've gone ahead and stuck 'em
in a helper. We can drop the call later, but it would be good to either
test that or get confirmation from Rob that it doesn't break anything
first.

Will

--->8

diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu.c b/drivers/iommu/arm/arm-smmu/arm-smmu.c
index dde912f8ef35..dec912c27141 100644
--- a/drivers/iommu/arm/arm-smmu/arm-smmu.c
+++ b/drivers/iommu/arm/arm-smmu/arm-smmu.c
@@ -82,6 +82,23 @@ static inline void arm_smmu_rpm_put(struct arm_smmu_device *smmu)
 		pm_runtime_put_autosuspend(smmu->dev);
 }
 
+static void arm_smmu_rpm_use_autosuspend(struct arm_smmu_device *smmu)
+{
+	/*
+	 * Setup an autosuspend delay to avoid bouncing runpm state.
+	 * Otherwise, if a driver for a suspended consumer device
+	 * unmaps buffers, it will runpm resume/suspend for each one.
+	 *
+	 * For example, when used by a GPU device, when an application
+	 * or game exits, it can trigger unmapping 100s or 1000s of
+	 * buffers.  With a runpm cycle for each buffer, that adds up
+	 * to 5-10sec worth of reprogramming the context bank, while
+	 * the system appears to be locked up to the user.
+	 */
+	pm_runtime_set_autosuspend_delay(smmu->dev, 20);
+	pm_runtime_use_autosuspend(smmu->dev);
+}
+
 static struct arm_smmu_domain *to_smmu_domain(struct iommu_domain *dom)
 {
 	return container_of(dom, struct arm_smmu_domain, domain);
@@ -1141,21 +1158,7 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
 	/* Looks ok, so add the device to the domain */
 	arm_smmu_master_install_s2crs(cfg, S2CR_TYPE_TRANS,
 				      smmu_domain->cfg.cbndx, fwspec);
-
-	/*
-	 * Setup an autosuspend delay to avoid bouncing runpm state.
-	 * Otherwise, if a driver for a suspended consumer device
-	 * unmaps buffers, it will runpm resume/suspend for each one.
-	 *
-	 * For example, when used by a GPU device, when an application
-	 * or game exits, it can trigger unmapping 100s or 1000s of
-	 * buffers.  With a runpm cycle for each buffer, that adds up
-	 * to 5-10sec worth of reprogramming the context bank, while
-	 * the system appears to be locked up to the user.
-	 */
-	pm_runtime_set_autosuspend_delay(smmu->dev, 20);
-	pm_runtime_use_autosuspend(smmu->dev);
-
+	arm_smmu_rpm_use_autosuspend(smmu);
 rpm_put:
 	arm_smmu_rpm_put(smmu);
 	return ret;
@@ -1178,9 +1181,7 @@ static int arm_smmu_attach_dev_identity(struct iommu_domain *domain,
 		return ret;
 
 	arm_smmu_master_install_s2crs(cfg, S2CR_TYPE_BYPASS, 0, fwspec);
-
-	pm_runtime_set_autosuspend_delay(smmu->dev, 20);
-	pm_runtime_use_autosuspend(smmu->dev);
+	arm_smmu_rpm_use_autosuspend(smmu);
 	arm_smmu_rpm_put(smmu);
 	return 0;
 }

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 2/5] iommu/arm-smmu: Convert to a global static identity domain

[Jason Gunthorpe]

On Wed, Dec 13, 2023 at 01:26:52PM +0000, Will Deacon wrote:
> On Tue, Dec 12, 2023 at 10:15:16AM -0400, Jason Gunthorpe wrote:
> > On Tue, Dec 12, 2023 at 01:27:08PM +0000, Will Deacon wrote:
> > > > +static int arm_smmu_attach_dev_identity(struct iommu_domain *domain,
> > > > +					struct device *dev)
> > > > +{
> > > > +	struct arm_smmu_master_cfg *cfg = dev_iommu_priv_get(dev);
> > > > +	struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
> > > > +	struct arm_smmu_device *smmu;
> > > > +	int ret;
> > > > +
> > > > +	if (!cfg)
> > > > +		return -ENODEV;
> > > > +	smmu = cfg->smmu;
> > > > +
> > > > +	ret = arm_smmu_rpm_get(smmu);
> > > > +	if (ret < 0)
> > > > +		return ret;
> > > > +
> > > > +	arm_smmu_master_install_s2crs(cfg, S2CR_TYPE_BYPASS, 0, fwspec);
> > > > +
> > > > +	pm_runtime_set_autosuspend_delay(smmu->dev, 20);
> > > > +	pm_runtime_use_autosuspend(smmu->dev);
> > > 
> > > This is cargo-culted from arm_smmu_attach_dev() with the comments dropped
> > > and it's not clear at all to me that the autosuspend delay makes any sense
> > > for the identity domain.
> > 
> > Indeed, however it was how it worked before this split up.
> 
> Yeah, and I suppose given that I certainly don't have an easy way to test
> this, there's a lot to be said for preserving the current behaviour.

Right, it is why I did it..

> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu.c b/drivers/iommu/arm/arm-smmu/arm-smmu.c
> index dde912f8ef35..dec912c27141 100644
> --- a/drivers/iommu/arm/arm-smmu/arm-smmu.c
> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu.c
> @@ -82,6 +82,23 @@ static inline void arm_smmu_rpm_put(struct arm_smmu_device *smmu)
>  		pm_runtime_put_autosuspend(smmu->dev);
>  }
>  
> +static void arm_smmu_rpm_use_autosuspend(struct arm_smmu_device *smmu)
> +{
> +	/*
> +	 * Setup an autosuspend delay to avoid bouncing runpm state.
> +	 * Otherwise, if a driver for a suspended consumer device
> +	 * unmaps buffers, it will runpm resume/suspend for each one.
> +	 *
> +	 * For example, when used by a GPU device, when an application
> +	 * or game exits, it can trigger unmapping 100s or 1000s of
> +	 * buffers.  With a runpm cycle for each buffer, that adds up
> +	 * to 5-10sec worth of reprogramming the context bank, while
> +	 * the system appears to be locked up to the user.
> +	 */
> +	pm_runtime_set_autosuspend_delay(smmu->dev, 20);
> +	pm_runtime_use_autosuspend(smmu->dev);
> +}
> +
>  static struct arm_smmu_domain *to_smmu_domain(struct iommu_domain *dom)
>  {
>  	return container_of(dom, struct arm_smmu_domain, domain);
> @@ -1141,21 +1158,7 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
>  	/* Looks ok, so add the device to the domain */
>  	arm_smmu_master_install_s2crs(cfg, S2CR_TYPE_TRANS,
>  				      smmu_domain->cfg.cbndx, fwspec);
> -
> -	/*
> -	 * Setup an autosuspend delay to avoid bouncing runpm state.
> -	 * Otherwise, if a driver for a suspended consumer device
> -	 * unmaps buffers, it will runpm resume/suspend for each one.
> -	 *
> -	 * For example, when used by a GPU device, when an application
> -	 * or game exits, it can trigger unmapping 100s or 1000s of
> -	 * buffers.  With a runpm cycle for each buffer, that adds up
> -	 * to 5-10sec worth of reprogramming the context bank, while
> -	 * the system appears to be locked up to the user.
> -	 */
> -	pm_runtime_set_autosuspend_delay(smmu->dev, 20);
> -	pm_runtime_use_autosuspend(smmu->dev);
> -
> +	arm_smmu_rpm_use_autosuspend(smmu);
>  rpm_put:
>  	arm_smmu_rpm_put(smmu);
>  	return ret;
> @@ -1178,9 +1181,7 @@ static int arm_smmu_attach_dev_identity(struct iommu_domain *domain,
>  		return ret;
>  
>  	arm_smmu_master_install_s2crs(cfg, S2CR_TYPE_BYPASS, 0, fwspec);
> -
> -	pm_runtime_set_autosuspend_delay(smmu->dev, 20);
> -	pm_runtime_use_autosuspend(smmu->dev);
> +	arm_smmu_rpm_use_autosuspend(smmu);
>  	arm_smmu_rpm_put(smmu);
>  	return 0;
>  }

Looks good, thanks

Jason

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 3/5] iommu/arm-smmu: Implement IOMMU_DOMAIN_BLOCKED

[Jason Gunthorpe]

Using the same design as IDENTITY setup a S2CR_TYPE_FAULT s2cr for the
device.

Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
 drivers/iommu/arm/arm-smmu/arm-smmu.c | 28 ++++++++++++++++++++++++---
 1 file changed, 25 insertions(+), 3 deletions(-)

diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu.c b/drivers/iommu/arm/arm-smmu/arm-smmu.c
index dde912f8ef35be..421f3a13c6a9b7 100644
--- a/drivers/iommu/arm/arm-smmu/arm-smmu.c
+++ b/drivers/iommu/arm/arm-smmu/arm-smmu.c
@@ -1161,8 +1161,8 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
 	return ret;
 }
 
-static int arm_smmu_attach_dev_identity(struct iommu_domain *domain,
-					struct device *dev)
+static int arm_smmu_attach_dev_type(struct device *dev,
+				    enum arm_smmu_s2cr_type type)
 {
 	struct arm_smmu_master_cfg *cfg = dev_iommu_priv_get(dev);
 	struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
@@ -1177,7 +1177,7 @@ static int arm_smmu_attach_dev_identity(struct iommu_domain *domain,
 	if (ret < 0)
 		return ret;
 
-	arm_smmu_master_install_s2crs(cfg, S2CR_TYPE_BYPASS, 0, fwspec);
+	arm_smmu_master_install_s2crs(cfg, type, 0, fwspec);
 
 	pm_runtime_set_autosuspend_delay(smmu->dev, 20);
 	pm_runtime_use_autosuspend(smmu->dev);
@@ -1185,6 +1185,12 @@ static int arm_smmu_attach_dev_identity(struct iommu_domain *domain,
 	return 0;
 }
 
+static int arm_smmu_attach_dev_identity(struct iommu_domain *domain,
+					struct device *dev)
+{
+	return arm_smmu_attach_dev_type(dev, S2CR_TYPE_BYPASS);
+}
+
 static const struct iommu_domain_ops arm_smmu_identity_ops = {
 	.attach_dev = arm_smmu_attach_dev_identity,
 };
@@ -1194,6 +1200,21 @@ static struct iommu_domain arm_smmu_identity_domain = {
 	.ops = &arm_smmu_identity_ops,
 };
 
+static int arm_smmu_attach_dev_blocked(struct iommu_domain *domain,
+				       struct device *dev)
+{
+	return arm_smmu_attach_dev_type(dev, S2CR_TYPE_FAULT);
+}
+
+static const struct iommu_domain_ops arm_smmu_blocked_ops = {
+	.attach_dev = arm_smmu_attach_dev_blocked,
+};
+
+static struct iommu_domain arm_smmu_blocked_domain = {
+	.type = IOMMU_DOMAIN_BLOCKED,
+	.ops = &arm_smmu_blocked_ops,
+};
+
 static int arm_smmu_map_pages(struct iommu_domain *domain, unsigned long iova,
 			      phys_addr_t paddr, size_t pgsize, size_t pgcount,
 			      int prot, gfp_t gfp, size_t *mapped)
@@ -1581,6 +1602,7 @@ static int arm_smmu_def_domain_type(struct device *dev)
 
 static struct iommu_ops arm_smmu_ops = {
 	.identity_domain	= &arm_smmu_identity_domain,
+	.blocked_domain		= &arm_smmu_blocked_domain,
 	.capable		= arm_smmu_capable,
 	.domain_alloc		= arm_smmu_domain_alloc,
 	.probe_device		= arm_smmu_probe_device,
-- 
2.42.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 4/5] iommu/arm-smmu: Pass arm_smmu_domain to internal functions

[Jason Gunthorpe]

Keep the types consistent, all the callers of these functions already have
obtained a struct arm_smmu_domain, don't needlessly go to/from an
iommu_domain through the internal call chains.

Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
 drivers/iommu/arm/arm-smmu/arm-smmu.c | 22 ++++++++++------------
 1 file changed, 10 insertions(+), 12 deletions(-)

diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu.c b/drivers/iommu/arm/arm-smmu/arm-smmu.c
index 421f3a13c6a9b7..6340ee8ab7c482 100644
--- a/drivers/iommu/arm/arm-smmu/arm-smmu.c
+++ b/drivers/iommu/arm/arm-smmu/arm-smmu.c
@@ -392,8 +392,7 @@ static irqreturn_t arm_smmu_context_fault(int irq, void *dev)
 {
 	u32 fsr, fsynr, cbfrsynra;
 	unsigned long iova;
-	struct iommu_domain *domain = dev;
-	struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
+	struct arm_smmu_domain *smmu_domain = dev;
 	struct arm_smmu_device *smmu = smmu_domain->smmu;
 	int idx = smmu_domain->cfg.cbndx;
 	int ret;
@@ -406,7 +405,7 @@ static irqreturn_t arm_smmu_context_fault(int irq, void *dev)
 	iova = arm_smmu_cb_readq(smmu, idx, ARM_SMMU_CB_FAR);
 	cbfrsynra = arm_smmu_gr1_read(smmu, ARM_SMMU_GR1_CBFRSYNRA(idx));
 
-	ret = report_iommu_fault(domain, NULL, iova,
+	ret = report_iommu_fault(&smmu_domain->domain, NULL, iova,
 		fsynr & ARM_SMMU_FSYNR0_WNR ? IOMMU_FAULT_WRITE : IOMMU_FAULT_READ);
 
 	if (ret == -ENOSYS)
@@ -607,7 +606,7 @@ static int arm_smmu_alloc_context_bank(struct arm_smmu_domain *smmu_domain,
 	return __arm_smmu_alloc_bitmap(smmu->context_map, start, smmu->num_context_banks);
 }
 
-static int arm_smmu_init_domain_context(struct iommu_domain *domain,
+static int arm_smmu_init_domain_context(struct arm_smmu_domain *smmu_domain,
 					struct arm_smmu_device *smmu,
 					struct device *dev)
 {
@@ -616,7 +615,7 @@ static int arm_smmu_init_domain_context(struct iommu_domain *domain,
 	struct io_pgtable_ops *pgtbl_ops;
 	struct io_pgtable_cfg pgtbl_cfg;
 	enum io_pgtable_fmt fmt;
-	struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
+	struct iommu_domain *domain = &smmu_domain->domain;
 	struct arm_smmu_cfg *cfg = &smmu_domain->cfg;
 	irqreturn_t (*context_fault)(int irq, void *dev);
 
@@ -790,8 +789,8 @@ static int arm_smmu_init_domain_context(struct iommu_domain *domain,
 	else
 		context_fault = arm_smmu_context_fault;
 
-	ret = devm_request_irq(smmu->dev, irq, context_fault,
-			       IRQF_SHARED, "arm-smmu-context-fault", domain);
+	ret = devm_request_irq(smmu->dev, irq, context_fault, IRQF_SHARED,
+			       "arm-smmu-context-fault", smmu_domain);
 	if (ret < 0) {
 		dev_err(smmu->dev, "failed to request context IRQ %d (%u)\n",
 			cfg->irptndx, irq);
@@ -812,9 +811,8 @@ static int arm_smmu_init_domain_context(struct iommu_domain *domain,
 	return ret;
 }
 
-static void arm_smmu_destroy_domain_context(struct iommu_domain *domain)
+static void arm_smmu_destroy_domain_context(struct arm_smmu_domain *smmu_domain)
 {
-	struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
 	struct arm_smmu_device *smmu = smmu_domain->smmu;
 	struct arm_smmu_cfg *cfg = &smmu_domain->cfg;
 	int ret, irq;
@@ -835,7 +833,7 @@ static void arm_smmu_destroy_domain_context(struct iommu_domain *domain)
 
 	if (cfg->irptndx != ARM_SMMU_INVALID_IRPTNDX) {
 		irq = smmu->irqs[cfg->irptndx];
-		devm_free_irq(smmu->dev, irq, domain);
+		devm_free_irq(smmu->dev, irq, smmu_domain);
 	}
 
 	free_io_pgtable_ops(smmu_domain->pgtbl_ops);
@@ -875,7 +873,7 @@ static void arm_smmu_domain_free(struct iommu_domain *domain)
 	 * Free the domain resources. We assume that all devices have
 	 * already been detached.
 	 */
-	arm_smmu_destroy_domain_context(domain);
+	arm_smmu_destroy_domain_context(smmu_domain);
 	kfree(smmu_domain);
 }
 
@@ -1125,7 +1123,7 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
 		return ret;
 
 	/* Ensure that the domain is finalised */
-	ret = arm_smmu_init_domain_context(domain, smmu, dev);
+	ret = arm_smmu_init_domain_context(smmu_domain, smmu, dev);
 	if (ret < 0)
 		goto rpm_put;
 
-- 
2.42.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 5/5] iommu/arm-smmu: Convert to domain_alloc_paging()

[Jason Gunthorpe]

Now that the BLOCKED and IDENTITY behaviors are managed with their own
domains change to the domain_alloc_paging() op.

The check for using_legacy_binding is now redundant,
arm_smmu_def_domain_type() always returns IOMMU_DOMAIN_IDENTITY for this
mode, so the core code will never attempt to create a DMA domain in the
first place.

Since commit a4fdd9762272 ("iommu: Use flush queue capability") the core
code only passes in IDENTITY/BLOCKED/UNMANAGED/DMA domain types. It will
not pass in IDENTITY or BLOCKED if the global statics exist, so the test
for DMA is also redundant now too.

Call arm_smmu_init_domain_context() early if a dev is available.

Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
 drivers/iommu/arm/arm-smmu/arm-smmu.c | 21 +++++++++++++++------
 1 file changed, 15 insertions(+), 6 deletions(-)

diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu.c b/drivers/iommu/arm/arm-smmu/arm-smmu.c
index 6340ee8ab7c482..cea25afe9e49db 100644
--- a/drivers/iommu/arm/arm-smmu/arm-smmu.c
+++ b/drivers/iommu/arm/arm-smmu/arm-smmu.c
@@ -842,14 +842,11 @@ static void arm_smmu_destroy_domain_context(struct arm_smmu_domain *smmu_domain)
 	arm_smmu_rpm_put(smmu);
 }
 
-static struct iommu_domain *arm_smmu_domain_alloc(unsigned type)
+static struct iommu_domain *arm_smmu_domain_alloc_paging(struct device *dev)
 {
+	struct arm_smmu_master_cfg *cfg = NULL;
 	struct arm_smmu_domain *smmu_domain;
 
-	if (type != IOMMU_DOMAIN_UNMANAGED) {
-		if (using_legacy_binding || type != IOMMU_DOMAIN_DMA)
-			return NULL;
-	}
 	/*
 	 * Allocate the domain and initialise some of its data structures.
 	 * We can't really do anything meaningful until we've added a
@@ -862,6 +859,18 @@ static struct iommu_domain *arm_smmu_domain_alloc(unsigned type)
 	mutex_init(&smmu_domain->init_mutex);
 	spin_lock_init(&smmu_domain->cb_lock);
 
+	if (dev)
+		cfg = dev_iommu_priv_get(dev);
+	if (cfg) {
+		int ret;
+
+		ret = arm_smmu_init_domain_context(smmu_domain, cfg->smmu, dev);
+		if (ret) {
+			kfree(smmu_domain);
+			return NULL;
+		}
+	}
+
 	return &smmu_domain->domain;
 }
 
@@ -1602,7 +1611,7 @@ static struct iommu_ops arm_smmu_ops = {
 	.identity_domain	= &arm_smmu_identity_domain,
 	.blocked_domain		= &arm_smmu_blocked_domain,
 	.capable		= arm_smmu_capable,
-	.domain_alloc		= arm_smmu_domain_alloc,
+	.domain_alloc_paging	= arm_smmu_domain_alloc_paging,
 	.probe_device		= arm_smmu_probe_device,
 	.release_device		= arm_smmu_release_device,
 	.probe_finalize		= arm_smmu_probe_finalize,
-- 
2.42.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 5/5] iommu/arm-smmu: Convert to domain_alloc_paging()

[Will Deacon]

On Tue, Oct 17, 2023 at 03:11:44PM -0300, Jason Gunthorpe wrote:
> Now that the BLOCKED and IDENTITY behaviors are managed with their own
> domains change to the domain_alloc_paging() op.
> 
> The check for using_legacy_binding is now redundant,
> arm_smmu_def_domain_type() always returns IOMMU_DOMAIN_IDENTITY for this
> mode, so the core code will never attempt to create a DMA domain in the
> first place.
> 
> Since commit a4fdd9762272 ("iommu: Use flush queue capability") the core
> code only passes in IDENTITY/BLOCKED/UNMANAGED/DMA domain types. It will
> not pass in IDENTITY or BLOCKED if the global statics exist, so the test
> for DMA is also redundant now too.
> 
> Call arm_smmu_init_domain_context() early if a dev is available.
> 
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> ---
>  drivers/iommu/arm/arm-smmu/arm-smmu.c | 21 +++++++++++++++------
>  1 file changed, 15 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu.c b/drivers/iommu/arm/arm-smmu/arm-smmu.c
> index 6340ee8ab7c482..cea25afe9e49db 100644
> --- a/drivers/iommu/arm/arm-smmu/arm-smmu.c
> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu.c
> @@ -842,14 +842,11 @@ static void arm_smmu_destroy_domain_context(struct arm_smmu_domain *smmu_domain)
>  	arm_smmu_rpm_put(smmu);
>  }
>  
> -static struct iommu_domain *arm_smmu_domain_alloc(unsigned type)
> +static struct iommu_domain *arm_smmu_domain_alloc_paging(struct device *dev)
>  {
> +	struct arm_smmu_master_cfg *cfg = NULL;
>  	struct arm_smmu_domain *smmu_domain;
>  
> -	if (type != IOMMU_DOMAIN_UNMANAGED) {
> -		if (using_legacy_binding || type != IOMMU_DOMAIN_DMA)
> -			return NULL;
> -	}
>  	/*
>  	 * Allocate the domain and initialise some of its data structures.
>  	 * We can't really do anything meaningful until we've added a
> @@ -862,6 +859,18 @@ static struct iommu_domain *arm_smmu_domain_alloc(unsigned type)
>  	mutex_init(&smmu_domain->init_mutex);
>  	spin_lock_init(&smmu_domain->cb_lock);
>  
> +	if (dev)
> +		cfg = dev_iommu_priv_get(dev);
> +	if (cfg) {
> +		int ret;
> +
> +		ret = arm_smmu_init_domain_context(smmu_domain, cfg->smmu, dev);
> +		if (ret) {
> +			kfree(smmu_domain);
> +			return NULL;

If we're just returning NULL, can we drop 'ret'?

> +		}
> +	}
> +
>  	return &smmu_domain->domain;

Why do we need to handle the 'dev && !cfg' case here, instead of just
returning NULL?

Will

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 5/5] iommu/arm-smmu: Convert to domain_alloc_paging()

[Jason Gunthorpe]

On Tue, Dec 12, 2023 at 01:26:51PM +0000, Will Deacon wrote:
> > @@ -862,6 +859,18 @@ static struct iommu_domain *arm_smmu_domain_alloc(unsigned type)
> >  	mutex_init(&smmu_domain->init_mutex);
> >  	spin_lock_init(&smmu_domain->cb_lock);
> >  
> > +	if (dev)
> > +		cfg = dev_iommu_priv_get(dev);
> > +	if (cfg) {
> > +		int ret;
> > +
> > +		ret = arm_smmu_init_domain_context(smmu_domain, cfg->smmu, dev);
> > +		if (ret) {
> > +			kfree(smmu_domain);
> > +			return NULL;
> 
> If we're just returning NULL, can we drop 'ret'?

It is waiting to be changed to ERR_PTR. The dependent patches are
merged to rc5 now, so I can fix it in v3.

> > +		}
> > +	}
> > +
> >  	return &smmu_domain->domain;
> 
> Why do we need to handle the 'dev && !cfg' case here, instead g
> returning NULL?

dev && !cfg is impossible these days, so we can shuffle the if and
then just crash on that case like the other drivers do.

Jason

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 5/5] iommu/arm-smmu: Convert to domain_alloc_paging()

[Will Deacon]

On Tue, Dec 12, 2023 at 10:03:07AM -0400, Jason Gunthorpe wrote:
> On Tue, Dec 12, 2023 at 01:26:51PM +0000, Will Deacon wrote:
> > > @@ -862,6 +859,18 @@ static struct iommu_domain *arm_smmu_domain_alloc(unsigned type)
> > >  	mutex_init(&smmu_domain->init_mutex);
> > >  	spin_lock_init(&smmu_domain->cb_lock);
> > >  
> > > +	if (dev)
> > > +		cfg = dev_iommu_priv_get(dev);
> > > +	if (cfg) {
> > > +		int ret;
> > > +
> > > +		ret = arm_smmu_init_domain_context(smmu_domain, cfg->smmu, dev);
> > > +		if (ret) {
> > > +			kfree(smmu_domain);
> > > +			return NULL;
> > 
> > If we're just returning NULL, can we drop 'ret'?
> 
> It is waiting to be changed to ERR_PTR. The dependent patches are
> merged to rc5 now, so I can fix it in v3.

Sorry, I missed that. You can leave it as-is, since my SMMU branch is based
on -rc3.

> > > +		}
> > > +	}
> > > +
> > >  	return &smmu_domain->domain;
> > 
> > Why do we need to handle the 'dev && !cfg' case here, instead g
> > returning NULL?
> 
> dev && !cfg is impossible these days, so we can shuffle the if and
> then just crash on that case like the other drivers do.

Thanks, that would simplify it a little.

Will

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 5/5] iommu/arm-smmu: Convert to domain_alloc_paging()

[Will Deacon]

On Tue, Dec 12, 2023 at 02:10:23PM +0000, Will Deacon wrote:
> On Tue, Dec 12, 2023 at 10:03:07AM -0400, Jason Gunthorpe wrote:
> > On Tue, Dec 12, 2023 at 01:26:51PM +0000, Will Deacon wrote:
> > > Why do we need to handle the 'dev && !cfg' case here, instead g
> > > returning NULL?
> > 
> > dev && !cfg is impossible these days, so we can shuffle the if and
> > then just crash on that case like the other drivers do.
> 
> Thanks, that would simplify it a little.

Folded in the diff below.

Will

--->8

diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu.c b/drivers/iommu/arm/arm-smmu/arm-smmu.c
index 536e47e3a65a..b0a6b367d8a2 100644
--- a/drivers/iommu/arm/arm-smmu/arm-smmu.c
+++ b/drivers/iommu/arm/arm-smmu/arm-smmu.c
@@ -861,7 +861,6 @@ static void arm_smmu_destroy_domain_context(struct arm_smmu_domain *smmu_domain)
 
 static struct iommu_domain *arm_smmu_domain_alloc_paging(struct device *dev)
 {
-	struct arm_smmu_master_cfg *cfg = NULL;
 	struct arm_smmu_domain *smmu_domain;
 
 	/*
@@ -876,13 +875,10 @@ static struct iommu_domain *arm_smmu_domain_alloc_paging(struct device *dev)
 	mutex_init(&smmu_domain->init_mutex);
 	spin_lock_init(&smmu_domain->cb_lock);
 
-	if (dev)
-		cfg = dev_iommu_priv_get(dev);
-	if (cfg) {
-		int ret;
+	if (dev) {
+		struct arm_smmu_master_cfg *cfg = dev_iommu_priv_get(dev);
 
-		ret = arm_smmu_init_domain_context(smmu_domain, cfg->smmu, dev);
-		if (ret) {
+		if (arm_smmu_init_domain_context(smmu_domain, cfg->smmu, dev)) {
 			kfree(smmu_domain);
 			return NULL;
 		}

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 5/5] iommu/arm-smmu: Convert to domain_alloc_paging()

[Jason Gunthorpe]

On Wed, Dec 13, 2023 at 01:27:01PM +0000, Will Deacon wrote:
> On Tue, Dec 12, 2023 at 02:10:23PM +0000, Will Deacon wrote:
> > On Tue, Dec 12, 2023 at 10:03:07AM -0400, Jason Gunthorpe wrote:
> > > On Tue, Dec 12, 2023 at 01:26:51PM +0000, Will Deacon wrote:
> > > > Why do we need to handle the 'dev && !cfg' case here, instead g
> > > > returning NULL?
> > > 
> > > dev && !cfg is impossible these days, so we can shuffle the if and
> > > then just crash on that case like the other drivers do.
> > 
> > Thanks, that would simplify it a little.
> 
> Folded in the diff below.
> 
> Will
> 
> --->8
> 
> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu.c b/drivers/iommu/arm/arm-smmu/arm-smmu.c
> index 536e47e3a65a..b0a6b367d8a2 100644
> --- a/drivers/iommu/arm/arm-smmu/arm-smmu.c
> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu.c
> @@ -861,7 +861,6 @@ static void arm_smmu_destroy_domain_context(struct arm_smmu_domain *smmu_domain)
>  
>  static struct iommu_domain *arm_smmu_domain_alloc_paging(struct device *dev)
>  {
> -	struct arm_smmu_master_cfg *cfg = NULL;
>  	struct arm_smmu_domain *smmu_domain;
>  
>  	/*
> @@ -876,13 +875,10 @@ static struct iommu_domain *arm_smmu_domain_alloc_paging(struct device *dev)
>  	mutex_init(&smmu_domain->init_mutex);
>  	spin_lock_init(&smmu_domain->cb_lock);
>  
> -	if (dev)
> -		cfg = dev_iommu_priv_get(dev);
> -	if (cfg) {
> -		int ret;
> +	if (dev) {
> +		struct arm_smmu_master_cfg *cfg = dev_iommu_priv_get(dev);
>  
> -		ret = arm_smmu_init_domain_context(smmu_domain, cfg->smmu, dev);
> -		if (ret) {
> +		if (arm_smmu_init_domain_context(smmu_domain, cfg->smmu, dev)) {
>  			kfree(smmu_domain);
>  			return NULL;
>  		}

Looks good, thanks

Jason

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 5/5] iommu/arm-smmu: Convert to domain_alloc_paging()

[Dmitry Baryshkov]

On Tue, 17 Oct 2023 Jason Gunthorpe <jgg@nvidia.com> wrote:
> Now that the BLOCKED and IDENTITY behaviors are managed with their own
> domains change to the domain_alloc_paging() op.
> 
> The check for using_legacy_binding is now redundant,
> arm_smmu_def_domain_type() always returns IOMMU_DOMAIN_IDENTITY for this
> mode, so the core code will never attempt to create a DMA domain in the
> first place.
> 
> Since commit a4fdd9762272 ("iommu: Use flush queue capability") the core
> code only passes in IDENTITY/BLOCKED/UNMANAGED/DMA domain types. It will
> not pass in IDENTITY or BLOCKED if the global statics exist, so the test
> for DMA is also redundant now too.
> 
> Call arm_smmu_init_domain_context() early if a dev is available.
> 
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> ---
>  drivers/iommu/arm/arm-smmu/arm-smmu.c | 21 +++++++++++++++------
>  1 file changed, 15 insertions(+), 6 deletions(-)

For some reason this patch breaks booting of the APQ8096 Dragonboard820c
(qcom/apq8096-db820c.dts). Dispbling display subsystem (mdss) and venus
devices makes the board boot in most of the cases. Most frequently the
last parts of the log loog in a following way:

arm-smmu b40000.iommu: probing hardware configuration...
arm-smmu b40000.iommu: SMMUv2 with:
arm-smmu b40000.iommu: 	stage 1 translation
arm-smmu b40000.iommu: 	address translation ops
arm-smmu b40000.iommu: 	non-coherent table walk
arm-smmu b40000.iommu: 	(IDR0.CTTW overridden by FW configuration)
arm-smmu b40000.iommu: 	stream matching with 2 register groups
arm-smmu b40000.iommu: 	2 context banks (0 stage-2 only)
arm-smmu b40000.iommu: 	Supported page sizes: 0x63315000
arm-smmu b40000.iommu: 	Stage-1: 48-bit VA -> 36-bit IPA
arm-smmu b40000.iommu: 	preserved 0 boot mappings
arm-smmu d00000.iommu: probing hardware configuration...
arm-smmu d00000.iommu: SMMUv2 with:
arm-smmu d00000.iommu: 	stage 1 translation
arm-smmu d00000.iommu: 	address translation ops
arm-smmu d00000.iommu: 	non-coherent table walk
arm-smmu d00000.iommu: 	(IDR0.CTTW overridden by FW configuration)
arm-smmu d00000.iommu: 	stream matching with 2 register groups
arm-smmu d00000.iommu: 	2 context banks (0 stage-2 only)
arm-smmu d00000.iommu: 	Supported page sizes: 0x63315000
arm-smmu d00000.iommu: 	Stage-1: 32-bit VA -> 36-bit IPA
arm-smmu d00000.iommu: 	preserved 0 boot mappings
arm-smmu d40000.iommu: probing hardware configuration...
arm-smmu d40000.iommu: SMMUv2 with:
arm-smmu d40000.iommu: 	stage 1 translation
arm-smmu d40000.iommu: 	address translation ops
arm-smmu d40000.iommu: 	non-coherent table walk
arm-smmu d40000.iommu: 	(IDR0.CTTW overridden by FW configuration)
arm-smmu d40000.iommu: 	stream matching with 42 register groups
arm-smmu d40000.iommu: 	7 context banks (0 stage-2 only)
arm-smmu d40000.iommu: 	Supported page sizes: 0x63315000
arm-smmu d40000.iommu: 	Stage-1: 32-bit VA -> 36-bit IPA
arm-smmu d40000.iommu: 	preserved 0 boot mappings
arm-smmu da0000.iommu: probing hardware configuration...
arm-smmu da0000.iommu: SMMUv2 with:
arm-smmu da0000.iommu: 	stage 1 translation
arm-smmu da0000.iommu: 	address translation ops
arm-smmu da0000.iommu: 	non-coherent table walk
arm-smmu da0000.iommu: 	(IDR0.CTTW overridden by FW configuration)
arm-smmu da0000.iommu: 	stream matching with 4 register groups
arm-smmu da0000.iommu: 	2 context banks (0 stage-2 only)
arm-smmu da0000.iommu: 	Supported page sizes: 0x63315000
arm-smmu da0000.iommu: 	Stage-1: 32-bit VA -> 36-bit IPA
arm-smmu da0000.iommu: 	preserved 0 boot mappings
arm-smmu 1600000.iommu: probing hardware configuration...
arm-smmu 1600000.iommu: SMMUv2 with:
arm-smmu 1600000.iommu: 	stage 1 translation
arm-smmu 1600000.iommu: 	address translation ops
arm-smmu 1600000.iommu: 	non-coherent table walk
arm-smmu 1600000.iommu: 	(IDR0.CTTW overridden by FW configuration)
arm-smmu 1600000.iommu: 	stream matching with 15 register groups
arm-smmu 1600000.iommu: 	12 context banks (0 stage-2 only)
arm-smmu 1600000.iommu: 	Supported page sizes: 0x63315000
arm-smmu 1600000.iommu: 	Stage-1: 36-bit VA -> 36-bit IPA
arm-smmu 1600000.iommu: 	preserved 0 boot mappings
adreno b00000.gpu: Adding to iommu group 0
Bluetooth: hci0: QCA Product ID   :0x00000008
Bluetooth: hci0: QCA SOC Version  :0x00000044
Bluetooth: hci0: QCA ROM Version  :0x00000302
Bluetooth: hci0: QCA Patch Version:0x00000111
Bluetooth: hci0: QCA controller version 0x00440302
platform 9a0000.hdmi-tx: Fixed dependency cycle(s) with /soc@0/display-subsystem@900000/display-controller@901000/ports/port@0/endpoint
Bluetooth: hci0: QCA Downloading qca/rampatch_00440302.bin
spi_qup 7575000.spi: IN:block:16, fifo:64, OUT:block:16, fifo:64
spi_qup 75ba000.spi: IN:block:16, fifo:64, OUT:block:16, fifo:64
phy phy-7411000.phy.5: QUSB2PHY pll lock failed: status reg = 0
phy phy-7411000.phy.5: phy init failed --> -16
dwc3 6a00000.usb: error -EBUSY: failed to initialize core
dwc3: probe of 6a00000.usb failed with error -16
phy phy-7412000.phy.6: QUSB2PHY pll lock failed: status reg = 0
phy phy-7412000.phy.6: phy init failed --> -16
dwc3 7600000.usb: error -EBUSY: failed to initialize core
dwc3: probe of 7600000.usb failed with error -16
i2c_qup 7577000.i2c: using default clock-frequency 100000
i2c_qup 75b5000.i2c: using default clock-frequency 100000
sdhci_msm 74a4900.mmc: Got CD GPIO
scsi host0: ufshcd
ufshcd-qcom 624000.ufshc: ufs_qcom_host_reset: reset control not set
remoteproc remoteproc0: 2080000.remoteproc is available
remoteproc remoteproc1: 9300000.remoteproc is available
mmc0: SDHCI controller on 74a4900.mmc [74a4900.mmc] using ADMA 64-bit
qcom-pcie 600000.pcie: host bridge /soc@0/bus@0/pcie@600000 ranges:
qcom-pcie 608000.pcie: supply vddpe-3v3 not found, using dummy regulator

[ The board resets to the bootloader ]


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 5/5] iommu/arm-smmu: Convert to domain_alloc_paging()

[Jason Gunthorpe]

On Fri, Feb 09, 2024 at 10:05:38PM +0200, Dmitry Baryshkov wrote:
> On Tue, 17 Oct 2023 Jason Gunthorpe <jgg@nvidia.com> wrote:
> > Now that the BLOCKED and IDENTITY behaviors are managed with their own
> > domains change to the domain_alloc_paging() op.
> > 
> > The check for using_legacy_binding is now redundant,
> > arm_smmu_def_domain_type() always returns IOMMU_DOMAIN_IDENTITY for this
> > mode, so the core code will never attempt to create a DMA domain in the
> > first place.
> > 
> > Since commit a4fdd9762272 ("iommu: Use flush queue capability") the core
> > code only passes in IDENTITY/BLOCKED/UNMANAGED/DMA domain types. It will
> > not pass in IDENTITY or BLOCKED if the global statics exist, so the test
> > for DMA is also redundant now too.
> > 
> > Call arm_smmu_init_domain_context() early if a dev is available.
> > 
> > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> > ---
> >  drivers/iommu/arm/arm-smmu/arm-smmu.c | 21 +++++++++++++++------
> >  1 file changed, 15 insertions(+), 6 deletions(-)
> 
> For some reason this patch breaks booting of the APQ8096 Dragonboard820c
> (qcom/apq8096-db820c.dts). Dispbling display subsystem (mdss) and venus
> devices makes the board boot in most of the cases. Most frequently the
> last parts of the log loog in a following way:

It is surprising we tested this patch on some tegra systems with this
iommu and didn't hit anything..

The only real functional thing this changes is to move the domain
initialization up in time, potentially a lot in time in some
cases. That function does alot of things including touching HW so
possibly there is some surprising interaction with something else.

So, I would expect this to not WARN_ON and to make it work the same as
before the patch:

--- a/drivers/iommu/arm/arm-smmu/arm-smmu.c
+++ b/drivers/iommu/arm/arm-smmu/arm-smmu.c
@@ -875,7 +875,9 @@ static struct iommu_domain *arm_smmu_domain_alloc_paging(struct device *dev)
        mutex_init(&smmu_domain->init_mutex);
        spin_lock_init(&smmu_domain->cb_lock);
 
-       if (dev) {
+       WARN_ON(using_legacy_binding);
+
+/*     if (dev) {
                struct arm_smmu_master_cfg *cfg = dev_iommu_priv_get(dev);
 
                if (arm_smmu_init_domain_context(smmu_domain, cfg->smmu, dev)) {
@@ -883,7 +885,7 @@ static struct iommu_domain *arm_smmu_domain_alloc_paging(struct device *dev)
                        return NULL;
                }
        }
-
+*/
        return &smmu_domain->domain;
 }

Then I'd ask you to remove the comment and do:

@@ -878,7 +878,9 @@ static struct iommu_domain *arm_smmu_domain_alloc_paging(struct device *dev)
        if (dev) {
                struct arm_smmu_master_cfg *cfg = dev_iommu_priv_get(dev);
 
+               WARN_ON(true);
                if (arm_smmu_init_domain_context(smmu_domain, cfg->smmu, dev)) {
+                       printk("Allocation failure in arm_smmu_domain_alloc_paging()\n");
                        kfree(smmu_domain);
                        return NULL;
                }


And then we may get a clue from the backtraces it generates. I only
saw one iommu group reported in your log so I'd expect one trace?

Thanks,
Jason

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 5/5] iommu/arm-smmu: Convert to domain_alloc_paging()

[Dmitry Baryshkov]

On Sat, 10 Feb 2024 at 00:23, Jason Gunthorpe <jgg@nvidia.com> wrote:
>
> On Fri, Feb 09, 2024 at 10:05:38PM +0200, Dmitry Baryshkov wrote:
> > On Tue, 17 Oct 2023 Jason Gunthorpe <jgg@nvidia.com> wrote:
> > > Now that the BLOCKED and IDENTITY behaviors are managed with their own
> > > domains change to the domain_alloc_paging() op.
> > >
> > > The check for using_legacy_binding is now redundant,
> > > arm_smmu_def_domain_type() always returns IOMMU_DOMAIN_IDENTITY for this
> > > mode, so the core code will never attempt to create a DMA domain in the
> > > first place.
> > >
> > > Since commit a4fdd9762272 ("iommu: Use flush queue capability") the core
> > > code only passes in IDENTITY/BLOCKED/UNMANAGED/DMA domain types. It will
> > > not pass in IDENTITY or BLOCKED if the global statics exist, so the test
> > > for DMA is also redundant now too.
> > >
> > > Call arm_smmu_init_domain_context() early if a dev is available.
> > >
> > > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> > > ---
> > >  drivers/iommu/arm/arm-smmu/arm-smmu.c | 21 +++++++++++++++------
> > >  1 file changed, 15 insertions(+), 6 deletions(-)
> >
> > For some reason this patch breaks booting of the APQ8096 Dragonboard820c
> > (qcom/apq8096-db820c.dts). Dispbling display subsystem (mdss) and venus
> > devices makes the board boot in most of the cases. Most frequently the
> > last parts of the log loog in a following way:
>
> It is surprising we tested this patch on some tegra systems with this
> iommu and didn't hit anything..
>
> The only real functional thing this changes is to move the domain
> initialization up in time, potentially a lot in time in some
> cases. That function does alot of things including touching HW so
> possibly there is some surprising interaction with something else.
>
> So, I would expect this to not WARN_ON and to make it work the same as
> before the patch:
>
> --- a/drivers/iommu/arm/arm-smmu/arm-smmu.c
> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu.c
> @@ -875,7 +875,9 @@ static struct iommu_domain *arm_smmu_domain_alloc_paging(struct device *dev)
>         mutex_init(&smmu_domain->init_mutex);
>         spin_lock_init(&smmu_domain->cb_lock);
>
> -       if (dev) {
> +       WARN_ON(using_legacy_binding);
> +
> +/*     if (dev) {
>                 struct arm_smmu_master_cfg *cfg = dev_iommu_priv_get(dev);
>
>                 if (arm_smmu_init_domain_context(smmu_domain, cfg->smmu, dev)) {
> @@ -883,7 +885,7 @@ static struct iommu_domain *arm_smmu_domain_alloc_paging(struct device *dev)
>                         return NULL;
>                 }
>         }
> -
> +*/
>         return &smmu_domain->domain;
>  }

With the full device tree, this crashed during the IOMMU probe, no warnings:

[   15.343477] bam-dma-engine 644000.dma-controller: num-channels
unspecified in dt
[   15.343675] bam-dma-engine 644000.dma-controller: num-ees unspecified in dt
[   15.394653] msm_serial 7570000.serial: msm_serial: detected port #2
[   15.395170] msm_serial 7570000.serial: uartclk = 19200000
[   15.401983] 7570000.serial: ttyMSM2 at MMIO 0x7570000 (irq = 37,
base_baud = 1200000) is a MSM
[   15.407549] serial serial0: tty port ttyMSM2 registered
[   15.421567] Bluetooth: hci0: setting up ROME/QCA6390
[   15.421728] arm-smmu b40000.iommu: probing hardware configuration...
[   15.425866] arm-smmu b40000.iommu: SMMUv2 with:
[   15.432153] arm-smmu b40000.iommu: stage 1 translation
[   15.436407] arm-smmu b40000.iommu: address translation ops
[   15.441580] arm-smmu b40000.iommu: non-coherent table walk
[   15.447136] arm-smmu b40000.iommu: (IDR0.CTTW overridden by FW configuration)
[   15.452750] arm-smmu b40000.iommu: stream matching with 2 register groups
[   15.460038] arm-smmu b40000.iommu: 2 context banks (0 stage-2 only)
[   15.466908] arm-smmu b40000.iommu: Supported page sizes: 0x63315000
[   15.473367] arm-smmu b40000.iommu: Stage-1: 48-bit VA -> 36-bit IPA
[   15.481638] arm-smmu b40000.iommu: preserved 0 boot mappings
[   15.491783] arm-smmu d00000.iommu: probing hardware configuration...
[   15.491955] arm-smmu d00000.iommu: SMMUv2 with:
[
Format: Log Type - Time(microsec) - Message - Optional Info
Log Type: B - Since Boot(Power On Reset),  D - Delta,  S - Statistic
S - QC_IMAGE_VERSION_STRING=BOOT.XF.1.0-00301


>
> Then I'd ask you to remove the comment and do:
>
> @@ -878,7 +878,9 @@ static struct iommu_domain *arm_smmu_domain_alloc_paging(struct device *dev)
>         if (dev) {
>                 struct arm_smmu_master_cfg *cfg = dev_iommu_priv_get(dev);
>
> +               WARN_ON(true);
>                 if (arm_smmu_init_domain_context(smmu_domain, cfg->smmu, dev)) {
> +                       printk("Allocation failure in arm_smmu_domain_alloc_paging()\n");
>                         kfree(smmu_domain);
>                         return NULL;
>                 }
>
>
> And then we may get a clue from the backtraces it generates. I only
> saw one iommu group reported in your log so I'd expect one trace?

With the full device tree, same result:

[   12.319303] bam-dma-engine 644000.dma-controller: num-channels
unspecified in dt
[   12.319502] bam-dma-engine 644000.dma-controller: num-ees unspecified in dt
[   12.370379] msm_serial 7570000.serial: msm_serial: detected port #2
[   12.370895] msm_serial 7570000.serial: uartclk = 19200000
[   12.377773] 7570000.serial: ttyMSM2 at MMIO 0x7570000 (irq = 37,
base_baud = 1200000) is a MSM
[   12.383228] serial serial0: tty port ttyMSM2 registered
[   12.397263] arm-smmu b40000.iommu: probing hardware configuration...
[   12.397441] arm-smmu b40000.iommu: SMMUv2 with:
[   12.398072] Bluetooth: hci0: setting up ROME/QCA6390
[   12.402779] arm-smmu b40000.iommu: stage 1 translation
[   12.402817] arm-smmu b40000.iommu: address translation ops
[   12.402832] arm-smmu b40000.iommu: non-coherent table walk
[   12.402848] arm-smmu b40000.iommu: (IDR0.CTTW overridden by FW configuration)
[   12.402881] arm-smmu b40000.iommu: stream matching with 2 register groups
[   12.402943] arm-smmu b40000.iommu: 2 context banks (0 stage-2 only)
[   12.402987] arm-smmu b40000.iommu: Supported page sizes: 0x63315000
[   12.403004] arm-smmu b40000.iommu: Stage-1: 48-bit VA -> 36-bit IPA
[   12.404941] arm-smmu b40000.iommu: preserved 0 boot mappings
[   12.467015] arm-smmu d00000.iommu: probing hardware configuration...
[   12.467318] arm-smmu d0
Format: Log Type - Time(microsec) - Message - Optional Info
Log Type: B - Since Boot(Power On Reset),  D - Delta,  S - Statistic
S - QC_IMAGE_VERSION_STRING=BOOT.XF.1.0-00301

Even with disabling display-subsystem and venus, it crashes at the
same place, I don't seem to be able to get past it anymore.

-- 
With best wishes
Dmitry

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 5/5] iommu/arm-smmu: Convert to domain_alloc_paging()

[Jason Gunthorpe]

On Tue, Feb 13, 2024 at 01:18:24AM +0200, Dmitry Baryshkov wrote:
> > > For some reason this patch breaks booting of the APQ8096 Dragonboard820c
> > > (qcom/apq8096-db820c.dts). Dispbling display subsystem (mdss) and venus
> > > devices makes the board boot in most of the cases. Most frequently the
> > > last parts of the log loog in a following way:
> >
> > It is surprising we tested this patch on some tegra systems with this
> > iommu and didn't hit anything..
> >
> > The only real functional thing this changes is to move the domain
> > initialization up in time, potentially a lot in time in some
> > cases. That function does alot of things including touching HW so
> > possibly there is some surprising interaction with something else.
> >
> > So, I would expect this to not WARN_ON and to make it work the same as
> > before the patch:
> >
> > --- a/drivers/iommu/arm/arm-smmu/arm-smmu.c
> > +++ b/drivers/iommu/arm/arm-smmu/arm-smmu.c
> > @@ -875,7 +875,9 @@ static struct iommu_domain *arm_smmu_domain_alloc_paging(struct device *dev)
> >         mutex_init(&smmu_domain->init_mutex);
> >         spin_lock_init(&smmu_domain->cb_lock);
> >
> > -       if (dev) {
> > +       WARN_ON(using_legacy_binding);
> > +
> > +/*     if (dev) {
> >                 struct arm_smmu_master_cfg *cfg = dev_iommu_priv_get(dev);
> >
> >                 if (arm_smmu_init_domain_context(smmu_domain, cfg->smmu, dev)) {
> > @@ -883,7 +885,7 @@ static struct iommu_domain *arm_smmu_domain_alloc_paging(struct device *dev)
> >                         return NULL;
> >                 }
> >         }
> > -
> > +*/
> >         return &smmu_domain->domain;
> >  }
> 
> With the full device tree, this crashed during the IOMMU probe, no warnings:

The above reverts nearly all the functional elements of the patch you
said caused the problem, are you certain of your bisection?

> > And then we may get a clue from the backtraces it generates. I only
> > saw one iommu group reported in your log so I'd expect one trace?
> 
> With the full device tree, same result:

This adds basically an unconditional WARN_ON on all the probe paths,
and nothing printed? That is even more surprising.

Those two together suggest that arm_smmu_domain_alloc_paging() isn't
even being called. But the caller is:

	if (alloc_type == IOMMU_DOMAIN_IDENTITY && ops->identity_domain)
		return ops->identity_domain;
	else if (alloc_type == IOMMU_DOMAIN_BLOCKED && ops->blocked_domain)
		return ops->blocked_domain;
	else if (type & __IOMMU_DOMAIN_PAGING && ops->domain_alloc_paging)
		domain = ops->domain_alloc_paging(dev);
	else if (ops->domain_alloc)
		domain = ops->domain_alloc(alloc_type);
	else
		return ERR_PTR(-EOPNOTSUPP);

Which, suggest that alloc_type is somehow garbage for your system? I
don't see how that can happen, but this patch will also cause a
garbage type to be rejected by the core code.

Does this reveal anything for you?

@@ -2118,6 +2118,7 @@ static struct iommu_domain *__iommu_domain_alloc(const struct iommu_ops *ops,
        struct iommu_domain *domain;
        unsigned int alloc_type = type & IOMMU_DOMAIN_ALLOC_FLAGS;
 
+       WARN(true, " __iommu_domain_alloc %u %u", alloc_type, type);
        if (alloc_type == IOMMU_DOMAIN_IDENTITY && ops->identity_domain)
                return ops->identity_domain;
        else if (alloc_type == IOMMU_DOMAIN_BLOCKED && ops->blocked_domain)
@@ -2126,8 +2127,10 @@ static struct iommu_domain *__iommu_domain_alloc(const struct iommu_ops *ops,
                domain = ops->domain_alloc_paging(dev);
        else if (ops->domain_alloc)
                domain = ops->domain_alloc(alloc_type);
-       else
+       else {
+               printk("Returning failure from __iommu_domain_alloc()\n");
                return ERR_PTR(-EOPNOTSUPP);
+       }
 
        /*
         * Many domain_alloc ops now return ERR_PTR, make things easier for the

It must print, something is wrong with the debugging process if this
doesn't generate back traces :\

Jason

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 5/5] iommu/arm-smmu: Convert to domain_alloc_paging()

[Dmitry Baryshkov]

On Sat, 10 Feb 2024 at 00:23, Jason Gunthorpe <jgg@nvidia.com> wrote:
>
> On Fri, Feb 09, 2024 at 10:05:38PM +0200, Dmitry Baryshkov wrote:
> > On Tue, 17 Oct 2023 Jason Gunthorpe <jgg@nvidia.com> wrote:
> > > Now that the BLOCKED and IDENTITY behaviors are managed with their own
> > > domains change to the domain_alloc_paging() op.
> > >
> > > The check for using_legacy_binding is now redundant,
> > > arm_smmu_def_domain_type() always returns IOMMU_DOMAIN_IDENTITY for this
> > > mode, so the core code will never attempt to create a DMA domain in the
> > > first place.
> > >
> > > Since commit a4fdd9762272 ("iommu: Use flush queue capability") the core
> > > code only passes in IDENTITY/BLOCKED/UNMANAGED/DMA domain types. It will
> > > not pass in IDENTITY or BLOCKED if the global statics exist, so the test
> > > for DMA is also redundant now too.
> > >
> > > Call arm_smmu_init_domain_context() early if a dev is available.
> > >
> > > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> > > ---
> > >  drivers/iommu/arm/arm-smmu/arm-smmu.c | 21 +++++++++++++++------
> > >  1 file changed, 15 insertions(+), 6 deletions(-)
> >
> > For some reason this patch breaks booting of the APQ8096 Dragonboard820c
> > (qcom/apq8096-db820c.dts). Dispbling display subsystem (mdss) and venus
> > devices makes the board boot in most of the cases. Most frequently the
> > last parts of the log loog in a following way:
>
> It is surprising we tested this patch on some tegra systems with this
> iommu and didn't hit anything..
>
> The only real functional thing this changes is to move the domain
> initialization up in time, potentially a lot in time in some
> cases. That function does alot of things including touching HW so
> possibly there is some surprising interaction with something else.

I should not be debugging strange platforms at 1 a.m. I forgot that
there was another patch to revert. So after reverting the MPM patch,
I'm getting the following results:

>
> So, I would expect this to not WARN_ON and to make it work the same as
> before the patch:

No warnings, the platform now boots up to the point of actually
bringing up the venus device:


[   11.906514] ath10k_pci 0000:01:00.0: qca6174 hw3.2 target
0x05030000 chip_id 0x00340aff sub 0000:0000
[   11.907119] ath10k_pci 0000:01:00.0: kconfig debug 1 debugfs 0
tracing 0 dfs 0 testmode 0
[   11.915881] ath10k_pci 0000:01:00.0: firmware ver
WLAN.RM.4.4.1-00288- api 6 features wowlan,ignore-otp,mfp crc32
bf907c7c
[   11.979972] Console: switching to colour frame buffer device 320x90
[   11.990756] ath10k_pci 0000:01:00.0: board_file api 2 bmi_id 0:1
crc32 d2863f91
[   12.060834] msm_mdp 901000.display-controller: [drm] fb0: msmdrmfb
frame buffer device
[   12.096203] qcom-pcie 608000.pcie: Phy link never came up
[   12.103785] qcom-pcie 608000.pcie: PCI host bridge to bus 0001:00
[   12.103970] qcom-venus c00000.video-codec: Adding to iommu group 3

Format: Log Type - Time(microsec) - Message - Optional Info
Log Type: B - Since Boot(Power On Reset),  D - Delta,  S - Statistic
S - QC_IMAGE_VERSION_STRING=BOOT.XF.1.0-00301
S - IMAGE_VARIANT_STRING=M8996LAB
S - OEM_IMAGE_VERSION_STRING=crm-ubuntu68
S - Boot Interface: UFS

>
> Then I'd ask you to remove the comment and do:
>
> @@ -878,7 +878,9 @@ static struct iommu_domain *arm_smmu_domain_alloc_paging(struct device *dev)
>         if (dev) {
>                 struct arm_smmu_master_cfg *cfg = dev_iommu_priv_get(dev);
>
> +               WARN_ON(true);
>                 if (arm_smmu_init_domain_context(smmu_domain, cfg->smmu, dev)) {
> +                       printk("Allocation failure in arm_smmu_domain_alloc_paging()\n");
>                         kfree(smmu_domain);
>                         return NULL;
>                 }
>
>
> And then we may get a clue from the backtraces it generates. I only
> saw one iommu group reported in your log so I'd expect one trace?

I added dev_info + mdelays() around the arm_smmu_init_domain_context()
and I can see that it crashes within that function.

[   29.819624] qcom-venus c00000.video-codec: Adding to iommu group 1
[   29.833181] ------------[ cut here ]------------
[   29.839198] WARNING: CPU: 1 PID: 35 at
drivers/iommu/arm/arm-smmu/arm-smmu.c:883
arm_smmu_domain_alloc_paging+0x80/0x174
[   29.843980] Modules linked in:
[   29.854824] CPU: 1 PID: 35 Comm: kworker/u18:0 Tainted: G     U
        6.8.0-rc3-next-20240208-05495-g20708c29957d-dirty #1739
[   29.857694] Hardware name: Qualcomm Technologies, Inc. DB820c (DT)
[   29.869410] Workqueue: events_unbound deferred_probe_work_func
[   29.875658] pstate: 40000005 (nZcv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
[   29.881474] pc : arm_smmu_domain_alloc_paging+0x80/0x174
[   29.888331] lr : arm_smmu_domain_alloc_paging+0x68/0x174
[   29.893885] sp : ffff8000830338c0
[   29.899179] x29: ffff8000830338c0 x28: 0000000000000000 x27: ffff800081e72000
[   29.902396] x26: ffff00008034ee48 x25: ffff000080b24810 x24: 0000000000000000
[   29.909513] x23: ffff800081e73000 x22: ffff000080b24810 x21: ffff800082e23258
[   29.916633] x20: ffff00008389a700 x19: ffff00008034f600 x18: ffffffffffffffff
[   29.918788] usb 1-1: new high-speed USB device number 2 using xhci-hcd
[   29.923746] x17: 0000000c0000000b x16: 0000000900000008 x15: 0000000000000000
[   29.923765] x14: 000000000000b0af x13: 0000000000000000 x12: 0000000000000166
[   29.923783] x11: 0000000000000001 x10: 0000000000001410 x9 : 0000000000000000
[   29.923801] x8 : ffff00008034f800 x7 : 0000000000000000 x6 : 0000000000000000
[   29.923819] x5 : 0000000000000000 x4 : 0000000000000002 x3 : 0000000000000000
[   29.923837] x2 : ffff800082e23290 x1 : dead4ead00000000 x0 : 0000000000000000
[   29.923855] Call trace:
[   29.923861]  arm_smmu_domain_alloc_paging+0x80/0x174
[   29.923872]  __iommu_domain_alloc+0xcc/0xf4
[   29.923884]  iommu_setup_default_domain+0x294/0x554
[   29.938567] Bluetooth: hci0: Frame reassembly failed (-84)
[   29.944494]  __iommu_probe_device+0x418/0x43c
[   29.944508]  iommu_probe_device+0x3c/0x80
[   29.944519]  of_iommu_configure+0x124/0x1b4
[   29.944529]  of_dma_configure_id+0x170/0x2f4
[   29.969874] mmc0: new ultra high speed SDR104 SDHC card at address 5048
[   29.972966]  platform_dma_configure+0xa8/0xb4
[   29.972983]  really_probe+0x70/0x2ac
[   29.972992]  __driver_probe_device+0x78/0x12c
[   29.973001]  driver_probe_device+0xd8/0x160
[   29.973010]  __device_attach_driver+0xb8/0x138
[   29.973019]  bus_for_each_drv+0x80/0xdc
[   29.973027]  __device_attach+0x9c/0x188
[   29.973037]  device_initial_probe+0x14/0x20
[   29.973046]  bus_probe_device+0xac/0xb0
[   29.973055]  deferred_probe_work_func+0x8c/0xc8
[   29.973064]  process_one_work+0x210/0x5e4
[   29.983596] mmcblk0: mmc0:5048 SD32G 28.8 GiB
[   29.987546]  worker_thread+0x1bc/0x38c
[   29.987558]  kthread+0x120/0x124
[   29.987568]  ret_from_fork+0x10/0x20
[   29.987579] irq event stamp: 109977
[   29.987584] hardirqs last  enabled at (109977):
[<ffff800080fbbc48>] _raw_spin_unlock_irqrestore+0x6c/0x70
[   29.987600] hardirqs last disabled at (109976):
[<ffff800080fbb0a8>] _raw_spin_lock_irqsave+0x84/0x88
[   29.987610] softirqs last  enabled at (109966):
[<ffff800080090680>] __do_softirq+0x498/0x4e0
[   29.987619] softirqs last disabled at (109961):
[<ffff800080096184>] ____do_softirq+0x10/0x1c
[   30.006747]  mmcblk0: p1
[   30.010291] ---[ end trace 0000000000000000 ]---
[   30.018630] remoteproc remoteproc1: remote processor
9300000.remoteproc is now up
[   30.024525] qcom-pcie 600000.pcie: iATU: unroll F, 32 ob, 8 ib,
align 4K, limit 4G
[   30.044747] qcom,apr remoteproc1:smd-edge.apr_audio_svc.-1.-1:
Adding APR/GPR dev: aprsvc:service:4:3
[   30.046118] qcom-pcie 600000.pcie: Invalid eDMA IRQs found
[   30.051718] qcom,apr remoteproc1:smd-edge.apr_audio_svc.-1.-1:
Adding APR/GPR dev: aprsvc:service:4:4
[   30.066435] Bluetooth: hci0: QCA Downloading qca/nvm_00440302.bin
[   30.130736] hub 1-1:1.0: USB hub found
[   30.150390] qcom-pcie 600000.pcie: PCIe Gen.1 x1 link up
[   30.156394] hub 1-1:1.0: 4 ports detected
[   30.161837] qcom-pcie 600000.pcie: PCI host bridge to bus 0000:00
[   30.189583] pci_bus 0000:00: root bus resource [bus 00-ff]
[   30.195652] pci_bus 0000:00: root bus resource [io  0x0000-0xfffff]
[   30.201035] pci_bus 0000:00: root bus resource [mem 0x0c300000-0x0cffffff]
[   30.205424] Bluetooth: hci0: QCA setup on UART is completed
[   30.207262] pci 0000:00:00.0: [17cb:0104] type 01 class 0x060400
PCIe Root Port
[   30.214380] usb 2-1: new SuperSpeed USB device number 2 using xhci-hcd
[   30.219636] qcom-venus c00000.video-codec: Allocating domain
[   30.221503] pci 0000:00:00.0: BAR 0 [mem 0x00000000-0x00000fff]
[   30.221680] pci 0000:00:00.0: PCI bridge to [bus 01-ff]
[   30.221772] pci 0000:00:00.0:   bridge window [io  0x0000-0x0fff]
[   30.221832] pci 0000:00:00.0:   bridge window [mem 0x00000000-0x000fffff]
[   30.221945] pci 0000:00:00.0:   bridge window [mem
0x00000000-0x000fffff 64bit pref]
[   30.222617] pci 0000:00:00.0: PME# supported from D0 D3hot
[   30.273673] hub 2-1:1.0: USB hub found
[   30.276567] hub 2-1:1.0: 4 ports detected

Format: Log Type - Time(microsec) - Message - Optional Info
Log Type: B - Since Boot(Power On Reset),  D - Delta,  S - Statistic
S - QC_IMAGE_VERSION_STRING=BOOT.XF.1.0-00301
S - IMAGE_VARIANT_STRING=M8996LAB
S - OEM_IMAGE_VERSION_STRING=crm-ubuntu68
S - Boot Interface: UFS

I traced this further, it crashes during arm_smmu_write_context_bank().



--
With best wishes
Dmitry

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 5/5] iommu/arm-smmu: Convert to domain_alloc_paging()

[Robin Murphy]

On 2024-02-13 7:51 am, Dmitry Baryshkov wrote:
> On Sat, 10 Feb 2024 at 00:23, Jason Gunthorpe <jgg@nvidia.com> wrote:
>>
>> On Fri, Feb 09, 2024 at 10:05:38PM +0200, Dmitry Baryshkov wrote:
>>> On Tue, 17 Oct 2023 Jason Gunthorpe <jgg@nvidia.com> wrote:
>>>> Now that the BLOCKED and IDENTITY behaviors are managed with their own
>>>> domains change to the domain_alloc_paging() op.
>>>>
>>>> The check for using_legacy_binding is now redundant,
>>>> arm_smmu_def_domain_type() always returns IOMMU_DOMAIN_IDENTITY for this
>>>> mode, so the core code will never attempt to create a DMA domain in the
>>>> first place.
>>>>
>>>> Since commit a4fdd9762272 ("iommu: Use flush queue capability") the core
>>>> code only passes in IDENTITY/BLOCKED/UNMANAGED/DMA domain types. It will
>>>> not pass in IDENTITY or BLOCKED if the global statics exist, so the test
>>>> for DMA is also redundant now too.
>>>>
>>>> Call arm_smmu_init_domain_context() early if a dev is available.
>>>>
>>>> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
>>>> ---
>>>>   drivers/iommu/arm/arm-smmu/arm-smmu.c | 21 +++++++++++++++------
>>>>   1 file changed, 15 insertions(+), 6 deletions(-)
>>>
>>> For some reason this patch breaks booting of the APQ8096 Dragonboard820c
>>> (qcom/apq8096-db820c.dts). Dispbling display subsystem (mdss) and venus
>>> devices makes the board boot in most of the cases. Most frequently the
>>> last parts of the log loog in a following way:
>>
>> It is surprising we tested this patch on some tegra systems with this
>> iommu and didn't hit anything..
>>
>> The only real functional thing this changes is to move the domain
>> initialization up in time, potentially a lot in time in some
>> cases. That function does alot of things including touching HW so
>> possibly there is some surprising interaction with something else.
> 
> I should not be debugging strange platforms at 1 a.m. I forgot that
> there was another patch to revert. So after reverting the MPM patch,
> I'm getting the following results:
> 
>>
>> So, I would expect this to not WARN_ON and to make it work the same as
>> before the patch:
> 
> No warnings, the platform now boots up to the point of actually
> bringing up the venus device:
> 
> 
> [   11.906514] ath10k_pci 0000:01:00.0: qca6174 hw3.2 target
> 0x05030000 chip_id 0x00340aff sub 0000:0000
> [   11.907119] ath10k_pci 0000:01:00.0: kconfig debug 1 debugfs 0
> tracing 0 dfs 0 testmode 0
> [   11.915881] ath10k_pci 0000:01:00.0: firmware ver
> WLAN.RM.4.4.1-00288- api 6 features wowlan,ignore-otp,mfp crc32
> bf907c7c
> [   11.979972] Console: switching to colour frame buffer device 320x90
> [   11.990756] ath10k_pci 0000:01:00.0: board_file api 2 bmi_id 0:1
> crc32 d2863f91
> [   12.060834] msm_mdp 901000.display-controller: [drm] fb0: msmdrmfb
> frame buffer device
> [   12.096203] qcom-pcie 608000.pcie: Phy link never came up
> [   12.103785] qcom-pcie 608000.pcie: PCI host bridge to bus 0001:00
> [   12.103970] qcom-venus c00000.video-codec: Adding to iommu group 3
> 
> Format: Log Type - Time(microsec) - Message - Optional Info
> Log Type: B - Since Boot(Power On Reset),  D - Delta,  S - Statistic
> S - QC_IMAGE_VERSION_STRING=BOOT.XF.1.0-00301
> S - IMAGE_VARIANT_STRING=M8996LAB
> S - OEM_IMAGE_VERSION_STRING=crm-ubuntu68
> S - Boot Interface: UFS
> 
>>
>> Then I'd ask you to remove the comment and do:
>>
>> @@ -878,7 +878,9 @@ static struct iommu_domain *arm_smmu_domain_alloc_paging(struct device *dev)
>>          if (dev) {
>>                  struct arm_smmu_master_cfg *cfg = dev_iommu_priv_get(dev);
>>
>> +               WARN_ON(true);
>>                  if (arm_smmu_init_domain_context(smmu_domain, cfg->smmu, dev)) {
>> +                       printk("Allocation failure in arm_smmu_domain_alloc_paging()\n");
>>                          kfree(smmu_domain);
>>                          return NULL;
>>                  }
>>
>>
>> And then we may get a clue from the backtraces it generates. I only
>> saw one iommu group reported in your log so I'd expect one trace?
> 
> I added dev_info + mdelays() around the arm_smmu_init_domain_context()
> and I can see that it crashes within that function.

Yeah, this is totally broken. We can't just call the unmodified 
arm_smmu_init_domain_context() at domain allocation because half of what 
it's doing belongs to the attach operation. We should not be allocating 
context banks, IRQs, etc. for a not-yet-attached domain, and we 
certainly shouldn't be touching hardware there outside of RPM.

Thanks,
Robin.

> 
> [   29.819624] qcom-venus c00000.video-codec: Adding to iommu group 1
> [   29.833181] ------------[ cut here ]------------
> [   29.839198] WARNING: CPU: 1 PID: 35 at
> drivers/iommu/arm/arm-smmu/arm-smmu.c:883
> arm_smmu_domain_alloc_paging+0x80/0x174
> [   29.843980] Modules linked in:
> [   29.854824] CPU: 1 PID: 35 Comm: kworker/u18:0 Tainted: G     U
>          6.8.0-rc3-next-20240208-05495-g20708c29957d-dirty #1739
> [   29.857694] Hardware name: Qualcomm Technologies, Inc. DB820c (DT)
> [   29.869410] Workqueue: events_unbound deferred_probe_work_func
> [   29.875658] pstate: 40000005 (nZcv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
> [   29.881474] pc : arm_smmu_domain_alloc_paging+0x80/0x174
> [   29.888331] lr : arm_smmu_domain_alloc_paging+0x68/0x174
> [   29.893885] sp : ffff8000830338c0
> [   29.899179] x29: ffff8000830338c0 x28: 0000000000000000 x27: ffff800081e72000
> [   29.902396] x26: ffff00008034ee48 x25: ffff000080b24810 x24: 0000000000000000
> [   29.909513] x23: ffff800081e73000 x22: ffff000080b24810 x21: ffff800082e23258
> [   29.916633] x20: ffff00008389a700 x19: ffff00008034f600 x18: ffffffffffffffff
> [   29.918788] usb 1-1: new high-speed USB device number 2 using xhci-hcd
> [   29.923746] x17: 0000000c0000000b x16: 0000000900000008 x15: 0000000000000000
> [   29.923765] x14: 000000000000b0af x13: 0000000000000000 x12: 0000000000000166
> [   29.923783] x11: 0000000000000001 x10: 0000000000001410 x9 : 0000000000000000
> [   29.923801] x8 : ffff00008034f800 x7 : 0000000000000000 x6 : 0000000000000000
> [   29.923819] x5 : 0000000000000000 x4 : 0000000000000002 x3 : 0000000000000000
> [   29.923837] x2 : ffff800082e23290 x1 : dead4ead00000000 x0 : 0000000000000000
> [   29.923855] Call trace:
> [   29.923861]  arm_smmu_domain_alloc_paging+0x80/0x174
> [   29.923872]  __iommu_domain_alloc+0xcc/0xf4
> [   29.923884]  iommu_setup_default_domain+0x294/0x554
> [   29.938567] Bluetooth: hci0: Frame reassembly failed (-84)
> [   29.944494]  __iommu_probe_device+0x418/0x43c
> [   29.944508]  iommu_probe_device+0x3c/0x80
> [   29.944519]  of_iommu_configure+0x124/0x1b4
> [   29.944529]  of_dma_configure_id+0x170/0x2f4
> [   29.969874] mmc0: new ultra high speed SDR104 SDHC card at address 5048
> [   29.972966]  platform_dma_configure+0xa8/0xb4
> [   29.972983]  really_probe+0x70/0x2ac
> [   29.972992]  __driver_probe_device+0x78/0x12c
> [   29.973001]  driver_probe_device+0xd8/0x160
> [   29.973010]  __device_attach_driver+0xb8/0x138
> [   29.973019]  bus_for_each_drv+0x80/0xdc
> [   29.973027]  __device_attach+0x9c/0x188
> [   29.973037]  device_initial_probe+0x14/0x20
> [   29.973046]  bus_probe_device+0xac/0xb0
> [   29.973055]  deferred_probe_work_func+0x8c/0xc8
> [   29.973064]  process_one_work+0x210/0x5e4
> [   29.983596] mmcblk0: mmc0:5048 SD32G 28.8 GiB
> [   29.987546]  worker_thread+0x1bc/0x38c
> [   29.987558]  kthread+0x120/0x124
> [   29.987568]  ret_from_fork+0x10/0x20
> [   29.987579] irq event stamp: 109977
> [   29.987584] hardirqs last  enabled at (109977):
> [<ffff800080fbbc48>] _raw_spin_unlock_irqrestore+0x6c/0x70
> [   29.987600] hardirqs last disabled at (109976):
> [<ffff800080fbb0a8>] _raw_spin_lock_irqsave+0x84/0x88
> [   29.987610] softirqs last  enabled at (109966):
> [<ffff800080090680>] __do_softirq+0x498/0x4e0
> [   29.987619] softirqs last disabled at (109961):
> [<ffff800080096184>] ____do_softirq+0x10/0x1c
> [   30.006747]  mmcblk0: p1
> [   30.010291] ---[ end trace 0000000000000000 ]---
> [   30.018630] remoteproc remoteproc1: remote processor
> 9300000.remoteproc is now up
> [   30.024525] qcom-pcie 600000.pcie: iATU: unroll F, 32 ob, 8 ib,
> align 4K, limit 4G
> [   30.044747] qcom,apr remoteproc1:smd-edge.apr_audio_svc.-1.-1:
> Adding APR/GPR dev: aprsvc:service:4:3
> [   30.046118] qcom-pcie 600000.pcie: Invalid eDMA IRQs found
> [   30.051718] qcom,apr remoteproc1:smd-edge.apr_audio_svc.-1.-1:
> Adding APR/GPR dev: aprsvc:service:4:4
> [   30.066435] Bluetooth: hci0: QCA Downloading qca/nvm_00440302.bin
> [   30.130736] hub 1-1:1.0: USB hub found
> [   30.150390] qcom-pcie 600000.pcie: PCIe Gen.1 x1 link up
> [   30.156394] hub 1-1:1.0: 4 ports detected
> [   30.161837] qcom-pcie 600000.pcie: PCI host bridge to bus 0000:00
> [   30.189583] pci_bus 0000:00: root bus resource [bus 00-ff]
> [   30.195652] pci_bus 0000:00: root bus resource [io  0x0000-0xfffff]
> [   30.201035] pci_bus 0000:00: root bus resource [mem 0x0c300000-0x0cffffff]
> [   30.205424] Bluetooth: hci0: QCA setup on UART is completed
> [   30.207262] pci 0000:00:00.0: [17cb:0104] type 01 class 0x060400
> PCIe Root Port
> [   30.214380] usb 2-1: new SuperSpeed USB device number 2 using xhci-hcd
> [   30.219636] qcom-venus c00000.video-codec: Allocating domain
> [   30.221503] pci 0000:00:00.0: BAR 0 [mem 0x00000000-0x00000fff]
> [   30.221680] pci 0000:00:00.0: PCI bridge to [bus 01-ff]
> [   30.221772] pci 0000:00:00.0:   bridge window [io  0x0000-0x0fff]
> [   30.221832] pci 0000:00:00.0:   bridge window [mem 0x00000000-0x000fffff]
> [   30.221945] pci 0000:00:00.0:   bridge window [mem
> 0x00000000-0x000fffff 64bit pref]
> [   30.222617] pci 0000:00:00.0: PME# supported from D0 D3hot
> [   30.273673] hub 2-1:1.0: USB hub found
> [   30.276567] hub 2-1:1.0: 4 ports detected
> 
> Format: Log Type - Time(microsec) - Message - Optional Info
> Log Type: B - Since Boot(Power On Reset),  D - Delta,  S - Statistic
> S - QC_IMAGE_VERSION_STRING=BOOT.XF.1.0-00301
> S - IMAGE_VARIANT_STRING=M8996LAB
> S - OEM_IMAGE_VERSION_STRING=crm-ubuntu68
> S - Boot Interface: UFS
> 
> I traced this further, it crashes during arm_smmu_write_context_bank().
> 
> 
> 
> --
> With best wishes
> Dmitry

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 5/5] iommu/arm-smmu: Convert to domain_alloc_paging()

[Dmitry Baryshkov]

On Tue, 13 Feb 2024 at 12:20, Robin Murphy <robin.murphy@arm.com> wrote:
>
> On 2024-02-13 7:51 am, Dmitry Baryshkov wrote:
> > On Sat, 10 Feb 2024 at 00:23, Jason Gunthorpe <jgg@nvidia.com> wrote:
> >>
> >> On Fri, Feb 09, 2024 at 10:05:38PM +0200, Dmitry Baryshkov wrote:
> >>> On Tue, 17 Oct 2023 Jason Gunthorpe <jgg@nvidia.com> wrote:
> >>>> Now that the BLOCKED and IDENTITY behaviors are managed with their own
> >>>> domains change to the domain_alloc_paging() op.
> >>>>
> >>>> The check for using_legacy_binding is now redundant,
> >>>> arm_smmu_def_domain_type() always returns IOMMU_DOMAIN_IDENTITY for this
> >>>> mode, so the core code will never attempt to create a DMA domain in the
> >>>> first place.
> >>>>
> >>>> Since commit a4fdd9762272 ("iommu: Use flush queue capability") the core
> >>>> code only passes in IDENTITY/BLOCKED/UNMANAGED/DMA domain types. It will
> >>>> not pass in IDENTITY or BLOCKED if the global statics exist, so the test
> >>>> for DMA is also redundant now too.
> >>>>
> >>>> Call arm_smmu_init_domain_context() early if a dev is available.
> >>>>
> >>>> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> >>>> ---
> >>>>   drivers/iommu/arm/arm-smmu/arm-smmu.c | 21 +++++++++++++++------
> >>>>   1 file changed, 15 insertions(+), 6 deletions(-)
> >>>
> >>> For some reason this patch breaks booting of the APQ8096 Dragonboard820c
> >>> (qcom/apq8096-db820c.dts). Dispbling display subsystem (mdss) and venus
> >>> devices makes the board boot in most of the cases. Most frequently the
> >>> last parts of the log loog in a following way:
> >>
> >> It is surprising we tested this patch on some tegra systems with this
> >> iommu and didn't hit anything..
> >>
> >> The only real functional thing this changes is to move the domain
> >> initialization up in time, potentially a lot in time in some
> >> cases. That function does alot of things including touching HW so
> >> possibly there is some surprising interaction with something else.
> >
> > I should not be debugging strange platforms at 1 a.m. I forgot that
> > there was another patch to revert. So after reverting the MPM patch,
> > I'm getting the following results:
> >
> >>
> >> So, I would expect this to not WARN_ON and to make it work the same as
> >> before the patch:
> >
> > No warnings, the platform now boots up to the point of actually
> > bringing up the venus device:
> >
> >
> > [   11.906514] ath10k_pci 0000:01:00.0: qca6174 hw3.2 target
> > 0x05030000 chip_id 0x00340aff sub 0000:0000
> > [   11.907119] ath10k_pci 0000:01:00.0: kconfig debug 1 debugfs 0
> > tracing 0 dfs 0 testmode 0
> > [   11.915881] ath10k_pci 0000:01:00.0: firmware ver
> > WLAN.RM.4.4.1-00288- api 6 features wowlan,ignore-otp,mfp crc32
> > bf907c7c
> > [   11.979972] Console: switching to colour frame buffer device 320x90
> > [   11.990756] ath10k_pci 0000:01:00.0: board_file api 2 bmi_id 0:1
> > crc32 d2863f91
> > [   12.060834] msm_mdp 901000.display-controller: [drm] fb0: msmdrmfb
> > frame buffer device
> > [   12.096203] qcom-pcie 608000.pcie: Phy link never came up
> > [   12.103785] qcom-pcie 608000.pcie: PCI host bridge to bus 0001:00
> > [   12.103970] qcom-venus c00000.video-codec: Adding to iommu group 3
> >
> > Format: Log Type - Time(microsec) - Message - Optional Info
> > Log Type: B - Since Boot(Power On Reset),  D - Delta,  S - Statistic
> > S - QC_IMAGE_VERSION_STRING=BOOT.XF.1.0-00301
> > S - IMAGE_VARIANT_STRING=M8996LAB
> > S - OEM_IMAGE_VERSION_STRING=crm-ubuntu68
> > S - Boot Interface: UFS
> >
> >>
> >> Then I'd ask you to remove the comment and do:
> >>
> >> @@ -878,7 +878,9 @@ static struct iommu_domain *arm_smmu_domain_alloc_paging(struct device *dev)
> >>          if (dev) {
> >>                  struct arm_smmu_master_cfg *cfg = dev_iommu_priv_get(dev);
> >>
> >> +               WARN_ON(true);
> >>                  if (arm_smmu_init_domain_context(smmu_domain, cfg->smmu, dev)) {
> >> +                       printk("Allocation failure in arm_smmu_domain_alloc_paging()\n");
> >>                          kfree(smmu_domain);
> >>                          return NULL;
> >>                  }
> >>
> >>
> >> And then we may get a clue from the backtraces it generates. I only
> >> saw one iommu group reported in your log so I'd expect one trace?
> >
> > I added dev_info + mdelays() around the arm_smmu_init_domain_context()
> > and I can see that it crashes within that function.
>
> Yeah, this is totally broken. We can't just call the unmodified
> arm_smmu_init_domain_context() at domain allocation because half of what
> it's doing belongs to the attach operation. We should not be allocating
> context banks, IRQs, etc. for a not-yet-attached domain, and we
> certainly shouldn't be touching hardware there outside of RPM.

Should I send a revert?

>
> Thanks,
> Robin.



-- 
With best wishes
Dmitry

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 5/5] iommu/arm-smmu: Convert to domain_alloc_paging()

[Will Deacon]

On Tue, Feb 13, 2024 at 12:55:38PM +0200, Dmitry Baryshkov wrote:
> On Tue, 13 Feb 2024 at 12:20, Robin Murphy <robin.murphy@arm.com> wrote:
> > On 2024-02-13 7:51 am, Dmitry Baryshkov wrote:
> > > On Sat, 10 Feb 2024 at 00:23, Jason Gunthorpe <jgg@nvidia.com> wrote:
> > >> And then we may get a clue from the backtraces it generates. I only
> > >> saw one iommu group reported in your log so I'd expect one trace?
> > >
> > > I added dev_info + mdelays() around the arm_smmu_init_domain_context()
> > > and I can see that it crashes within that function.
> >
> > Yeah, this is totally broken. We can't just call the unmodified
> > arm_smmu_init_domain_context() at domain allocation because half of what
> > it's doing belongs to the attach operation. We should not be allocating
> > context banks, IRQs, etc. for a not-yet-attached domain, and we
> > certainly shouldn't be touching hardware there outside of RPM.
> 
> Should I send a revert?

If reverting the patch fixes the issue for you, then yes please!

Hopefully you can help Jason test a reworked verson for the future, as
it's evident that Tegra doesn't tickle the power management side of things
in the same way.

Will

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 5/5] iommu/arm-smmu: Convert to domain_alloc_paging()

[Jason Gunthorpe]

On Tue, Feb 13, 2024 at 11:16:23AM +0000, Will Deacon wrote:
> On Tue, Feb 13, 2024 at 12:55:38PM +0200, Dmitry Baryshkov wrote:
> > On Tue, 13 Feb 2024 at 12:20, Robin Murphy <robin.murphy@arm.com> wrote:
> > > On 2024-02-13 7:51 am, Dmitry Baryshkov wrote:
> > > > On Sat, 10 Feb 2024 at 00:23, Jason Gunthorpe <jgg@nvidia.com> wrote:
> > > >> And then we may get a clue from the backtraces it generates. I only
> > > >> saw one iommu group reported in your log so I'd expect one trace?
> > > >
> > > > I added dev_info + mdelays() around the arm_smmu_init_domain_context()
> > > > and I can see that it crashes within that function.
> > >
> > > Yeah, this is totally broken. We can't just call the unmodified
> > > arm_smmu_init_domain_context() at domain allocation because half of what
> > > it's doing belongs to the attach operation. We should not be allocating
> > > context banks, IRQs, etc. for a not-yet-attached domain, and we
> > > certainly shouldn't be touching hardware there outside of RPM.
> > 
> > Should I send a revert?
> 
> If reverting the patch fixes the issue for you, then yes please!

Not the whole thing though, just remove the 'if (dev)' like you
tested, thanks.

If you want I will send it

> Hopefully you can help Jason test a reworked verson for the future, as
> it's evident that Tegra doesn't tickle the power management side of things
> in the same way.

It can stay, as long as it uses the alloc_domain_paging() that is
enough for the core code to move forward.

I included it only because we were able to test it, most of the other
drivers I did not try to move their "finalize".

Jason

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 0/5] Convert SMMU to domain_alloc_paging()

[Will Deacon]

On Tue, 17 Oct 2023 15:11:39 -0300, Jason Gunthorpe wrote:
> Add the global statics for IDENTITY and BLOCKED to the SMMU driver and
> change to use domain_alloc_paging(). This allows SMMU to finalize the
> domain during allocation.
> 
> This relies on the first few patches of the dart conversion series:
> 
> https://lore.kernel.org/r/0-v2-bff223cf6409+282-dart_paging_jgg@nvidia.com
> 
> [...]

Applied to will (for-joerg/arm-smmu/updates), thanks!

[1/5] iommu/arm-smmu: Reorganize arm_smmu_domain_add_master()
      https://git.kernel.org/will/c/ff0f80297413
[2/5] iommu/arm-smmu: Convert to a global static identity domain
      https://git.kernel.org/will/c/22bb7b41476a
[3/5] iommu/arm-smmu: Implement IOMMU_DOMAIN_BLOCKED
      https://git.kernel.org/will/c/bbbf11eea38c
[4/5] iommu/arm-smmu: Pass arm_smmu_domain to internal functions
      https://git.kernel.org/will/c/e0976331ad11
[5/5] iommu/arm-smmu: Convert to domain_alloc_paging()
      https://git.kernel.org/will/c/9b3febc3a3da

Cheers,
-- 
Will

https://fixes.arm64.dev
https://next.arm64.dev
https://will.arm64.dev

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel