On 16/10/2023 03:07, Baolu Lu wrote:
> 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.
>
OK
>> +
>> + 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?
>
It is spurious (...)
> The iommu_dirty_bitmap is defined in iommu core. The iommu driver has no
> need to understand it and check its member anyway.
>
(...) The iommu driver has no need to understand it. iommu_dirty_bitmap_record()
already makes those checks in case there's no iova_bitmap to set bits to.
> 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?
>
As I had mentioned in an earlier comment, this was not meant to be here.
>> + 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
