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

[Jason Gunthorpe <jgg@nvidia.com>]

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