Message ID | 19-v5-9a37e0c884ce+31e3-smmuv3_newapi_p2_jgg@nvidia.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | Update SMMUv3 to the modern iommu API (part 2/3) | expand |
On Tue, Mar 5, 2024 at 7:44 AM Jason Gunthorpe <jgg@nvidia.com> wrote: > > Currently the smmu_domain->devices list is unused for SVA domains. > Fill it in with the SSID and master of every arm_smmu_set_pasid() > using the same logic as the RID attach. > > Tested-by: Nicolin Chen <nicolinc@nvidia.com> > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> > --- > drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 23 +++++++++++++++++++-- > 1 file changed, 21 insertions(+), 2 deletions(-) > > 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 2db2b822292a87..6d15fe3a160acf 100644 > --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c > +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c > @@ -2603,7 +2603,8 @@ to_smmu_domain_devices(struct iommu_domain *domain) > /* The domain can be NULL only when processing the first attach */ > if (!domain) > return NULL; > - if (domain->type & __IOMMU_DOMAIN_PAGING) > + if ((domain->type & __IOMMU_DOMAIN_PAGING) || > + domain->type == IOMMU_DOMAIN_SVA) > return to_smmu_domain(domain); > return NULL; > } > @@ -2813,7 +2814,9 @@ int arm_smmu_set_pasid(struct arm_smmu_master *master, > struct arm_smmu_domain *smmu_domain, ioasid_t pasid, > const struct arm_smmu_cd *cd) > { > + struct attach_state state = {.ssid = pasid}; > struct arm_smmu_cd *cdptr; > + int ret; > > if (!arm_smmu_is_s1_domain(iommu_get_domain_for_dev(master->dev))) > return -ENODEV; > @@ -2821,14 +2824,30 @@ int arm_smmu_set_pasid(struct arm_smmu_master *master, > cdptr = arm_smmu_get_cd_ptr(master, pasid); > if (!cdptr) > return -ENOMEM; > + > + mutex_lock(&arm_smmu_asid_lock); > + ret = arm_smmu_attach_prepare(master, &smmu_domain->domain, &state); > + if (ret) > + goto out_unlock; > + > arm_smmu_write_cd_entry(master, pasid, cdptr, cd); > - return 0; > + > + arm_smmu_attach_commit(master, &state); > + > +out_unlock: > + mutex_unlock(&arm_smmu_asid_lock); > + return ret; > } arm_smmu_attach_commit tries to remove the master_domain entry from the previous domain that this master was attached to. It gets a pointer to the previous domain from the iommu framework with iommu_get_domain_for_dev(). But in this path, arm_smmu_attach_prepare is creating a master_domain entry for the pasid domain, which may be different from the one returned by iommu_get_domain_for_dev() on the next attach. I think this ended up being safe in the end because afaict the iommu framework requires detaching the previous pasid domain before attaching a new one. But nonetheless this is pretty fragile and doesn't look intentional. > > void arm_smmu_remove_pasid(struct arm_smmu_master *master, > struct arm_smmu_domain *smmu_domain, ioasid_t pasid) > { > + mutex_lock(&arm_smmu_asid_lock); > arm_smmu_clear_cd(master, pasid); > + if (master->ats_enabled) > + arm_smmu_atc_inv_master(master, pasid); > + arm_smmu_remove_master_domain(master, &smmu_domain->domain, pasid); > + mutex_unlock(&arm_smmu_asid_lock); > } > > static int arm_smmu_attach_dev_ste(struct iommu_domain *domain, > -- > 2.43.2 > >
On Thu, Mar 21, 2024 at 06:47:41PM +0800, Michael Shavit wrote: > On Tue, Mar 5, 2024 at 7:44 AM Jason Gunthorpe <jgg@nvidia.com> wrote: > > > > Currently the smmu_domain->devices list is unused for SVA domains. > > Fill it in with the SSID and master of every arm_smmu_set_pasid() > > using the same logic as the RID attach. > > > > Tested-by: Nicolin Chen <nicolinc@nvidia.com> > > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> > > --- > > drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 23 +++++++++++++++++++-- > > 1 file changed, 21 insertions(+), 2 deletions(-) > > > > 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 2db2b822292a87..6d15fe3a160acf 100644 > > --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c > > +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c > > @@ -2603,7 +2603,8 @@ to_smmu_domain_devices(struct iommu_domain *domain) > > /* The domain can be NULL only when processing the first attach */ > > if (!domain) > > return NULL; > > - if (domain->type & __IOMMU_DOMAIN_PAGING) > > + if ((domain->type & __IOMMU_DOMAIN_PAGING) || > > + domain->type == IOMMU_DOMAIN_SVA) > > return to_smmu_domain(domain); > > return NULL; > > } > > @@ -2813,7 +2814,9 @@ int arm_smmu_set_pasid(struct arm_smmu_master *master, > > struct arm_smmu_domain *smmu_domain, ioasid_t pasid, > > const struct arm_smmu_cd *cd) > > { > > + struct attach_state state = {.ssid = pasid}; > > struct arm_smmu_cd *cdptr; > > + int ret; > > > > if (!arm_smmu_is_s1_domain(iommu_get_domain_for_dev(master->dev))) > > return -ENODEV; > > @@ -2821,14 +2824,30 @@ int arm_smmu_set_pasid(struct arm_smmu_master *master, > > cdptr = arm_smmu_get_cd_ptr(master, pasid); > > if (!cdptr) > > return -ENOMEM; > > + > > + mutex_lock(&arm_smmu_asid_lock); > > + ret = arm_smmu_attach_prepare(master, &smmu_domain->domain, &state); > > + if (ret) > > + goto out_unlock; > > + > > arm_smmu_write_cd_entry(master, pasid, cdptr, cd); > > - return 0; > > + > > + arm_smmu_attach_commit(master, &state); > > + > > +out_unlock: > > + mutex_unlock(&arm_smmu_asid_lock); > > + return ret; > > } > > arm_smmu_attach_commit tries to remove the master_domain entry from > the previous domain that this master was attached to. It gets a > pointer to the previous domain from the iommu framework with > iommu_get_domain_for_dev(). > But in this path, arm_smmu_attach_prepare is creating a master_domain > entry for the pasid domain, which may be different from the one > returned by iommu_get_domain_for_dev() on the next attach. > > I think this ended up being safe in the end because afaict the iommu > framework requires detaching the previous pasid domain before > attaching a new one. But nonetheless this is pretty fragile and > doesn't look intentional. Oh yeah, this is a mistake! Commit needs to take in the old domain from the caller which knows what it actually is. Thanks, Jason
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 2db2b822292a87..6d15fe3a160acf 100644 --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c @@ -2603,7 +2603,8 @@ to_smmu_domain_devices(struct iommu_domain *domain) /* The domain can be NULL only when processing the first attach */ if (!domain) return NULL; - if (domain->type & __IOMMU_DOMAIN_PAGING) + if ((domain->type & __IOMMU_DOMAIN_PAGING) || + domain->type == IOMMU_DOMAIN_SVA) return to_smmu_domain(domain); return NULL; } @@ -2813,7 +2814,9 @@ int arm_smmu_set_pasid(struct arm_smmu_master *master, struct arm_smmu_domain *smmu_domain, ioasid_t pasid, const struct arm_smmu_cd *cd) { + struct attach_state state = {.ssid = pasid}; struct arm_smmu_cd *cdptr; + int ret; if (!arm_smmu_is_s1_domain(iommu_get_domain_for_dev(master->dev))) return -ENODEV; @@ -2821,14 +2824,30 @@ int arm_smmu_set_pasid(struct arm_smmu_master *master, cdptr = arm_smmu_get_cd_ptr(master, pasid); if (!cdptr) return -ENOMEM; + + mutex_lock(&arm_smmu_asid_lock); + ret = arm_smmu_attach_prepare(master, &smmu_domain->domain, &state); + if (ret) + goto out_unlock; + arm_smmu_write_cd_entry(master, pasid, cdptr, cd); - return 0; + + arm_smmu_attach_commit(master, &state); + +out_unlock: + mutex_unlock(&arm_smmu_asid_lock); + return ret; } void arm_smmu_remove_pasid(struct arm_smmu_master *master, struct arm_smmu_domain *smmu_domain, ioasid_t pasid) { + mutex_lock(&arm_smmu_asid_lock); arm_smmu_clear_cd(master, pasid); + if (master->ats_enabled) + arm_smmu_atc_inv_master(master, pasid); + arm_smmu_remove_master_domain(master, &smmu_domain->domain, pasid); + mutex_unlock(&arm_smmu_asid_lock); } static int arm_smmu_attach_dev_ste(struct iommu_domain *domain,