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
