On Fri, Aug 16, 2024 at 05:43:18PM +0800, Yi Liu wrote:
On 2024/7/18 16:27, Tian, Kevin wrote:
From: Liu, Yi L yi.l.liu@intel.com Sent: Friday, June 28, 2024 5:06 PM
@@ -3289,7 +3290,20 @@ static int __iommu_set_group_pasid(struct iommu_domain *domain,
if (device == last_gdev) break;
ops->remove_dev_pasid(device->dev, pasid, domain);
/* If no old domain, undo the succeeded devices/pasid */if (!old) {ops->remove_dev_pasid(device->dev, pasid, domain);continue;}/** Rollback the succeeded devices/pasid to the old domain.* And it is a driver bug to fail attaching with a previously* good domain.*/if (WARN_ON(old->ops->set_dev_pasid(old, device->dev,pasid, domain)))ops->remove_dev_pasid(device->dev, pasid, domain);I wonder whether @remove_dev_pasid() can be replaced by having blocking_domain support @set_dev_pasid?
how about your thought, @Jason?
I think we talked about doing that once before, I forget why it was not done. Maybe there was an issue?
But it seems worth trying.
I would like to see set_dev_pasid pass in the old domain first:
int (*set_dev_pasid)(struct iommu_domain *new_domain, struct iommu_domain *old_domain, struct device *dev, ioasid_t pasid);
Replace includes the old_domain as an argument and it is necessary information..
A quick try on SMMUv3 seems reasonable:
diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c index 9bc50bded5af72..f512bfe5cd202c 100644 --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c @@ -2931,13 +2931,12 @@ int arm_smmu_set_pasid(struct arm_smmu_master *master, return ret; }
-static void arm_smmu_remove_dev_pasid(struct device *dev, ioasid_t pasid, - struct iommu_domain *domain) +static void arm_smmu_blocking_set_dev_pasid(struct iommu_domain *new_domain, + struct iommu_domain *old_domain, + struct device *dev, ioasid_t pasid) { struct arm_smmu_master *master = dev_iommu_priv_get(dev); - struct arm_smmu_domain *smmu_domain; - - smmu_domain = to_smmu_domain(domain); + struct arm_smmu_domain *smmu_domain = to_smmu_domain(old_domain);
mutex_lock(&arm_smmu_asid_lock); arm_smmu_clear_cd(master, pasid); @@ -3039,6 +3038,7 @@ static int arm_smmu_attach_dev_blocked(struct iommu_domain *domain,
static const struct iommu_domain_ops arm_smmu_blocked_ops = { .attach_dev = arm_smmu_attach_dev_blocked, + .set_dev_pasid = arm_smmu_blocked_set_dev_pasid, };
static struct iommu_domain arm_smmu_blocked_domain = { @@ -3487,7 +3487,6 @@ static struct iommu_ops arm_smmu_ops = { .device_group = arm_smmu_device_group, .of_xlate = arm_smmu_of_xlate, .get_resv_regions = arm_smmu_get_resv_regions, - .remove_dev_pasid = arm_smmu_remove_dev_pasid, .dev_enable_feat = arm_smmu_dev_enable_feature, .dev_disable_feat = arm_smmu_dev_disable_feature, .page_response = arm_smmu_page_response,
Jason