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

[Jason Gunthorpe <jgg@nvidia.com>]

On Tue, Apr 30, 2024 at 02:43:06PM +0100, Shameer Kolothum wrote:
> +static int io_pgtable_visit_dirty(struct arm_lpae_io_pgtable *data,
> +				  struct io_pgtable_walk_data *walk_data,
> +				  arm_lpae_iopte *ptep, int lvl)
> +{
> +	struct io_pgtable *iop = &data->iop;
> +	arm_lpae_iopte pte = READ_ONCE(*ptep);
> +
> +	if (WARN_ON(!pte))
> +		return -EINVAL;

This seems poorly placed, why would pte ever be zero?

> +	if (iopte_leaf(pte, lvl, iop->fmt)) {
> +		size_t size = ARM_LPAE_BLOCK_SIZE(lvl, data);
> +
> +		if (iopte_hw_dirty(pte)) {
> +			iommu_dirty_bitmap_record(walk_data->dirty,
> +						  walk_data->addr, size);
> +			if (!(walk_data->flags & IOMMU_DIRTY_NO_CLEAR))
> +				set_bit(ARM_LPAE_PTE_AP_RDONLY_BIT,
> +					(unsigned long *)ptep);
> +		}
> +		walk_data->addr += size;
> +		return 0;
> +	}
> +
> +	ptep = iopte_deref(pte, data);

This would be a better spot, if the pte doesn't indicate a next level
table then something has gone wrong, otherwise the returned pointer
has to be valid.

Something like this maybe?

static inline bool iopte_table(arm_lpae_iopte pte, int lvl,
			      enum io_pgtable_fmt fmt)
{
	if (lvl == (ARM_LPAE_MAX_LEVELS - 1))
		return false;
	return iopte_type(pte) == ARM_LPAE_PTE_TYPE_TABLE;
}


> +	return __arm_lpae_iopte_walk_dirty(data, walk_data, ptep, lvl + 1);
> +}
> +
> +static int __arm_lpae_iopte_walk_dirty(struct arm_lpae_io_pgtable *data,
> +				       struct io_pgtable_walk_data *walk_data,
> +				       arm_lpae_iopte *ptep,
> +				       int lvl)
> +{
> +	u32 idx;
> +	int max_entries, ret;
> +
> +	if (WARN_ON(lvl == ARM_LPAE_MAX_LEVELS))
> +		return -EINVAL;

And if the above ipte_deref() could check for a valid next level (with
no valid level at MAX_LEVELS) then this can never happen either and
can be removed.

Jason