| Message ID | 9-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: > > Avoid arm_smmu_attach_dev() having to undo the changes to the > smmu_domain->devices list, acquire the cdptr earlier so we don't need to > handle that error. > > Now there is a clear break in arm_smmu_attach_dev() where all the > prep-work has been done non-disruptively and we commit to making the HW > change, which cannot fail. > > This completes transforming arm_smmu_attach_dev() so that it does not > disturb the HW if it fails. > > 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 | 24 +++++++-------------- > 1 file changed, 8 insertions(+), 16 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 2dd6cb17112e98..39081d828a2132 100644 > --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c > +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c > @@ -2676,6 +2676,7 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev) > struct arm_smmu_device *smmu; > struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain); > struct arm_smmu_master *master; > + struct arm_smmu_cd *cdptr; > > if (!fwspec) > return -ENOENT; > @@ -2704,6 +2705,12 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev) > if (ret) > return ret; > > + if (smmu_domain->stage == ARM_SMMU_DOMAIN_S1) { > + cdptr = arm_smmu_get_cd_ptr(master, IOMMU_NO_PASID); > + if (!cdptr) > + return -ENOMEM; > + } > + > /* > * Prevent arm_smmu_share_asid() from trying to change the ASID > * of either the old or new domain while we are working on it. > @@ -2723,13 +2730,6 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev) > switch (smmu_domain->stage) { > case ARM_SMMU_DOMAIN_S1: { > struct arm_smmu_cd target_cd; > - struct arm_smmu_cd *cdptr; > - > - cdptr = arm_smmu_get_cd_ptr(master, IOMMU_NO_PASID); > - if (!cdptr) { > - ret = -ENOMEM; > - goto out_list_del; > - } > > arm_smmu_make_s1_cd(&target_cd, master, smmu_domain); > arm_smmu_write_cd_entry(master, IOMMU_NO_PASID, cdptr, > @@ -2746,16 +2746,8 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev) > } > > arm_smmu_enable_ats(master, smmu_domain); > - goto out_unlock; > - > -out_list_del: > - spin_lock_irqsave(&smmu_domain->devices_lock, flags); > - list_del_init(&master->domain_head); > - spin_unlock_irqrestore(&smmu_domain->devices_lock, flags); > - > -out_unlock: > mutex_unlock(&arm_smmu_asid_lock); > - return ret; > + return 0; > } > > static int arm_smmu_attach_dev_ste(struct device *dev, > -- > 2.43.2 > Reviewed-by: Michael Shavit <mshavit@google.com>
On Mon, Mar 04, 2024 at 07:43:57PM -0400, Jason Gunthorpe wrote: > Avoid arm_smmu_attach_dev() having to undo the changes to the > smmu_domain->devices list, acquire the cdptr earlier so we don't need to > handle that error. I should probably mention this in the other patch, yet PATCH-14 adding arm_smmu_attach_prepare() to this function doesn't have a rollback for CD table allocation. I assume we're fine with that? > Now there is a clear break in arm_smmu_attach_dev() where all the > prep-work has been done non-disruptively and we commit to making the HW > change, which cannot fail. > > This completes transforming arm_smmu_attach_dev() so that it does not > disturb the HW if it fails. > > Tested-by: Nicolin Chen <nicolinc@nvidia.com> > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com> Reviewed-by: Nicolin Chen <nicolinc@nvidia.com>
On Fri, Mar 15, 2024 at 09:16:14PM -0700, Nicolin Chen wrote: > On Mon, Mar 04, 2024 at 07:43:57PM -0400, Jason Gunthorpe wrote: > > Avoid arm_smmu_attach_dev() having to undo the changes to the > > smmu_domain->devices list, acquire the cdptr earlier so we don't need to > > handle that error. > > I should probably mention this in the other patch, yet PATCH-14 > adding arm_smmu_attach_prepare() to this function doesn't have a > rollback for CD table allocation. I assume we're fine with that? Yeah, I think so. The CD table leafs are never freed by anything, so even if you did succeed to attach a PASID and then detach the leaf will still hang around. If we care we could try to clean it up by consulting the PASID xarray and checking if the entire leaf is unused. Thanks, Jason
Hi Jason, On Mon, Mar 04, 2024 at 07:43:57PM -0400, Jason Gunthorpe wrote: > Avoid arm_smmu_attach_dev() having to undo the changes to the > smmu_domain->devices list, acquire the cdptr earlier so we don't need to > handle that error. > > Now there is a clear break in arm_smmu_attach_dev() where all the > prep-work has been done non-disruptively and we commit to making the HW > change, which cannot fail. > > This completes transforming arm_smmu_attach_dev() so that it does not > disturb the HW if it fails. > > 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 | 24 +++++++-------------- > 1 file changed, 8 insertions(+), 16 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 2dd6cb17112e98..39081d828a2132 100644 > --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c > +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c > @@ -2676,6 +2676,7 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev) > struct arm_smmu_device *smmu; > struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain); > struct arm_smmu_master *master; > + struct arm_smmu_cd *cdptr; > > if (!fwspec) > return -ENOENT; > @@ -2704,6 +2705,12 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev) > if (ret) > return ret; > > + if (smmu_domain->stage == ARM_SMMU_DOMAIN_S1) { > + cdptr = arm_smmu_get_cd_ptr(master, IOMMU_NO_PASID); > + if (!cdptr) > + return -ENOMEM; > + } > + > /* > * Prevent arm_smmu_share_asid() from trying to change the ASID > * of either the old or new domain while we are working on it. > @@ -2723,13 +2730,6 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev) > switch (smmu_domain->stage) { > case ARM_SMMU_DOMAIN_S1: { > struct arm_smmu_cd target_cd; > - struct arm_smmu_cd *cdptr; > - > - cdptr = arm_smmu_get_cd_ptr(master, IOMMU_NO_PASID); > - if (!cdptr) { > - ret = -ENOMEM; > - goto out_list_del; > - } > > arm_smmu_make_s1_cd(&target_cd, master, smmu_domain); > arm_smmu_write_cd_entry(master, IOMMU_NO_PASID, cdptr, > @@ -2746,16 +2746,8 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev) > } > > arm_smmu_enable_ats(master, smmu_domain); > - goto out_unlock; > - > -out_list_del: > - spin_lock_irqsave(&smmu_domain->devices_lock, flags); > - list_del_init(&master->domain_head); > - spin_unlock_irqrestore(&smmu_domain->devices_lock, flags); > - > -out_unlock: > mutex_unlock(&arm_smmu_asid_lock); > - return ret; > + return 0; > } > > static int arm_smmu_attach_dev_ste(struct device *dev, > -- > 2.43.2 > I believe this is fine, I couldn’t break it. With the comment on the previous patch, where we explicitly allocate the CD here and not inside arm_smmu_get_cd_ptr(). Reviewed-by: Mostafa Saleh <smostafa@google.com> Thanks, Mostafa
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 2dd6cb17112e98..39081d828a2132 100644 --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c @@ -2676,6 +2676,7 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev) struct arm_smmu_device *smmu; struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain); struct arm_smmu_master *master; + struct arm_smmu_cd *cdptr; if (!fwspec) return -ENOENT; @@ -2704,6 +2705,12 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev) if (ret) return ret; + if (smmu_domain->stage == ARM_SMMU_DOMAIN_S1) { + cdptr = arm_smmu_get_cd_ptr(master, IOMMU_NO_PASID); + if (!cdptr) + return -ENOMEM; + } + /* * Prevent arm_smmu_share_asid() from trying to change the ASID * of either the old or new domain while we are working on it. @@ -2723,13 +2730,6 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev) switch (smmu_domain->stage) { case ARM_SMMU_DOMAIN_S1: { struct arm_smmu_cd target_cd; - struct arm_smmu_cd *cdptr; - - cdptr = arm_smmu_get_cd_ptr(master, IOMMU_NO_PASID); - if (!cdptr) { - ret = -ENOMEM; - goto out_list_del; - } arm_smmu_make_s1_cd(&target_cd, master, smmu_domain); arm_smmu_write_cd_entry(master, IOMMU_NO_PASID, cdptr, @@ -2746,16 +2746,8 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev) } arm_smmu_enable_ats(master, smmu_domain); - goto out_unlock; - -out_list_del: - spin_lock_irqsave(&smmu_domain->devices_lock, flags); - list_del_init(&master->domain_head); - spin_unlock_irqrestore(&smmu_domain->devices_lock, flags); - -out_unlock: mutex_unlock(&arm_smmu_asid_lock); - return ret; + return 0; } static int arm_smmu_attach_dev_ste(struct device *dev,