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

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

> -----Original Message-----
> From: Tian, Kevin <kevin.tian@intel.com>
> Sent: Wednesday, May 22, 2024 8:16 AM
> To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>;
> iommu@lists.linux.dev; linux-arm-kernel@lists.infradead.org
> Cc: robin.murphy@arm.com; will@kernel.org; joro@8bytes.org;
> jgg@nvidia.com; ryan.roberts@arm.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 v3 3/4] iommu/arm-smmu-v3: Add support for dirty tracking
> in domain alloc
> 
> > From: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
> > Sent: Tuesday, April 30, 2024 9:43 PM
> >
> > @@ -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;
> > +		pgtbl_cfg.ias = min_t(unsigned long, ias, VA_BITS);
> > +		pgtbl_cfg.oas = smmu->ias;
> > +		if (enable_dirty)
> > +			pgtbl_cfg.quirks |= IO_PGTABLE_QUIRK_ARM_HD;
> 
> why is dirty tracking considered as a quirk?

Yes, that is a bit unconventional. But this was discussed earlier and in SMMUv3 
driver the word "quirk" is considered in the broadest sense and there are
precedent for this in the driver already.

From Robin:

"Indeed these features aren't decorative grooves on a piece of furniture, 
but in the case of io-pgtable we're merely using "quirk" in its broadest 
sense to imply something that differs from the baseline default 
behaviour - ARM_MTK_EXT, ARM_TTBR1 and ARM_OUTER_WBWA (or whatever it's 
called this week) are all just indicating extra hardware features 
entirely comparable to HTTU;..."

https://lore.kernel.org/linux-iommu/5ada4a8b-8852-f83c-040a-9ef5dac51de2@arm.com/

> 
> >  		fmt = ARM_64_LPAE_S1;
> >  		break;
> >  	case ARM_SMMU_DOMAIN_S2:
> > -		ias = smmu->ias;
> > -		oas = smmu->oas;
> > +		pgtbl_cfg.ias = smmu->ias;
> > +		pgtbl_cfg.oas = smmu->oas;
> >  		fmt = ARM_64_LPAE_S2;
> >  		break;
> 
> so dirty-tracking is not supported by s2? what about nesting?

It will be added later when we have the nested support.
 
> if this is desired then attempting to set dirty_tracking on a s2 domain
> should be rejected with an error.

Ok. I will add that.

Thanks,
Shameer