Re: [PATCH v3 01/12] iommu/arm-smmu-v3: Put iopf enablement in the domain attach path

[Baolu Lu]

On 3/12/25 00:13, Will Deacon wrote:
> On Fri, Feb 28, 2025 at 05:26:20PM +0800, Lu Baolu wrote:
> > From: Jason Gunthorpe <jgg@xxxxxxxxxx>
> >
> > SMMUv3 co-mingles FEAT_IOPF and FEAT_SVA behaviors so that fault reporting
> > doesn't work unless both are enabled. This is not correct and causes
> > problems for iommufd which does not enable FEAT_SVA for it's fault capable
> > domains.
> >
> > These APIs are both obsolete, update SMMUv3 to use the new method like AMD
> > implements.
> >
> > A driver should enable iopf support when a domain with an iopf_handler is
> > attached, and disable iopf support when the domain is removed.
> >
> > Move the fault support logic to sva domain allocation and to domain
> > attach, refusing to create or attach fault capable domains if the HW
> > doesn't support it.
> >
> > Move all the logic for controlling the iopf queue under
> > arm_smmu_attach_prepare(). Keep track of the number of domains on the
> > master (over all the SSIDs) that require iopf. When the first domain
> > requiring iopf is attached create the iopf queue, when the last domain is
> > detached destroy it.
> >
> > Turn FEAT_IOPF and FEAT_SVA into no ops.
> >
> > Remove the sva_lock, this is all protected by the group mutex.
> >
> > Signed-off-by: Jason Gunthorpe <jgg@xxxxxxxxxx>
> > Signed-off-by: Lu Baolu <baolu.lu@xxxxxxxxxxxxxxx>
> > Tested-by: Zhangfei Gao <zhangfei.gao@xxxxxxxxxx>
> > ---
> >   .../iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c   |  86 +-------------
> >   drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c   | 105 +++++++++++++-----
> >   drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h   |  39 ++-----
> >   3 files changed, 91 insertions(+), 139 deletions(-)
>
> [...]
>
> > @@ -2748,6 +2750,54 @@ to_smmu_domain_devices(struct iommu_domain *domain)
> >   	return NULL;
> >   }
> >   
> > +static int arm_smmu_enable_iopf(struct arm_smmu_master *master,
> > +				struct arm_smmu_master_domain *master_domain)
> > +{
> > +	int ret;
> > +
> > +	iommu_group_mutex_assert(master->dev);
> > +
> > +	if (!IS_ENABLED(CONFIG_ARM_SMMU_V3_SVA))
> > +		return -EOPNOTSUPP;
> > +
> > +	/*
> > +	 * Drivers for devices supporting PRI or stall require iopf others have
> > +	 * device-specific fault handlers and don't need IOPF, so this is not a
> > +	 * failure.
> > +	 */
> > +	if (!master->stall_enabled)
> > +		return 0;
> > +
> > +	/* We're not keeping track of SIDs in fault events */
> > +	if (master->num_streams != 1)
> > +		return -EOPNOTSUPP;
> > +
> > +	if (master->iopf_refcount) {
> > +		master->iopf_refcount++;
> > +		master_domain->using_iopf = true;
> > +		return 0;
> > +	}
> > +
> > +	ret = iopf_queue_add_device(master->smmu->evtq.iopf, master->dev);
> > +	if (ret)
> > +		return ret;
> > +	master->iopf_refcount = 1;
> > +	master_domain->using_iopf = true;
> > +	return 0;
> > +}
> > +
> > +static void arm_smmu_disable_iopf(struct arm_smmu_master *master)
> > +{
> > +	iommu_group_mutex_assert(master->dev);
> > +
> > +	if (!IS_ENABLED(CONFIG_ARM_SMMU_V3_SVA))
> > +		return;
>
> I think it would be a little cleaner to push the '->using_iopf' check
> in here rather than have the callers check it. Then the SVA check above
> makes more sense and I think the enable/disable paths are a bit more
> symmetric.

Yes, sure. I will address it like this,

diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c 
b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
index 4aaf9b7bf862..a519854b3798 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
@@ -2786,13 +2786,17 @@ static int arm_smmu_enable_iopf(struct 
arm_smmu_master *master,
        return 0;
 }

-static void arm_smmu_disable_iopf(struct arm_smmu_master *master)
+static void arm_smmu_disable_iopf(struct arm_smmu_master *master,
+                                 struct arm_smmu_master_domain 
*master_domain)
 {
        iommu_group_mutex_assert(master->dev);

        if (!IS_ENABLED(CONFIG_ARM_SMMU_V3_SVA))
                return;

+       if (!master_domain || !master_domain->using_iopf)
+               return;
+
        master->iopf_refcount--;
        if (master->iopf_refcount == 0)
                iopf_queue_remove_device(master->smmu->evtq.iopf, 
master->dev);
@@ -2823,11 +2827,8 @@ static void arm_smmu_remove_master_domain(struct 
arm_smmu_master *master,
        }
        spin_unlock_irqrestore(&smmu_domain->devices_lock, flags);

-       if (master_domain) {
-               if (master_domain->using_iopf)
-                       arm_smmu_disable_iopf(master);
-               kfree(master_domain);
-       }
+       arm_smmu_disable_iopf(master, master_domain);
+       kfree(master_domain);
 }

 /*
@@ -2945,8 +2946,7 @@ int arm_smmu_attach_prepare(struct 
arm_smmu_attach_state *state,
        return 0;

 err_iopf:
-       if (master_domain && master_domain->using_iopf)
-               arm_smmu_disable_iopf(master);
+       arm_smmu_disable_iopf(master);
 err_free_master_domain:
        kfree(master_domain);
        return ret;

> With that:
>
> Acked-by: Will Deacon <will@xxxxxxxxxx>
>
> Will

Thanks,
baolu