diff mbox series

[v7,02/14] iommu/arm-smmu-v3: Start building a generic PASID layer

Message ID 2-v7-9597c885796c+d2-smmuv3_newapi_p2b_jgg@nvidia.com (mailing list archive)
State New
Headers show
Series Update SMMUv3 to the modern iommu API (part 2b/3) | expand

Commit Message

Jason Gunthorpe May 8, 2024, 6:57 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 most of this logic up into
__arm_smmu_sva_bind(), move it to it's final home.

Following patches will relieve some of the remaining SVA restrictions:

 - The RID domain is a S1 domain and has already setup the STE to point to
   the CD table
 - The programmed PASID is the mm_get_enqcmd_pasid()
 - Nothing changes while SVA is running (sva_enable)

SVA invalidation will still iterate over the S1 domain's master list,
later patches will resolve that.

Tested-by: Nicolin Chen <nicolinc@nvidia.com>
Tested-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
 .../iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c   | 57 ++++++++++---------
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c   | 32 ++++++++++-
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h   |  9 ++-
 3 files changed, 67 insertions(+), 31 deletions(-)

Comments

Nicolin Chen May 11, 2024, 2:32 a.m. UTC | #1 
On Wed, May 08, 2024 at 03:57:10PM -0300, Jason Gunthorpe wrote:
> @@ -611,10 +599,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);
> -

Should the new arm_smmu_remove_pasid() be inside the sva_lock as
the arm_smmu_clear_cd() previously? This would also seem to match
with the arm_smmu_set_pasid() that's inside the sva_lock too.

With that, arm_smmu_remove_pasid() seems to be removed entirely
by a following change, though its function declare still exists
in arm-smmu-v3.h (I probably should find what following patch and
comment there..)

> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> @@ -2412,6 +2412,10 @@ static void arm_smmu_install_ste_for_dev(struct arm_smmu_master *master,
>  	int i, j;
>  	struct arm_smmu_device *smmu = master->smmu;
>  
> +	master->cd_table.in_ste =
> +		FIELD_GET(STRTAB_STE_0_CFG, le64_to_cpu(target->data[0])) ==
> +		STRTAB_STE_0_CFG_S1_TRANS;
> +

> +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;
> +
> +	/* The core code validates pasid */
> +
> +	if (!master->cd_table.in_ste)
> +		return -ENODEV;
> +
> +	cdptr = arm_smmu_alloc_cd_ptr(master, pasid);
> +	if (!cdptr)
> +		return -ENOMEM;

Though I might be missing some piece, the in_ste is set in the
arm_smmu_install_ste_for_dev() when STE.Config=S1_TRANS, in which
case cdptr is already allocated? If so, do we still need to call
arm_smmu_alloc_cd_ptr()? Or just arm_smmu_get_cd_ptr() instead?

Thanks
Nicolin
Jason Gunthorpe May 12, 2024, 1:12 p.m. UTC | #2 
On Fri, May 10, 2024 at 07:32:16PM -0700, Nicolin Chen wrote:
> On Wed, May 08, 2024 at 03:57:10PM -0300, Jason Gunthorpe wrote:
> > @@ -611,10 +599,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);
> > -
> 
> Should the new arm_smmu_remove_pasid() be inside the sva_lock as
> the arm_smmu_clear_cd() previously? This would also seem to match
> with the arm_smmu_set_pasid() that's inside the sva_lock too.

Not needed, the sva_lock primarily protects the bond/etc stuff. The CD
is locked by the group mutex. By the end of the series the sva_lock is
just protecting the set_feature stuff, none of the attach flow.

> With that, arm_smmu_remove_pasid() seems to be removed entirely
> by a following change, though its function declare still exists
> in arm-smmu-v3.h (I probably should find what following patch and
> comment there..)

Woops, yep, I missed that.

> > +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;
> > +
> > +	/* The core code validates pasid */
> > +
> > +	if (!master->cd_table.in_ste)
> > +		return -ENODEV;
> > +
> > +	cdptr = arm_smmu_alloc_cd_ptr(master, pasid);
> > +	if (!cdptr)
> > +		return -ENOMEM;
> 
> Though I might be missing some piece, the in_ste is set in the
> arm_smmu_install_ste_for_dev() when STE.Config=S1_TRANS, in which
> case cdptr is already allocated? 

This only means that at least the first level of a two level (or
entire linear) table is allocated.

alloc_cd_ptr() is sill needed to allocate any second level. For
instance if the PASID is > 1024 then we will need a new second level
page as only SSID=0 will have been allocated.

When we get further along in the series the above changes to:

	if (!master->cd_table.in_ste &&
	    sid_domain->type != IOMMU_DOMAIN_IDENTITY &&
	    sid_domain->type != IOMMU_DOMAIN_BLOCKED)
		return -EINVAL;

	cdptr = arm_smmu_alloc_cd_ptr(master, pasid);

In those additional cases the entire allocation can be needed.

Jason
Nicolin Chen May 12, 2024, 8:54 p.m. UTC | #3 
On Sun, May 12, 2024 at 10:12:15AM -0300, Jason Gunthorpe wrote:
> On Fri, May 10, 2024 at 07:32:16PM -0700, Nicolin Chen wrote:
> > On Wed, May 08, 2024 at 03:57:10PM -0300, Jason Gunthorpe wrote:
> > > @@ -611,10 +599,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);
> > > -
> > 
> > Should the new arm_smmu_remove_pasid() be inside the sva_lock as
> > the arm_smmu_clear_cd() previously? This would also seem to match
> > with the arm_smmu_set_pasid() that's inside the sva_lock too.
> 
> Not needed, the sva_lock primarily protects the bond/etc stuff. The CD
> is locked by the group mutex. By the end of the series the sva_lock is
> just protecting the set_feature stuff, none of the attach flow.

I see. That lock in the end product does look cleaner.

> > > +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;
> > > +
> > > +	/* The core code validates pasid */
> > > +
> > > +	if (!master->cd_table.in_ste)
> > > +		return -ENODEV;
> > > +
> > > +	cdptr = arm_smmu_alloc_cd_ptr(master, pasid);
> > > +	if (!cdptr)
> > > +		return -ENOMEM;
> > 
> > Though I might be missing some piece, the in_ste is set in the
> > arm_smmu_install_ste_for_dev() when STE.Config=S1_TRANS, in which
> > case cdptr is already allocated? 
> 
> This only means that at least the first level of a two level (or
> entire linear) table is allocated.
> 
> alloc_cd_ptr() is sill needed to allocate any second level. For
> instance if the PASID is > 1024 then we will need a new second level
> page as only SSID=0 will have been allocated.

Oh, I had misread that v.s. arm_smmu_alloc_cd_tables, while a
cdptr here is a CD entry.

Thanks
Nicolin
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 28f8bf4327f69a..71ca87c2c5c3b6 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
@@ -417,29 +417,27 @@  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 struct arm_smmu_bond *__arm_smmu_sva_bind(struct device *dev,
+						 struct mm_struct *mm)
 {
 	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);
 	struct arm_smmu_domain *smmu_domain;
 
 	if (!(domain->type & __IOMMU_DOMAIN_PAGING))
-		return -ENODEV;
+		return ERR_PTR(-ENODEV);
 	smmu_domain = to_smmu_domain(domain);
 	if (smmu_domain->stage != ARM_SMMU_DOMAIN_S1)
-		return -ENODEV;
+		return ERR_PTR(-ENODEV);
 
 	if (!master || !master->sva_enabled)
-		return -ENODEV;
+		return ERR_PTR(-ENODEV);
 
 	bond = kzalloc(sizeof(*bond), GFP_KERNEL);
 	if (!bond)
-		return -ENOMEM;
+		return ERR_PTR(-ENOMEM);
 
 	bond->mm = mm;
 
@@ -449,22 +447,12 @@  static int __arm_smmu_sva_bind(struct device *dev, ioasid_t pasid,
 		goto err_free_bond;
 	}
 
-	cdptr = arm_smmu_alloc_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);
-
 	list_add(&bond->list, &master->bonds);
-	return 0;
+	return bond;
 
-err_put_notifier:
-	arm_smmu_mmu_notifier_put(bond->smmu_mn);
 err_free_bond:
 	kfree(bond);
-	return ret;
+	return ERR_PTR(ret);
 }
 
 bool arm_smmu_sva_supported(struct arm_smmu_device *smmu)
@@ -611,10 +599,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;
@@ -633,17 +620,33 @@  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)
 {
-	int ret = 0;
+	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);
-	ret = __arm_smmu_sva_bind(dev, id, mm);
-	mutex_unlock(&sva_lock);
+	bond = __arm_smmu_sva_bind(dev, mm);
+	if (IS_ERR(bond)) {
+		mutex_unlock(&sva_lock);
+		return PTR_ERR(bond);
+	}
 
-	return ret;
+	arm_smmu_make_sva_cd(&target, master, mm, bond->smmu_mn->cd->asid);
+	ret = arm_smmu_set_pasid(master, NULL, 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;
 }
 
 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 bd79422f7b6f50..3f19436fe86a37 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
@@ -1211,8 +1211,8 @@  struct arm_smmu_cd *arm_smmu_get_cd_ptr(struct arm_smmu_master *master,
 	return &l1_desc->l2ptr[ssid % CTXDESC_L2_ENTRIES];
 }
 
-struct arm_smmu_cd *arm_smmu_alloc_cd_ptr(struct arm_smmu_master *master,
-					  u32 ssid)
+static struct arm_smmu_cd *arm_smmu_alloc_cd_ptr(struct arm_smmu_master *master,
+						 u32 ssid)
 {
 	struct arm_smmu_ctx_desc_cfg *cd_table = &master->cd_table;
 	struct arm_smmu_device *smmu = master->smmu;
@@ -2412,6 +2412,10 @@  static void arm_smmu_install_ste_for_dev(struct arm_smmu_master *master,
 	int i, j;
 	struct arm_smmu_device *smmu = master->smmu;
 
+	master->cd_table.in_ste =
+		FIELD_GET(STRTAB_STE_0_CFG, le64_to_cpu(target->data[0])) ==
+		STRTAB_STE_0_CFG_S1_TRANS;
+
 	for (i = 0; i < master->num_streams; ++i) {
 		u32 sid = master->streams[i].id;
 		struct arm_smmu_ste *step =
@@ -2632,6 +2636,30 @@  static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
 	return 0;
 }
 
+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;
+
+	/* The core code validates pasid */
+
+	if (!master->cd_table.in_ste)
+		return -ENODEV;
+
+	cdptr = arm_smmu_alloc_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 4c6c843669aaf9..a1b400e3e41878 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
@@ -602,6 +602,7 @@  struct arm_smmu_ctx_desc_cfg {
 	dma_addr_t			cdtab_dma;
 	struct arm_smmu_l1_ctx_desc	*l1_desc;
 	unsigned int			num_l1_ents;
+	u8				in_ste;
 	u8				s1fmt;
 	/* log2 of the maximum number of CDs supported by this table */
 	u8				s1cdmax;
@@ -777,8 +778,6 @@  extern struct mutex arm_smmu_asid_lock;
 void arm_smmu_clear_cd(struct arm_smmu_master *master, ioasid_t ssid);
 struct arm_smmu_cd *arm_smmu_get_cd_ptr(struct arm_smmu_master *master,
 					u32 ssid);
-struct arm_smmu_cd *arm_smmu_alloc_cd_ptr(struct arm_smmu_master *master,
-					  u32 ssid);
 void arm_smmu_make_s1_cd(struct arm_smmu_cd *target,
 			 struct arm_smmu_master *master,
 			 struct arm_smmu_domain *smmu_domain);
@@ -786,6 +785,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,