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