RE: [PATCH v3 2/4] iommu/io-pgtable-arm: Add read_and_clear_dirty() support

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

> -----Original Message-----
> From: Tian, Kevin <kevin.tian@intel.com>
> Sent: Wednesday, May 22, 2024 8:12 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 2/4] iommu/io-pgtable-arm: Add read_and_clear_dirty()
> support
> 
> > From: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
> > Sent: Tuesday, April 30, 2024 9:43 PM
> >
> > @@ -92,7 +93,11 @@
> >
> >  /* Stage-1 PTE */
> >  #define ARM_LPAE_PTE_AP_UNPRIV		(((arm_lpae_iopte)1) << 6)
> > -#define ARM_LPAE_PTE_AP_RDONLY		(((arm_lpae_iopte)2) << 6)
> > +#define ARM_LPAE_PTE_AP_RDONLY_BIT	7
> > +#define ARM_LPAE_PTE_AP_RDONLY		(((arm_lpae_iopte)1) << \
> > +					   ARM_LPAE_PTE_AP_RDONLY_BIT)
> > +#define ARM_LPAE_PTE_AP_WRITABLE_CLEAN
> > 	(ARM_LPAE_PTE_AP_RDONLY | \
> > +					 ARM_LPAE_PTE_DBM)
> 
> based on the usage is it clearer to be xxx_WRITEABLE_MASK?

I think in patch 4 we use this to set the PTE Writeable Clean. Anyway I will
revisit this following Jason's comment on just setting the DBM bit there.

> 
> >  #define ARM_LPAE_PTE_ATTRINDX_SHIFT	2
> >  #define ARM_LPAE_PTE_nG			(((arm_lpae_iopte)1) << 11)
> >
> > @@ -138,6 +143,9 @@
> >
> >  #define iopte_prot(pte)	((pte) & ARM_LPAE_PTE_ATTR_MASK)
> >
> > +#define iopte_hw_dirty(pte)	(((pte) &
> > ARM_LPAE_PTE_AP_WRITABLE_CLEAN) == \
> > +				   ARM_LPAE_PTE_DBM)
> > +
> 
> iopte_is_writeable_dirty()?
> 
> and the following "set_bit(ARM_LPAE_PTE_AP_RDONLY_BIT,
> (unsigned long *)ptep);" could be wrapped as:
> 
> 	iopte_set_writable_clean(ptep);

Ok. Will consider this during respin.
 
> > +
> > +static int arm_lpae_read_and_clear_dirty(struct io_pgtable_ops *ops,
> > +					 unsigned long iova, size_t size,
> > +					 unsigned long flags,
> > +					 struct iommu_dirty_bitmap *dirty)
> > +{
> > +	struct arm_lpae_io_pgtable *data = io_pgtable_ops_to_data(ops);
> > +	struct io_pgtable_cfg *cfg = &data->iop.cfg;
> > +	struct io_pgtable_walk_data walk_data = {
> > +		.dirty = dirty,
> > +		.flags = flags,
> > +		.addr = iova,
> > +		.end = iova + size,
> > +	};
> > +	arm_lpae_iopte *ptep = data->pgd;
> > +	int lvl = data->start_level;
> > +
> > +	if (WARN_ON(!size))
> > +		return -EINVAL;
> > +	if (WARN_ON((iova + size - 1) & ~(BIT(cfg->ias) - 1)))
> > +		return -EINVAL;
> > +	if (data->iop.fmt != ARM_64_LPAE_S1)
> > +		return -EINVAL;
> > +
> > +	return __arm_lpae_iopte_walk_dirty(data, &walk_data, ptep, lvl);
> 
> Intel/AMD drivers also checks:
> 
>         if (!dmar_domain->dirty_tracking && dirty->bitmap)
>                 return -EINVAL;

Is that really required? Is the concern here is user may issue
GET_DIRTY_BITMAP IOCTL without any validation and that may
result in unnecessary scanning of page tables? May be we should
handle it in core code then I think.

Thanks,
Shameer