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

[Jason Gunthorpe <jgg@nvidia.com>]

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