Re: [PATCH v3 3/4] iommu/arm-smmu-v3: Add support for dirty tracking in domain alloc

From: Jason Gunthorpe <jgg@nvidia.com>
To: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
Cc: iommu@lists.linux.dev, linux-arm-kernel@lists.infradead.org,
	robin.murphy@arm.com, will@kernel.org, joro@8bytes.org,
	ryan.roberts@arm.com, kevin.tian@intel.com, nicolinc@nvidia.com,
	mshavit@google.com, eric.auger@redhat.com,
	joao.m.martins@oracle.com, jiangkunkun@huawei.com,
	zhukeqian1@huawei.com, linuxarm@huawei.com
Subject: Re: [PATCH v3 3/4] iommu/arm-smmu-v3: Add support for dirty tracking in domain alloc
Date: Sun, 12 May 2024 09:57:06 -0300	[thread overview]
Message-ID: <ZkC8oiSZsS5WbsKd@nvidia.com> (raw)
In-Reply-To: <20240430134308.1604-4-shameerali.kolothum.thodi@huawei.com>

On Tue, Apr 30, 2024 at 02:43:07PM +0100, Shameer Kolothum wrote:
> @@ -2408,13 +2410,14 @@ static void arm_smmu_domain_free_paging(struct iommu_domain *domain)
>  }
>  
>  static int arm_smmu_domain_finalise(struct arm_smmu_domain *smmu_domain,
> -				    struct arm_smmu_device *smmu)
> +				    struct arm_smmu_device *smmu,
> +				    u32 flags)
>  {
>  	int ret;
> -	unsigned long ias, oas;
>  	enum io_pgtable_fmt fmt;
>  	struct io_pgtable_cfg pgtbl_cfg;
>  	struct io_pgtable_ops *pgtbl_ops;
> +	bool enable_dirty = flags & IOMMU_HWPT_ALLOC_DIRTY_TRACKING;
>  
>  	/* Restrict the stage to what we can actually support */
>  	if (!(smmu->features & ARM_SMMU_FEAT_TRANS_S1))
> @@ -2422,31 +2425,32 @@ static int arm_smmu_domain_finalise(struct arm_smmu_domain *smmu_domain,
>  	if (!(smmu->features & ARM_SMMU_FEAT_TRANS_S2))
>  		smmu_domain->stage = ARM_SMMU_DOMAIN_S1;
>  
> +	pgtbl_cfg = (struct io_pgtable_cfg) {
> +		.pgsize_bitmap	= smmu->pgsize_bitmap,
> +		.coherent_walk	= smmu->features & ARM_SMMU_FEAT_COHERENCY,
> +		.tlb		= &arm_smmu_flush_ops,
> +		.iommu_dev	= smmu->dev,
> +	};
> +
>  	switch (smmu_domain->stage) {
>  	case ARM_SMMU_DOMAIN_S1:
> -		ias = (smmu->features & ARM_SMMU_FEAT_VAX) ? 52 : 48;
> -		ias = min_t(unsigned long, ias, VA_BITS);
> -		oas = smmu->ias;
> +		unsigned long ias = (smmu->features &
> +				     ARM_SMMU_FEAT_VAX) ? 52 : 48;

It is a good idea to wrap this in a {} scope starting at the case when
declaring varaibles

> @@ -3188,6 +3194,10 @@ arm_smmu_domain_alloc_user(struct device *dev, u32 flags,
>  	if (user_data)
>  		return ERR_PTR(-EINVAL);
>  
> +	if ((flags & IOMMU_HWPT_ALLOC_DIRTY_TRACKING) &&
> +	    !device_iommu_capable(dev, IOMMU_CAP_DIRTY_TRACKING))
> +		return ERR_PTR(-EOPNOTSUPP);

Let's put this device_iommu_capable hunk in the iommufd core code
instead of in drivers

> +
>  	smmu_domain = arm_smmu_domain_alloc();
>  	if (!smmu_domain)
>  		return ERR_PTR(-ENOMEM);

This needs the alloc_user function... Can you cherry pick it from my
series? Just remove all the nesting parts. I'd like to see this be
self-contained on top of v6.10-rc1. I think it is in good shape, lets
see it go early enough in that cycle

The arm_smmu_domain_alloc() will need to be picked as well.

Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>

Thanks,
Jason

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel