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
