On 2024/8/16 21:02, Jason Gunthorpe wrote:
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@xxxxxxxxx>
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.
Since remove_dev_pasid() does not return a result, so caller does not
need to check the result of it. If we want to replace it with the
blocked_domain->ops->set_dev_pasid(), shall we enforce that the
set_dev_pasid() op of blocked_domain to be always success. Is it?
Otherwise, this is not an apple-to-apple replacement.
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..
sure. Intel iommu driver should be able to support it as well. While
AMD iommu driver does no have the blocking domain stuff yet. Vasant
may keep me honest.
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
--
Regards,
Yi Liu
