Re: [PATCH v5 3/3] iommu/arm-smmu-v3: Allow ATS to be always on

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Silent failure to enforce ATS requirement leads to device malfunction.
- [High] Inconsistent STE configuration (Mode 1 vs Mode 2) for BLOCKED domains when ats_always_on is true.
--

commit 592bdfd18801a5a10cbb239b47e71f0772eea39e
Author: Nicolin Chen <nicolinc@nvidia.com>

iommu/arm-smmu-v3: Allow ATS to be always on

This patch modifies the ARM SMMUv3 driver to allow ATS to remain active even
when a device's default substream is attached to an identity domain. It does
this by retaining a CD table and ensuring the STE stays in Mode 1 for devices
that require ATS to be always on. It also ensures the STE is not downgraded
to Mode 2 when the last active substream becomes inactive.

> 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 e8d7dbe495f03..5c9d4bb542249 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
[ ... ]
> @@ -3875,6 +3895,8 @@ static void arm_smmu_attach_dev_ste(struct iommu_domain *domain,
>  		.old_domain = old_domain,
>  		.ssid = IOMMU_NO_PASID,
>  	};
> +	bool ats_always_on = master->ats_always_on &&
> +			     s1dss != STRTAB_STE_1_S1DSS_TERMINATE;

[Severity: High]
Does this explicitly force ats_always_on to false for BLOCKED domains?

If s1dss is STRTAB_STE_1_S1DSS_TERMINATE, the device will initially use
Mode 2 (CFG=ABORT), which disables EATS and appears to contradict the goal
of always keeping ATS on for these devices.

Additionally, if a PASID is attached (upgrading the STE to Mode 1) and then
subsequently removed:

arm_smmu_blocking_set_dev_pasid() {
    ...
    if (!master->ats_always_on &&
        !arm_smmu_ssids_in_use(&master->cd_table)) {
            ...
    }
}

The revert operation is skipped here because master->ats_always_on
evaluates to true, leaving the STE permanently in Mode 1.

Can this create an inconsistent state machine where the initial Mode 2 state
could trigger fatal URs for ATS requests, but detaching a PASID silently
leaves the device in Mode 1?

[ ... ]
> @@ -4215,6 +4237,42 @@ static void arm_smmu_remove_master(struct arm_smmu_master *master)
[ ... ]
> +static int arm_smmu_master_prepare_ats(struct arm_smmu_master *master)
> +{
[ ... ]
> +	if (!pci_ats_required(pdev))
> +		goto out_prepare;
> +
> +	/*
> +	 * S1DSS is required for ATS to be always on for identity domain cases.
> +	 * However, the S1DSS field is ignored if !IDR0_S1P or !IDR1_SSIDSIZE.
> +	 */
> +	if (!s1p || !master->smmu->ssid_bits) {
> +		dev_info_once(master->dev,
> +			      "SMMU doesn't support ATS to be always on\n");
> +		goto out_prepare;

[Severity: High]
If the SMMU lacks the necessary features to support ats_always_on when it is
strictly required by the device, should this code return an error like
-EOPNOTSUPP instead of returning success?

Returning success here allows the IOMMU probe to succeed without configuring
ats_always_on. When the device is later attached to an IDENTITY or BLOCKED
domain, it will be configured in a mode where EATS is ignored.

Could this result in the SMMU aborting ATS Translation Requests as
Unsupported Requests (UR) on the PCIe bus and triggering AER errors, rather
than cleanly failing the probe when the hardware requirement cannot be met?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1779304390.git.nicolinc@nvidia.com?part=3