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

[Baolu Lu <baolu.lu@linux.intel.com>]

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.

> +	    list_empty(&dmar_domain->devices)) {

list_for_each_entry is no op if list is empty, so no need to check it.

> +		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.

> +
> +	}

The VT-d driver also support attaching domain to a pasid of a device. We
also need to enable dirty tracking on those devices.

> +
> +	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.

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