Re: [PATCH v2 07/30] iommu/arm-smmu: implement iova_to_phys_length

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:

New issues:
- [High] The `iova_to_phys_length` API contract is violated because `mapped_length` is not populated during hardware translation or its software fallback.
- [High] `arm_smmu_iova_to_phys_hard` returns `0` instead of `PHYS_ADDR_MAX` on translation failure, masquerading as a successful translation.
- [Low] The error check for software table walk failures in `qcom_smmu_context_fault` is broken, failing to detect `PHYS_ADDR_MAX`.

Pre-existing issues:
- [High] `arm_smmu_rpm_get()` can sleep, causing deadlocks if `iova_to_phys` is called from atomic contexts.
--

commit caa5d047d0fcd8b121e03e1968cc870e79540cdb
Author: Guanghui Feng <guanghuifeng@linux.alibaba.com>

iommu/arm-smmu: implement iova_to_phys_length

Migrate ARM SMMU to implement iova_to_phys_length, calling
ops->iova_to_phys_length on the io-pgtable layer. Update qcom-debug
caller accordingly.

> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom-debug.c b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom-debug.c
> index 65e0ef6539fe7..4fd01341157f3 100644
> --- a/drivers/iommu/arm/arm-smmu/arm-smmu-qcom-debug.c
> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu-qcom-debug.c
> @@ -415,7 +415,7 @@ irqreturn_t qcom_smmu_context_fault(int irq, void *dev)
>  		return IRQ_HANDLED;
>  	}
>  
> -	phys_soft = ops->iova_to_phys(ops, cfi.iova);
> +	phys_soft = ops->iova_to_phys_length(ops, cfi.iova, NULL);

[Severity: Low]
With the new iova_to_phys_length API returning PHYS_ADDR_MAX on failure,
should the error check further down in qcom_smmu_context_fault() be
updated?

It currently checks for zero:

qcom_smmu_context_fault() {
    ...
    phys_soft = ops->iova_to_phys_length(ops, cfi.iova, NULL);
    ...
    if (!phys_soft)
        dev_err(smmu->dev,
            "SOFTWARE TABLE WALK FAILED! ...");
    ...
}

If the software walk fails and returns PHYS_ADDR_MAX, will this check
silently skip the diagnostic error message?

>  
>  	tmp = report_iommu_fault(&smmu_domain->domain, NULL, cfi.iova,
>  				 cfi.fsynr & ARM_SMMU_CB_FSYNR0_WNR ? IOMMU_FAULT_WRITE : IOMMU_FAULT_READ);

[ ... ]

> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu.c b/drivers/iommu/arm/arm-smmu/arm-smmu.c
> index 0bd21d206eb3e..dfbd541f9e3e0 100644
> --- a/drivers/iommu/arm/arm-smmu/arm-smmu.c
> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu.c
> @@ -1366,7 +1366,7 @@ static phys_addr_t arm_smmu_iova_to_phys_hard(struct iommu_domain *domain,
>  			"iova to phys timed out on %pad. Falling back to software table walk.\n",
>  			&iova);
>  		arm_smmu_rpm_put(smmu);
> -		return ops->iova_to_phys(ops, iova);
> +		return ops->iova_to_phys_length(ops, iova, NULL);

[Severity: High]
If we fall back to the software table walk here and pass NULL for the
mapped_length, does this violate the iova_to_phys_length API contract?
Callers might expect the mapped_length to be populated.

[Severity: High]
Is it necessary to update arm_smmu_iova_to_phys_hard() to return
PHYS_ADDR_MAX instead of 0 on failures?

The new iova_to_phys_length API contract requires returning PHYS_ADDR_MAX
on mapping failures. If this continues returning 0 on arm_smmu_rpm_get()
errors or translation faults, callers could incorrectly interpret it as a
valid mapping at physical address 0.

[Severity: High]
This is a pre-existing issue, but it looks like arm_smmu_iova_to_phys_hard()
calls arm_smmu_rpm_get() earlier in this function, which can sleep.

Since iommu_iova_to_phys() is frequently called from atomic contexts like
NAPI fast paths, could this cause a sleeping-in-atomic regression?

>  	}
>  
>  	phys = arm_smmu_cb_readq(smmu, idx, ARM_SMMU_CB_PAR);

[ ... ]

> @@ -1384,20 +1384,23 @@ static phys_addr_t arm_smmu_iova_to_phys_hard(struct iommu_domain *domain,
>  	return addr;
>  }
>  
> -static phys_addr_t arm_smmu_iova_to_phys(struct iommu_domain *domain,
> -					dma_addr_t iova)
> +static phys_addr_t arm_smmu_iova_to_phys_length(struct iommu_domain *domain,
> +					dma_addr_t iova, size_t *mapped_length)
>  {
>  	struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
>  	struct io_pgtable_ops *ops = smmu_domain->pgtbl_ops;
>  
> +	if (mapped_length)
> +		*mapped_length = 0;
> +
>  	if (!ops)
> -		return 0;
> +		return PHYS_ADDR_MAX;
>  
>  	if (smmu_domain->smmu->features & ARM_SMMU_FEAT_TRANS_OPS &&
>  			smmu_domain->stage == ARM_SMMU_DOMAIN_S1)
>  		return arm_smmu_iova_to_phys_hard(domain, iova);

[Severity: High]
When delegating to arm_smmu_iova_to_phys_hard(), the mapped_length pointer
is not passed and remains 0. Will callers that use this length to advance
their IOVA iterators enter an infinite loop when the size remains 0 after
a successful hardware translation?

>  
> -	return ops->iova_to_phys(ops, iova);
> +	return ops->iova_to_phys_length(ops, iova, mapped_length);
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260602104637.1219810-1-guanghuifeng@linux.alibaba.com?part=7