diff mbox series

[v5,11/17] iommu/arm-smmu-v3: Remove arm_smmu_master->domain

Message ID 11-v5-cd1be8dd9c71+3fa-smmuv3_newapi_p1_jgg@nvidia.com (mailing list archive)
State New, archived
Headers show
Series [v5,01/17] iommu/arm-smmu-v3: Make STE programming independent of the callers | expand

Commit Message

Jason Gunthorpe Feb. 6, 2024, 3:12 p.m. UTC 
Introducing global statics which are of type struct iommu_domain, not
struct arm_smmu_domain makes it difficult to retain
arm_smmu_master->domain, as it can no longer point to an IDENTITY or
BLOCKED domain.

The only place that uses the value is arm_smmu_detach_dev(). Change things
to work like other drivers and call iommu_get_domain_for_dev() to obtain
the current domain.

The master->domain is subtly protecting the domain_head against being
unused, change the domain_head to be INIT'd when the master is not
attached to a domain instead of garbage/zero.

Tested-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
Tested-by: Nicolin Chen <nicolinc@nvidia.com>
Tested-by: Moritz Fischer <moritzf@google.com>
Reviewed-by: Nicolin Chen <nicolinc@nvidia.com>
Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 26 ++++++++-------------
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h |  1 -
 2 files changed, 10 insertions(+), 17 deletions(-)

Comments

Mostafa Saleh Feb. 13, 2024, 3:45 p.m. UTC | #1 
Hi Jason,

On Tue, Feb 06, 2024 at 11:12:48AM -0400, Jason Gunthorpe wrote:
> Introducing global statics which are of type struct iommu_domain, not
> struct arm_smmu_domain makes it difficult to retain
> arm_smmu_master->domain, as it can no longer point to an IDENTITY or
> BLOCKED domain.
> 
> The only place that uses the value is arm_smmu_detach_dev(). Change things
> to work like other drivers and call iommu_get_domain_for_dev() to obtain
> the current domain.
> 
> The master->domain is subtly protecting the domain_head against being
> unused, change the domain_head to be INIT'd when the master is not
> attached to a domain instead of garbage/zero.

I don't this the problem here, neither the reason for initialising the
domain_head, can you please clarify the issue?

> Tested-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
> Tested-by: Nicolin Chen <nicolinc@nvidia.com>
> Tested-by: Moritz Fischer <moritzf@google.com>
> Reviewed-by: Nicolin Chen <nicolinc@nvidia.com>
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> ---
>  drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 26 ++++++++-------------
>  drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h |  1 -
>  2 files changed, 10 insertions(+), 17 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 133f13f33df124..a98707cd1efccb 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> @@ -2560,19 +2560,20 @@ static void arm_smmu_disable_pasid(struct arm_smmu_master *master)
>  
>  static void arm_smmu_detach_dev(struct arm_smmu_master *master)
>  {
> +	struct iommu_domain *domain = iommu_get_domain_for_dev(master->dev);
> +	struct arm_smmu_domain *smmu_domain;
>  	unsigned long flags;
> -	struct arm_smmu_domain *smmu_domain = master->domain;
>  
> -	if (!smmu_domain)
> +	if (!domain)
>  		return;
>  
> +	smmu_domain = to_smmu_domain(domain);
>  	arm_smmu_disable_ats(master, smmu_domain);
>  
>  	spin_lock_irqsave(&smmu_domain->devices_lock, flags);
> -	list_del(&master->domain_head);
> +	list_del_init(&master->domain_head);
>  	spin_unlock_irqrestore(&smmu_domain->devices_lock, flags);
>  
> -	master->domain = NULL;
>  	master->ats_enabled = false;
>  }
>  
> @@ -2626,8 +2627,6 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
>  
>  	arm_smmu_detach_dev(master);
>  
> -	master->domain = smmu_domain;
> -
>  	/*
>  	 * The SMMU does not support enabling ATS with bypass. When the STE is
>  	 * in bypass (STE.Config[2:0] == 0b100), ATS Translation Requests and
> @@ -2646,10 +2645,8 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
>  	case ARM_SMMU_DOMAIN_S1:
>  		if (!master->cd_table.cdtab) {
>  			ret = arm_smmu_alloc_cd_tables(master);
> -			if (ret) {
> -				master->domain = NULL;
> +			if (ret)
>  				goto out_list_del;
> -			}
>  		} else {
>  			/*
>  			 * arm_smmu_write_ctx_desc() relies on the entry being
> @@ -2657,17 +2654,13 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
>  			 */
>  			ret = arm_smmu_write_ctx_desc(master, IOMMU_NO_PASID,
>  						      NULL);
> -			if (ret) {
> -				master->domain = NULL;
> +			if (ret)
>  				goto out_list_del;
> -			}
>  		}
>  
>  		ret = arm_smmu_write_ctx_desc(master, IOMMU_NO_PASID, &smmu_domain->cd);
> -		if (ret) {
> -			master->domain = NULL;
> +		if (ret)
>  			goto out_list_del;
> -		}
>  
>  		arm_smmu_make_cdtable_ste(&target, master);
>  		arm_smmu_install_ste_for_dev(master, &target);
> @@ -2693,7 +2686,7 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
>  
>  out_list_del:
>  	spin_lock_irqsave(&smmu_domain->devices_lock, flags);
> -	list_del(&master->domain_head);
> +	list_del_init(&master->domain_head);
>  	spin_unlock_irqrestore(&smmu_domain->devices_lock, flags);
>  
>  out_unlock:
> @@ -2894,6 +2887,7 @@ static struct iommu_device *arm_smmu_probe_device(struct device *dev)
>  	master->dev = dev;
>  	master->smmu = smmu;
>  	INIT_LIST_HEAD(&master->bonds);
> +	INIT_LIST_HEAD(&master->domain_head);
>  	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 cbf4b57719b7b9..587f99701ad30f 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
> @@ -696,7 +696,6 @@ struct arm_smmu_stream {
>  struct arm_smmu_master {
>  	struct arm_smmu_device		*smmu;
>  	struct device			*dev;
> -	struct arm_smmu_domain		*domain;
>  	struct list_head		domain_head;
>  	struct arm_smmu_stream		*streams;
>  	/* Locked by the iommu core using the group mutex */
> -- 
> 2.43.0
>
Thanks,
Mostafa
Jason Gunthorpe Feb. 13, 2024, 4:37 p.m. UTC | #2 
On Tue, Feb 13, 2024 at 03:45:34PM +0000, Mostafa Saleh wrote:
> Hi Jason,
> 
> On Tue, Feb 06, 2024 at 11:12:48AM -0400, Jason Gunthorpe wrote:
> > Introducing global statics which are of type struct iommu_domain, not
> > struct arm_smmu_domain makes it difficult to retain
> > arm_smmu_master->domain, as it can no longer point to an IDENTITY or
> > BLOCKED domain.
> > 
> > The only place that uses the value is arm_smmu_detach_dev(). Change things
> > to work like other drivers and call iommu_get_domain_for_dev() to obtain
> > the current domain.
> > 
> > The master->domain is subtly protecting the domain_head against being
> > unused, change the domain_head to be INIT'd when the master is not
> > attached to a domain instead of garbage/zero.
> 
> I don't this the problem here, neither the reason for initialising the
> domain_head, can you please clarify the issue?

I didn't notice it either. Eric found it:

https://lore.kernel.org/linux-iommu/6fff20dd-46d5-4974-a4a5-fb4e7a59ce44@redhat.com/

> > @@ -2560,19 +2560,20 @@ static void arm_smmu_disable_pasid(struct arm_smmu_master *master)
> >  
> >  static void arm_smmu_detach_dev(struct arm_smmu_master *master)
> >  {
> > +	struct iommu_domain *domain = iommu_get_domain_for_dev(master->dev);
> > +	struct arm_smmu_domain *smmu_domain;
> >  	unsigned long flags;
> > -	struct arm_smmu_domain *smmu_domain = master->domain;

master->domain is NULL here which happens in cases where the current
RID domain is not a PAGING domain.

> > -	if (!smmu_domain)
> > +	if (!domain)
> >  		return;

Which used to early exit

> >  
> > +	smmu_domain = to_smmu_domain(domain);
> >  	arm_smmu_disable_ats(master, smmu_domain);
> >  
> >  	spin_lock_irqsave(&smmu_domain->devices_lock, flags);
> > -	list_del(&master->domain_head);
> > +	list_del_init(&master->domain_head);
> >  	spin_unlock_irqrestore(&smmu_domain->devices_lock, flags);

But now would cause the list_del() to hit a non-inited list_head and
explode.

Instead we keep the list head init'd and the list_del is a NOP.

Tricky right??

I changed the comment like this:

The master->domain is subtly protecting the master->domain_head against
being unused as only PAGING domains will set master->domain and only
paging domains use the master->domain_head. To make it simple keep the
master->domain_head initialized so that the list_del() logic just does
nothing for non-PAGING domains.

OK?

Jason
Mostafa Saleh Feb. 13, 2024, 5 p.m. UTC | #3 
On Tue, Feb 13, 2024 at 12:37:39PM -0400, Jason Gunthorpe wrote:
> On Tue, Feb 13, 2024 at 03:45:34PM +0000, Mostafa Saleh wrote:
> > Hi Jason,
> > 
> > On Tue, Feb 06, 2024 at 11:12:48AM -0400, Jason Gunthorpe wrote:
> > > Introducing global statics which are of type struct iommu_domain, not
> > > struct arm_smmu_domain makes it difficult to retain
> > > arm_smmu_master->domain, as it can no longer point to an IDENTITY or
> > > BLOCKED domain.
> > > 
> > > The only place that uses the value is arm_smmu_detach_dev(). Change things
> > > to work like other drivers and call iommu_get_domain_for_dev() to obtain
> > > the current domain.
> > > 
> > > The master->domain is subtly protecting the domain_head against being
> > > unused, change the domain_head to be INIT'd when the master is not
> > > attached to a domain instead of garbage/zero.
> > 
> > I don't this the problem here, neither the reason for initialising the
> > domain_head, can you please clarify the issue?
> 
> I didn't notice it either. Eric found it:
> 
> https://lore.kernel.org/linux-iommu/6fff20dd-46d5-4974-a4a5-fb4e7a59ce44@redhat.com/
> 
> > > @@ -2560,19 +2560,20 @@ static void arm_smmu_disable_pasid(struct arm_smmu_master *master)
> > >  
> > >  static void arm_smmu_detach_dev(struct arm_smmu_master *master)
> > >  {
> > > +	struct iommu_domain *domain = iommu_get_domain_for_dev(master->dev);
> > > +	struct arm_smmu_domain *smmu_domain;
> > >  	unsigned long flags;
> > > -	struct arm_smmu_domain *smmu_domain = master->domain;
> 
> master->domain is NULL here which happens in cases where the current
> RID domain is not a PAGING domain.
> 
> > > -	if (!smmu_domain)
> > > +	if (!domain)
> > >  		return;
> 
> Which used to early exit
> 
> > >  
> > > +	smmu_domain = to_smmu_domain(domain);
> > >  	arm_smmu_disable_ats(master, smmu_domain);
> > >  
> > >  	spin_lock_irqsave(&smmu_domain->devices_lock, flags);
> > > -	list_del(&master->domain_head);
> > > +	list_del_init(&master->domain_head);
> > >  	spin_unlock_irqrestore(&smmu_domain->devices_lock, flags);
> 
> But now would cause the list_del() to hit a non-inited list_head and
> explode.
> 
> Instead we keep the list head init'd and the list_del is a NOP.
> 
> Tricky right??
> 
> I changed the comment like this:
> 
> The master->domain is subtly protecting the master->domain_head against
> being unused as only PAGING domains will set master->domain and only
> paging domains use the master->domain_head. To make it simple keep the
> master->domain_head initialized so that the list_del() logic just does
> nothing for non-PAGING domains.
> 
> OK?

Ahh, I see, as iommu_get_domain_for_dev() now returns a valid domain.
Thanks for the explanation, that makes sense.

Reviewed-by: Mostafa Saleh <smostafa@google.com>

Thanks,
Mostafa
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 133f13f33df124..a98707cd1efccb 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
@@ -2560,19 +2560,20 @@  static void arm_smmu_disable_pasid(struct arm_smmu_master *master)
 
 static void arm_smmu_detach_dev(struct arm_smmu_master *master)
 {
+	struct iommu_domain *domain = iommu_get_domain_for_dev(master->dev);
+	struct arm_smmu_domain *smmu_domain;
 	unsigned long flags;
-	struct arm_smmu_domain *smmu_domain = master->domain;
 
-	if (!smmu_domain)
+	if (!domain)
 		return;
 
+	smmu_domain = to_smmu_domain(domain);
 	arm_smmu_disable_ats(master, smmu_domain);
 
 	spin_lock_irqsave(&smmu_domain->devices_lock, flags);
-	list_del(&master->domain_head);
+	list_del_init(&master->domain_head);
 	spin_unlock_irqrestore(&smmu_domain->devices_lock, flags);
 
-	master->domain = NULL;
 	master->ats_enabled = false;
 }
 
@@ -2626,8 +2627,6 @@  static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
 
 	arm_smmu_detach_dev(master);
 
-	master->domain = smmu_domain;
-
 	/*
 	 * The SMMU does not support enabling ATS with bypass. When the STE is
 	 * in bypass (STE.Config[2:0] == 0b100), ATS Translation Requests and
@@ -2646,10 +2645,8 @@  static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
 	case ARM_SMMU_DOMAIN_S1:
 		if (!master->cd_table.cdtab) {
 			ret = arm_smmu_alloc_cd_tables(master);
-			if (ret) {
-				master->domain = NULL;
+			if (ret)
 				goto out_list_del;
-			}
 		} else {
 			/*
 			 * arm_smmu_write_ctx_desc() relies on the entry being
@@ -2657,17 +2654,13 @@  static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
 			 */
 			ret = arm_smmu_write_ctx_desc(master, IOMMU_NO_PASID,
 						      NULL);
-			if (ret) {
-				master->domain = NULL;
+			if (ret)
 				goto out_list_del;
-			}
 		}
 
 		ret = arm_smmu_write_ctx_desc(master, IOMMU_NO_PASID, &smmu_domain->cd);
-		if (ret) {
-			master->domain = NULL;
+		if (ret)
 			goto out_list_del;
-		}
 
 		arm_smmu_make_cdtable_ste(&target, master);
 		arm_smmu_install_ste_for_dev(master, &target);
@@ -2693,7 +2686,7 @@  static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
 
 out_list_del:
 	spin_lock_irqsave(&smmu_domain->devices_lock, flags);
-	list_del(&master->domain_head);
+	list_del_init(&master->domain_head);
 	spin_unlock_irqrestore(&smmu_domain->devices_lock, flags);
 
 out_unlock:
@@ -2894,6 +2887,7 @@  static struct iommu_device *arm_smmu_probe_device(struct device *dev)
 	master->dev = dev;
 	master->smmu = smmu;
 	INIT_LIST_HEAD(&master->bonds);
+	INIT_LIST_HEAD(&master->domain_head);
 	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 cbf4b57719b7b9..587f99701ad30f 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.h
@@ -696,7 +696,6 @@  struct arm_smmu_stream {
 struct arm_smmu_master {
 	struct arm_smmu_device		*smmu;
 	struct device			*dev;
-	struct arm_smmu_domain		*domain;
 	struct list_head		domain_head;
 	struct arm_smmu_stream		*streams;
 	/* Locked by the iommu core using the group mutex */