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

[Nicolin Chen <nicolinc@nvidia.com>]

Hi All,

Sashiko pointed a couple of valid points.

On Wed, May 20, 2026 at 12:46:10PM -0700, Nicolin Chen wrote:
> @@ -3851,7 +3870,8 @@ static int arm_smmu_blocking_set_dev_pasid(struct iommu_domain *new_domain,
>  	 * When the last user of the CD table goes away downgrade the STE back
>  	 * to a non-cd_table one, by re-attaching its sid_domain.
>  	 */
> -	if (!arm_smmu_ssids_in_use(&master->cd_table)) {
> +	if (!master->ats_always_on &&
> +	    !arm_smmu_ssids_in_use(&master->cd_table)) {
>  		struct iommu_domain *sid_domain =
>  			iommu_driver_get_domain_for_dev(master->dev);

Here the detach path doesn't check sid_domain's type, mismatching..

> @@ -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;
[...]
> -	if (arm_smmu_ssids_in_use(&master->cd_table)) {
> +	if (ats_always_on || arm_smmu_ssids_in_use(&master->cd_table)) {

.. the attach path where it only applies to IOMMU_DOMAIN_IDENTITY.

I am addressing this with:

@ -3870,13 +3870,15 @@ static int arm_smmu_blocking_set_dev_pasid(struct iommu_domain *new_domain,
         * When the last user of the CD table goes away downgrade the STE back
         * to a non-cd_table one, by re-attaching its sid_domain.
         */
-       if (!master->ats_always_on &&
-           !arm_smmu_ssids_in_use(&master->cd_table)) {
+       if (!arm_smmu_ssids_in_use(&master->cd_table)) {
                struct iommu_domain *sid_domain =
                        iommu_driver_get_domain_for_dev(master->dev);
+               bool ats_always_on = master->ats_always_on &&
+                                    sid_domain->type != IOMMU_DOMAIN_BLOCKED;
+               bool downgrade = sid_domain->type == IOMMU_DOMAIN_IDENTITY ||
+                                sid_domain->type == IOMMU_DOMAIN_BLOCKED;

-               if (sid_domain->type == IOMMU_DOMAIN_IDENTITY ||
-                   sid_domain->type == IOMMU_DOMAIN_BLOCKED)
+               if (!ats_always_on && downgrade)
                        sid_domain->ops->attach_dev(sid_domain, dev,
                                                    sid_domain);
        }

> +static int arm_smmu_master_prepare_ats(struct arm_smmu_master *master)
> +{
> +	bool s1p = master->smmu->features & ARM_SMMU_FEAT_TRANS_S1;
> +	unsigned int stu = __ffs(master->smmu->pgsize_bitmap);
> +	struct pci_dev *pdev;
> +	int ret;
> +
> +	if (!arm_smmu_ats_supported(master))
> +		return 0;
> +
> +	pdev = to_pci_dev(master->dev);
> +
> +	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;
> +	}
> +
> +	master->ats_always_on = true;
> +
> +	ret = arm_smmu_alloc_cd_tables(master);
> +	if (ret)
> +		return ret;
> +
> +out_prepare:
> +	pci_prepare_ats(pdev, stu);
> +	return 0;
> +}

Another issue is: arm_smmu_master_prepare_ats() here doesn't guard
against cases when !arm_smmu_ats_supported() && pci_ats_required().

"!s1p || !master->smmu->ssid_bits" also needs to fail the function.

Actually, this function does not return error at all. This reminds
me of the !pci_prepare_ats() issue Pranj is fixing. For this series,
at least we should propagate the pci_prepare_ats() return value to
the caller.

I see this single patch is getting large. Maybe it's a good idea to
split a bit. I will keep the reviewed parts into patches that retain
the given reviewed-by tags.

Thanks
Nicolin