Re: [PATCH v3 19/19] iommu/intel: Access/Dirty bit support for SL domains

[Baolu Lu <baolu.lu@linux.intel.com>]

On 9/23/23 9:25 AM, Joao Martins wrote:
[...]
> +static int intel_iommu_read_and_clear_dirty(struct iommu_domain *domain,
> +					    unsigned long iova, size_t size,
> +					    unsigned long flags,
> +					    struct iommu_dirty_bitmap *dirty)
> +{
> +	struct dmar_domain *dmar_domain = to_dmar_domain(domain);
> +	unsigned long end = iova + size - 1;
> +	unsigned long pgsize;
> +	bool ad_enabled;
> +
> +	spin_lock(&dmar_domain->lock);
> +	ad_enabled = dmar_domain->dirty_tracking;
> +	spin_unlock(&dmar_domain->lock);

The spin lock is to protect the RID and PASID device tracking list. No
need to use it here.

> +
> +	if (!ad_enabled && dirty->bitmap)
> +		return -EINVAL;

I don't understand above check of "dirty->bitmap". Isn't it always
invalid to call this if dirty tracking is not enabled on the domain?

The iommu_dirty_bitmap is defined in iommu core. The iommu driver has no
need to understand it and check its member anyway.

Or, I overlooked anything?

> +
> +	rcu_read_lock();

Do we really need a rcu lock here? This operation is protected by
iopt->iova_rwsem. Is it reasonable to remove it? If not, how about put
some comments around it?

> +	do {
> +		struct dma_pte *pte;
> +		int lvl = 0;
> +
> +		pte = pfn_to_dma_pte(dmar_domain, iova >> VTD_PAGE_SHIFT, &lvl,
> +				     GFP_ATOMIC);
> +		pgsize = level_size(lvl) << VTD_PAGE_SHIFT;
> +		if (!pte || !dma_pte_present(pte)) {
> +			iova += pgsize;
> +			continue;
> +		}
> +
> +		/* It is writable, set the bitmap */
> +		if (((flags & IOMMU_DIRTY_NO_CLEAR) &&
> +				dma_sl_pte_dirty(pte)) ||
> +		    dma_sl_pte_test_and_clear_dirty(pte))
> +			iommu_dirty_bitmap_record(dirty, iova, pgsize);
> +		iova += pgsize;
> +	} while (iova < end);
> +	rcu_read_unlock();
> +
> +	return 0;
> +}

Best regards,
baolu