[PATCH v1 0/3] Clean-up arm-smmu-v3-sva.c: remove arm_smmu_bond

[PATCH v1 0/3] Clean-up arm-smmu-v3-sva.c: remove arm_smmu_bond

[Michael Shavit]

This small series was originally part of a larger effort to support
set_dev_pasid in arm-smmu-v3.c and a related SVA refactoring. But it can
also stand on its own as an initial and prepatory clean-up.

The crux of this series relies on the observation that SVA won't
allocate multiple SVA domains for the same device and mm pair. There's
therefore no reason for the driver to try to normalize data allocated
for a device/mm pair across set_dev_pasid calls. This simplification
then allows set_dev_pasid to use the SVA iommu_domain to hold
information instead of allocating a "bond" to represent the attachement.
Note that long term, we'll likely want to represent the SVA domain using
the same arm_smmu_domain struct used in arm-smmu-v3. This series serves
as an interim step to make those later refactors easier to reason about.

Note that arm-smmu-v3-sva performs a second level of normalization by
mapping multiple bonds (now SVA domains) attached to devices with the
same SMMU (if those devices have the same RID domain attached) to a
single arm_smmu_mmu_notifier. This is not affected by these patches.


Michael Shavit (3):
  iommu/arm-smmu-v3-sva: Remove unused iommu_sva handle
  iommu/arm-smmu-v3-sva: Remove bond refcount
  iommu/arm-smmu-v3-sva: Remove arm_smmu_bond

 .../iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c   | 90 ++++++-------------
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c   |  1 -
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h   |  2 +-
 3 files changed, 28 insertions(+), 65 deletions(-)


base-commit: 6eaae198076080886b9e7d57f4ae06fa782f90ef
-- 
2.42.0.283.g2d96d420d3-goog


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v1 1/3] iommu/arm-smmu-v3-sva: Remove unused iommu_sva handle

[Michael Shavit]

The __arm_smmu_sva_bind function returned an unused iommu_sva handle
that can be removed.

Signed-off-by: Michael Shavit <mshavit@google.com>
---

 .../iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c    | 18 ++++++------------
 1 file changed, 6 insertions(+), 12 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 a5a63b1c947eb..32784758ccce6 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
@@ -25,7 +25,6 @@ struct arm_smmu_mmu_notifier {
 #define mn_to_smmu(mn) container_of(mn, struct arm_smmu_mmu_notifier, mn)
 
 struct arm_smmu_bond {
-	struct iommu_sva		sva;
 	struct mm_struct		*mm;
 	struct arm_smmu_mmu_notifier	*smmu_mn;
 	struct list_head		list;
@@ -320,8 +319,7 @@ static void arm_smmu_mmu_notifier_put(struct arm_smmu_mmu_notifier *smmu_mn)
 	arm_smmu_free_shared_cd(cd);
 }
 
-static struct iommu_sva *
-__arm_smmu_sva_bind(struct device *dev, struct mm_struct *mm)
+static int __arm_smmu_sva_bind(struct device *dev, struct mm_struct *mm)
 {
 	int ret;
 	struct arm_smmu_bond *bond;
@@ -330,7 +328,7 @@ __arm_smmu_sva_bind(struct device *dev, struct mm_struct *mm)
 	struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
 
 	if (!master || !master->sva_enabled)
-		return ERR_PTR(-ENODEV);
+		return -ENODEV;
 
 	/* If bind() was already called for this {dev, mm} pair, reuse it. */
 	list_for_each_entry(bond, &master->bonds, list) {
@@ -342,10 +340,9 @@ __arm_smmu_sva_bind(struct device *dev, struct mm_struct *mm)
 
 	bond = kzalloc(sizeof(*bond), GFP_KERNEL);
 	if (!bond)
-		return ERR_PTR(-ENOMEM);
+		return -ENOMEM;
 
 	bond->mm = mm;
-	bond->sva.dev = dev;
 	refcount_set(&bond->refs, 1);
 
 	bond->smmu_mn = arm_smmu_mmu_notifier_get(smmu_domain, mm);
@@ -355,11 +352,11 @@ __arm_smmu_sva_bind(struct device *dev, struct mm_struct *mm)
 	}
 
 	list_add(&bond->list, &master->bonds);
-	return &bond->sva;
+	return 0;
 
 err_free_bond:
 	kfree(bond);
-	return ERR_PTR(ret);
+	return ret;
 }
 
 bool arm_smmu_sva_supported(struct arm_smmu_device *smmu)
@@ -537,13 +534,10 @@ static int arm_smmu_sva_set_dev_pasid(struct iommu_domain *domain,
 				      struct device *dev, ioasid_t id)
 {
 	int ret = 0;
-	struct iommu_sva *handle;
 	struct mm_struct *mm = domain->mm;
 
 	mutex_lock(&sva_lock);
-	handle = __arm_smmu_sva_bind(dev, mm);
-	if (IS_ERR(handle))
-		ret = PTR_ERR(handle);
+	ret = __arm_smmu_sva_bind(dev, mm);
 	mutex_unlock(&sva_lock);
 
 	return ret;
-- 
2.42.0.283.g2d96d420d3-goog


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v1 2/3] iommu/arm-smmu-v3-sva: Remove bond refcount

[Michael Shavit]

Always allocate a new arm_smmu_bond in __arm_smmu_sva_bind and remove
the bond refcount since arm_smmu_bond can never be shared across calls
to __arm_smmu_sva_bind.

The iommu framework will not allocate multiple SVA domains for the same
(device/mm) pair, nor will it call set_dev_pasid for a device if a
domain is already attached on the given pasid. There's also a one-to-one
mapping between MM and PASID. __arm_smmu_sva_bind is therefore never
called with the same (device/mm) pair, and so there's no reason to try
and normalize allocations of the arm_smmu_bond struct for a (device/mm)
pair across set_dev_pasid.

Signed-off-by: Michael Shavit <mshavit@google.com>
---
Note that this is true today because iommu_sva_bind_device calls
iommu_get_domain_for_dev_pasid to elude the iommu_attach_device_pasid if
a domain is already attached.
But even with Tina's patch series where iommu_get_domain_for_dev_pasid
is no longer used, iommu_attach_device_pasid also checks whether a
domain is already attached in the group's pasid_array.

 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c | 12 +-----------
 1 file changed, 1 insertion(+), 11 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 32784758ccce6..9fb6907c5e7d4 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
@@ -28,7 +28,6 @@ struct arm_smmu_bond {
 	struct mm_struct		*mm;
 	struct arm_smmu_mmu_notifier	*smmu_mn;
 	struct list_head		list;
-	refcount_t			refs;
 };
 
 #define sva_to_bond(handle) \
@@ -330,20 +329,11 @@ static int __arm_smmu_sva_bind(struct device *dev, struct mm_struct *mm)
 	if (!master || !master->sva_enabled)
 		return -ENODEV;
 
-	/* If bind() was already called for this {dev, mm} pair, reuse it. */
-	list_for_each_entry(bond, &master->bonds, list) {
-		if (bond->mm == mm) {
-			refcount_inc(&bond->refs);
-			return &bond->sva;
-		}
-	}
-
 	bond = kzalloc(sizeof(*bond), GFP_KERNEL);
 	if (!bond)
 		return -ENOMEM;
 
 	bond->mm = mm;
-	refcount_set(&bond->refs, 1);
 
 	bond->smmu_mn = arm_smmu_mmu_notifier_get(smmu_domain, mm);
 	if (IS_ERR(bond->smmu_mn)) {
@@ -522,7 +512,7 @@ void arm_smmu_sva_remove_dev_pasid(struct iommu_domain *domain,
 		}
 	}
 
-	if (!WARN_ON(!bond) && refcount_dec_and_test(&bond->refs)) {
+	if (!WARN_ON(!bond)) {
 		list_del(&bond->list);
 		arm_smmu_mmu_notifier_put(bond->smmu_mn);
 		kfree(bond);
-- 
2.42.0.283.g2d96d420d3-goog


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v1 3/3] iommu/arm-smmu-v3-sva: Remove arm_smmu_bond

[Michael Shavit]

Create a new iommu_domain subclass for SVA iommu domains to hold the
data previously stored in the dynamically allocated arm_smmu_bond. Add a
simple count of attached SVA domains to arm_smmu_master to replace the
list of bonds.

Signed-off-by: Michael Shavit <mshavit@google.com>
---

 .../iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c   | 70 +++++++------------
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c   |  1 -
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h   |  2 +-
 3 files changed, 26 insertions(+), 47 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 9fb6907c5e7d4..0342c0f35d55a 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
@@ -24,14 +24,13 @@ struct arm_smmu_mmu_notifier {
 
 #define mn_to_smmu(mn) container_of(mn, struct arm_smmu_mmu_notifier, mn)
 
-struct arm_smmu_bond {
-	struct mm_struct		*mm;
+struct arm_smmu_sva_domain {
+	struct iommu_domain		iommu_domain;
 	struct arm_smmu_mmu_notifier	*smmu_mn;
-	struct list_head		list;
 };
 
-#define sva_to_bond(handle) \
-	container_of(handle, struct arm_smmu_bond, sva)
+#define to_sva_domain(domain) \
+	container_of(domain, struct arm_smmu_sva_domain, iommu_domain)
 
 static DEFINE_MUTEX(sva_lock);
 
@@ -318,10 +317,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, struct mm_struct *mm)
+static int __arm_smmu_sva_bind(struct device *dev,
+			       struct arm_smmu_sva_domain *sva_domain,
+			       struct mm_struct *mm)
 {
-	int ret;
-	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 = to_smmu_domain(domain);
@@ -329,24 +328,14 @@ static int __arm_smmu_sva_bind(struct device *dev, struct mm_struct *mm)
 	if (!master || !master->sva_enabled)
 		return -ENODEV;
 
-	bond = kzalloc(sizeof(*bond), GFP_KERNEL);
-	if (!bond)
-		return -ENOMEM;
-
-	bond->mm = mm;
-
-	bond->smmu_mn = arm_smmu_mmu_notifier_get(smmu_domain, mm);
-	if (IS_ERR(bond->smmu_mn)) {
-		ret = PTR_ERR(bond->smmu_mn);
-		goto err_free_bond;
+	sva_domain->smmu_mn = arm_smmu_mmu_notifier_get(smmu_domain,
+							mm);
+	if (IS_ERR(sva_domain->smmu_mn)) {
+		sva_domain->smmu_mn = NULL;
+		return PTR_ERR(sva_domain->smmu_mn);
 	}
-
-	list_add(&bond->list, &master->bonds);
+	master->nr_attached_sva_domains += 1;
 	return 0;
-
-err_free_bond:
-	kfree(bond);
-	return ret;
 }
 
 bool arm_smmu_sva_supported(struct arm_smmu_device *smmu)
@@ -476,7 +465,7 @@ int arm_smmu_master_enable_sva(struct arm_smmu_master *master)
 int arm_smmu_master_disable_sva(struct arm_smmu_master *master)
 {
 	mutex_lock(&sva_lock);
-	if (!list_empty(&master->bonds)) {
+	if (master->nr_attached_sva_domains != 0) {
 		dev_err(master->dev, "cannot disable SVA, device is bound\n");
 		mutex_unlock(&sva_lock);
 		return -EBUSY;
@@ -500,22 +489,14 @@ void arm_smmu_sva_notifier_synchronize(void)
 void arm_smmu_sva_remove_dev_pasid(struct iommu_domain *domain,
 				   struct device *dev, ioasid_t id)
 {
-	struct mm_struct *mm = domain->mm;
-	struct arm_smmu_bond *bond = NULL, *t;
+	struct arm_smmu_sva_domain *sva_domain = to_sva_domain(domain);
 	struct arm_smmu_master *master = dev_iommu_priv_get(dev);
 
 	mutex_lock(&sva_lock);
-	list_for_each_entry(t, &master->bonds, list) {
-		if (t->mm == mm) {
-			bond = t;
-			break;
-		}
-	}
-
-	if (!WARN_ON(!bond)) {
-		list_del(&bond->list);
-		arm_smmu_mmu_notifier_put(bond->smmu_mn);
-		kfree(bond);
+	if (!WARN_ON(!sva_domain->smmu_mn)) {
+		master->nr_attached_sva_domains -= 1;
+		arm_smmu_mmu_notifier_put(sva_domain->smmu_mn);
+		sva_domain->smmu_mn = NULL;
 	}
 	mutex_unlock(&sva_lock);
 }
@@ -527,7 +508,7 @@ static int arm_smmu_sva_set_dev_pasid(struct iommu_domain *domain,
 	struct mm_struct *mm = domain->mm;
 
 	mutex_lock(&sva_lock);
-	ret = __arm_smmu_sva_bind(dev, mm);
+	ret = __arm_smmu_sva_bind(dev, to_sva_domain(domain), mm);
 	mutex_unlock(&sva_lock);
 
 	return ret;
@@ -545,12 +526,11 @@ static const struct iommu_domain_ops arm_smmu_sva_domain_ops = {
 
 struct iommu_domain *arm_smmu_sva_domain_alloc(void)
 {
-	struct iommu_domain *domain;
+	struct arm_smmu_sva_domain *sva_domain;
 
-	domain = kzalloc(sizeof(*domain), GFP_KERNEL);
-	if (!domain)
+	sva_domain = kzalloc(sizeof(*sva_domain), GFP_KERNEL);
+	if (!sva_domain)
 		return NULL;
-	domain->ops = &arm_smmu_sva_domain_ops;
-
-	return domain;
+	sva_domain->iommu_domain.ops = &arm_smmu_sva_domain_ops;
+	return &sva_domain->iommu_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 9b0dc35056019..911bcfd90cd85 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
@@ -2685,7 +2685,6 @@ static struct iommu_device *arm_smmu_probe_device(struct device *dev)
 
 	master->dev = dev;
 	master->smmu = smmu;
-	INIT_LIST_HEAD(&master->bonds);
 	dev_iommu_priv_set(dev, master);
 
 	ret = arm_smmu_insert_master(smmu, master);
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 dcab85698a4e2..3a518834429b1 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
@@ -702,7 +702,7 @@ struct arm_smmu_master {
 	bool				stall_enabled;
 	bool				sva_enabled;
 	bool				iopf_enabled;
-	struct list_head		bonds;
+	unsigned int			nr_attached_sva_domains;
 	unsigned int			ssid_bits;
 };
 
-- 
2.42.0.283.g2d96d420d3-goog


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1 0/3] Clean-up arm-smmu-v3-sva.c: remove arm_smmu_bond

[Jason Gunthorpe]

On Tue, Sep 05, 2023 at 07:49:11PM +0800, Michael Shavit wrote:
> 
> This small series was originally part of a larger effort to support
> set_dev_pasid in arm-smmu-v3.c and a related SVA refactoring. But it can
> also stand on its own as an initial and prepatory clean-up.
> 
> The crux of this series relies on the observation that SVA won't
> allocate multiple SVA domains for the same device and mm pair. 

Yes, I think that is true, certainly no-intree user of this stuff
wants to do that.

It is enforced to be true after Tina's series:

https://lore.kernel.org/r/20230905000930.24515-1-tina.zhang@intel.com

Which makes one SVA domain per mm.

> There's therefore no reason for the driver to try to normalize data
> allocated for a device/mm pair across set_dev_pasid calls. 

Indeed, this is where we are trying to get to. This de-duplication
code in every driver is quite horrible.

> Note that arm-smmu-v3-sva performs a second level of normalization by
> mapping multiple bonds (now SVA domains) attached to devices with the
> same SMMU (if those devices have the same RID domain attached) to a
> single arm_smmu_mmu_notifier. This is not affected by these patches.

Ultimately the notifier should be per-iommu_domain as well.

Thanks,
Jason

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1 1/3] iommu/arm-smmu-v3-sva: Remove unused iommu_sva handle

[Jason Gunthorpe]

On Tue, Sep 05, 2023 at 07:49:12PM +0800, Michael Shavit wrote:
> The __arm_smmu_sva_bind function returned an unused iommu_sva handle
> that can be removed.
> 
> Signed-off-by: Michael Shavit <mshavit@google.com>
> ---
> 
>  .../iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c    | 18 ++++++------------
>  1 file changed, 6 insertions(+), 12 deletions(-)

Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>

Jason

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1 2/3] iommu/arm-smmu-v3-sva: Remove bond refcount

[Jason Gunthorpe]

On Tue, Sep 05, 2023 at 07:49:13PM +0800, Michael Shavit wrote:
> Always allocate a new arm_smmu_bond in __arm_smmu_sva_bind and remove
> the bond refcount since arm_smmu_bond can never be shared across calls
> to __arm_smmu_sva_bind.
> 
> The iommu framework will not allocate multiple SVA domains for the same
> (device/mm) pair, nor will it call set_dev_pasid for a device if a
> domain is already attached on the given pasid. There's also a one-to-one
> mapping between MM and PASID. __arm_smmu_sva_bind is therefore never
> called with the same (device/mm) pair, and so there's no reason to try
> and normalize allocations of the arm_smmu_bond struct for a (device/mm)
> pair across set_dev_pasid.
> 
> Signed-off-by: Michael Shavit <mshavit@google.com>
> ---
> Note that this is true today because iommu_sva_bind_device calls
> iommu_get_domain_for_dev_pasid to elude the iommu_attach_device_pasid if
> a domain is already attached.
> But even with Tina's patch series where iommu_get_domain_for_dev_pasid
> is no longer used, iommu_attach_device_pasid also checks whether a
> domain is already attached in the group's pasid_array.
> 
>  drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c | 12 +-----------
>  1 file changed, 1 insertion(+), 11 deletions(-)

Reviewed-by: Jason Gunthorpe <jgg@nvidia.com>

Jason

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1 3/3] iommu/arm-smmu-v3-sva: Remove arm_smmu_bond

[Jason Gunthorpe]

On Tue, Sep 05, 2023 at 07:49:14PM +0800, Michael Shavit wrote:
> Create a new iommu_domain subclass for SVA iommu domains to hold the
> data previously stored in the dynamically allocated arm_smmu_bond. Add a
> simple count of attached SVA domains to arm_smmu_master to replace the
> list of bonds.
> 
> Signed-off-by: Michael Shavit <mshavit@google.com>
> ---
> 
>  .../iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c   | 70 +++++++------------
>  drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c   |  1 -
>  drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h   |  2 +-
>  3 files changed, 26 insertions(+), 47 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 9fb6907c5e7d4..0342c0f35d55a 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
> @@ -24,14 +24,13 @@ struct arm_smmu_mmu_notifier {
>  
>  #define mn_to_smmu(mn) container_of(mn, struct arm_smmu_mmu_notifier, mn)
>  
> -struct arm_smmu_bond {
> -	struct mm_struct		*mm;
> +struct arm_smmu_sva_domain {
> +	struct iommu_domain		iommu_domain;
>  	struct arm_smmu_mmu_notifier	*smmu_mn;
> -	struct list_head		list;
>  };
>  
> -#define sva_to_bond(handle) \
> -	container_of(handle, struct arm_smmu_bond, sva)
> +#define to_sva_domain(domain) \
> +	container_of(domain, struct arm_smmu_sva_domain, iommu_domain)

I'm not sure about this? This seems like a strange direction

The SVA domain and a UNMANAGED/PAGING domain should be basically the
same thing. Making a sva_domain a completely different type looks like
it would stand in the way of that?
> @@ -545,12 +526,11 @@ static const struct iommu_domain_ops arm_smmu_sva_domain_ops = {
>  
>  struct iommu_domain *arm_smmu_sva_domain_alloc(void)
>  {
> -	struct iommu_domain *domain;
> +	struct arm_smmu_sva_domain *sva_domain;
>  
> -	domain = kzalloc(sizeof(*domain), GFP_KERNEL);
> -	if (!domain)
> +	sva_domain = kzalloc(sizeof(*sva_domain), GFP_KERNEL);
> +	if (!sva_domain)
>  		return NULL;
> -	domain->ops = &arm_smmu_sva_domain_ops;
> -
> -	return domain;
> +	sva_domain->iommu_domain.ops = &arm_smmu_sva_domain_ops;

arm_smmu_sva_domain_free() should container_of before freeing, but
gross to assume the iommu_domain is the first member.

Jason

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1 3/3] iommu/arm-smmu-v3-sva: Remove arm_smmu_bond

[Michael Shavit]

On Tue, Sep 5, 2023 at 8:42 PM Jason Gunthorpe <jgg@nvidia.com> wrote:
>
> On Tue, Sep 05, 2023 at 07:49:14PM +0800, Michael Shavit wrote:
> > Create a new iommu_domain subclass for SVA iommu domains to hold the
> > data previously stored in the dynamically allocated arm_smmu_bond. Add a
> > simple count of attached SVA domains to arm_smmu_master to replace the
> > list of bonds.
> >
> > Signed-off-by: Michael Shavit <mshavit@google.com>
> > ---
> >
> >  .../iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c   | 70 +++++++------------
> >  drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c   |  1 -
> >  drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h   |  2 +-
> >  3 files changed, 26 insertions(+), 47 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 9fb6907c5e7d4..0342c0f35d55a 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
> > @@ -24,14 +24,13 @@ struct arm_smmu_mmu_notifier {
> >
> >  #define mn_to_smmu(mn) container_of(mn, struct arm_smmu_mmu_notifier, mn)
> >
> > -struct arm_smmu_bond {
> > -     struct mm_struct                *mm;
> > +struct arm_smmu_sva_domain {
> > +     struct iommu_domain             iommu_domain;
> >       struct arm_smmu_mmu_notifier    *smmu_mn;
> > -     struct list_head                list;
> >  };
> >
> > -#define sva_to_bond(handle) \
> > -     container_of(handle, struct arm_smmu_bond, sva)
> > +#define to_sva_domain(domain) \
> > +     container_of(domain, struct arm_smmu_sva_domain, iommu_domain)
>
> I'm not sure about this? This seems like a strange direction
>
> The SVA domain and a UNMANAGED/PAGING domain should be basically the
> same thing. Making a sva_domain a completely different type looks like
> it would stand in the way of that?

Agreed that's the eventual destination of all these re-works, but the
stage isn't fully set for that yet. IMO this is a simpler improvement
to get through for now, and I don't see it being an obstacle in the
future.

> > @@ -545,12 +526,11 @@ static const struct iommu_domain_ops arm_smmu_sva_domain_ops = {
> >
> >  struct iommu_domain *arm_smmu_sva_domain_alloc(void)
> >  {
> > -     struct iommu_domain *domain;
> > +     struct arm_smmu_sva_domain *sva_domain;
> >
> > -     domain = kzalloc(sizeof(*domain), GFP_KERNEL);
> > -     if (!domain)
> > +     sva_domain = kzalloc(sizeof(*sva_domain), GFP_KERNEL);
> > +     if (!sva_domain)
> >               return NULL;
> > -     domain->ops = &arm_smmu_sva_domain_ops;
> > -
> > -     return domain;
> > +     sva_domain->iommu_domain.ops = &arm_smmu_sva_domain_ops;
>
> arm_smmu_sva_domain_free() should container_of before freeing, but
> gross to assume the iommu_domain is the first member.

Oh good catch I missed updating the free.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1 0/3] Clean-up arm-smmu-v3-sva.c: remove arm_smmu_bond

[Michael Shavit]

On Tue, Sep 5, 2023 at 8:35 PM Jason Gunthorpe <jgg@nvidia.com> wrote:
>
> On Tue, Sep 05, 2023 at 07:49:11PM +0800, Michael Shavit wrote:
> >
> > Note that arm-smmu-v3-sva performs a second level of normalization by
> > mapping multiple bonds (now SVA domains) attached to devices with the
> > same SMMU (if those devices have the same RID domain attached) to a
> > single arm_smmu_mmu_notifier. This is not affected by these patches.
>
> Ultimately the notifier should be per-iommu_domain as well.

Speaking of, I'm questioning whether the multi-SMMU domain patchseries
and Tina's sva domain sharing are really prerequisites to get rid of
the notifier sharing. Is anyone really depending on or taking
advantage of this? The optimization only kicks in if multiple devices
with the same SMMU, share the same RID iommu domain (although this
would be improved by fixing SVA to not depend on the RID domain) , and
are bound to common MMs.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1 0/3] Clean-up arm-smmu-v3-sva.c: remove arm_smmu_bond

[Jason Gunthorpe]

On Tue, Sep 05, 2023 at 09:24:22PM +0800, Michael Shavit wrote:
> On Tue, Sep 5, 2023 at 8:35 PM Jason Gunthorpe <jgg@nvidia.com> wrote:
> >
> > On Tue, Sep 05, 2023 at 07:49:11PM +0800, Michael Shavit wrote:
> > >
> > > Note that arm-smmu-v3-sva performs a second level of normalization by
> > > mapping multiple bonds (now SVA domains) attached to devices with the
> > > same SMMU (if those devices have the same RID domain attached) to a
> > > single arm_smmu_mmu_notifier. This is not affected by these patches.
> >
> > Ultimately the notifier should be per-iommu_domain as well.
> 
> Speaking of, I'm questioning whether the multi-SMMU domain patchseries
> and Tina's sva domain sharing are really prerequisites to get rid of
> the notifier sharing. Is anyone really depending on or taking
> advantage of this?

Currently I don't see any in-tree user of SVA on ARM except for uacce,
which seems to work with only one device (HiSillicon QM).

So, yes, you can probably make it slightly less efficient for a
time. It will be functionally correct still with multiple registered
notifiers.

Jason

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1 3/3] iommu/arm-smmu-v3-sva: Remove arm_smmu_bond

[Jason Gunthorpe]

On Tue, Sep 05, 2023 at 09:14:09PM +0800, Michael Shavit wrote:
> On Tue, Sep 5, 2023 at 8:42 PM Jason Gunthorpe <jgg@nvidia.com> wrote:
> >
> > On Tue, Sep 05, 2023 at 07:49:14PM +0800, Michael Shavit wrote:
> > > Create a new iommu_domain subclass for SVA iommu domains to hold the
> > > data previously stored in the dynamically allocated arm_smmu_bond. Add a
> > > simple count of attached SVA domains to arm_smmu_master to replace the
> > > list of bonds.
> > >
> > > Signed-off-by: Michael Shavit <mshavit@google.com>
> > > ---
> > >
> > >  .../iommu/arm/arm-smmu-v3/arm-smmu-v3-sva.c   | 70 +++++++------------
> > >  drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c   |  1 -
> > >  drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h   |  2 +-
> > >  3 files changed, 26 insertions(+), 47 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 9fb6907c5e7d4..0342c0f35d55a 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
> > > @@ -24,14 +24,13 @@ struct arm_smmu_mmu_notifier {
> > >
> > >  #define mn_to_smmu(mn) container_of(mn, struct arm_smmu_mmu_notifier, mn)
> > >
> > > -struct arm_smmu_bond {
> > > -     struct mm_struct                *mm;
> > > +struct arm_smmu_sva_domain {
> > > +     struct iommu_domain             iommu_domain;
> > >       struct arm_smmu_mmu_notifier    *smmu_mn;
> > > -     struct list_head                list;
> > >  };
> > >
> > > -#define sva_to_bond(handle) \
> > > -     container_of(handle, struct arm_smmu_bond, sva)
> > > +#define to_sva_domain(domain) \
> > > +     container_of(domain, struct arm_smmu_sva_domain, iommu_domain)
> >
> > I'm not sure about this? This seems like a strange direction
> >
> > The SVA domain and a UNMANAGED/PAGING domain should be basically the
> > same thing. Making a sva_domain a completely different type looks like
> > it would stand in the way of that?
> 
> Agreed that's the eventual destination of all these re-works, but the
> stage isn't fully set for that yet. IMO this is a simpler improvement
> to get through for now, and I don't see it being an obstacle in the
> future.

Well, OK, you have the followup patches..

But I don't want to get in a spot where we continue to have "primary
domains" for SVA..

Jason

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v1 0/3] Clean-up arm-smmu-v3-sva.c: remove arm_smmu_bond

[Will Deacon]

On Tue, 5 Sep 2023 19:49:11 +0800, Michael Shavit wrote:
> This small series was originally part of a larger effort to support
> set_dev_pasid in arm-smmu-v3.c and a related SVA refactoring. But it can
> also stand on its own as an initial and prepatory clean-up.
> 
> The crux of this series relies on the observation that SVA won't
> allocate multiple SVA domains for the same device and mm pair. There's
> therefore no reason for the driver to try to normalize data allocated
> for a device/mm pair across set_dev_pasid calls. This simplification
> then allows set_dev_pasid to use the SVA iommu_domain to hold
> information instead of allocating a "bond" to represent the attachement.
> Note that long term, we'll likely want to represent the SVA domain using
> the same arm_smmu_domain struct used in arm-smmu-v3. This series serves
> as an interim step to make those later refactors easier to reason about.
> 
> [...]

Applied first two patches to will (for-joerg/arm-smmu/updates), thanks!

[1/3] iommu/arm-smmu-v3-sva: Remove unused iommu_sva handle
      https://git.kernel.org/will/c/d912aed14fe4
[2/3] iommu/arm-smmu-v3-sva: Remove bond refcount
      https://git.kernel.org/will/c/37ed36448fcd

Cheers,
-- 
Will

https://fixes.arm64.dev
https://next.arm64.dev
https://will.arm64.dev

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel