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
