RE: [PATCH v2 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: Joao Martins <joao.m.martins@oracle.com>
> Sent: Thursday, February 22, 2024 11:38 AM
> To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>
> Cc: joro@8bytes.org; jgg@nvidia.com; kevin.tian@intel.com;
> nicolinc@nvidia.com; iommu@lists.linux.dev; mshavit@google.com;
> robin.murphy@arm.com; will@kernel.org; jiangkunkun
> <jiangkunkun@huawei.com>; zhukeqian <zhukeqian1@huawei.com>; Linuxarm
> <linuxarm@huawei.com>; linux-arm-kernel@lists.infradead.org
> Subject: Re: [PATCH v2 3/4] iommu/arm-smmu-v3: Add support for dirty tracking
> in domain alloc
> 
> On 22/02/2024 11:31, Shameerali Kolothum Thodi wrote:
> >
> >
> >> -----Original Message-----
> >> From: Joao Martins <joao.m.martins@oracle.com>
> >> Sent: Thursday, February 22, 2024 11:04 AM
> >> To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>
> >> Cc: joro@8bytes.org; jgg@nvidia.com; kevin.tian@intel.com;
> >> nicolinc@nvidia.com; iommu@lists.linux.dev; mshavit@google.com;
> >> robin.murphy@arm.com; will@kernel.org; jiangkunkun
> >> <jiangkunkun@huawei.com>; zhukeqian <zhukeqian1@huawei.com>;
> >> Linuxarm <linuxarm@huawei.com>; linux-arm-kernel@lists.infradead.org
> >> Subject: Re: [PATCH v2 3/4] iommu/arm-smmu-v3: Add support for dirty
> >> tracking in domain alloc
> >>
> >> On 22/02/2024 09:49, Shameer Kolothum wrote:
> >>> From: Joao Martins <joao.m.martins@oracle.com>
> >>>
> >>> This provides all the infrastructure to enable dirty tracking if the
> >>> hardware has the capability and domain alloc request for it.
> >>>
> >>> Please note, we still report no support for IOMMU_CAP_DIRTY_TRACKING
> >>> as it will finally be enabled in a subsequent patch.
> >>>
> >>> Signed-off-by: Joao Martins <joao.m.martins@oracle.com>
> >>> Signed-off-by: Shameer Kolothum
> >> <shameerali.kolothum.thodi@huawei.com>
> >>> ---
> >>>  drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 95
> >> ++++++++++++++++-----
> >>>  include/linux/io-pgtable.h                  |  4 +
> >>>  2 files changed, 77 insertions(+), 22 deletions(-)
> >>>
> >>> diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> >>> b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> >>> index bd30739e3588..058bbb0dbe2e 100644
> >>> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> >>> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> >>> @@ -43,6 +43,7 @@ MODULE_PARM_DESC(disable_msipolling,
> >>>  	"Disable MSI-based polling for CMD_SYNC completion.");
> >>>
> >>>  static struct iommu_ops arm_smmu_ops;
> >>> +static struct iommu_dirty_ops arm_smmu_dirty_ops;
> >>>
> >>>  enum arm_smmu_msi_index {
> >>>  	EVTQ_MSI_INDEX,
> >>> @@ -86,7 +87,8 @@ static struct arm_smmu_option_prop
> >>> arm_smmu_options[] = {
> >>>
> >>>  static void arm_smmu_rmr_install_bypass_ste(struct arm_smmu_device
> >>> *smmu);  static int arm_smmu_domain_finalise(struct arm_smmu_domain
> >> *smmu_domain,
> >>> -				    struct arm_smmu_device *smmu);
> >>> +				    struct arm_smmu_device *smmu,
> >>> +				    bool enable_dirty);
> >>>  static int arm_smmu_alloc_cd_tables(struct arm_smmu_master *master);
> >>> static void arm_smmu_tlb_inv_all_s2(struct arm_smmu_domain
> >>> *smmu_domain);
> >>>
> >>> @@ -2378,7 +2380,7 @@ static struct iommu_domain
> >> *arm_smmu_domain_alloc_paging(struct device *dev)
> >>>  		struct arm_smmu_master *master =
> >> dev_iommu_priv_get(dev);
> >>>  		int ret;
> >>>
> >>> -		ret = arm_smmu_domain_finalise(smmu_domain, master-
> >>> smmu);
> >>> +		ret = arm_smmu_domain_finalise(smmu_domain, master-
> >>> smmu, false);
> >>>  		if (ret) {
> >>>  			kfree(smmu_domain);
> >>>  			return ERR_PTR(ret);
> >>> @@ -2445,10 +2447,11 @@ static void arm_smmu_domain_free(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,
> >>> +				    bool enable_dirty)
> >>>  {
> >>>  	int ret;
> >>> -	unsigned long ias, oas;
> >>> +	unsigned long ias;
> >>>  	enum io_pgtable_fmt fmt;
> >>>  	struct io_pgtable_cfg pgtbl_cfg;
> >>>  	struct io_pgtable_ops *pgtbl_ops;
> >>> @@ -2459,31 +2462,31 @@ 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;
> >>> +		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;
> >>>  		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;
> >>>  	default:
> >>>  		return -EINVAL;
> >>>  	}
> >>>
> >>> -	pgtbl_cfg = (struct io_pgtable_cfg) {
> >>> -		.pgsize_bitmap	= smmu->pgsize_bitmap,
> >>> -		.ias		= ias,
> >>> -		.oas		= oas,
> >>> -		.coherent_walk	= smmu->features &
> >> ARM_SMMU_FEAT_COHERENCY,
> >>> -		.tlb		= &arm_smmu_flush_ops,
> >>> -		.iommu_dev	= smmu->dev,
> >>> -	};
> >>> -
> >>>  	pgtbl_ops = alloc_io_pgtable_ops(fmt, &pgtbl_cfg, smmu_domain);
> >>>  	if (!pgtbl_ops)
> >>>  		return -ENOMEM;
> >>> @@ -2491,7 +2494,8 @@ static int arm_smmu_domain_finalise(struct
> >> arm_smmu_domain *smmu_domain,
> >>>  	smmu_domain->domain.pgsize_bitmap = pgtbl_cfg.pgsize_bitmap;
> >>>  	smmu_domain->domain.geometry.aperture_end = (1UL <<
> >> pgtbl_cfg.ias) - 1;
> >>>  	smmu_domain->domain.geometry.force_aperture = true;
> >>> -
> >>> +	if (enable_dirty && smmu_domain->stage ==
> >> ARM_SMMU_DOMAIN_S1)
> >>> +		smmu_domain->domain.dirty_ops =
> >> &arm_smmu_dirty_ops;
> >>>  	ret = arm_smmu_domain_alloc_id(smmu, smmu_domain);
> >>>  	if (ret < 0) {
> >>>  		free_io_pgtable_ops(pgtbl_ops);
> >>> @@ -2811,7 +2815,7 @@ static int arm_smmu_attach_dev(struct
> >> iommu_domain *domain, struct device *dev)
> >>>  	mutex_lock(&smmu_domain->init_mutex);
> >>>
> >>>  	if (!smmu_domain->smmu) {
> >>> -		ret = arm_smmu_domain_finalise(smmu_domain, smmu);
> >>> +		ret = arm_smmu_domain_finalise(smmu_domain, smmu,
> >> false);
> >>>  	} else if (smmu_domain->smmu != smmu)
> >>>  		ret = -EINVAL;
> >>>
> >>
> >>
> >> I think we are missing the domain attach_dev check for dirty tracking.
> >>
> >> Something like:
> >>
> >> if (domain->dirty_ops && !arm_smmu_dbm_capable(smmu))
> >> 	return -EINVAL;
> >>
> >> But that helper is only introduced in the last patch, so maybe:
> >>
> >> if (domain->dirty_ops &&
> >>     !device_iommu_capable(dev, IOMMU_CAP_DIRTY_TRACKING))
> >> 	return -EINVAL;
> >
> > Ok. But do we really need to check this in attach()? As dirty_ops are added only
> > if it is requested in alloc_user() and there we return err when hardware doesn't
> > have the capability.  So not sure how this matters in attach() path. May be I am
> > missing something.
> 
> That's when you create the domain with dev A. Afterwards that dev A is
> attached,
> but later on you can attach another device B to the domain. So this check is
> there such that a domain with dirty tracking ops set will only have devices in
> there that support dirty tracking.

But that only matters if dev B is on  different smmu without dbm capability, right?
In that case we already fail the attach with,

>>>  	} else if (smmu_domain->smmu != smmu)
> >>>  		ret = -EINVAL;
> >>>

Is that right?

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