[PATCH v5 06/15] iommu/arm-smmu-v3: Allocate IOPF queue without FEAT_SVA

[Jason Gunthorpe]

> arm_smmu_init_queues() allocates the IOPF queue only for FEAT_SVA together
> with FEAT_STALLS. This queue has no SVA dependency, and FEAT_STALLS alone
> can produce events that need IOPF.

Until iommufd there was no other way to create faults in the first
place. Doesn't matter if the HW supports it.

> 
> On HW with FEAT_STALLS but no FEAT_SVA, a master with the "dma-can-stall"
> device-tree property has master->stall_enabled set at probe. Then also with
> CONFIG_ARM_SMMU_V3_SVA=y, arm_smmu_enable_iopf() sees stall_enabled, calls
> iopf_queue_add_device(NULL, ...) at fault-handling domain attach, which can
> oops on mutex_lock(&queue->lock).

Yeah the guards got mismatched. There should have been a

arm_smmu_iopf_enabled(smmu)
	if (!IS_ENABLED(CONFIG_ARM_SMMU_V3_SVA)) return false
        if (!(smmu->features & ARM_SMMU_FEAT_SVA)) return false

And that is used everywhere. The fact one place used the feat and the
other used the CONFIG is the cause of this crash.

> [ ... 32 lines skipped ... ]
> @@ -3215,9 +3215,6 @@ static int arm_smmu_enable_iopf(struct arm_smmu_master *master,
>  
>  	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
> @@ -3249,9 +3246,6 @@ 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;
> -
>  	if (!master_domain || !master_domain->using_iopf)
>  		return;
>

I think this is fine

> @@ -4667,8 +4661,7 @@ static int arm_smmu_init_queues(struct arm_smmu_device *smmu)
>  	if (ret)
>  		return ret;
>  
> -	if ((smmu->features & ARM_SMMU_FEAT_SVA) &&
> -	    (smmu->features & ARM_SMMU_FEAT_STALLS)) {
> +	if (smmu->features & ARM_SMMU_FEAT_STALLS) {
>  		smmu->evtq.iopf = iopf_queue_alloc(dev_name(smmu->dev));
>  		if (!smmu->evtq.iopf)
>  			return -ENOMEM;

But here it would still be nice to support the non-SVA optimization..

I'd change the kconfig to move the select IOMMU_IOPF down to the
iommufd and SVA blocks

Then guard this with an IS_ENABLED(CONFIG_IOMMU_IOPF)

That way you can choose to compile out IOPF if you don't want to use
any SMMU features that rely on it.

-- 
Jason