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

Joao Martins
2023-10-16T11:26:57Z

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