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

[Baolu Lu <baolu.lu@linux.intel.com>]

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@nvidia.com>
>>
>> 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@nvidia.com>
>> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
>> Tested-by: Zhangfei Gao <zhangfei.gao@linaro.org>
>> ---
>>   .../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@kernel.org>
> 
> Will

Thanks,
baolu