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

Baolu Lu
2023-10-16T02:11:04Z

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