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

Joao Martins
2023-10-16T10:58:15Z

On 16/10/2023 02:37, Baolu Lu wrote:
> On 9/23/23 9:25 AM, Joao Martins wrote:
> [...]
>>   +static int intel_iommu_set_dirty_tracking(struct iommu_domain *domain,
>> +                      bool enable)
>> +{
>> +    struct dmar_domain *dmar_domain = to_dmar_domain(domain);
>> +    struct device_domain_info *info;
>> +    int ret = -EINVAL;
>> +
>> +    spin_lock(&dmar_domain->lock);
>> +    if (!(dmar_domain->dirty_tracking ^ enable) ||
>
> Just out of curiosity, can we simply write
>
> dmar_domain->dirty_tracking == enable
>
> instead? I am not sure whether the compiler will be happy with this.
>

Part of the check above, was just trying to avoid same-state transitions. the
above you wrote should work (...)

>> +        list_empty(&dmar_domain->devices)) {
>
list_for_each_entry is no op if list is empty, so no need to check it.
>

Though this is unnecessary yes.

>> +        spin_unlock(&dmar_domain->lock);
>> +        return 0;
>> +    }
>> +
>> +    list_for_each_entry(info, &dmar_domain->devices, link) {
>> +        /* First-level page table always enables dirty bit*/
>> +        if (dmar_domain->use_first_level) {
>
> Since we leave out domain->use_first_level in the user_domain_alloc
> function, we no longer need to check it here.
>
>> +            ret = 0;
>> +            break;
>> +        }
>> +
>> +        ret = intel_pasid_setup_dirty_tracking(info->iommu, info->domain,
>> +                             info->dev, IOMMU_NO_PASID,
>> +                             enable);
>> +        if (ret)
>> +            break;
>
> We need to unwind to the previous status here. We cannot leave some
> devices with status @enable while others do not.
>
Ugh, yes

>> +
>> +    }
>
> The VT-d driver also support attaching domain to a pasid of a device. We
> also need to enable dirty tracking on those devices.
>
True. But to be honest, I thought we weren't quite there yet in PASID support
from IOMMUFD perspective; hence why I didn't aim at it. Or do I have the wrong
impression? From the code below, it clearly looks the driver does.

>> +
>> +    if (!ret)
>> +        dmar_domain->dirty_tracking = enable;
>> +    spin_unlock(&dmar_domain->lock);
>> +
>> +    return ret;
>> +}
>
> I have made some changes to the code based on my above comments. Please
> let me know what you think.
>

Looks great; thanks a lot for these changes!

> static int intel_iommu_set_dirty_tracking(struct iommu_domain *domain,
>                                           bool enable)
> {
>         struct dmar_domain *dmar_domain = to_dmar_domain(domain);
>         struct dev_pasid_info *dev_pasid;
>         struct device_domain_info *info;
>         int ret;
>
>         spin_lock(&dmar_domain->lock);
>         if (!(dmar_domain->dirty_tracking ^ enable))
>                 goto out_unlock;
>
>         list_for_each_entry(info, &dmar_domain->devices, link) {
>                 ret = intel_pasid_setup_dirty_tracking(info->iommu, dmar_domain,
>                                                        info->dev, IOMMU_NO_PASID,
>                                                        enable);
>                 if (ret)
>                         goto err_unwind;
>         }
>
>         list_for_each_entry(dev_pasid, &dmar_domain->dev_pasids, link_domain) {
>                 info = dev_iommu_priv_get(dev_pasid->dev);
>                 ret = intel_pasid_setup_dirty_tracking(info->iommu, dmar_domain,
>                                                        info->dev, dev_pasid->pasid,
>                                                        enable);
>                 if (ret)
>                         goto err_unwind;
>         }
>
>         dmar_domain->dirty_tracking = enable;
> out_unlock:
>         spin_unlock(&dmar_domain->lock);
>
>         return 0;
>
> err_unwind:
>         list_for_each_entry(info, &dmar_domain->devices, link)
>                 intel_pasid_setup_dirty_tracking(info->iommu, dmar_domain,
> info->dev,
>                                                  IOMMU_NO_PASID,
>
> dmar_domain->dirty_tracking);
>         list_for_each_entry(dev_pasid, &dmar_domain->dev_pasids, link_domain) {
>                 info = dev_iommu_priv_get(dev_pasid->dev);
>                 intel_pasid_setup_dirty_tracking(info->iommu, dmar_domain,
>                                                  info->dev, dev_pasid->pasid,
>
> dmar_domain->dirty_tracking);
>         }
>         spin_unlock(&dmar_domain->lock);
>
>         return ret;
> }
>
> Best regards,
> baolu