diff mbox series

[v5,19/27] iommu/arm-smmu-v3: Keep track of arm_smmu_master_domain for SVA

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

Commit Message

Jason Gunthorpe March 4, 2024, 11:44 p.m. UTC
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(-)

Comments

Michael Shavit March 21, 2024, 10:47 a.m. UTC | #1
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
>
>
Jason Gunthorpe March 21, 2024, 1:55 p.m. UTC | #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 mbox series

Patch

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,