diff mbox series

[v5,12/27] iommu/arm-smmu-v3: Start building a generic PASID layer

Message ID 12-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

Commit Message

Jason Gunthorpe March 4, 2024, 11:44 p.m. UTC 
Add arm_smmu_set_pasid()/arm_smmu_remove_pasid() which are to be used by
callers that already constructed the arm_smmu_cd they wish to program.

These functions will encapsulate the shared logic to setup a CD entry that
will be shared by SVA and S1 domain cases.

Prior fixes had already moved this logic most up into
__arm_smmu_sva_bind(), move it to it's final home.

This does not yet relieve the SVA restrictions:
 - The RID domain is a S1 domain
 - The programmed PASID is the mm_get_enqcmd_pasid()
 - Nothing changes while SVA is running (sva_enable)

Also, SVA invalidation will still iterate over the S1 domain's master
list.

Tested-by: Nicolin Chen <nicolinc@nvidia.com>
Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
 .../iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c   | 39 +++++++++----------
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c   | 29 ++++++++++++++
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h   |  6 +++
 3 files changed, 53 insertions(+), 21 deletions(-)

Comments

Michael Shavit March 19, 2024, 4:11 p.m. UTC | #1 
On Tue, Mar 5, 2024 at 7:44 AM Jason Gunthorpe <jgg@nvidia.com> wrote:
>
> Add arm_smmu_set_pasid()/arm_smmu_remove_pasid() which are to be used by
> callers that already constructed the arm_smmu_cd they wish to program.
>
> These functions will encapsulate the shared logic to setup a CD entry that
> will be shared by SVA and S1 domain cases.
>
> Prior fixes had already moved this logic most up into
> __arm_smmu_sva_bind(), move it to it's final home.
>
> This does not yet relieve the SVA restrictions:
>  - The RID domain is a S1 domain
>  - The programmed PASID is the mm_get_enqcmd_pasid()
>  - Nothing changes while SVA is running (sva_enable)
>
> Also, SVA invalidation will still iterate over the S1 domain's master
> list.
>
> Tested-by: Nicolin Chen <nicolinc@nvidia.com>
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> ---
>  .../iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c   | 39 +++++++++----------
>  drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c   | 29 ++++++++++++++
>  drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h   |  6 +++
>  3 files changed, 53 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
> index 159350a78d4cf9..e1fa2074c0b37b 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
> @@ -416,12 +416,10 @@ static void arm_smmu_mmu_notifier_put(struct arm_smmu_mmu_notifier *smmu_mn)
>         arm_smmu_free_shared_cd(cd);
>  }
>
> -static int __arm_smmu_sva_bind(struct device *dev, ioasid_t pasid,
> -                              struct mm_struct *mm)
> +static int __arm_smmu_sva_bind(struct device *dev, struct mm_struct *mm,
> +                              struct arm_smmu_cd *target)
>  {
>         int ret;
> -       struct arm_smmu_cd target;
> -       struct arm_smmu_cd *cdptr;
>         struct arm_smmu_bond *bond;
>         struct arm_smmu_master *master = dev_iommu_priv_get(dev);
>         struct iommu_domain *domain = iommu_get_domain_for_dev(dev);
> @@ -448,19 +446,10 @@ static int __arm_smmu_sva_bind(struct device *dev, ioasid_t pasid,
>                 goto err_free_bond;
>         }
>
> -       cdptr = arm_smmu_get_cd_ptr(master, mm_get_enqcmd_pasid(mm));
> -       if (!cdptr) {
> -               ret = -ENOMEM;
> -               goto err_put_notifier;
> -       }
> -       arm_smmu_make_sva_cd(&target, master, mm, bond->smmu_mn->cd->asid);
> -       arm_smmu_write_cd_entry(master, pasid, cdptr, &target);
> -
> +       arm_smmu_make_sva_cd(target, master, mm, bond->smmu_mn->cd->asid);
This can probably already move out to arm_smmu_sva_set_dev_pasid in
this commit. Removing the arm_smmu_get_cd_ptr pre-alloc might also be
possible if we're careful with failure of arm_smmu_set_pasid.
This is eventually addressed in "iommu/arm-smmu-v3: Put the SVA mmu
notifier in the smmu_domain", but that patch is already big enough as
it is and the change fits nicely here.

>         list_add(&bond->list, &master->bonds);
>         return 0;
>
> -err_put_notifier:
> -       arm_smmu_mmu_notifier_put(bond->smmu_mn);
>  err_free_bond:
>         kfree(bond);
>         return ret;
> @@ -621,10 +610,9 @@ void arm_smmu_sva_remove_dev_pasid(struct iommu_domain *domain,
>         struct arm_smmu_bond *bond = NULL, *t;
>         struct arm_smmu_master *master = dev_iommu_priv_get(dev);
>
> +       arm_smmu_remove_pasid(master, to_smmu_domain(domain), id);
> +
>         mutex_lock(&sva_lock);
> -
> -       arm_smmu_clear_cd(master, id);
> -
>         list_for_each_entry(t, &master->bonds, list) {
>                 if (t->mm == mm) {
>                         bond = t;
> @@ -643,17 +631,26 @@ void arm_smmu_sva_remove_dev_pasid(struct iommu_domain *domain,
>  static int arm_smmu_sva_set_dev_pasid(struct iommu_domain *domain,
>                                       struct device *dev, ioasid_t id)
>  {
> +       struct arm_smmu_master *master = dev_iommu_priv_get(dev);
>         int ret = 0;
>         struct mm_struct *mm = domain->mm;
> +       struct arm_smmu_cd target;
>
>         if (mm_get_enqcmd_pasid(mm) != id)
>                 return -EINVAL;
>
> -       mutex_lock(&sva_lock);
> -       ret = __arm_smmu_sva_bind(dev, id, mm);
> -       mutex_unlock(&sva_lock);
> +       if (!arm_smmu_get_cd_ptr(master, id))
> +               return -ENOMEM;
>
> -       return ret;
> +       mutex_lock(&sva_lock);
> +       ret = __arm_smmu_sva_bind(dev, mm, &target);
> +       mutex_unlock(&sva_lock);
> +       if (ret)
> +               return ret;
> +
> +       /* This cannot fail since we preallocated the cdptr */
> +       arm_smmu_set_pasid(master, to_smmu_domain(domain), id, &target);
> +       return 0;
>  }
>
>  static void arm_smmu_sva_domain_free(struct iommu_domain *domain)
> 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 dfdd48cf217c4e..7a0be7dd719793 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> @@ -2656,6 +2656,35 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
>         return 0;
>  }
>
> +static bool arm_smmu_is_s1_domain(struct iommu_domain *domain)
> +{
> +       if (!domain || !(domain->type & __IOMMU_DOMAIN_PAGING))
> +               return false;
> +       return to_smmu_domain(domain)->stage == ARM_SMMU_DOMAIN_S1;
> +}
> +
> +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 arm_smmu_cd *cdptr;
> +
> +       if (!arm_smmu_is_s1_domain(iommu_get_domain_for_dev(master->dev)))
> +               return -ENODEV;
> +
> +       cdptr = arm_smmu_get_cd_ptr(master, pasid);
> +       if (!cdptr)
> +               return -ENOMEM;
> +       arm_smmu_write_cd_entry(master, pasid, cdptr, cd);
> +       return 0;
> +}
> +
> +void arm_smmu_remove_pasid(struct arm_smmu_master *master,
> +                          struct arm_smmu_domain *smmu_domain, ioasid_t pasid)
> +{
> +       arm_smmu_clear_cd(master, pasid);
> +}
> +
>  static int arm_smmu_attach_dev_ste(struct device *dev,
>                                    struct arm_smmu_ste *ste)
>  {
> diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
> index 468cd33b80ac35..ab0a6587e07ec8 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
> @@ -753,6 +753,12 @@ void arm_smmu_write_cd_entry(struct arm_smmu_master *master, int ssid,
>                              struct arm_smmu_cd *cdptr,
>                              const struct arm_smmu_cd *target);
>
> +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);
> +void arm_smmu_remove_pasid(struct arm_smmu_master *master,
> +                          struct arm_smmu_domain *smmu_domain, ioasid_t pasid);
> +
>  void arm_smmu_tlb_inv_asid(struct arm_smmu_device *smmu, u16 asid);
>  void arm_smmu_tlb_inv_range_asid(unsigned long iova, size_t size, int asid,
>                                  size_t granule, bool leaf,
> --
> 2.43.2
>
Jason Gunthorpe March 20, 2024, 6:32 p.m. UTC | #2 
On Wed, Mar 20, 2024 at 12:11:36AM +0800, Michael Shavit wrote:
> > @@ -448,19 +446,10 @@ static int __arm_smmu_sva_bind(struct device *dev, ioasid_t pasid,
> >                 goto err_free_bond;
> >         }
> >
> > -       cdptr = arm_smmu_get_cd_ptr(master, mm_get_enqcmd_pasid(mm));
> > -       if (!cdptr) {
> > -               ret = -ENOMEM;
> > -               goto err_put_notifier;
> > -       }
> > -       arm_smmu_make_sva_cd(&target, master, mm, bond->smmu_mn->cd->asid);
> > -       arm_smmu_write_cd_entry(master, pasid, cdptr, &target);
> > -
> > +       arm_smmu_make_sva_cd(target, master, mm, bond->smmu_mn->cd->asid);
> This can probably already move out to arm_smmu_sva_set_dev_pasid in
> this commit. Removing the arm_smmu_get_cd_ptr pre-alloc might also be
> possible if we're careful with failure of arm_smmu_set_pasid.
> This is eventually addressed in "iommu/arm-smmu-v3: Put the SVA mmu
> notifier in the smmu_domain", but that patch is already big enough as
> it is and the change fits nicely here.

Since all this bond related code is going to be deleted I didn't want
to mess with it too much, but sure it looks like this:

static int arm_smmu_sva_set_dev_pasid(struct iommu_domain *domain,
				      struct device *dev, ioasid_t id)
{
	struct arm_smmu_master *master = dev_iommu_priv_get(dev);
	struct mm_struct *mm = domain->mm;
	struct arm_smmu_bond *bond;
	struct arm_smmu_cd target;
	int ret;

	if (mm_get_enqcmd_pasid(mm) != id)
		return -EINVAL;

	mutex_lock(&sva_lock);
	bond = __arm_smmu_sva_bind(dev, mm);
	if (IS_ERR(bond)) {
		mutex_unlock(&sva_lock);
		return PTR_ERR(bond);
	}

	arm_smmu_make_sva_cd(&target, master, mm, bond->smmu_mn->cd->asid);
	ret = arm_smmu_set_pasid(master, to_smmu_domain(domain), id, &target);
	if (ret) {
		list_del(&bond->list);
		arm_smmu_mmu_notifier_put(bond->smmu_mn);
		kfree(bond);
		mutex_unlock(&sva_lock);
		return ret;
	}
	mutex_unlock(&sva_lock);
	return 0;
}

Which is much closer to the final arrangment in later patches.

Jason
diff mbox series

Patch

diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
index 159350a78d4cf9..e1fa2074c0b37b 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c
@@ -416,12 +416,10 @@  static void arm_smmu_mmu_notifier_put(struct arm_smmu_mmu_notifier *smmu_mn)
 	arm_smmu_free_shared_cd(cd);
 }
 
-static int __arm_smmu_sva_bind(struct device *dev, ioasid_t pasid,
-			       struct mm_struct *mm)
+static int __arm_smmu_sva_bind(struct device *dev, struct mm_struct *mm,
+			       struct arm_smmu_cd *target)
 {
 	int ret;
-	struct arm_smmu_cd target;
-	struct arm_smmu_cd *cdptr;
 	struct arm_smmu_bond *bond;
 	struct arm_smmu_master *master = dev_iommu_priv_get(dev);
 	struct iommu_domain *domain = iommu_get_domain_for_dev(dev);
@@ -448,19 +446,10 @@  static int __arm_smmu_sva_bind(struct device *dev, ioasid_t pasid,
 		goto err_free_bond;
 	}
 
-	cdptr = arm_smmu_get_cd_ptr(master, mm_get_enqcmd_pasid(mm));
-	if (!cdptr) {
-		ret = -ENOMEM;
-		goto err_put_notifier;
-	}
-	arm_smmu_make_sva_cd(&target, master, mm, bond->smmu_mn->cd->asid);
-	arm_smmu_write_cd_entry(master, pasid, cdptr, &target);
-
+	arm_smmu_make_sva_cd(target, master, mm, bond->smmu_mn->cd->asid);
 	list_add(&bond->list, &master->bonds);
 	return 0;
 
-err_put_notifier:
-	arm_smmu_mmu_notifier_put(bond->smmu_mn);
 err_free_bond:
 	kfree(bond);
 	return ret;
@@ -621,10 +610,9 @@  void arm_smmu_sva_remove_dev_pasid(struct iommu_domain *domain,
 	struct arm_smmu_bond *bond = NULL, *t;
 	struct arm_smmu_master *master = dev_iommu_priv_get(dev);
 
+	arm_smmu_remove_pasid(master, to_smmu_domain(domain), id);
+
 	mutex_lock(&sva_lock);
-
-	arm_smmu_clear_cd(master, id);
-
 	list_for_each_entry(t, &master->bonds, list) {
 		if (t->mm == mm) {
 			bond = t;
@@ -643,17 +631,26 @@  void arm_smmu_sva_remove_dev_pasid(struct iommu_domain *domain,
 static int arm_smmu_sva_set_dev_pasid(struct iommu_domain *domain,
 				      struct device *dev, ioasid_t id)
 {
+	struct arm_smmu_master *master = dev_iommu_priv_get(dev);
 	int ret = 0;
 	struct mm_struct *mm = domain->mm;
+	struct arm_smmu_cd target;
 
 	if (mm_get_enqcmd_pasid(mm) != id)
 		return -EINVAL;
 
-	mutex_lock(&sva_lock);
-	ret = __arm_smmu_sva_bind(dev, id, mm);
-	mutex_unlock(&sva_lock);
+	if (!arm_smmu_get_cd_ptr(master, id))
+		return -ENOMEM;
 
-	return ret;
+	mutex_lock(&sva_lock);
+	ret = __arm_smmu_sva_bind(dev, mm, &target);
+	mutex_unlock(&sva_lock);
+	if (ret)
+		return ret;
+
+	/* This cannot fail since we preallocated the cdptr */
+	arm_smmu_set_pasid(master, to_smmu_domain(domain), id, &target);
+	return 0;
 }
 
 static void arm_smmu_sva_domain_free(struct iommu_domain *domain)
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 dfdd48cf217c4e..7a0be7dd719793 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
@@ -2656,6 +2656,35 @@  static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
 	return 0;
 }
 
+static bool arm_smmu_is_s1_domain(struct iommu_domain *domain)
+{
+	if (!domain || !(domain->type & __IOMMU_DOMAIN_PAGING))
+		return false;
+	return to_smmu_domain(domain)->stage == ARM_SMMU_DOMAIN_S1;
+}
+
+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 arm_smmu_cd *cdptr;
+
+	if (!arm_smmu_is_s1_domain(iommu_get_domain_for_dev(master->dev)))
+		return -ENODEV;
+
+	cdptr = arm_smmu_get_cd_ptr(master, pasid);
+	if (!cdptr)
+		return -ENOMEM;
+	arm_smmu_write_cd_entry(master, pasid, cdptr, cd);
+	return 0;
+}
+
+void arm_smmu_remove_pasid(struct arm_smmu_master *master,
+			   struct arm_smmu_domain *smmu_domain, ioasid_t pasid)
+{
+	arm_smmu_clear_cd(master, pasid);
+}
+
 static int arm_smmu_attach_dev_ste(struct device *dev,
 				   struct arm_smmu_ste *ste)
 {
diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
index 468cd33b80ac35..ab0a6587e07ec8 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
@@ -753,6 +753,12 @@  void arm_smmu_write_cd_entry(struct arm_smmu_master *master, int ssid,
 			     struct arm_smmu_cd *cdptr,
 			     const struct arm_smmu_cd *target);
 
+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);
+void arm_smmu_remove_pasid(struct arm_smmu_master *master,
+			   struct arm_smmu_domain *smmu_domain, ioasid_t pasid);
+
 void arm_smmu_tlb_inv_asid(struct arm_smmu_device *smmu, u16 asid);
 void arm_smmu_tlb_inv_range_asid(unsigned long iova, size_t size, int asid,
 				 size_t granule, bool leaf,