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

Baolu Lu
2023-10-16T01:41:05Z

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