Re: [PATCH 5/5] iommu/arm-smmu-v3: Enforce dirty tracking in domain attach/alloc

[Jason Gunthorpe <jgg@nvidia.com>]

On Tue, Nov 28, 2023 at 09:49:40AM +0000, Shameer Kolothum wrote:
> @@ -2701,6 +2703,9 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
>  	master = dev_iommu_priv_get(dev);
>  	smmu = master->smmu;
>  
> +	if (domain->dirty_ops && !arm_smmu_dbm_capable(smmu))
> +		return -EINVAL;
> +

This is not necessary, a domain can be attached to a single smmu and
finalize was run on that smmu already. So dirty ops should only be set
if this is a S1 domain finalized ona smmu that was dbm capable.

Otherwise none of this makes any sense.

> @@ -3104,6 +3115,9 @@ arm_smmu_domain_alloc_user(struct device *dev, u32 flags,
>  
>  	smmu_domain->domain.type = IOMMU_DOMAIN_UNMANAGED;
>  	smmu_domain->domain.ops = arm_smmu_ops.default_domain_ops;
> +	if (enforce_dirty)
> +		smmu_domain->domain.dirty_ops = &arm_smmu_dirty_ops;

Ah, this seems in the wrong place, perhaps that is the confusion
everywhere?

If the finalize actually enables dirty tracking in the pgtbl_ops then
it should set the diryty_ops, they should not be set in alloc_user.

Specifically, a S2 domain should never have dirty_ops set.

IOW if domain.dirty_ops != NULL then pgtbl_ops != NULL && pgtbl_ops->read_and_clear_dirty

Thus no need to have all the other prints/etc then.

So I'd move this into finalize.

> @@ -4152,11 +4166,13 @@ static void arm_smmu_get_httu(struct arm_smmu_device *smmu, u32 reg)
>  
>  	if (smmu->dev->of_node)
>  		smmu->features |= features;
> -	else if (features != fw_features)
> +	else if (features != fw_features) {
>  		/* ACPI IORT sets the HTTU bits */
>  		dev_warn(smmu->dev,
> -			 "IDR0.HTTU overridden by FW configuration (0x%x)\n",
> +			 "IDR0.HTTU not overridden by FW configuration (0x%x)\n",
>  			 fw_features);
> +		smmu->features |= features;
> +	}
>  }

Is this hunk misplaced?

Jason