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

[Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>]

> -----Original Message-----
> From: Jason Gunthorpe <jgg@nvidia.com>
> Sent: Wednesday, November 29, 2023 7:49 PM
> To: Shameerali Kolothum Thodi <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;
> kevin.tian@intel.com; nicolinc@nvidia.com; mshavit@google.com;
> eric.auger@redhat.com; joao.m.martins@oracle.com; jiangkunkun
> <jiangkunkun@huawei.com>; zhukeqian <zhukeqian1@huawei.com>; Linuxarm
> <linuxarm@huawei.com>
> Subject: Re: [PATCH 5/5] iommu/arm-smmu-v3: Enforce dirty tracking in domain
> attach/alloc
> 
> 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.

Ok. Make sense. I will move it.

> 
> > @@ -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?

Oops... My bad. Added for testing. 

Thanks,
Shameer