[PATCH 0/7] Convert SMMU to domain_alloc_paging()

[PATCH 0/7] Convert SMMU to domain_alloc_paging()

[Jason Gunthorpe]

Add the global statics for IDENTITY and BLOCKED to the SMMU driver and
change to use domain_alloc_paging(). This allows SMMU to finalize the
domain during allocation.

This is a bit more urgent as I noticed while looking at Nicolin's patches
that the disable_bypass module parameter no longer works. The system will
boot fine but the domains will be set to IDENTITY now. To fix this the
core code must request BLOCKED domains when using ARM DMA ops with this
module option set.

This series fixes SMMU, which seems like the more important one of the
two. SMMUv3 needs a similar repair, but it is more complex to get the two
global static domains into the SMMUv3 driver. As SMMUv3 is primarily an
ARM64 driver this is less important since the normal DMA API flow already
substantially establishes blocking domains via empty IOMMU_DOMAIN_DMA
attachments. Regardless I have addressed SMMUv3 seperately.

This relies on the first few patches of the dart conversion series:

https://lore.kernel.org/r/0-v2-bff223cf6409+282-dart_paging_jgg@nvidia.com

To enable the BLOCKED global static.

Jason Gunthorpe (7):
  iommu/arm-smmu: Reorganize arm_smmu_domain_add_master()
  iommu/arm-smmu: Convert to a global static identity domain
  iommu/arm-smmu: Implement IOMMU_DOMAIN_BLOCKED
  iommu/arm-smmu: Pass arm_smmu_domain to arm_smmu_init_domain_context()
  iommu/arm-smmu: Convert to domain_alloc_paging()
  iommu: Compute dev_iommu->require_direct sooner
  iommu: Restore SMMU "disable_bypass"

 drivers/iommu/arm/arm-smmu/arm-smmu.c | 133 ++++++++++++++++++--------
 drivers/iommu/arm/arm-smmu/arm-smmu.h |   1 -
 drivers/iommu/iommu.c                 |  49 ++++++++--
 include/linux/iommu.h                 |   3 +
 4 files changed, 136 insertions(+), 50 deletions(-)


base-commit: 4dbf5230cbdc65dd71a8df9e0710e66f2997057c
-- 
2.42.0

[PATCH 0/7] Convert SMMU to domain_alloc_paging()

[Jason Gunthorpe]

Add the global statics for IDENTITY and BLOCKED to the SMMU driver and
change to use domain_alloc_paging(). This allows SMMU to finalize the
domain during allocation.

This is a bit more urgent as I noticed while looking at Nicolin's patches
that the disable_bypass module parameter no longer works. The system will
boot fine but the domains will be set to IDENTITY now. To fix this the
core code must request BLOCKED domains when using ARM DMA ops with this
module option set.

This series fixes SMMU, which seems like the more important one of the
two. SMMUv3 needs a similar repair, but it is more complex to get the two
global static domains into the SMMUv3 driver. As SMMUv3 is primarily an
ARM64 driver this is less important since the normal DMA API flow already
substantially establishes blocking domains via empty IOMMU_DOMAIN_DMA
attachments. Regardless I have addressed SMMUv3 seperately.

This relies on the first few patches of the dart conversion series:

https://lore.kernel.org/r/0-v2-bff223cf6409+282-dart_paging_jgg@nvidia.com

To enable the BLOCKED global static.

Jason Gunthorpe (7):
  iommu/arm-smmu: Reorganize arm_smmu_domain_add_master()
  iommu/arm-smmu: Convert to a global static identity domain
  iommu/arm-smmu: Implement IOMMU_DOMAIN_BLOCKED
  iommu/arm-smmu: Pass arm_smmu_domain to arm_smmu_init_domain_context()
  iommu/arm-smmu: Convert to domain_alloc_paging()
  iommu: Compute dev_iommu->require_direct sooner
  iommu: Restore SMMU "disable_bypass"

 drivers/iommu/arm/arm-smmu/arm-smmu.c | 133 ++++++++++++++++++--------
 drivers/iommu/arm/arm-smmu/arm-smmu.h |   1 -
 drivers/iommu/iommu.c                 |  49 ++++++++--
 include/linux/iommu.h                 |   3 +
 4 files changed, 136 insertions(+), 50 deletions(-)


base-commit: 4dbf5230cbdc65dd71a8df9e0710e66f2997057c
-- 
2.42.0


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

[PATCH 1/7] iommu/arm-smmu: Reorganize arm_smmu_domain_add_master()

[Jason Gunthorpe]

Make arm_smmu_domain_add_master() not use the smmu_domain to detect the
s2cr configuration, instead pass it in as a parameter. It always returns
zero so make it return void.

This is done to make the next two patches able to re-use this code without
forcing the creation of a struct arm_smmu_domain.

Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
 drivers/iommu/arm/arm-smmu/arm-smmu.c | 23 ++++++++++-------------
 1 file changed, 10 insertions(+), 13 deletions(-)

diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu.c b/drivers/iommu/arm/arm-smmu/arm-smmu.c
index d6d1a2a55cc069..7f33363719f4ac 100644
--- a/drivers/iommu/arm/arm-smmu/arm-smmu.c
+++ b/drivers/iommu/arm/arm-smmu/arm-smmu.c
@@ -1081,21 +1081,14 @@ static void arm_smmu_master_free_smes(struct arm_smmu_master_cfg *cfg,
 	mutex_unlock(&smmu->stream_map_mutex);
 }
 
-static int arm_smmu_domain_add_master(struct arm_smmu_domain *smmu_domain,
-				      struct arm_smmu_master_cfg *cfg,
-				      struct iommu_fwspec *fwspec)
+static void arm_smmu_domain_add_master(struct arm_smmu_device *smmu,
+				       struct arm_smmu_master_cfg *cfg,
+				       enum arm_smmu_s2cr_type type, u8 cbndx,
+				       struct iommu_fwspec *fwspec)
 {
-	struct arm_smmu_device *smmu = smmu_domain->smmu;
 	struct arm_smmu_s2cr *s2cr = smmu->s2crs;
-	u8 cbndx = smmu_domain->cfg.cbndx;
-	enum arm_smmu_s2cr_type type;
 	int i, idx;
 
-	if (smmu_domain->stage == ARM_SMMU_DOMAIN_BYPASS)
-		type = S2CR_TYPE_BYPASS;
-	else
-		type = S2CR_TYPE_TRANS;
-
 	for_each_cfg_sme(cfg, fwspec, i, idx) {
 		if (type == s2cr[idx].type && cbndx == s2cr[idx].cbndx)
 			continue;
@@ -1105,7 +1098,6 @@ static int arm_smmu_domain_add_master(struct arm_smmu_domain *smmu_domain,
 		s2cr[idx].cbndx = cbndx;
 		arm_smmu_write_s2cr(smmu, idx);
 	}
-	return 0;
 }
 
 static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
@@ -1153,7 +1145,12 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
 	}
 
 	/* Looks ok, so add the device to the domain */
-	ret = arm_smmu_domain_add_master(smmu_domain, cfg, fwspec);
+	arm_smmu_domain_add_master(smmu, cfg,
+				   smmu_domain->stage ==
+						   ARM_SMMU_DOMAIN_BYPASS ?
+					   S2CR_TYPE_BYPASS :
+					   S2CR_TYPE_TRANS,
+				   smmu_domain->cfg.cbndx, fwspec);
 
 	/*
 	 * Setup an autosuspend delay to avoid bouncing runpm state.
-- 
2.42.0

[PATCH 1/7] iommu/arm-smmu: Reorganize arm_smmu_domain_add_master()

[Jason Gunthorpe]

Make arm_smmu_domain_add_master() not use the smmu_domain to detect the
s2cr configuration, instead pass it in as a parameter. It always returns
zero so make it return void.

This is done to make the next two patches able to re-use this code without
forcing the creation of a struct arm_smmu_domain.

Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
 drivers/iommu/arm/arm-smmu/arm-smmu.c | 23 ++++++++++-------------
 1 file changed, 10 insertions(+), 13 deletions(-)

diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu.c b/drivers/iommu/arm/arm-smmu/arm-smmu.c
index d6d1a2a55cc069..7f33363719f4ac 100644
--- a/drivers/iommu/arm/arm-smmu/arm-smmu.c
+++ b/drivers/iommu/arm/arm-smmu/arm-smmu.c
@@ -1081,21 +1081,14 @@ static void arm_smmu_master_free_smes(struct arm_smmu_master_cfg *cfg,
 	mutex_unlock(&smmu->stream_map_mutex);
 }
 
-static int arm_smmu_domain_add_master(struct arm_smmu_domain *smmu_domain,
-				      struct arm_smmu_master_cfg *cfg,
-				      struct iommu_fwspec *fwspec)
+static void arm_smmu_domain_add_master(struct arm_smmu_device *smmu,
+				       struct arm_smmu_master_cfg *cfg,
+				       enum arm_smmu_s2cr_type type, u8 cbndx,
+				       struct iommu_fwspec *fwspec)
 {
-	struct arm_smmu_device *smmu = smmu_domain->smmu;
 	struct arm_smmu_s2cr *s2cr = smmu->s2crs;
-	u8 cbndx = smmu_domain->cfg.cbndx;
-	enum arm_smmu_s2cr_type type;
 	int i, idx;
 
-	if (smmu_domain->stage == ARM_SMMU_DOMAIN_BYPASS)
-		type = S2CR_TYPE_BYPASS;
-	else
-		type = S2CR_TYPE_TRANS;
-
 	for_each_cfg_sme(cfg, fwspec, i, idx) {
 		if (type == s2cr[idx].type && cbndx == s2cr[idx].cbndx)
 			continue;
@@ -1105,7 +1098,6 @@ static int arm_smmu_domain_add_master(struct arm_smmu_domain *smmu_domain,
 		s2cr[idx].cbndx = cbndx;
 		arm_smmu_write_s2cr(smmu, idx);
 	}
-	return 0;
 }
 
 static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
@@ -1153,7 +1145,12 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
 	}
 
 	/* Looks ok, so add the device to the domain */
-	ret = arm_smmu_domain_add_master(smmu_domain, cfg, fwspec);
+	arm_smmu_domain_add_master(smmu, cfg,
+				   smmu_domain->stage ==
+						   ARM_SMMU_DOMAIN_BYPASS ?
+					   S2CR_TYPE_BYPASS :
+					   S2CR_TYPE_TRANS,
+				   smmu_domain->cfg.cbndx, fwspec);
 
 	/*
 	 * Setup an autosuspend delay to avoid bouncing runpm state.
-- 
2.42.0


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

Re: [PATCH 1/7] iommu/arm-smmu: Reorganize arm_smmu_domain_add_master()

[Robin Murphy]

On 2023-10-05 19:28, Jason Gunthorpe wrote:
> Make arm_smmu_domain_add_master() not use the smmu_domain to detect the
> s2cr configuration, instead pass it in as a parameter. It always returns
> zero so make it return void.

It doesn't follow that a function named arm_smmu_domain_<operation>() 
should not operate on an arm_smmu_domain... I think this is the point to 
rename it to something like arm_smmu_master_install_s2crs() to reflect 
that what it's actually doing by now is a lot less than it did 10 years ago.

> This is done to make the next two patches able to re-use this code without
> forcing the creation of a struct arm_smmu_domain.
> 
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> ---
>   drivers/iommu/arm/arm-smmu/arm-smmu.c | 23 ++++++++++-------------
>   1 file changed, 10 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu.c b/drivers/iommu/arm/arm-smmu/arm-smmu.c
> index d6d1a2a55cc069..7f33363719f4ac 100644
> --- a/drivers/iommu/arm/arm-smmu/arm-smmu.c
> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu.c
> @@ -1081,21 +1081,14 @@ static void arm_smmu_master_free_smes(struct arm_smmu_master_cfg *cfg,
>   	mutex_unlock(&smmu->stream_map_mutex);
>   }
>   
> -static int arm_smmu_domain_add_master(struct arm_smmu_domain *smmu_domain,
> -				      struct arm_smmu_master_cfg *cfg,
> -				      struct iommu_fwspec *fwspec)
> +static void arm_smmu_domain_add_master(struct arm_smmu_device *smmu,

We already have the SMMU device in cfg->smmu, no need to pass it twice.

Thanks,
Robin.

> +				       struct arm_smmu_master_cfg *cfg,
> +				       enum arm_smmu_s2cr_type type, u8 cbndx,
> +				       struct iommu_fwspec *fwspec)
>   {
> -	struct arm_smmu_device *smmu = smmu_domain->smmu;
>   	struct arm_smmu_s2cr *s2cr = smmu->s2crs;
> -	u8 cbndx = smmu_domain->cfg.cbndx;
> -	enum arm_smmu_s2cr_type type;
>   	int i, idx;
>   
> -	if (smmu_domain->stage == ARM_SMMU_DOMAIN_BYPASS)
> -		type = S2CR_TYPE_BYPASS;
> -	else
> -		type = S2CR_TYPE_TRANS;
> -
>   	for_each_cfg_sme(cfg, fwspec, i, idx) {
>   		if (type == s2cr[idx].type && cbndx == s2cr[idx].cbndx)
>   			continue;
> @@ -1105,7 +1098,6 @@ static int arm_smmu_domain_add_master(struct arm_smmu_domain *smmu_domain,
>   		s2cr[idx].cbndx = cbndx;
>   		arm_smmu_write_s2cr(smmu, idx);
>   	}
> -	return 0;
>   }
>   
>   static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
> @@ -1153,7 +1145,12 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
>   	}
>   
>   	/* Looks ok, so add the device to the domain */
> -	ret = arm_smmu_domain_add_master(smmu_domain, cfg, fwspec);
> +	arm_smmu_domain_add_master(smmu, cfg,
> +				   smmu_domain->stage ==
> +						   ARM_SMMU_DOMAIN_BYPASS ?
> +					   S2CR_TYPE_BYPASS :
> +					   S2CR_TYPE_TRANS,
> +				   smmu_domain->cfg.cbndx, fwspec);
>   
>   	/*
>   	 * Setup an autosuspend delay to avoid bouncing runpm state.

Re: [PATCH 1/7] iommu/arm-smmu: Reorganize arm_smmu_domain_add_master()

[Robin Murphy]

On 2023-10-05 19:28, Jason Gunthorpe wrote:
> Make arm_smmu_domain_add_master() not use the smmu_domain to detect the
> s2cr configuration, instead pass it in as a parameter. It always returns
> zero so make it return void.

It doesn't follow that a function named arm_smmu_domain_<operation>() 
should not operate on an arm_smmu_domain... I think this is the point to 
rename it to something like arm_smmu_master_install_s2crs() to reflect 
that what it's actually doing by now is a lot less than it did 10 years ago.

> This is done to make the next two patches able to re-use this code without
> forcing the creation of a struct arm_smmu_domain.
> 
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> ---
>   drivers/iommu/arm/arm-smmu/arm-smmu.c | 23 ++++++++++-------------
>   1 file changed, 10 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu.c b/drivers/iommu/arm/arm-smmu/arm-smmu.c
> index d6d1a2a55cc069..7f33363719f4ac 100644
> --- a/drivers/iommu/arm/arm-smmu/arm-smmu.c
> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu.c
> @@ -1081,21 +1081,14 @@ static void arm_smmu_master_free_smes(struct arm_smmu_master_cfg *cfg,
>   	mutex_unlock(&smmu->stream_map_mutex);
>   }
>   
> -static int arm_smmu_domain_add_master(struct arm_smmu_domain *smmu_domain,
> -				      struct arm_smmu_master_cfg *cfg,
> -				      struct iommu_fwspec *fwspec)
> +static void arm_smmu_domain_add_master(struct arm_smmu_device *smmu,

We already have the SMMU device in cfg->smmu, no need to pass it twice.

Thanks,
Robin.

> +				       struct arm_smmu_master_cfg *cfg,
> +				       enum arm_smmu_s2cr_type type, u8 cbndx,
> +				       struct iommu_fwspec *fwspec)
>   {
> -	struct arm_smmu_device *smmu = smmu_domain->smmu;
>   	struct arm_smmu_s2cr *s2cr = smmu->s2crs;
> -	u8 cbndx = smmu_domain->cfg.cbndx;
> -	enum arm_smmu_s2cr_type type;
>   	int i, idx;
>   
> -	if (smmu_domain->stage == ARM_SMMU_DOMAIN_BYPASS)
> -		type = S2CR_TYPE_BYPASS;
> -	else
> -		type = S2CR_TYPE_TRANS;
> -
>   	for_each_cfg_sme(cfg, fwspec, i, idx) {
>   		if (type == s2cr[idx].type && cbndx == s2cr[idx].cbndx)
>   			continue;
> @@ -1105,7 +1098,6 @@ static int arm_smmu_domain_add_master(struct arm_smmu_domain *smmu_domain,
>   		s2cr[idx].cbndx = cbndx;
>   		arm_smmu_write_s2cr(smmu, idx);
>   	}
> -	return 0;
>   }
>   
>   static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
> @@ -1153,7 +1145,12 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
>   	}
>   
>   	/* Looks ok, so add the device to the domain */
> -	ret = arm_smmu_domain_add_master(smmu_domain, cfg, fwspec);
> +	arm_smmu_domain_add_master(smmu, cfg,
> +				   smmu_domain->stage ==
> +						   ARM_SMMU_DOMAIN_BYPASS ?
> +					   S2CR_TYPE_BYPASS :
> +					   S2CR_TYPE_TRANS,
> +				   smmu_domain->cfg.cbndx, fwspec);
>   
>   	/*
>   	 * Setup an autosuspend delay to avoid bouncing runpm state.

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

Re: [PATCH 1/7] iommu/arm-smmu: Reorganize arm_smmu_domain_add_master()

[Jason Gunthorpe]

On Fri, Oct 06, 2023 at 01:05:03PM +0100, Robin Murphy wrote:
> On 2023-10-05 19:28, Jason Gunthorpe wrote:
> > Make arm_smmu_domain_add_master() not use the smmu_domain to detect the
> > s2cr configuration, instead pass it in as a parameter. It always returns
> > zero so make it return void.
> 
> It doesn't follow that a function named arm_smmu_domain_<operation>() should
> not operate on an arm_smmu_domain... I think this is the point to rename it
> to something like arm_smmu_master_install_s2crs() to reflect that what it's
> actually doing by now is a lot less than it did 10 years ago.

heh, yes, I did not notice
 
> > diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu.c b/drivers/iommu/arm/arm-smmu/arm-smmu.c
> > index d6d1a2a55cc069..7f33363719f4ac 100644
> > --- a/drivers/iommu/arm/arm-smmu/arm-smmu.c
> > +++ b/drivers/iommu/arm/arm-smmu/arm-smmu.c
> > @@ -1081,21 +1081,14 @@ static void arm_smmu_master_free_smes(struct arm_smmu_master_cfg *cfg,
> >   	mutex_unlock(&smmu->stream_map_mutex);
> >   }
> > -static int arm_smmu_domain_add_master(struct arm_smmu_domain *smmu_domain,
> > -				      struct arm_smmu_master_cfg *cfg,
> > -				      struct iommu_fwspec *fwspec)
> > +static void arm_smmu_domain_add_master(struct arm_smmu_device *smmu,
> 
> We already have the SMMU device in cfg->smmu, no need to pass it twice.

Sure, but the caller also already has it on its stack..

Thanks,
Jason

Re: [PATCH 1/7] iommu/arm-smmu: Reorganize arm_smmu_domain_add_master()

[Jason Gunthorpe]

On Fri, Oct 06, 2023 at 01:05:03PM +0100, Robin Murphy wrote:
> On 2023-10-05 19:28, Jason Gunthorpe wrote:
> > Make arm_smmu_domain_add_master() not use the smmu_domain to detect the
> > s2cr configuration, instead pass it in as a parameter. It always returns
> > zero so make it return void.
> 
> It doesn't follow that a function named arm_smmu_domain_<operation>() should
> not operate on an arm_smmu_domain... I think this is the point to rename it
> to something like arm_smmu_master_install_s2crs() to reflect that what it's
> actually doing by now is a lot less than it did 10 years ago.

heh, yes, I did not notice
 
> > diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu.c b/drivers/iommu/arm/arm-smmu/arm-smmu.c
> > index d6d1a2a55cc069..7f33363719f4ac 100644
> > --- a/drivers/iommu/arm/arm-smmu/arm-smmu.c
> > +++ b/drivers/iommu/arm/arm-smmu/arm-smmu.c
> > @@ -1081,21 +1081,14 @@ static void arm_smmu_master_free_smes(struct arm_smmu_master_cfg *cfg,
> >   	mutex_unlock(&smmu->stream_map_mutex);
> >   }
> > -static int arm_smmu_domain_add_master(struct arm_smmu_domain *smmu_domain,
> > -				      struct arm_smmu_master_cfg *cfg,
> > -				      struct iommu_fwspec *fwspec)
> > +static void arm_smmu_domain_add_master(struct arm_smmu_device *smmu,
> 
> We already have the SMMU device in cfg->smmu, no need to pass it twice.

Sure, but the caller also already has it on its stack..

Thanks,
Jason

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

Re: [PATCH 1/7] iommu/arm-smmu: Reorganize arm_smmu_domain_add_master()

[Robin Murphy]

On 2023-10-06 13:30, Jason Gunthorpe wrote:
> On Fri, Oct 06, 2023 at 01:05:03PM +0100, Robin Murphy wrote:
>> On 2023-10-05 19:28, Jason Gunthorpe wrote:
>>> Make arm_smmu_domain_add_master() not use the smmu_domain to detect the
>>> s2cr configuration, instead pass it in as a parameter. It always returns
>>> zero so make it return void.
>>
>> It doesn't follow that a function named arm_smmu_domain_<operation>() should
>> not operate on an arm_smmu_domain... I think this is the point to rename it
>> to something like arm_smmu_master_install_s2crs() to reflect that what it's
>> actually doing by now is a lot less than it did 10 years ago.
> 
> heh, yes, I did not notice
>   
>>> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu.c b/drivers/iommu/arm/arm-smmu/arm-smmu.c
>>> index d6d1a2a55cc069..7f33363719f4ac 100644
>>> --- a/drivers/iommu/arm/arm-smmu/arm-smmu.c
>>> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu.c
>>> @@ -1081,21 +1081,14 @@ static void arm_smmu_master_free_smes(struct arm_smmu_master_cfg *cfg,
>>>    	mutex_unlock(&smmu->stream_map_mutex);
>>>    }
>>> -static int arm_smmu_domain_add_master(struct arm_smmu_domain *smmu_domain,
>>> -				      struct arm_smmu_master_cfg *cfg,
>>> -				      struct iommu_fwspec *fwspec)
>>> +static void arm_smmu_domain_add_master(struct arm_smmu_device *smmu,
>>
>> We already have the SMMU device in cfg->smmu, no need to pass it twice.
> 
> Sure, but the caller also already has it on its stack..

Stack? Caller? Not in my build ;)

00000000000040b8 <arm_smmu_attach_dev>:
...
         smmu = cfg->smmu;
     410c:       f94002d3        ldr     x19, [x22]
...
         smmu_domain->smmu = smmu;
     4268:       f90002f3        str     x19, [x23]
...
         struct arm_smmu_s2cr *s2cr = smmu->s2crs;
     44e0:       f9403a79        ldr     x25, [x19, #112]
...
         ret = arm_smmu_domain_add_master(smmu_domain, cfg, fwspec);
     4544:       5280001c        mov     w28, #0x0
...

Even if the compiler didn't inline the heck out of an easy inlining 
candidate, this is hardly a critical hot path where an extra load would 
make any difference, so the only thing this code needs optimising for is 
readability, or Donald Knuth will be sad :(

Thanks,
Robin.

Re: [PATCH 1/7] iommu/arm-smmu: Reorganize arm_smmu_domain_add_master()

[Robin Murphy]

On 2023-10-06 13:30, Jason Gunthorpe wrote:
> On Fri, Oct 06, 2023 at 01:05:03PM +0100, Robin Murphy wrote:
>> On 2023-10-05 19:28, Jason Gunthorpe wrote:
>>> Make arm_smmu_domain_add_master() not use the smmu_domain to detect the
>>> s2cr configuration, instead pass it in as a parameter. It always returns
>>> zero so make it return void.
>>
>> It doesn't follow that a function named arm_smmu_domain_<operation>() should
>> not operate on an arm_smmu_domain... I think this is the point to rename it
>> to something like arm_smmu_master_install_s2crs() to reflect that what it's
>> actually doing by now is a lot less than it did 10 years ago.
> 
> heh, yes, I did not notice
>   
>>> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu.c b/drivers/iommu/arm/arm-smmu/arm-smmu.c
>>> index d6d1a2a55cc069..7f33363719f4ac 100644
>>> --- a/drivers/iommu/arm/arm-smmu/arm-smmu.c
>>> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu.c
>>> @@ -1081,21 +1081,14 @@ static void arm_smmu_master_free_smes(struct arm_smmu_master_cfg *cfg,
>>>    	mutex_unlock(&smmu->stream_map_mutex);
>>>    }
>>> -static int arm_smmu_domain_add_master(struct arm_smmu_domain *smmu_domain,
>>> -				      struct arm_smmu_master_cfg *cfg,
>>> -				      struct iommu_fwspec *fwspec)
>>> +static void arm_smmu_domain_add_master(struct arm_smmu_device *smmu,
>>
>> We already have the SMMU device in cfg->smmu, no need to pass it twice.
> 
> Sure, but the caller also already has it on its stack..

Stack? Caller? Not in my build ;)

00000000000040b8 <arm_smmu_attach_dev>:
...
         smmu = cfg->smmu;
     410c:       f94002d3        ldr     x19, [x22]
...
         smmu_domain->smmu = smmu;
     4268:       f90002f3        str     x19, [x23]
...
         struct arm_smmu_s2cr *s2cr = smmu->s2crs;
     44e0:       f9403a79        ldr     x25, [x19, #112]
...
         ret = arm_smmu_domain_add_master(smmu_domain, cfg, fwspec);
     4544:       5280001c        mov     w28, #0x0
...

Even if the compiler didn't inline the heck out of an easy inlining 
candidate, this is hardly a critical hot path where an extra load would 
make any difference, so the only thing this code needs optimising for is 
readability, or Donald Knuth will be sad :(

Thanks,
Robin.

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

[PATCH 2/7] iommu/arm-smmu: Convert to a global static identity domain

[Jason Gunthorpe]

Create a global static identity domain with it's own
arm_smmu_attach_dev_identity() that simply calls
arm_smmu_domain_add_master() with the identity parameters.

This is done by giving the attach path for identity its own unique
implementation that simply calls arm_smmu_domain_add_master().

Remove ARM_SMMU_DOMAIN_BYPASS and all checks of IOMMU_DOMAIN_IDENTITY.

Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
 drivers/iommu/arm/arm-smmu/arm-smmu.c | 50 ++++++++++++++++++++-------
 drivers/iommu/arm/arm-smmu/arm-smmu.h |  1 -
 2 files changed, 37 insertions(+), 14 deletions(-)

diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu.c b/drivers/iommu/arm/arm-smmu/arm-smmu.c
index 7f33363719f4ac..141b8d1266d808 100644
--- a/drivers/iommu/arm/arm-smmu/arm-smmu.c
+++ b/drivers/iommu/arm/arm-smmu/arm-smmu.c
@@ -624,12 +624,6 @@ static int arm_smmu_init_domain_context(struct iommu_domain *domain,
 	if (smmu_domain->smmu)
 		goto out_unlock;
 
-	if (domain->type == IOMMU_DOMAIN_IDENTITY) {
-		smmu_domain->stage = ARM_SMMU_DOMAIN_BYPASS;
-		smmu_domain->smmu = smmu;
-		goto out_unlock;
-	}
-
 	/*
 	 * Mapping the requested stage onto what we support is surprisingly
 	 * complicated, mainly because the spec allows S1+S2 SMMUs without
@@ -825,7 +819,7 @@ static void arm_smmu_destroy_domain_context(struct iommu_domain *domain)
 	struct arm_smmu_cfg *cfg = &smmu_domain->cfg;
 	int ret, irq;
 
-	if (!smmu || domain->type == IOMMU_DOMAIN_IDENTITY)
+	if (!smmu)
 		return;
 
 	ret = arm_smmu_rpm_get(smmu);
@@ -854,7 +848,7 @@ static struct iommu_domain *arm_smmu_domain_alloc(unsigned type)
 {
 	struct arm_smmu_domain *smmu_domain;
 
-	if (type != IOMMU_DOMAIN_UNMANAGED && type != IOMMU_DOMAIN_IDENTITY) {
+	if (type != IOMMU_DOMAIN_UNMANAGED) {
 		if (using_legacy_binding || type != IOMMU_DOMAIN_DMA)
 			return NULL;
 	}
@@ -1145,11 +1139,7 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
 	}
 
 	/* Looks ok, so add the device to the domain */
-	arm_smmu_domain_add_master(smmu, cfg,
-				   smmu_domain->stage ==
-						   ARM_SMMU_DOMAIN_BYPASS ?
-					   S2CR_TYPE_BYPASS :
-					   S2CR_TYPE_TRANS,
+	arm_smmu_domain_add_master(smmu, cfg, S2CR_TYPE_TRANS,
 				   smmu_domain->cfg.cbndx, fwspec);
 
 	/*
@@ -1171,6 +1161,39 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
 	return ret;
 }
 
+static int arm_smmu_attach_dev_identity(struct iommu_domain *domain,
+					struct device *dev)
+{
+	struct arm_smmu_master_cfg *cfg = dev_iommu_priv_get(dev);
+	struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
+	struct arm_smmu_device *smmu;
+	int ret;
+
+	if (!cfg)
+		return -ENODEV;
+	smmu = cfg->smmu;
+
+	ret = arm_smmu_rpm_get(smmu);
+	if (ret < 0)
+		return ret;
+
+	arm_smmu_domain_add_master(smmu, cfg, S2CR_TYPE_BYPASS, 0, fwspec);
+
+	pm_runtime_set_autosuspend_delay(smmu->dev, 20);
+	pm_runtime_use_autosuspend(smmu->dev);
+	arm_smmu_rpm_put(smmu);
+	return 0;
+}
+
+static const struct iommu_domain_ops arm_smmu_identity_ops = {
+	.attach_dev = arm_smmu_attach_dev_identity,
+};
+
+static struct iommu_domain arm_smmu_identity_domain = {
+	.type = IOMMU_DOMAIN_IDENTITY,
+	.ops = &arm_smmu_identity_ops,
+};
+
 static int arm_smmu_map_pages(struct iommu_domain *domain, unsigned long iova,
 			      phys_addr_t paddr, size_t pgsize, size_t pgcount,
 			      int prot, gfp_t gfp, size_t *mapped)
@@ -1557,6 +1580,7 @@ static int arm_smmu_def_domain_type(struct device *dev)
 }
 
 static struct iommu_ops arm_smmu_ops = {
+	.identity_domain	= &arm_smmu_identity_domain,
 	.capable		= arm_smmu_capable,
 	.domain_alloc		= arm_smmu_domain_alloc,
 	.probe_device		= arm_smmu_probe_device,
diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu.h b/drivers/iommu/arm/arm-smmu/arm-smmu.h
index 703fd5817ec11f..836ed6799a801f 100644
--- a/drivers/iommu/arm/arm-smmu/arm-smmu.h
+++ b/drivers/iommu/arm/arm-smmu/arm-smmu.h
@@ -361,7 +361,6 @@ enum arm_smmu_domain_stage {
 	ARM_SMMU_DOMAIN_S1 = 0,
 	ARM_SMMU_DOMAIN_S2,
 	ARM_SMMU_DOMAIN_NESTED,
-	ARM_SMMU_DOMAIN_BYPASS,
 };
 
 struct arm_smmu_domain {
-- 
2.42.0

[PATCH 2/7] iommu/arm-smmu: Convert to a global static identity domain

[Jason Gunthorpe]

Create a global static identity domain with it's own
arm_smmu_attach_dev_identity() that simply calls
arm_smmu_domain_add_master() with the identity parameters.

This is done by giving the attach path for identity its own unique
implementation that simply calls arm_smmu_domain_add_master().

Remove ARM_SMMU_DOMAIN_BYPASS and all checks of IOMMU_DOMAIN_IDENTITY.

Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
 drivers/iommu/arm/arm-smmu/arm-smmu.c | 50 ++++++++++++++++++++-------
 drivers/iommu/arm/arm-smmu/arm-smmu.h |  1 -
 2 files changed, 37 insertions(+), 14 deletions(-)

diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu.c b/drivers/iommu/arm/arm-smmu/arm-smmu.c
index 7f33363719f4ac..141b8d1266d808 100644
--- a/drivers/iommu/arm/arm-smmu/arm-smmu.c
+++ b/drivers/iommu/arm/arm-smmu/arm-smmu.c
@@ -624,12 +624,6 @@ static int arm_smmu_init_domain_context(struct iommu_domain *domain,
 	if (smmu_domain->smmu)
 		goto out_unlock;
 
-	if (domain->type == IOMMU_DOMAIN_IDENTITY) {
-		smmu_domain->stage = ARM_SMMU_DOMAIN_BYPASS;
-		smmu_domain->smmu = smmu;
-		goto out_unlock;
-	}
-
 	/*
 	 * Mapping the requested stage onto what we support is surprisingly
 	 * complicated, mainly because the spec allows S1+S2 SMMUs without
@@ -825,7 +819,7 @@ static void arm_smmu_destroy_domain_context(struct iommu_domain *domain)
 	struct arm_smmu_cfg *cfg = &smmu_domain->cfg;
 	int ret, irq;
 
-	if (!smmu || domain->type == IOMMU_DOMAIN_IDENTITY)
+	if (!smmu)
 		return;
 
 	ret = arm_smmu_rpm_get(smmu);
@@ -854,7 +848,7 @@ static struct iommu_domain *arm_smmu_domain_alloc(unsigned type)
 {
 	struct arm_smmu_domain *smmu_domain;
 
-	if (type != IOMMU_DOMAIN_UNMANAGED && type != IOMMU_DOMAIN_IDENTITY) {
+	if (type != IOMMU_DOMAIN_UNMANAGED) {
 		if (using_legacy_binding || type != IOMMU_DOMAIN_DMA)
 			return NULL;
 	}
@@ -1145,11 +1139,7 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
 	}
 
 	/* Looks ok, so add the device to the domain */
-	arm_smmu_domain_add_master(smmu, cfg,
-				   smmu_domain->stage ==
-						   ARM_SMMU_DOMAIN_BYPASS ?
-					   S2CR_TYPE_BYPASS :
-					   S2CR_TYPE_TRANS,
+	arm_smmu_domain_add_master(smmu, cfg, S2CR_TYPE_TRANS,
 				   smmu_domain->cfg.cbndx, fwspec);
 
 	/*
@@ -1171,6 +1161,39 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
 	return ret;
 }
 
+static int arm_smmu_attach_dev_identity(struct iommu_domain *domain,
+					struct device *dev)
+{
+	struct arm_smmu_master_cfg *cfg = dev_iommu_priv_get(dev);
+	struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
+	struct arm_smmu_device *smmu;
+	int ret;
+
+	if (!cfg)
+		return -ENODEV;
+	smmu = cfg->smmu;
+
+	ret = arm_smmu_rpm_get(smmu);
+	if (ret < 0)
+		return ret;
+
+	arm_smmu_domain_add_master(smmu, cfg, S2CR_TYPE_BYPASS, 0, fwspec);
+
+	pm_runtime_set_autosuspend_delay(smmu->dev, 20);
+	pm_runtime_use_autosuspend(smmu->dev);
+	arm_smmu_rpm_put(smmu);
+	return 0;
+}
+
+static const struct iommu_domain_ops arm_smmu_identity_ops = {
+	.attach_dev = arm_smmu_attach_dev_identity,
+};
+
+static struct iommu_domain arm_smmu_identity_domain = {
+	.type = IOMMU_DOMAIN_IDENTITY,
+	.ops = &arm_smmu_identity_ops,
+};
+
 static int arm_smmu_map_pages(struct iommu_domain *domain, unsigned long iova,
 			      phys_addr_t paddr, size_t pgsize, size_t pgcount,
 			      int prot, gfp_t gfp, size_t *mapped)
@@ -1557,6 +1580,7 @@ static int arm_smmu_def_domain_type(struct device *dev)
 }
 
 static struct iommu_ops arm_smmu_ops = {
+	.identity_domain	= &arm_smmu_identity_domain,
 	.capable		= arm_smmu_capable,
 	.domain_alloc		= arm_smmu_domain_alloc,
 	.probe_device		= arm_smmu_probe_device,
diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu.h b/drivers/iommu/arm/arm-smmu/arm-smmu.h
index 703fd5817ec11f..836ed6799a801f 100644
--- a/drivers/iommu/arm/arm-smmu/arm-smmu.h
+++ b/drivers/iommu/arm/arm-smmu/arm-smmu.h
@@ -361,7 +361,6 @@ enum arm_smmu_domain_stage {
 	ARM_SMMU_DOMAIN_S1 = 0,
 	ARM_SMMU_DOMAIN_S2,
 	ARM_SMMU_DOMAIN_NESTED,
-	ARM_SMMU_DOMAIN_BYPASS,
 };
 
 struct arm_smmu_domain {
-- 
2.42.0


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

[PATCH 3/7] iommu/arm-smmu: Implement IOMMU_DOMAIN_BLOCKED

[Jason Gunthorpe]

Using the same design as IDENTITY setup a S2CR_TYPE_FAULT s2cr for the
device.

Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
 drivers/iommu/arm/arm-smmu/arm-smmu.c | 28 ++++++++++++++++++++++++---
 1 file changed, 25 insertions(+), 3 deletions(-)

diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu.c b/drivers/iommu/arm/arm-smmu/arm-smmu.c
index 141b8d1266d808..0fc4f2e8bf3ed5 100644
--- a/drivers/iommu/arm/arm-smmu/arm-smmu.c
+++ b/drivers/iommu/arm/arm-smmu/arm-smmu.c
@@ -1161,8 +1161,8 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
 	return ret;
 }
 
-static int arm_smmu_attach_dev_identity(struct iommu_domain *domain,
-					struct device *dev)
+static int arm_smmu_attach_dev_type(struct device *dev,
+				    enum arm_smmu_s2cr_type type)
 {
 	struct arm_smmu_master_cfg *cfg = dev_iommu_priv_get(dev);
 	struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
@@ -1177,7 +1177,7 @@ static int arm_smmu_attach_dev_identity(struct iommu_domain *domain,
 	if (ret < 0)
 		return ret;
 
-	arm_smmu_domain_add_master(smmu, cfg, S2CR_TYPE_BYPASS, 0, fwspec);
+	arm_smmu_domain_add_master(smmu, cfg, type, 0, fwspec);
 
 	pm_runtime_set_autosuspend_delay(smmu->dev, 20);
 	pm_runtime_use_autosuspend(smmu->dev);
@@ -1185,6 +1185,12 @@ static int arm_smmu_attach_dev_identity(struct iommu_domain *domain,
 	return 0;
 }
 
+static int arm_smmu_attach_dev_identity(struct iommu_domain *domain,
+					struct device *dev)
+{
+	return arm_smmu_attach_dev_type(dev, S2CR_TYPE_BYPASS);
+}
+
 static const struct iommu_domain_ops arm_smmu_identity_ops = {
 	.attach_dev = arm_smmu_attach_dev_identity,
 };
@@ -1194,6 +1200,21 @@ static struct iommu_domain arm_smmu_identity_domain = {
 	.ops = &arm_smmu_identity_ops,
 };
 
+static int arm_smmu_attach_dev_blocked(struct iommu_domain *domain,
+				       struct device *dev)
+{
+	return arm_smmu_attach_dev_type(dev, S2CR_TYPE_FAULT);
+}
+
+static const struct iommu_domain_ops arm_smmu_blocked_ops = {
+	.attach_dev = arm_smmu_attach_dev_blocked,
+};
+
+static struct iommu_domain arm_smmu_blocked_domain = {
+	.type = IOMMU_DOMAIN_BLOCKED,
+	.ops = &arm_smmu_blocked_ops,
+};
+
 static int arm_smmu_map_pages(struct iommu_domain *domain, unsigned long iova,
 			      phys_addr_t paddr, size_t pgsize, size_t pgcount,
 			      int prot, gfp_t gfp, size_t *mapped)
@@ -1581,6 +1602,7 @@ static int arm_smmu_def_domain_type(struct device *dev)
 
 static struct iommu_ops arm_smmu_ops = {
 	.identity_domain	= &arm_smmu_identity_domain,
+	.blocked_domain		= &arm_smmu_blocked_domain,
 	.capable		= arm_smmu_capable,
 	.domain_alloc		= arm_smmu_domain_alloc,
 	.probe_device		= arm_smmu_probe_device,
-- 
2.42.0

[PATCH 3/7] iommu/arm-smmu: Implement IOMMU_DOMAIN_BLOCKED

[Jason Gunthorpe]

Using the same design as IDENTITY setup a S2CR_TYPE_FAULT s2cr for the
device.

Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
 drivers/iommu/arm/arm-smmu/arm-smmu.c | 28 ++++++++++++++++++++++++---
 1 file changed, 25 insertions(+), 3 deletions(-)

diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu.c b/drivers/iommu/arm/arm-smmu/arm-smmu.c
index 141b8d1266d808..0fc4f2e8bf3ed5 100644
--- a/drivers/iommu/arm/arm-smmu/arm-smmu.c
+++ b/drivers/iommu/arm/arm-smmu/arm-smmu.c
@@ -1161,8 +1161,8 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
 	return ret;
 }
 
-static int arm_smmu_attach_dev_identity(struct iommu_domain *domain,
-					struct device *dev)
+static int arm_smmu_attach_dev_type(struct device *dev,
+				    enum arm_smmu_s2cr_type type)
 {
 	struct arm_smmu_master_cfg *cfg = dev_iommu_priv_get(dev);
 	struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
@@ -1177,7 +1177,7 @@ static int arm_smmu_attach_dev_identity(struct iommu_domain *domain,
 	if (ret < 0)
 		return ret;
 
-	arm_smmu_domain_add_master(smmu, cfg, S2CR_TYPE_BYPASS, 0, fwspec);
+	arm_smmu_domain_add_master(smmu, cfg, type, 0, fwspec);
 
 	pm_runtime_set_autosuspend_delay(smmu->dev, 20);
 	pm_runtime_use_autosuspend(smmu->dev);
@@ -1185,6 +1185,12 @@ static int arm_smmu_attach_dev_identity(struct iommu_domain *domain,
 	return 0;
 }
 
+static int arm_smmu_attach_dev_identity(struct iommu_domain *domain,
+					struct device *dev)
+{
+	return arm_smmu_attach_dev_type(dev, S2CR_TYPE_BYPASS);
+}
+
 static const struct iommu_domain_ops arm_smmu_identity_ops = {
 	.attach_dev = arm_smmu_attach_dev_identity,
 };
@@ -1194,6 +1200,21 @@ static struct iommu_domain arm_smmu_identity_domain = {
 	.ops = &arm_smmu_identity_ops,
 };
 
+static int arm_smmu_attach_dev_blocked(struct iommu_domain *domain,
+				       struct device *dev)
+{
+	return arm_smmu_attach_dev_type(dev, S2CR_TYPE_FAULT);
+}
+
+static const struct iommu_domain_ops arm_smmu_blocked_ops = {
+	.attach_dev = arm_smmu_attach_dev_blocked,
+};
+
+static struct iommu_domain arm_smmu_blocked_domain = {
+	.type = IOMMU_DOMAIN_BLOCKED,
+	.ops = &arm_smmu_blocked_ops,
+};
+
 static int arm_smmu_map_pages(struct iommu_domain *domain, unsigned long iova,
 			      phys_addr_t paddr, size_t pgsize, size_t pgcount,
 			      int prot, gfp_t gfp, size_t *mapped)
@@ -1581,6 +1602,7 @@ static int arm_smmu_def_domain_type(struct device *dev)
 
 static struct iommu_ops arm_smmu_ops = {
 	.identity_domain	= &arm_smmu_identity_domain,
+	.blocked_domain		= &arm_smmu_blocked_domain,
 	.capable		= arm_smmu_capable,
 	.domain_alloc		= arm_smmu_domain_alloc,
 	.probe_device		= arm_smmu_probe_device,
-- 
2.42.0


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

[PATCH 4/7] iommu/arm-smmu: Pass arm_smmu_domain to arm_smmu_init_domain_context()

[Jason Gunthorpe]

Instead of putting container_of() casts in the internals, use the proper
type in this call chain. This makes it easier to check that the two global
static domains are not leaking into call chains they should not.

Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
 drivers/iommu/arm/arm-smmu/arm-smmu.c | 28 +++++++++++++--------------
 1 file changed, 13 insertions(+), 15 deletions(-)

diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu.c b/drivers/iommu/arm/arm-smmu/arm-smmu.c
index 0fc4f2e8bf3ed5..bf5f541be2399f 100644
--- a/drivers/iommu/arm/arm-smmu/arm-smmu.c
+++ b/drivers/iommu/arm/arm-smmu/arm-smmu.c
@@ -393,7 +393,7 @@ static irqreturn_t arm_smmu_context_fault(int irq, void *dev)
 	u32 fsr, fsynr, cbfrsynra;
 	unsigned long iova;
 	struct iommu_domain *domain = dev;
-	struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
+	struct arm_smmu_domain *smmu_domain = dev;
 	struct arm_smmu_device *smmu = smmu_domain->smmu;
 	int idx = smmu_domain->cfg.cbndx;
 	int ret;
@@ -607,7 +607,7 @@ static int arm_smmu_alloc_context_bank(struct arm_smmu_domain *smmu_domain,
 	return __arm_smmu_alloc_bitmap(smmu->context_map, start, smmu->num_context_banks);
 }
 
-static int arm_smmu_init_domain_context(struct iommu_domain *domain,
+static int arm_smmu_init_domain_context(struct arm_smmu_domain *smmu_domain,
 					struct arm_smmu_device *smmu,
 					struct device *dev)
 {
@@ -616,7 +616,6 @@ static int arm_smmu_init_domain_context(struct iommu_domain *domain,
 	struct io_pgtable_ops *pgtbl_ops;
 	struct io_pgtable_cfg pgtbl_cfg;
 	enum io_pgtable_fmt fmt;
-	struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
 	struct arm_smmu_cfg *cfg = &smmu_domain->cfg;
 	irqreturn_t (*context_fault)(int irq, void *dev);
 
@@ -764,16 +763,16 @@ static int arm_smmu_init_domain_context(struct iommu_domain *domain,
 	}
 
 	/* Update the domain's page sizes to reflect the page table format */
-	domain->pgsize_bitmap = pgtbl_cfg.pgsize_bitmap;
+	smmu_domain->domain.pgsize_bitmap = pgtbl_cfg.pgsize_bitmap;
 
 	if (pgtbl_cfg.quirks & IO_PGTABLE_QUIRK_ARM_TTBR1) {
-		domain->geometry.aperture_start = ~0UL << ias;
-		domain->geometry.aperture_end = ~0UL;
+		smmu_domain->domain.geometry.aperture_start = ~0UL << ias;
+		smmu_domain->domain.geometry.aperture_end = ~0UL;
 	} else {
-		domain->geometry.aperture_end = (1UL << ias) - 1;
+		smmu_domain->domain.geometry.aperture_end = (1UL << ias) - 1;
 	}
 
-	domain->geometry.force_aperture = true;
+	smmu_domain->domain.geometry.force_aperture = true;
 
 	/* Initialise the context bank with our page table cfg */
 	arm_smmu_init_context_bank(smmu_domain, &pgtbl_cfg);
@@ -790,8 +789,8 @@ static int arm_smmu_init_domain_context(struct iommu_domain *domain,
 	else
 		context_fault = arm_smmu_context_fault;
 
-	ret = devm_request_irq(smmu->dev, irq, context_fault,
-			       IRQF_SHARED, "arm-smmu-context-fault", domain);
+	ret = devm_request_irq(smmu->dev, irq, context_fault, IRQF_SHARED,
+			       "arm-smmu-context-fault", smmu_domain);
 	if (ret < 0) {
 		dev_err(smmu->dev, "failed to request context IRQ %d (%u)\n",
 			cfg->irptndx, irq);
@@ -812,9 +811,8 @@ static int arm_smmu_init_domain_context(struct iommu_domain *domain,
 	return ret;
 }
 
-static void arm_smmu_destroy_domain_context(struct iommu_domain *domain)
+static void arm_smmu_destroy_domain_context(struct arm_smmu_domain *smmu_domain)
 {
-	struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
 	struct arm_smmu_device *smmu = smmu_domain->smmu;
 	struct arm_smmu_cfg *cfg = &smmu_domain->cfg;
 	int ret, irq;
@@ -835,7 +833,7 @@ static void arm_smmu_destroy_domain_context(struct iommu_domain *domain)
 
 	if (cfg->irptndx != ARM_SMMU_INVALID_IRPTNDX) {
 		irq = smmu->irqs[cfg->irptndx];
-		devm_free_irq(smmu->dev, irq, domain);
+		devm_free_irq(smmu->dev, irq, smmu_domain);
 	}
 
 	free_io_pgtable_ops(smmu_domain->pgtbl_ops);
@@ -875,7 +873,7 @@ static void arm_smmu_domain_free(struct iommu_domain *domain)
 	 * Free the domain resources. We assume that all devices have
 	 * already been detached.
 	 */
-	arm_smmu_destroy_domain_context(domain);
+	arm_smmu_destroy_domain_context(smmu_domain);
 	kfree(smmu_domain);
 }
 
@@ -1125,7 +1123,7 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
 		return ret;
 
 	/* Ensure that the domain is finalised */
-	ret = arm_smmu_init_domain_context(domain, smmu, dev);
+	ret = arm_smmu_init_domain_context(smmu_domain, smmu, dev);
 	if (ret < 0)
 		goto rpm_put;
 
-- 
2.42.0

[PATCH 4/7] iommu/arm-smmu: Pass arm_smmu_domain to arm_smmu_init_domain_context()

[Jason Gunthorpe]

Instead of putting container_of() casts in the internals, use the proper
type in this call chain. This makes it easier to check that the two global
static domains are not leaking into call chains they should not.

Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
 drivers/iommu/arm/arm-smmu/arm-smmu.c | 28 +++++++++++++--------------
 1 file changed, 13 insertions(+), 15 deletions(-)

diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu.c b/drivers/iommu/arm/arm-smmu/arm-smmu.c
index 0fc4f2e8bf3ed5..bf5f541be2399f 100644
--- a/drivers/iommu/arm/arm-smmu/arm-smmu.c
+++ b/drivers/iommu/arm/arm-smmu/arm-smmu.c
@@ -393,7 +393,7 @@ static irqreturn_t arm_smmu_context_fault(int irq, void *dev)
 	u32 fsr, fsynr, cbfrsynra;
 	unsigned long iova;
 	struct iommu_domain *domain = dev;
-	struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
+	struct arm_smmu_domain *smmu_domain = dev;
 	struct arm_smmu_device *smmu = smmu_domain->smmu;
 	int idx = smmu_domain->cfg.cbndx;
 	int ret;
@@ -607,7 +607,7 @@ static int arm_smmu_alloc_context_bank(struct arm_smmu_domain *smmu_domain,
 	return __arm_smmu_alloc_bitmap(smmu->context_map, start, smmu->num_context_banks);
 }
 
-static int arm_smmu_init_domain_context(struct iommu_domain *domain,
+static int arm_smmu_init_domain_context(struct arm_smmu_domain *smmu_domain,
 					struct arm_smmu_device *smmu,
 					struct device *dev)
 {
@@ -616,7 +616,6 @@ static int arm_smmu_init_domain_context(struct iommu_domain *domain,
 	struct io_pgtable_ops *pgtbl_ops;
 	struct io_pgtable_cfg pgtbl_cfg;
 	enum io_pgtable_fmt fmt;
-	struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
 	struct arm_smmu_cfg *cfg = &smmu_domain->cfg;
 	irqreturn_t (*context_fault)(int irq, void *dev);
 
@@ -764,16 +763,16 @@ static int arm_smmu_init_domain_context(struct iommu_domain *domain,
 	}
 
 	/* Update the domain's page sizes to reflect the page table format */
-	domain->pgsize_bitmap = pgtbl_cfg.pgsize_bitmap;
+	smmu_domain->domain.pgsize_bitmap = pgtbl_cfg.pgsize_bitmap;
 
 	if (pgtbl_cfg.quirks & IO_PGTABLE_QUIRK_ARM_TTBR1) {
-		domain->geometry.aperture_start = ~0UL << ias;
-		domain->geometry.aperture_end = ~0UL;
+		smmu_domain->domain.geometry.aperture_start = ~0UL << ias;
+		smmu_domain->domain.geometry.aperture_end = ~0UL;
 	} else {
-		domain->geometry.aperture_end = (1UL << ias) - 1;
+		smmu_domain->domain.geometry.aperture_end = (1UL << ias) - 1;
 	}
 
-	domain->geometry.force_aperture = true;
+	smmu_domain->domain.geometry.force_aperture = true;
 
 	/* Initialise the context bank with our page table cfg */
 	arm_smmu_init_context_bank(smmu_domain, &pgtbl_cfg);
@@ -790,8 +789,8 @@ static int arm_smmu_init_domain_context(struct iommu_domain *domain,
 	else
 		context_fault = arm_smmu_context_fault;
 
-	ret = devm_request_irq(smmu->dev, irq, context_fault,
-			       IRQF_SHARED, "arm-smmu-context-fault", domain);
+	ret = devm_request_irq(smmu->dev, irq, context_fault, IRQF_SHARED,
+			       "arm-smmu-context-fault", smmu_domain);
 	if (ret < 0) {
 		dev_err(smmu->dev, "failed to request context IRQ %d (%u)\n",
 			cfg->irptndx, irq);
@@ -812,9 +811,8 @@ static int arm_smmu_init_domain_context(struct iommu_domain *domain,
 	return ret;
 }
 
-static void arm_smmu_destroy_domain_context(struct iommu_domain *domain)
+static void arm_smmu_destroy_domain_context(struct arm_smmu_domain *smmu_domain)
 {
-	struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
 	struct arm_smmu_device *smmu = smmu_domain->smmu;
 	struct arm_smmu_cfg *cfg = &smmu_domain->cfg;
 	int ret, irq;
@@ -835,7 +833,7 @@ static void arm_smmu_destroy_domain_context(struct iommu_domain *domain)
 
 	if (cfg->irptndx != ARM_SMMU_INVALID_IRPTNDX) {
 		irq = smmu->irqs[cfg->irptndx];
-		devm_free_irq(smmu->dev, irq, domain);
+		devm_free_irq(smmu->dev, irq, smmu_domain);
 	}
 
 	free_io_pgtable_ops(smmu_domain->pgtbl_ops);
@@ -875,7 +873,7 @@ static void arm_smmu_domain_free(struct iommu_domain *domain)
 	 * Free the domain resources. We assume that all devices have
 	 * already been detached.
 	 */
-	arm_smmu_destroy_domain_context(domain);
+	arm_smmu_destroy_domain_context(smmu_domain);
 	kfree(smmu_domain);
 }
 
@@ -1125,7 +1123,7 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
 		return ret;
 
 	/* Ensure that the domain is finalised */
-	ret = arm_smmu_init_domain_context(domain, smmu, dev);
+	ret = arm_smmu_init_domain_context(smmu_domain, smmu, dev);
 	if (ret < 0)
 		goto rpm_put;
 
-- 
2.42.0


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

Re: [PATCH 4/7] iommu/arm-smmu: Pass arm_smmu_domain to arm_smmu_init_domain_context()

[Robin Murphy]

On 2023-10-05 19:28, Jason Gunthorpe wrote:
> Instead of putting container_of() casts in the internals, use the proper
> type in this call chain. This makes it easier to check that the two global
> static domains are not leaking into call chains they should not.

Is there something inherently difficult about to_smmu_domain()? It's 
hard to tell how the aforementioned checks might expect to work since 
they don't appear to be added anywhere :/

That's not to say that there's anything necessarily wrong with the logic 
of "use the internal type internally" if it doesn't hurt the code 
structure or readability, but this isn't justifying that for its own sake...

> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> ---
>   drivers/iommu/arm/arm-smmu/arm-smmu.c | 28 +++++++++++++--------------
>   1 file changed, 13 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu.c b/drivers/iommu/arm/arm-smmu/arm-smmu.c
> index 0fc4f2e8bf3ed5..bf5f541be2399f 100644
> --- a/drivers/iommu/arm/arm-smmu/arm-smmu.c
> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu.c
> @@ -393,7 +393,7 @@ static irqreturn_t arm_smmu_context_fault(int irq, void *dev)
>   	u32 fsr, fsynr, cbfrsynra;
>   	unsigned long iova;
>   	struct iommu_domain *domain = dev;
> -	struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
> +	struct arm_smmu_domain *smmu_domain = dev;
>   	struct arm_smmu_device *smmu = smmu_domain->smmu;
>   	int idx = smmu_domain->cfg.cbndx;
>   	int ret;
> @@ -607,7 +607,7 @@ static int arm_smmu_alloc_context_bank(struct arm_smmu_domain *smmu_domain,
>   	return __arm_smmu_alloc_bitmap(smmu->context_map, start, smmu->num_context_banks);
>   }
>   
> -static int arm_smmu_init_domain_context(struct iommu_domain *domain,
> +static int arm_smmu_init_domain_context(struct arm_smmu_domain *smmu_domain,
>   					struct arm_smmu_device *smmu,
>   					struct device *dev)
>   {
> @@ -616,7 +616,6 @@ static int arm_smmu_init_domain_context(struct iommu_domain *domain,
>   	struct io_pgtable_ops *pgtbl_ops;
>   	struct io_pgtable_cfg pgtbl_cfg;
>   	enum io_pgtable_fmt fmt;
> -	struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);

If at all, I think I'd rather just flip this to a local "struct 
iommu_domain *domain = &smmu_domain->domain;" and avoid the hunk of 
churn below. For a non-functional change with only a readability and 
maintenance angle (the compiler generally won't mind whether load/store 
offsets are positive or negative within the size of the overall 
structure), making those geometry assignments even denser isn't really 
helping.

Thanks,
Robin.

>   	struct arm_smmu_cfg *cfg = &smmu_domain->cfg;
>   	irqreturn_t (*context_fault)(int irq, void *dev);
>   
> @@ -764,16 +763,16 @@ static int arm_smmu_init_domain_context(struct iommu_domain *domain,
>   	}
>   
>   	/* Update the domain's page sizes to reflect the page table format */
> -	domain->pgsize_bitmap = pgtbl_cfg.pgsize_bitmap;
> +	smmu_domain->domain.pgsize_bitmap = pgtbl_cfg.pgsize_bitmap;
>   
>   	if (pgtbl_cfg.quirks & IO_PGTABLE_QUIRK_ARM_TTBR1) {
> -		domain->geometry.aperture_start = ~0UL << ias;
> -		domain->geometry.aperture_end = ~0UL;
> +		smmu_domain->domain.geometry.aperture_start = ~0UL << ias;
> +		smmu_domain->domain.geometry.aperture_end = ~0UL;
>   	} else {
> -		domain->geometry.aperture_end = (1UL << ias) - 1;
> +		smmu_domain->domain.geometry.aperture_end = (1UL << ias) - 1;
>   	}
>   
> -	domain->geometry.force_aperture = true;
> +	smmu_domain->domain.geometry.force_aperture = true;
>   
>   	/* Initialise the context bank with our page table cfg */
>   	arm_smmu_init_context_bank(smmu_domain, &pgtbl_cfg);
> @@ -790,8 +789,8 @@ static int arm_smmu_init_domain_context(struct iommu_domain *domain,
>   	else
>   		context_fault = arm_smmu_context_fault;
>   
> -	ret = devm_request_irq(smmu->dev, irq, context_fault,
> -			       IRQF_SHARED, "arm-smmu-context-fault", domain);
> +	ret = devm_request_irq(smmu->dev, irq, context_fault, IRQF_SHARED,
> +			       "arm-smmu-context-fault", smmu_domain);
>   	if (ret < 0) {
>   		dev_err(smmu->dev, "failed to request context IRQ %d (%u)\n",
>   			cfg->irptndx, irq);
> @@ -812,9 +811,8 @@ static int arm_smmu_init_domain_context(struct iommu_domain *domain,
>   	return ret;
>   }
>   
> -static void arm_smmu_destroy_domain_context(struct iommu_domain *domain)
> +static void arm_smmu_destroy_domain_context(struct arm_smmu_domain *smmu_domain)
>   {
> -	struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
>   	struct arm_smmu_device *smmu = smmu_domain->smmu;
>   	struct arm_smmu_cfg *cfg = &smmu_domain->cfg;
>   	int ret, irq;
> @@ -835,7 +833,7 @@ static void arm_smmu_destroy_domain_context(struct iommu_domain *domain)
>   
>   	if (cfg->irptndx != ARM_SMMU_INVALID_IRPTNDX) {
>   		irq = smmu->irqs[cfg->irptndx];
> -		devm_free_irq(smmu->dev, irq, domain);
> +		devm_free_irq(smmu->dev, irq, smmu_domain);
>   	}
>   
>   	free_io_pgtable_ops(smmu_domain->pgtbl_ops);
> @@ -875,7 +873,7 @@ static void arm_smmu_domain_free(struct iommu_domain *domain)
>   	 * Free the domain resources. We assume that all devices have
>   	 * already been detached.
>   	 */
> -	arm_smmu_destroy_domain_context(domain);
> +	arm_smmu_destroy_domain_context(smmu_domain);
>   	kfree(smmu_domain);
>   }
>   
> @@ -1125,7 +1123,7 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
>   		return ret;
>   
>   	/* Ensure that the domain is finalised */
> -	ret = arm_smmu_init_domain_context(domain, smmu, dev);
> +	ret = arm_smmu_init_domain_context(smmu_domain, smmu, dev);
>   	if (ret < 0)
>   		goto rpm_put;
>   

Re: [PATCH 4/7] iommu/arm-smmu: Pass arm_smmu_domain to arm_smmu_init_domain_context()

[Robin Murphy]

On 2023-10-05 19:28, Jason Gunthorpe wrote:
> Instead of putting container_of() casts in the internals, use the proper
> type in this call chain. This makes it easier to check that the two global
> static domains are not leaking into call chains they should not.

Is there something inherently difficult about to_smmu_domain()? It's 
hard to tell how the aforementioned checks might expect to work since 
they don't appear to be added anywhere :/

That's not to say that there's anything necessarily wrong with the logic 
of "use the internal type internally" if it doesn't hurt the code 
structure or readability, but this isn't justifying that for its own sake...

> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> ---
>   drivers/iommu/arm/arm-smmu/arm-smmu.c | 28 +++++++++++++--------------
>   1 file changed, 13 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu.c b/drivers/iommu/arm/arm-smmu/arm-smmu.c
> index 0fc4f2e8bf3ed5..bf5f541be2399f 100644
> --- a/drivers/iommu/arm/arm-smmu/arm-smmu.c
> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu.c
> @@ -393,7 +393,7 @@ static irqreturn_t arm_smmu_context_fault(int irq, void *dev)
>   	u32 fsr, fsynr, cbfrsynra;
>   	unsigned long iova;
>   	struct iommu_domain *domain = dev;
> -	struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
> +	struct arm_smmu_domain *smmu_domain = dev;
>   	struct arm_smmu_device *smmu = smmu_domain->smmu;
>   	int idx = smmu_domain->cfg.cbndx;
>   	int ret;
> @@ -607,7 +607,7 @@ static int arm_smmu_alloc_context_bank(struct arm_smmu_domain *smmu_domain,
>   	return __arm_smmu_alloc_bitmap(smmu->context_map, start, smmu->num_context_banks);
>   }
>   
> -static int arm_smmu_init_domain_context(struct iommu_domain *domain,
> +static int arm_smmu_init_domain_context(struct arm_smmu_domain *smmu_domain,
>   					struct arm_smmu_device *smmu,
>   					struct device *dev)
>   {
> @@ -616,7 +616,6 @@ static int arm_smmu_init_domain_context(struct iommu_domain *domain,
>   	struct io_pgtable_ops *pgtbl_ops;
>   	struct io_pgtable_cfg pgtbl_cfg;
>   	enum io_pgtable_fmt fmt;
> -	struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);

If at all, I think I'd rather just flip this to a local "struct 
iommu_domain *domain = &smmu_domain->domain;" and avoid the hunk of 
churn below. For a non-functional change with only a readability and 
maintenance angle (the compiler generally won't mind whether load/store 
offsets are positive or negative within the size of the overall 
structure), making those geometry assignments even denser isn't really 
helping.

Thanks,
Robin.

>   	struct arm_smmu_cfg *cfg = &smmu_domain->cfg;
>   	irqreturn_t (*context_fault)(int irq, void *dev);
>   
> @@ -764,16 +763,16 @@ static int arm_smmu_init_domain_context(struct iommu_domain *domain,
>   	}
>   
>   	/* Update the domain's page sizes to reflect the page table format */
> -	domain->pgsize_bitmap = pgtbl_cfg.pgsize_bitmap;
> +	smmu_domain->domain.pgsize_bitmap = pgtbl_cfg.pgsize_bitmap;
>   
>   	if (pgtbl_cfg.quirks & IO_PGTABLE_QUIRK_ARM_TTBR1) {
> -		domain->geometry.aperture_start = ~0UL << ias;
> -		domain->geometry.aperture_end = ~0UL;
> +		smmu_domain->domain.geometry.aperture_start = ~0UL << ias;
> +		smmu_domain->domain.geometry.aperture_end = ~0UL;
>   	} else {
> -		domain->geometry.aperture_end = (1UL << ias) - 1;
> +		smmu_domain->domain.geometry.aperture_end = (1UL << ias) - 1;
>   	}
>   
> -	domain->geometry.force_aperture = true;
> +	smmu_domain->domain.geometry.force_aperture = true;
>   
>   	/* Initialise the context bank with our page table cfg */
>   	arm_smmu_init_context_bank(smmu_domain, &pgtbl_cfg);
> @@ -790,8 +789,8 @@ static int arm_smmu_init_domain_context(struct iommu_domain *domain,
>   	else
>   		context_fault = arm_smmu_context_fault;
>   
> -	ret = devm_request_irq(smmu->dev, irq, context_fault,
> -			       IRQF_SHARED, "arm-smmu-context-fault", domain);
> +	ret = devm_request_irq(smmu->dev, irq, context_fault, IRQF_SHARED,
> +			       "arm-smmu-context-fault", smmu_domain);
>   	if (ret < 0) {
>   		dev_err(smmu->dev, "failed to request context IRQ %d (%u)\n",
>   			cfg->irptndx, irq);
> @@ -812,9 +811,8 @@ static int arm_smmu_init_domain_context(struct iommu_domain *domain,
>   	return ret;
>   }
>   
> -static void arm_smmu_destroy_domain_context(struct iommu_domain *domain)
> +static void arm_smmu_destroy_domain_context(struct arm_smmu_domain *smmu_domain)
>   {
> -	struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
>   	struct arm_smmu_device *smmu = smmu_domain->smmu;
>   	struct arm_smmu_cfg *cfg = &smmu_domain->cfg;
>   	int ret, irq;
> @@ -835,7 +833,7 @@ static void arm_smmu_destroy_domain_context(struct iommu_domain *domain)
>   
>   	if (cfg->irptndx != ARM_SMMU_INVALID_IRPTNDX) {
>   		irq = smmu->irqs[cfg->irptndx];
> -		devm_free_irq(smmu->dev, irq, domain);
> +		devm_free_irq(smmu->dev, irq, smmu_domain);
>   	}
>   
>   	free_io_pgtable_ops(smmu_domain->pgtbl_ops);
> @@ -875,7 +873,7 @@ static void arm_smmu_domain_free(struct iommu_domain *domain)
>   	 * Free the domain resources. We assume that all devices have
>   	 * already been detached.
>   	 */
> -	arm_smmu_destroy_domain_context(domain);
> +	arm_smmu_destroy_domain_context(smmu_domain);
>   	kfree(smmu_domain);
>   }
>   
> @@ -1125,7 +1123,7 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
>   		return ret;
>   
>   	/* Ensure that the domain is finalised */
> -	ret = arm_smmu_init_domain_context(domain, smmu, dev);
> +	ret = arm_smmu_init_domain_context(smmu_domain, smmu, dev);
>   	if (ret < 0)
>   		goto rpm_put;
>   

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

Re: [PATCH 4/7] iommu/arm-smmu: Pass arm_smmu_domain to arm_smmu_init_domain_context()

[Jason Gunthorpe]

On Fri, Oct 06, 2023 at 02:43:51PM +0100, Robin Murphy wrote:
> On 2023-10-05 19:28, Jason Gunthorpe wrote:
> > Instead of putting container_of() casts in the internals, use the proper
> > type in this call chain. This makes it easier to check that the two global
> > static domains are not leaking into call chains they should not.
>
> Is there something inherently difficult about to_smmu_domain()? It's hard to
> tell how the aforementioned checks might expect to work since they don't
> appear to be added anywhere :/

?? There are not added checks, this is talking about static checks and
code auditing.

Let's try the commit paragraph again:

Now that we have IDENTITY and BLOCKED domains that do not use the
struct arm_smmu_domain it is important that to_smmu_domain() is only
called on iommu_domain structs passed to the paging domain ops (aka
default_domain_ops). Use the more specific type in several call
chains and remove the few to_smmu_domain() calls that are not
obviously in an op call chain.

This makes it easier to audit the code that the two IDENTITY and
BLOCKED domains are not leaking someplace they should not.

> > @@ -616,7 +616,6 @@ static int arm_smmu_init_domain_context(struct iommu_domain *domain,
> >   	struct io_pgtable_ops *pgtbl_ops;
> >   	struct io_pgtable_cfg pgtbl_cfg;
> >   	enum io_pgtable_fmt fmt;
> > -	struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
> 
> If at all, I think I'd rather just flip this to a local "struct iommu_domain
> *domain = &smmu_domain->domain;" and avoid the hunk of churn below.

Sure

Jason

Re: [PATCH 4/7] iommu/arm-smmu: Pass arm_smmu_domain to arm_smmu_init_domain_context()

[Jason Gunthorpe]

On Fri, Oct 06, 2023 at 02:43:51PM +0100, Robin Murphy wrote:
> On 2023-10-05 19:28, Jason Gunthorpe wrote:
> > Instead of putting container_of() casts in the internals, use the proper
> > type in this call chain. This makes it easier to check that the two global
> > static domains are not leaking into call chains they should not.
>
> Is there something inherently difficult about to_smmu_domain()? It's hard to
> tell how the aforementioned checks might expect to work since they don't
> appear to be added anywhere :/

?? There are not added checks, this is talking about static checks and
code auditing.

Let's try the commit paragraph again:

Now that we have IDENTITY and BLOCKED domains that do not use the
struct arm_smmu_domain it is important that to_smmu_domain() is only
called on iommu_domain structs passed to the paging domain ops (aka
default_domain_ops). Use the more specific type in several call
chains and remove the few to_smmu_domain() calls that are not
obviously in an op call chain.

This makes it easier to audit the code that the two IDENTITY and
BLOCKED domains are not leaking someplace they should not.

> > @@ -616,7 +616,6 @@ static int arm_smmu_init_domain_context(struct iommu_domain *domain,
> >   	struct io_pgtable_ops *pgtbl_ops;
> >   	struct io_pgtable_cfg pgtbl_cfg;
> >   	enum io_pgtable_fmt fmt;
> > -	struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
> 
> If at all, I think I'd rather just flip this to a local "struct iommu_domain
> *domain = &smmu_domain->domain;" and avoid the hunk of churn below.

Sure

Jason

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

Re: [PATCH 4/7] iommu/arm-smmu: Pass arm_smmu_domain to arm_smmu_init_domain_context()

[Robin Murphy]

On 2023-10-06 14:53, Jason Gunthorpe wrote:
> On Fri, Oct 06, 2023 at 02:43:51PM +0100, Robin Murphy wrote:
>> On 2023-10-05 19:28, Jason Gunthorpe wrote:
>>> Instead of putting container_of() casts in the internals, use the proper
>>> type in this call chain. This makes it easier to check that the two global
>>> static domains are not leaking into call chains they should not.
>>
>> Is there something inherently difficult about to_smmu_domain()? It's hard to
>> tell how the aforementioned checks might expect to work since they don't
>> appear to be added anywhere :/
> 
> ?? There are not added checks, this is talking about static checks and
> code auditing.
> 
> Let's try the commit paragraph again:
> 
> Now that we have IDENTITY and BLOCKED domains that do not use the
> struct arm_smmu_domain it is important that to_smmu_domain() is only
> called on iommu_domain structs passed to the paging domain ops (aka
> default_domain_ops). Use the more specific type in several call
> chains and remove the few to_smmu_domain() calls that are not
> obviously in an op call chain.
> 
> This makes it easier to audit the code that the two IDENTITY and
> BLOCKED domains are not leaking someplace they should not.

How? It should already be trivial to confirm that the driver is not 
itself making any reference to arm_smmu_identity_domain or 
arm_smmu_blocked_domain other than exposing them to the core API, with 
no more than a simple grep. And if the core code does somehow screw up 
at runtime and they get passed back into arm_smmu_attach_dev() then 
that's still going to use to_smmu_domain() on them and propagate that to 
its callees, with exactly the same effect as if they did so themselves. 
I fail to understand what you think you can achieve here.

The only code that deals with the static domains is 
arm_smmu_attach_dev_identity() and arm_smmu_attach_dev_blocked(), and 
you've already implemented those quite happily prior to this change, 
leaving no foreseeable reason to rework them further. So AFAICS the only 
scenario where this could make any practical difference is if someone 
now makes an unnecessary and nonsensical change to have one of those 
call into random other parts of the driver. But by that point, why would 
you be confident that they wouldn't simply cargo-cult a to_smmu_domain() 
invocation to "fix" the type mismatch at the callsite, since that's what 
other domain callbacks do?

It's never worth doing anything for the sole reason of trying to make it 
slightly harder for someone who doesn't understand the code to break the 
code. Either it's an entirely imagined problem to begin with, or 
otherwise nature will always find a way...

Thanks,
Robin.

Re: [PATCH 4/7] iommu/arm-smmu: Pass arm_smmu_domain to arm_smmu_init_domain_context()

[Robin Murphy]

On 2023-10-06 14:53, Jason Gunthorpe wrote:
> On Fri, Oct 06, 2023 at 02:43:51PM +0100, Robin Murphy wrote:
>> On 2023-10-05 19:28, Jason Gunthorpe wrote:
>>> Instead of putting container_of() casts in the internals, use the proper
>>> type in this call chain. This makes it easier to check that the two global
>>> static domains are not leaking into call chains they should not.
>>
>> Is there something inherently difficult about to_smmu_domain()? It's hard to
>> tell how the aforementioned checks might expect to work since they don't
>> appear to be added anywhere :/
> 
> ?? There are not added checks, this is talking about static checks and
> code auditing.
> 
> Let's try the commit paragraph again:
> 
> Now that we have IDENTITY and BLOCKED domains that do not use the
> struct arm_smmu_domain it is important that to_smmu_domain() is only
> called on iommu_domain structs passed to the paging domain ops (aka
> default_domain_ops). Use the more specific type in several call
> chains and remove the few to_smmu_domain() calls that are not
> obviously in an op call chain.
> 
> This makes it easier to audit the code that the two IDENTITY and
> BLOCKED domains are not leaking someplace they should not.

How? It should already be trivial to confirm that the driver is not 
itself making any reference to arm_smmu_identity_domain or 
arm_smmu_blocked_domain other than exposing them to the core API, with 
no more than a simple grep. And if the core code does somehow screw up 
at runtime and they get passed back into arm_smmu_attach_dev() then 
that's still going to use to_smmu_domain() on them and propagate that to 
its callees, with exactly the same effect as if they did so themselves. 
I fail to understand what you think you can achieve here.

The only code that deals with the static domains is 
arm_smmu_attach_dev_identity() and arm_smmu_attach_dev_blocked(), and 
you've already implemented those quite happily prior to this change, 
leaving no foreseeable reason to rework them further. So AFAICS the only 
scenario where this could make any practical difference is if someone 
now makes an unnecessary and nonsensical change to have one of those 
call into random other parts of the driver. But by that point, why would 
you be confident that they wouldn't simply cargo-cult a to_smmu_domain() 
invocation to "fix" the type mismatch at the callsite, since that's what 
other domain callbacks do?

It's never worth doing anything for the sole reason of trying to make it 
slightly harder for someone who doesn't understand the code to break the 
code. Either it's an entirely imagined problem to begin with, or 
otherwise nature will always find a way...

Thanks,
Robin.

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

Re: [PATCH 4/7] iommu/arm-smmu: Pass arm_smmu_domain to arm_smmu_init_domain_context()

[Jason Gunthorpe]

On Fri, Oct 06, 2023 at 03:56:15PM +0100, Robin Murphy wrote:
> On 2023-10-06 14:53, Jason Gunthorpe wrote:
> > On Fri, Oct 06, 2023 at 02:43:51PM +0100, Robin Murphy wrote:
> > > On 2023-10-05 19:28, Jason Gunthorpe wrote:
> > > > Instead of putting container_of() casts in the internals, use the proper
> > > > type in this call chain. This makes it easier to check that the two global
> > > > static domains are not leaking into call chains they should not.
> > > 
> > > Is there something inherently difficult about to_smmu_domain()? It's hard to
> > > tell how the aforementioned checks might expect to work since they don't
> > > appear to be added anywhere :/
> > 
> > ?? There are not added checks, this is talking about static checks and
> > code auditing.
> > 
> > Let's try the commit paragraph again:
> > 
> > Now that we have IDENTITY and BLOCKED domains that do not use the
> > struct arm_smmu_domain it is important that to_smmu_domain() is only
> > called on iommu_domain structs passed to the paging domain ops (aka
> > default_domain_ops). Use the more specific type in several call
> > chains and remove the few to_smmu_domain() calls that are not
> > obviously in an op call chain.
> > 
> > This makes it easier to audit the code that the two IDENTITY and
> > BLOCKED domains are not leaking someplace they should not.
> 
> How? It should already be trivial to confirm that the driver is not itself
> making any reference to arm_smmu_identity_domain or arm_smmu_blocked_domain
> other than exposing them to the core API, with no more than a simple grep.
> And if the core code does somehow screw up at runtime and they get passed
> back into arm_smmu_attach_dev() then that's still going to use
> to_smmu_domain() on them and propagate that to its callees, with exactly the
> same effect as if they did so themselves. I fail to understand what you
> think you can achieve here.

I had to audit all of this to make sure. The to_smmu_domain() calls
here required some extra work to check because they are in an
interrupt, not a domain op.

I went through and checked it, why shouldn't I more fully document
that I checked it and it is in fact OK?

Why are you objecting to this? It clearly makes the thing easier to
follow to use the more specific types?!

> It's never worth doing anything for the sole reason of trying to make it
> slightly harder for someone who doesn't understand the code to break the
> code.

Maintainabiliy is making the code easier to understand as a merit on
its own :(

Jason

Re: [PATCH 4/7] iommu/arm-smmu: Pass arm_smmu_domain to arm_smmu_init_domain_context()

[Jason Gunthorpe]

On Fri, Oct 06, 2023 at 03:56:15PM +0100, Robin Murphy wrote:
> On 2023-10-06 14:53, Jason Gunthorpe wrote:
> > On Fri, Oct 06, 2023 at 02:43:51PM +0100, Robin Murphy wrote:
> > > On 2023-10-05 19:28, Jason Gunthorpe wrote:
> > > > Instead of putting container_of() casts in the internals, use the proper
> > > > type in this call chain. This makes it easier to check that the two global
> > > > static domains are not leaking into call chains they should not.
> > > 
> > > Is there something inherently difficult about to_smmu_domain()? It's hard to
> > > tell how the aforementioned checks might expect to work since they don't
> > > appear to be added anywhere :/
> > 
> > ?? There are not added checks, this is talking about static checks and
> > code auditing.
> > 
> > Let's try the commit paragraph again:
> > 
> > Now that we have IDENTITY and BLOCKED domains that do not use the
> > struct arm_smmu_domain it is important that to_smmu_domain() is only
> > called on iommu_domain structs passed to the paging domain ops (aka
> > default_domain_ops). Use the more specific type in several call
> > chains and remove the few to_smmu_domain() calls that are not
> > obviously in an op call chain.
> > 
> > This makes it easier to audit the code that the two IDENTITY and
> > BLOCKED domains are not leaking someplace they should not.
> 
> How? It should already be trivial to confirm that the driver is not itself
> making any reference to arm_smmu_identity_domain or arm_smmu_blocked_domain
> other than exposing them to the core API, with no more than a simple grep.
> And if the core code does somehow screw up at runtime and they get passed
> back into arm_smmu_attach_dev() then that's still going to use
> to_smmu_domain() on them and propagate that to its callees, with exactly the
> same effect as if they did so themselves. I fail to understand what you
> think you can achieve here.

I had to audit all of this to make sure. The to_smmu_domain() calls
here required some extra work to check because they are in an
interrupt, not a domain op.

I went through and checked it, why shouldn't I more fully document
that I checked it and it is in fact OK?

Why are you objecting to this? It clearly makes the thing easier to
follow to use the more specific types?!

> It's never worth doing anything for the sole reason of trying to make it
> slightly harder for someone who doesn't understand the code to break the
> code.

Maintainabiliy is making the code easier to understand as a merit on
its own :(

Jason

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

Re: [PATCH 4/7] iommu/arm-smmu: Pass arm_smmu_domain to arm_smmu_init_domain_context()

[Steven Price]

On 05/10/2023 19:28, Jason Gunthorpe wrote:
> Instead of putting container_of() casts in the internals, use the proper
> type in this call chain. This makes it easier to check that the two global
> static domains are not leaking into call chains they should not.
> 
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> ---
>  drivers/iommu/arm/arm-smmu/arm-smmu.c | 28 +++++++++++++--------------
>  1 file changed, 13 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu.c b/drivers/iommu/arm/arm-smmu/arm-smmu.c
> index 0fc4f2e8bf3ed5..bf5f541be2399f 100644
> --- a/drivers/iommu/arm/arm-smmu/arm-smmu.c
> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu.c
> @@ -393,7 +393,7 @@ static irqreturn_t arm_smmu_context_fault(int irq, void *dev)
>  	u32 fsr, fsynr, cbfrsynra;
>  	unsigned long iova;
>  	struct iommu_domain *domain = dev;
> -	struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
> +	struct arm_smmu_domain *smmu_domain = dev;

Leaving aside Robin's objections - this change is clearly bogus. 'dev'
is now being case to both struct iommu_domain and struct
arm_smmu_domain. And AFAICT that won't even "happen to work" because the
struct iommu_domain isn't the first element of struct arm_smmu_domain.

Steve

Re: [PATCH 4/7] iommu/arm-smmu: Pass arm_smmu_domain to arm_smmu_init_domain_context()

[Steven Price]

On 05/10/2023 19:28, Jason Gunthorpe wrote:
> Instead of putting container_of() casts in the internals, use the proper
> type in this call chain. This makes it easier to check that the two global
> static domains are not leaking into call chains they should not.
> 
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> ---
>  drivers/iommu/arm/arm-smmu/arm-smmu.c | 28 +++++++++++++--------------
>  1 file changed, 13 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu.c b/drivers/iommu/arm/arm-smmu/arm-smmu.c
> index 0fc4f2e8bf3ed5..bf5f541be2399f 100644
> --- a/drivers/iommu/arm/arm-smmu/arm-smmu.c
> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu.c
> @@ -393,7 +393,7 @@ static irqreturn_t arm_smmu_context_fault(int irq, void *dev)
>  	u32 fsr, fsynr, cbfrsynra;
>  	unsigned long iova;
>  	struct iommu_domain *domain = dev;
> -	struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
> +	struct arm_smmu_domain *smmu_domain = dev;

Leaving aside Robin's objections - this change is clearly bogus. 'dev'
is now being case to both struct iommu_domain and struct
arm_smmu_domain. And AFAICT that won't even "happen to work" because the
struct iommu_domain isn't the first element of struct arm_smmu_domain.

Steve


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

Re: [PATCH 4/7] iommu/arm-smmu: Pass arm_smmu_domain to arm_smmu_init_domain_context()

[Jason Gunthorpe]

On Fri, Oct 06, 2023 at 04:11:22PM +0100, Steven Price wrote:
> On 05/10/2023 19:28, Jason Gunthorpe wrote:
> > Instead of putting container_of() casts in the internals, use the proper
> > type in this call chain. This makes it easier to check that the two global
> > static domains are not leaking into call chains they should not.
> > 
> > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> > ---
> >  drivers/iommu/arm/arm-smmu/arm-smmu.c | 28 +++++++++++++--------------
> >  1 file changed, 13 insertions(+), 15 deletions(-)
> > 
> > diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu.c b/drivers/iommu/arm/arm-smmu/arm-smmu.c
> > index 0fc4f2e8bf3ed5..bf5f541be2399f 100644
> > --- a/drivers/iommu/arm/arm-smmu/arm-smmu.c
> > +++ b/drivers/iommu/arm/arm-smmu/arm-smmu.c
> > @@ -393,7 +393,7 @@ static irqreturn_t arm_smmu_context_fault(int irq, void *dev)
> >  	u32 fsr, fsynr, cbfrsynra;
> >  	unsigned long iova;
> >  	struct iommu_domain *domain = dev;
> > -	struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
> > +	struct arm_smmu_domain *smmu_domain = dev;
> 
> Leaving aside Robin's objections - this change is clearly bogus. 'dev'
> is now being case to both struct iommu_domain and struct
> arm_smmu_domain. And AFAICT that won't even "happen to work" because the
> struct iommu_domain isn't the first element of struct
> arm_smmu_domain.

Oh I totally missed that! I fixed it like this:

--- a/drivers/iommu/arm/arm-smmu/arm-smmu.c
+++ b/drivers/iommu/arm/arm-smmu/arm-smmu.c
@@ -392,7 +392,6 @@ static irqreturn_t arm_smmu_context_fault(int irq, void *dev)
 {
        u32 fsr, fsynr, cbfrsynra;
        unsigned long iova;
-       struct iommu_domain *domain = dev;
        struct arm_smmu_domain *smmu_domain = dev;
        struct arm_smmu_device *smmu = smmu_domain->smmu;
        int idx = smmu_domain->cfg.cbndx;
@@ -406,7 +405,7 @@ static irqreturn_t arm_smmu_context_fault(int irq, void *dev)
        iova = arm_smmu_cb_readq(smmu, idx, ARM_SMMU_CB_FAR);
        cbfrsynra = arm_smmu_gr1_read(smmu, ARM_SMMU_GR1_CBFRSYNRA(idx));
 
-       ret = report_iommu_fault(domain, NULL, iova,
+       ret = report_iommu_fault(&smmu_domain->domain, NULL, iova,
                fsynr & ARM_SMMU_FSYNR0_WNR ? IOMMU_FAULT_WRITE : IOMMU_FAULT_READ);

Thanks!
Jason

Re: [PATCH 4/7] iommu/arm-smmu: Pass arm_smmu_domain to arm_smmu_init_domain_context()

[Jason Gunthorpe]

On Fri, Oct 06, 2023 at 04:11:22PM +0100, Steven Price wrote:
> On 05/10/2023 19:28, Jason Gunthorpe wrote:
> > Instead of putting container_of() casts in the internals, use the proper
> > type in this call chain. This makes it easier to check that the two global
> > static domains are not leaking into call chains they should not.
> > 
> > Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> > ---
> >  drivers/iommu/arm/arm-smmu/arm-smmu.c | 28 +++++++++++++--------------
> >  1 file changed, 13 insertions(+), 15 deletions(-)
> > 
> > diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu.c b/drivers/iommu/arm/arm-smmu/arm-smmu.c
> > index 0fc4f2e8bf3ed5..bf5f541be2399f 100644
> > --- a/drivers/iommu/arm/arm-smmu/arm-smmu.c
> > +++ b/drivers/iommu/arm/arm-smmu/arm-smmu.c
> > @@ -393,7 +393,7 @@ static irqreturn_t arm_smmu_context_fault(int irq, void *dev)
> >  	u32 fsr, fsynr, cbfrsynra;
> >  	unsigned long iova;
> >  	struct iommu_domain *domain = dev;
> > -	struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
> > +	struct arm_smmu_domain *smmu_domain = dev;
> 
> Leaving aside Robin's objections - this change is clearly bogus. 'dev'
> is now being case to both struct iommu_domain and struct
> arm_smmu_domain. And AFAICT that won't even "happen to work" because the
> struct iommu_domain isn't the first element of struct
> arm_smmu_domain.

Oh I totally missed that! I fixed it like this:

--- a/drivers/iommu/arm/arm-smmu/arm-smmu.c
+++ b/drivers/iommu/arm/arm-smmu/arm-smmu.c
@@ -392,7 +392,6 @@ static irqreturn_t arm_smmu_context_fault(int irq, void *dev)
 {
        u32 fsr, fsynr, cbfrsynra;
        unsigned long iova;
-       struct iommu_domain *domain = dev;
        struct arm_smmu_domain *smmu_domain = dev;
        struct arm_smmu_device *smmu = smmu_domain->smmu;
        int idx = smmu_domain->cfg.cbndx;
@@ -406,7 +405,7 @@ static irqreturn_t arm_smmu_context_fault(int irq, void *dev)
        iova = arm_smmu_cb_readq(smmu, idx, ARM_SMMU_CB_FAR);
        cbfrsynra = arm_smmu_gr1_read(smmu, ARM_SMMU_GR1_CBFRSYNRA(idx));
 
-       ret = report_iommu_fault(domain, NULL, iova,
+       ret = report_iommu_fault(&smmu_domain->domain, NULL, iova,
                fsynr & ARM_SMMU_FSYNR0_WNR ? IOMMU_FAULT_WRITE : IOMMU_FAULT_READ);

Thanks!
Jason

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

[PATCH 5/7] iommu/arm-smmu: Convert to domain_alloc_paging()

[Jason Gunthorpe]

Now that the BLOCKED and IDENTITY behaviors are managed with their own
domains change to the domain_alloc_paging() op.

The check for using_legacy_binding is now redundant,
arm_smmu_def_domain_type() always returns IOMMU_DOMAIN_IDENTITY for this
mode, so the core code will never attempt to create a DMA domain in the
first place.

Since commit a4fdd9762272 ("iommu: Use flush queue capability") the core
code only passes in IDENTITY/BLOCKED/UNMANAGED/DMA domain types. It will
not pass in IDENTITY or BLOCKED if the global statics exist, so the test
for DMA is also redundant now too.

Call arm_smmu_init_domain_context() early if a dev is available.

Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
 drivers/iommu/arm/arm-smmu/arm-smmu.c | 21 +++++++++++++++------
 1 file changed, 15 insertions(+), 6 deletions(-)

diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu.c b/drivers/iommu/arm/arm-smmu/arm-smmu.c
index bf5f541be2399f..4d69bb63ba1a3e 100644
--- a/drivers/iommu/arm/arm-smmu/arm-smmu.c
+++ b/drivers/iommu/arm/arm-smmu/arm-smmu.c
@@ -842,14 +842,11 @@ static void arm_smmu_destroy_domain_context(struct arm_smmu_domain *smmu_domain)
 	arm_smmu_rpm_put(smmu);
 }
 
-static struct iommu_domain *arm_smmu_domain_alloc(unsigned type)
+static struct iommu_domain *arm_smmu_domain_alloc_paging(struct device *dev)
 {
+	struct arm_smmu_master_cfg *cfg = NULL;
 	struct arm_smmu_domain *smmu_domain;
 
-	if (type != IOMMU_DOMAIN_UNMANAGED) {
-		if (using_legacy_binding || type != IOMMU_DOMAIN_DMA)
-			return NULL;
-	}
 	/*
 	 * Allocate the domain and initialise some of its data structures.
 	 * We can't really do anything meaningful until we've added a
@@ -862,6 +859,18 @@ static struct iommu_domain *arm_smmu_domain_alloc(unsigned type)
 	mutex_init(&smmu_domain->init_mutex);
 	spin_lock_init(&smmu_domain->cb_lock);
 
+	if (dev)
+		cfg = dev_iommu_priv_get(dev);
+	if (cfg) {
+		int ret;
+
+		ret = arm_smmu_init_domain_context(smmu_domain, cfg->smmu, dev);
+		if (ret) {
+			kfree(smmu_domain);
+			return NULL;
+		}
+	}
+
 	return &smmu_domain->domain;
 }
 
@@ -1602,7 +1611,7 @@ static struct iommu_ops arm_smmu_ops = {
 	.identity_domain	= &arm_smmu_identity_domain,
 	.blocked_domain		= &arm_smmu_blocked_domain,
 	.capable		= arm_smmu_capable,
-	.domain_alloc		= arm_smmu_domain_alloc,
+	.domain_alloc_paging	= arm_smmu_domain_alloc_paging,
 	.probe_device		= arm_smmu_probe_device,
 	.release_device		= arm_smmu_release_device,
 	.probe_finalize		= arm_smmu_probe_finalize,
-- 
2.42.0

[PATCH 5/7] iommu/arm-smmu: Convert to domain_alloc_paging()

[Jason Gunthorpe]

Now that the BLOCKED and IDENTITY behaviors are managed with their own
domains change to the domain_alloc_paging() op.

The check for using_legacy_binding is now redundant,
arm_smmu_def_domain_type() always returns IOMMU_DOMAIN_IDENTITY for this
mode, so the core code will never attempt to create a DMA domain in the
first place.

Since commit a4fdd9762272 ("iommu: Use flush queue capability") the core
code only passes in IDENTITY/BLOCKED/UNMANAGED/DMA domain types. It will
not pass in IDENTITY or BLOCKED if the global statics exist, so the test
for DMA is also redundant now too.

Call arm_smmu_init_domain_context() early if a dev is available.

Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
 drivers/iommu/arm/arm-smmu/arm-smmu.c | 21 +++++++++++++++------
 1 file changed, 15 insertions(+), 6 deletions(-)

diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu.c b/drivers/iommu/arm/arm-smmu/arm-smmu.c
index bf5f541be2399f..4d69bb63ba1a3e 100644
--- a/drivers/iommu/arm/arm-smmu/arm-smmu.c
+++ b/drivers/iommu/arm/arm-smmu/arm-smmu.c
@@ -842,14 +842,11 @@ static void arm_smmu_destroy_domain_context(struct arm_smmu_domain *smmu_domain)
 	arm_smmu_rpm_put(smmu);
 }
 
-static struct iommu_domain *arm_smmu_domain_alloc(unsigned type)
+static struct iommu_domain *arm_smmu_domain_alloc_paging(struct device *dev)
 {
+	struct arm_smmu_master_cfg *cfg = NULL;
 	struct arm_smmu_domain *smmu_domain;
 
-	if (type != IOMMU_DOMAIN_UNMANAGED) {
-		if (using_legacy_binding || type != IOMMU_DOMAIN_DMA)
-			return NULL;
-	}
 	/*
 	 * Allocate the domain and initialise some of its data structures.
 	 * We can't really do anything meaningful until we've added a
@@ -862,6 +859,18 @@ static struct iommu_domain *arm_smmu_domain_alloc(unsigned type)
 	mutex_init(&smmu_domain->init_mutex);
 	spin_lock_init(&smmu_domain->cb_lock);
 
+	if (dev)
+		cfg = dev_iommu_priv_get(dev);
+	if (cfg) {
+		int ret;
+
+		ret = arm_smmu_init_domain_context(smmu_domain, cfg->smmu, dev);
+		if (ret) {
+			kfree(smmu_domain);
+			return NULL;
+		}
+	}
+
 	return &smmu_domain->domain;
 }
 
@@ -1602,7 +1611,7 @@ static struct iommu_ops arm_smmu_ops = {
 	.identity_domain	= &arm_smmu_identity_domain,
 	.blocked_domain		= &arm_smmu_blocked_domain,
 	.capable		= arm_smmu_capable,
-	.domain_alloc		= arm_smmu_domain_alloc,
+	.domain_alloc_paging	= arm_smmu_domain_alloc_paging,
 	.probe_device		= arm_smmu_probe_device,
 	.release_device		= arm_smmu_release_device,
 	.probe_finalize		= arm_smmu_probe_finalize,
-- 
2.42.0


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

[PATCH 6/7] iommu: Compute dev_iommu->require_direct sooner

[Jason Gunthorpe]

The next patch will need it. To reduce the cost cache if there are
any direct mappings at all and exit iommu_create_device_direct_mappings()
early.

Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
 drivers/iommu/iommu.c | 26 +++++++++++++++++++-------
 include/linux/iommu.h |  1 +
 2 files changed, 20 insertions(+), 7 deletions(-)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 89db35e2c21771..efa65b3d1bd405 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -392,7 +392,9 @@ static u32 dev_iommu_get_max_pasids(struct device *dev)
  */
 static int iommu_init_device(struct device *dev, const struct iommu_ops *ops)
 {
+	struct iommu_resv_region *entry;
 	struct iommu_device *iommu_dev;
+	LIST_HEAD(mappings);
 	struct iommu_group *group;
 	int ret;
 
@@ -427,6 +429,17 @@ static int iommu_init_device(struct device *dev, const struct iommu_ops *ops)
 	dev->iommu->max_pasids = dev_iommu_get_max_pasids(dev);
 	if (ops->is_attach_deferred)
 		dev->iommu->attach_deferred = ops->is_attach_deferred(dev);
+
+	iommu_get_resv_regions(dev, &mappings);
+	list_for_each_entry(entry, &mappings, list) {
+		if (entry->type == IOMMU_RESV_DIRECT)
+			dev->iommu->require_direct = 1;
+		if (entry->type != IOMMU_RESV_DIRECT &&
+		    entry->type != IOMMU_RESV_DIRECT_RELAXABLE)
+			dev->iommu->has_direct = 1;
+	}
+	iommu_put_resv_regions(dev, &mappings);
+
 	return 0;
 
 err_unlink:
@@ -1068,7 +1081,10 @@ static int iommu_create_device_direct_mappings(struct iommu_domain *domain,
 	pg_size = domain->pgsize_bitmap ? 1UL << __ffs(domain->pgsize_bitmap) : 0;
 	INIT_LIST_HEAD(&mappings);
 
-	if (WARN_ON_ONCE(iommu_is_dma_domain(domain) && !pg_size))
+	if (!iommu_is_dma_domain(domain) || !dev->iommu->has_direct)
+		return 0;
+
+	if (WARN_ON_ONCE(!pg_size))
 		return -EINVAL;
 
 	iommu_get_resv_regions(dev, &mappings);
@@ -1078,12 +1094,8 @@ static int iommu_create_device_direct_mappings(struct iommu_domain *domain,
 		dma_addr_t start, end, addr;
 		size_t map_size = 0;
 
-		if (entry->type == IOMMU_RESV_DIRECT)
-			dev->iommu->require_direct = 1;
-
-		if ((entry->type != IOMMU_RESV_DIRECT &&
-		     entry->type != IOMMU_RESV_DIRECT_RELAXABLE) ||
-		    !iommu_is_dma_domain(domain))
+		if (entry->type != IOMMU_RESV_DIRECT &&
+		    entry->type != IOMMU_RESV_DIRECT_RELAXABLE)
 			continue;
 
 		start = ALIGN(entry->start, pg_size);
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index e1a4c2c2c34d42..e27d2e67e48ef7 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -442,6 +442,7 @@ struct dev_iommu {
 	u32				attach_deferred:1;
 	u32				pci_32bit_workaround:1;
 	u32				require_direct:1;
+	u32				has_direct:1;
 };
 
 int iommu_device_register(struct iommu_device *iommu,
-- 
2.42.0

[PATCH 6/7] iommu: Compute dev_iommu->require_direct sooner

[Jason Gunthorpe]

The next patch will need it. To reduce the cost cache if there are
any direct mappings at all and exit iommu_create_device_direct_mappings()
early.

Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
 drivers/iommu/iommu.c | 26 +++++++++++++++++++-------
 include/linux/iommu.h |  1 +
 2 files changed, 20 insertions(+), 7 deletions(-)

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 89db35e2c21771..efa65b3d1bd405 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -392,7 +392,9 @@ static u32 dev_iommu_get_max_pasids(struct device *dev)
  */
 static int iommu_init_device(struct device *dev, const struct iommu_ops *ops)
 {
+	struct iommu_resv_region *entry;
 	struct iommu_device *iommu_dev;
+	LIST_HEAD(mappings);
 	struct iommu_group *group;
 	int ret;
 
@@ -427,6 +429,17 @@ static int iommu_init_device(struct device *dev, const struct iommu_ops *ops)
 	dev->iommu->max_pasids = dev_iommu_get_max_pasids(dev);
 	if (ops->is_attach_deferred)
 		dev->iommu->attach_deferred = ops->is_attach_deferred(dev);
+
+	iommu_get_resv_regions(dev, &mappings);
+	list_for_each_entry(entry, &mappings, list) {
+		if (entry->type == IOMMU_RESV_DIRECT)
+			dev->iommu->require_direct = 1;
+		if (entry->type != IOMMU_RESV_DIRECT &&
+		    entry->type != IOMMU_RESV_DIRECT_RELAXABLE)
+			dev->iommu->has_direct = 1;
+	}
+	iommu_put_resv_regions(dev, &mappings);
+
 	return 0;
 
 err_unlink:
@@ -1068,7 +1081,10 @@ static int iommu_create_device_direct_mappings(struct iommu_domain *domain,
 	pg_size = domain->pgsize_bitmap ? 1UL << __ffs(domain->pgsize_bitmap) : 0;
 	INIT_LIST_HEAD(&mappings);
 
-	if (WARN_ON_ONCE(iommu_is_dma_domain(domain) && !pg_size))
+	if (!iommu_is_dma_domain(domain) || !dev->iommu->has_direct)
+		return 0;
+
+	if (WARN_ON_ONCE(!pg_size))
 		return -EINVAL;
 
 	iommu_get_resv_regions(dev, &mappings);
@@ -1078,12 +1094,8 @@ static int iommu_create_device_direct_mappings(struct iommu_domain *domain,
 		dma_addr_t start, end, addr;
 		size_t map_size = 0;
 
-		if (entry->type == IOMMU_RESV_DIRECT)
-			dev->iommu->require_direct = 1;
-
-		if ((entry->type != IOMMU_RESV_DIRECT &&
-		     entry->type != IOMMU_RESV_DIRECT_RELAXABLE) ||
-		    !iommu_is_dma_domain(domain))
+		if (entry->type != IOMMU_RESV_DIRECT &&
+		    entry->type != IOMMU_RESV_DIRECT_RELAXABLE)
 			continue;
 
 		start = ALIGN(entry->start, pg_size);
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index e1a4c2c2c34d42..e27d2e67e48ef7 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -442,6 +442,7 @@ struct dev_iommu {
 	u32				attach_deferred:1;
 	u32				pci_32bit_workaround:1;
 	u32				require_direct:1;
+	u32				has_direct:1;
 };
 
 int iommu_device_register(struct iommu_device *iommu,
-- 
2.42.0


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

[PATCH 7/7] iommu: Restore SMMU "disable_bypass"

[Jason Gunthorpe]

The module parameter "disable_bypass" changes the IOMMU configuration when
using CONFIG_ARM_DMA_USE_IOMMU to establish a faulting/blocked domain when
the ARM DMA ops are not being used.

SMMU does this by defaulting the device configuration to faulting during
probe. This keeps things faulting up until the ARM DMA ops first attach,
then the faulting is lost.

Since we removed NULL domains and immediately attach an IDENTIY domain
during probe this now doesn't work properly on either driver.

Reflect the SMMU module option to the core code and have it request a
BLOCKED or IDENTITY domain directly, only for CONFIG_ARM_DMA_USE_IOMMU.

Fixes: 98ac73f99bc4 ("iommu: Require a default_domain for all iommu drivers")
Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
 drivers/iommu/arm/arm-smmu/arm-smmu.c |  1 +
 drivers/iommu/iommu.c                 | 23 ++++++++++++++++++++++-
 include/linux/iommu.h                 |  2 ++
 3 files changed, 25 insertions(+), 1 deletion(-)

diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu.c b/drivers/iommu/arm/arm-smmu/arm-smmu.c
index 4d69bb63ba1a3e..e164477a224dac 100644
--- a/drivers/iommu/arm/arm-smmu/arm-smmu.c
+++ b/drivers/iommu/arm/arm-smmu/arm-smmu.c
@@ -2211,6 +2211,7 @@ static int arm_smmu_device_probe(struct platform_device *pdev)
 		return err;
 	}
 
+	iommu_arm_disable_bypass |= disable_bypass;
 	err = iommu_device_register(&smmu->iommu, &arm_smmu_ops, dev);
 	if (err) {
 		dev_err(dev, "Failed to register iommu\n");
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index efa65b3d1bd405..e2798d8318b7bb 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -46,6 +46,13 @@ static unsigned int iommu_def_domain_type __read_mostly;
 static bool iommu_dma_strict __read_mostly = IS_ENABLED(CONFIG_IOMMU_DEFAULT_DMA_STRICT);
 static u32 iommu_cmd_line __read_mostly;
 
+/*
+ * This supports the module option "disable_bypass" that ARM SMMU and SMMUv3
+ * drivers supported. It causes the default domain to be BLOCKED when
+ * CONFIG_ARM_DMA_USE_IOMMU.
+ */
+bool iommu_arm_disable_bypass = false;
+
 struct iommu_group {
 	struct kobject kobj;
 	struct kobject *devices_kobj;
@@ -1914,7 +1921,13 @@ static int iommu_get_default_domain_type(struct iommu_group *group,
 	if (IS_ENABLED(CONFIG_ARM_DMA_USE_IOMMU)) {
 		static_assert(!(IS_ENABLED(CONFIG_ARM_DMA_USE_IOMMU) &&
 				IS_ENABLED(CONFIG_IOMMU_DMA)));
-		driver_type = IOMMU_DOMAIN_IDENTITY;
+		if (!iommu_arm_disable_bypass ||
+		    !group_iommu_ops(group)->blocked_domain)
+			driver_type = IOMMU_DOMAIN_IDENTITY;
+		else
+			for_each_group_device(group, gdev)
+				if (gdev->dev->iommu->require_direct)
+					driver_type = IOMMU_DOMAIN_IDENTITY;
 	}
 
 	for_each_group_device(group, gdev) {
@@ -1932,6 +1945,14 @@ static int iommu_get_default_domain_type(struct iommu_group *group,
 		}
 	}
 
+	/*
+	 * For iommu_arm_disable_bypass mode the driver is allowed to force
+	 * IDENTITY, otherwise returning 0 from def_domain_type will mean
+	 * BLOCKED.
+	 */
+	if (IS_ENABLED(CONFIG_ARM_DMA_USE_IOMMU) && driver_type == 0)
+		driver_type = IOMMU_DOMAIN_BLOCKED;
+
 	if (untrusted) {
 		if (driver_type && driver_type != IOMMU_DOMAIN_DMA) {
 			dev_err_ratelimited(
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index e27d2e67e48ef7..c88150300bff71 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -42,6 +42,8 @@ struct iommu_sva;
 struct iommu_fault_event;
 struct iommu_dma_cookie;
 
+extern bool iommu_arm_disable_bypass;
+
 /* iommu fault flags */
 #define IOMMU_FAULT_READ	0x0
 #define IOMMU_FAULT_WRITE	0x1
-- 
2.42.0

[PATCH 7/7] iommu: Restore SMMU "disable_bypass"

[Jason Gunthorpe]

The module parameter "disable_bypass" changes the IOMMU configuration when
using CONFIG_ARM_DMA_USE_IOMMU to establish a faulting/blocked domain when
the ARM DMA ops are not being used.

SMMU does this by defaulting the device configuration to faulting during
probe. This keeps things faulting up until the ARM DMA ops first attach,
then the faulting is lost.

Since we removed NULL domains and immediately attach an IDENTIY domain
during probe this now doesn't work properly on either driver.

Reflect the SMMU module option to the core code and have it request a
BLOCKED or IDENTITY domain directly, only for CONFIG_ARM_DMA_USE_IOMMU.

Fixes: 98ac73f99bc4 ("iommu: Require a default_domain for all iommu drivers")
Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
---
 drivers/iommu/arm/arm-smmu/arm-smmu.c |  1 +
 drivers/iommu/iommu.c                 | 23 ++++++++++++++++++++++-
 include/linux/iommu.h                 |  2 ++
 3 files changed, 25 insertions(+), 1 deletion(-)

diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu.c b/drivers/iommu/arm/arm-smmu/arm-smmu.c
index 4d69bb63ba1a3e..e164477a224dac 100644
--- a/drivers/iommu/arm/arm-smmu/arm-smmu.c
+++ b/drivers/iommu/arm/arm-smmu/arm-smmu.c
@@ -2211,6 +2211,7 @@ static int arm_smmu_device_probe(struct platform_device *pdev)
 		return err;
 	}
 
+	iommu_arm_disable_bypass |= disable_bypass;
 	err = iommu_device_register(&smmu->iommu, &arm_smmu_ops, dev);
 	if (err) {
 		dev_err(dev, "Failed to register iommu\n");
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index efa65b3d1bd405..e2798d8318b7bb 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -46,6 +46,13 @@ static unsigned int iommu_def_domain_type __read_mostly;
 static bool iommu_dma_strict __read_mostly = IS_ENABLED(CONFIG_IOMMU_DEFAULT_DMA_STRICT);
 static u32 iommu_cmd_line __read_mostly;
 
+/*
+ * This supports the module option "disable_bypass" that ARM SMMU and SMMUv3
+ * drivers supported. It causes the default domain to be BLOCKED when
+ * CONFIG_ARM_DMA_USE_IOMMU.
+ */
+bool iommu_arm_disable_bypass = false;
+
 struct iommu_group {
 	struct kobject kobj;
 	struct kobject *devices_kobj;
@@ -1914,7 +1921,13 @@ static int iommu_get_default_domain_type(struct iommu_group *group,
 	if (IS_ENABLED(CONFIG_ARM_DMA_USE_IOMMU)) {
 		static_assert(!(IS_ENABLED(CONFIG_ARM_DMA_USE_IOMMU) &&
 				IS_ENABLED(CONFIG_IOMMU_DMA)));
-		driver_type = IOMMU_DOMAIN_IDENTITY;
+		if (!iommu_arm_disable_bypass ||
+		    !group_iommu_ops(group)->blocked_domain)
+			driver_type = IOMMU_DOMAIN_IDENTITY;
+		else
+			for_each_group_device(group, gdev)
+				if (gdev->dev->iommu->require_direct)
+					driver_type = IOMMU_DOMAIN_IDENTITY;
 	}
 
 	for_each_group_device(group, gdev) {
@@ -1932,6 +1945,14 @@ static int iommu_get_default_domain_type(struct iommu_group *group,
 		}
 	}
 
+	/*
+	 * For iommu_arm_disable_bypass mode the driver is allowed to force
+	 * IDENTITY, otherwise returning 0 from def_domain_type will mean
+	 * BLOCKED.
+	 */
+	if (IS_ENABLED(CONFIG_ARM_DMA_USE_IOMMU) && driver_type == 0)
+		driver_type = IOMMU_DOMAIN_BLOCKED;
+
 	if (untrusted) {
 		if (driver_type && driver_type != IOMMU_DOMAIN_DMA) {
 			dev_err_ratelimited(
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index e27d2e67e48ef7..c88150300bff71 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -42,6 +42,8 @@ struct iommu_sva;
 struct iommu_fault_event;
 struct iommu_dma_cookie;
 
+extern bool iommu_arm_disable_bypass;
+
 /* iommu fault flags */
 #define IOMMU_FAULT_READ	0x0
 #define IOMMU_FAULT_WRITE	0x1
-- 
2.42.0


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

Re: [PATCH 7/7] iommu: Restore SMMU "disable_bypass"

[Robin Murphy]

On 2023-10-05 19:28, Jason Gunthorpe wrote:
> The module parameter "disable_bypass" changes the IOMMU configuration when
> using CONFIG_ARM_DMA_USE_IOMMU to establish a faulting/blocked domain when
> the ARM DMA ops are not being used.

Nothing to do with ARM DMA ops, it is purely about the behaviour for 
StreamIDs not attached to domains. This includes known StreamIDs during 
the window between SMMU initialisation and those devices first being 
probed and attached to something, but these days is mostly concerned 
with unknown StreamIDs that would never be attached either way.

If the driver *has* knowingly and willingly attached a device to an 
identity domain, that's fine.

Also we're now tantalisingly close to having ARM use regular default 
domains anyway - the full conversion to iommu-dma still depends on 
significant changes to the IOVA allocator, but I'm 99% sure I've got a 
viable intermediate step which at least gets it out of the way from the 
rest of the core API's PoV (probe/release shenanigans etc.). 
Furthermore I'd imagine that frankly the number of users of arm-smmu 
with 32-bit mainline kernels is, to within experimental error, 0, so 
honestly it's not worth complicating core code with this.

Thanks,
Robin.

> SMMU does this by defaulting the device configuration to faulting during
> probe. This keeps things faulting up until the ARM DMA ops first attach,
> then the faulting is lost.
> 
> Since we removed NULL domains and immediately attach an IDENTIY domain
> during probe this now doesn't work properly on either driver.
> 
> Reflect the SMMU module option to the core code and have it request a
> BLOCKED or IDENTITY domain directly, only for CONFIG_ARM_DMA_USE_IOMMU.
> 
> Fixes: 98ac73f99bc4 ("iommu: Require a default_domain for all iommu drivers")
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> ---
>   drivers/iommu/arm/arm-smmu/arm-smmu.c |  1 +
>   drivers/iommu/iommu.c                 | 23 ++++++++++++++++++++++-
>   include/linux/iommu.h                 |  2 ++
>   3 files changed, 25 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu.c b/drivers/iommu/arm/arm-smmu/arm-smmu.c
> index 4d69bb63ba1a3e..e164477a224dac 100644
> --- a/drivers/iommu/arm/arm-smmu/arm-smmu.c
> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu.c
> @@ -2211,6 +2211,7 @@ static int arm_smmu_device_probe(struct platform_device *pdev)
>   		return err;
>   	}
>   
> +	iommu_arm_disable_bypass |= disable_bypass;
>   	err = iommu_device_register(&smmu->iommu, &arm_smmu_ops, dev);
>   	if (err) {
>   		dev_err(dev, "Failed to register iommu\n");
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index efa65b3d1bd405..e2798d8318b7bb 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -46,6 +46,13 @@ static unsigned int iommu_def_domain_type __read_mostly;
>   static bool iommu_dma_strict __read_mostly = IS_ENABLED(CONFIG_IOMMU_DEFAULT_DMA_STRICT);
>   static u32 iommu_cmd_line __read_mostly;
>   
> +/*
> + * This supports the module option "disable_bypass" that ARM SMMU and SMMUv3
> + * drivers supported. It causes the default domain to be BLOCKED when
> + * CONFIG_ARM_DMA_USE_IOMMU.
> + */
> +bool iommu_arm_disable_bypass = false;
> +
>   struct iommu_group {
>   	struct kobject kobj;
>   	struct kobject *devices_kobj;
> @@ -1914,7 +1921,13 @@ static int iommu_get_default_domain_type(struct iommu_group *group,
>   	if (IS_ENABLED(CONFIG_ARM_DMA_USE_IOMMU)) {
>   		static_assert(!(IS_ENABLED(CONFIG_ARM_DMA_USE_IOMMU) &&
>   				IS_ENABLED(CONFIG_IOMMU_DMA)));
> -		driver_type = IOMMU_DOMAIN_IDENTITY;
> +		if (!iommu_arm_disable_bypass ||
> +		    !group_iommu_ops(group)->blocked_domain)
> +			driver_type = IOMMU_DOMAIN_IDENTITY;
> +		else
> +			for_each_group_device(group, gdev)
> +				if (gdev->dev->iommu->require_direct)
> +					driver_type = IOMMU_DOMAIN_IDENTITY;
>   	}
>   
>   	for_each_group_device(group, gdev) {
> @@ -1932,6 +1945,14 @@ static int iommu_get_default_domain_type(struct iommu_group *group,
>   		}
>   	}
>   
> +	/*
> +	 * For iommu_arm_disable_bypass mode the driver is allowed to force
> +	 * IDENTITY, otherwise returning 0 from def_domain_type will mean
> +	 * BLOCKED.
> +	 */
> +	if (IS_ENABLED(CONFIG_ARM_DMA_USE_IOMMU) && driver_type == 0)
> +		driver_type = IOMMU_DOMAIN_BLOCKED;
> +
>   	if (untrusted) {
>   		if (driver_type && driver_type != IOMMU_DOMAIN_DMA) {
>   			dev_err_ratelimited(
> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> index e27d2e67e48ef7..c88150300bff71 100644
> --- a/include/linux/iommu.h
> +++ b/include/linux/iommu.h
> @@ -42,6 +42,8 @@ struct iommu_sva;
>   struct iommu_fault_event;
>   struct iommu_dma_cookie;
>   
> +extern bool iommu_arm_disable_bypass;
> +
>   /* iommu fault flags */
>   #define IOMMU_FAULT_READ	0x0
>   #define IOMMU_FAULT_WRITE	0x1

Re: [PATCH 7/7] iommu: Restore SMMU "disable_bypass"

[Robin Murphy]

On 2023-10-05 19:28, Jason Gunthorpe wrote:
> The module parameter "disable_bypass" changes the IOMMU configuration when
> using CONFIG_ARM_DMA_USE_IOMMU to establish a faulting/blocked domain when
> the ARM DMA ops are not being used.

Nothing to do with ARM DMA ops, it is purely about the behaviour for 
StreamIDs not attached to domains. This includes known StreamIDs during 
the window between SMMU initialisation and those devices first being 
probed and attached to something, but these days is mostly concerned 
with unknown StreamIDs that would never be attached either way.

If the driver *has* knowingly and willingly attached a device to an 
identity domain, that's fine.

Also we're now tantalisingly close to having ARM use regular default 
domains anyway - the full conversion to iommu-dma still depends on 
significant changes to the IOVA allocator, but I'm 99% sure I've got a 
viable intermediate step which at least gets it out of the way from the 
rest of the core API's PoV (probe/release shenanigans etc.). 
Furthermore I'd imagine that frankly the number of users of arm-smmu 
with 32-bit mainline kernels is, to within experimental error, 0, so 
honestly it's not worth complicating core code with this.

Thanks,
Robin.

> SMMU does this by defaulting the device configuration to faulting during
> probe. This keeps things faulting up until the ARM DMA ops first attach,
> then the faulting is lost.
> 
> Since we removed NULL domains and immediately attach an IDENTIY domain
> during probe this now doesn't work properly on either driver.
> 
> Reflect the SMMU module option to the core code and have it request a
> BLOCKED or IDENTITY domain directly, only for CONFIG_ARM_DMA_USE_IOMMU.
> 
> Fixes: 98ac73f99bc4 ("iommu: Require a default_domain for all iommu drivers")
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> ---
>   drivers/iommu/arm/arm-smmu/arm-smmu.c |  1 +
>   drivers/iommu/iommu.c                 | 23 ++++++++++++++++++++++-
>   include/linux/iommu.h                 |  2 ++
>   3 files changed, 25 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iommu/arm/arm-smmu/arm-smmu.c b/drivers/iommu/arm/arm-smmu/arm-smmu.c
> index 4d69bb63ba1a3e..e164477a224dac 100644
> --- a/drivers/iommu/arm/arm-smmu/arm-smmu.c
> +++ b/drivers/iommu/arm/arm-smmu/arm-smmu.c
> @@ -2211,6 +2211,7 @@ static int arm_smmu_device_probe(struct platform_device *pdev)
>   		return err;
>   	}
>   
> +	iommu_arm_disable_bypass |= disable_bypass;
>   	err = iommu_device_register(&smmu->iommu, &arm_smmu_ops, dev);
>   	if (err) {
>   		dev_err(dev, "Failed to register iommu\n");
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index efa65b3d1bd405..e2798d8318b7bb 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -46,6 +46,13 @@ static unsigned int iommu_def_domain_type __read_mostly;
>   static bool iommu_dma_strict __read_mostly = IS_ENABLED(CONFIG_IOMMU_DEFAULT_DMA_STRICT);
>   static u32 iommu_cmd_line __read_mostly;
>   
> +/*
> + * This supports the module option "disable_bypass" that ARM SMMU and SMMUv3
> + * drivers supported. It causes the default domain to be BLOCKED when
> + * CONFIG_ARM_DMA_USE_IOMMU.
> + */
> +bool iommu_arm_disable_bypass = false;
> +
>   struct iommu_group {
>   	struct kobject kobj;
>   	struct kobject *devices_kobj;
> @@ -1914,7 +1921,13 @@ static int iommu_get_default_domain_type(struct iommu_group *group,
>   	if (IS_ENABLED(CONFIG_ARM_DMA_USE_IOMMU)) {
>   		static_assert(!(IS_ENABLED(CONFIG_ARM_DMA_USE_IOMMU) &&
>   				IS_ENABLED(CONFIG_IOMMU_DMA)));
> -		driver_type = IOMMU_DOMAIN_IDENTITY;
> +		if (!iommu_arm_disable_bypass ||
> +		    !group_iommu_ops(group)->blocked_domain)
> +			driver_type = IOMMU_DOMAIN_IDENTITY;
> +		else
> +			for_each_group_device(group, gdev)
> +				if (gdev->dev->iommu->require_direct)
> +					driver_type = IOMMU_DOMAIN_IDENTITY;
>   	}
>   
>   	for_each_group_device(group, gdev) {
> @@ -1932,6 +1945,14 @@ static int iommu_get_default_domain_type(struct iommu_group *group,
>   		}
>   	}
>   
> +	/*
> +	 * For iommu_arm_disable_bypass mode the driver is allowed to force
> +	 * IDENTITY, otherwise returning 0 from def_domain_type will mean
> +	 * BLOCKED.
> +	 */
> +	if (IS_ENABLED(CONFIG_ARM_DMA_USE_IOMMU) && driver_type == 0)
> +		driver_type = IOMMU_DOMAIN_BLOCKED;
> +
>   	if (untrusted) {
>   		if (driver_type && driver_type != IOMMU_DOMAIN_DMA) {
>   			dev_err_ratelimited(
> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> index e27d2e67e48ef7..c88150300bff71 100644
> --- a/include/linux/iommu.h
> +++ b/include/linux/iommu.h
> @@ -42,6 +42,8 @@ struct iommu_sva;
>   struct iommu_fault_event;
>   struct iommu_dma_cookie;
>   
> +extern bool iommu_arm_disable_bypass;
> +
>   /* iommu fault flags */
>   #define IOMMU_FAULT_READ	0x0
>   #define IOMMU_FAULT_WRITE	0x1

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

Re: [PATCH 7/7] iommu: Restore SMMU "disable_bypass"

[Jason Gunthorpe]

On Fri, Oct 06, 2023 at 01:06:08PM +0100, Robin Murphy wrote:
> On 2023-10-05 19:28, Jason Gunthorpe wrote:
> > The module parameter "disable_bypass" changes the IOMMU configuration when
> > using CONFIG_ARM_DMA_USE_IOMMU to establish a faulting/blocked domain when
> > the ARM DMA ops are not being used.
> 
> Nothing to do with ARM DMA ops, it is purely about the behaviour for
> StreamIDs not attached to domains. This includes known StreamIDs during the
> window between SMMU initialisation and those devices first being probed and
> attached to something, but these days is mostly concerned with unknown
> StreamIDs that would never be attached either way.

So, I'm happy with this explanation and we can drop this patch. Let's
still update the driver to the new API anyhow

But it does not completely match the impression I got when I went
digging in the git history. The commit messages seemed to convay a
concern that SIDs which have DT nodes and struct devices but did not
have attached drivers trigger DMA errors to flush out bad stuff.

> Also we're now tantalisingly close to having ARM use regular default domains
> anyway - the full conversion to iommu-dma still depends on significant
> changes to the IOVA allocator, but I'm 99% sure I've got a viable
> intermediate step which at least gets it out of the way from the rest of the
> core API's PoV (probe/release shenanigans etc.).

What I wanted to get to was an arrangement where arm iommu doesn't
leak out everywhere. Get it out of the GPU drivers and related, get it
out of the iommu drivers.

That seems like a necessary step before even considering replacing it
entirely.

I was going to take a break and see what your of_xlate changes look
like first :)

> that frankly the number of users of arm-smmu with 32-bit mainline kernels
> is, to within experimental error, 0, so honestly it's not worth complicating
> core code with this.

Okay! We won't worry about it for smmuv3 either then

Thanks,
Jason

Re: [PATCH 7/7] iommu: Restore SMMU "disable_bypass"

[Jason Gunthorpe]

On Fri, Oct 06, 2023 at 01:06:08PM +0100, Robin Murphy wrote:
> On 2023-10-05 19:28, Jason Gunthorpe wrote:
> > The module parameter "disable_bypass" changes the IOMMU configuration when
> > using CONFIG_ARM_DMA_USE_IOMMU to establish a faulting/blocked domain when
> > the ARM DMA ops are not being used.
> 
> Nothing to do with ARM DMA ops, it is purely about the behaviour for
> StreamIDs not attached to domains. This includes known StreamIDs during the
> window between SMMU initialisation and those devices first being probed and
> attached to something, but these days is mostly concerned with unknown
> StreamIDs that would never be attached either way.

So, I'm happy with this explanation and we can drop this patch. Let's
still update the driver to the new API anyhow

But it does not completely match the impression I got when I went
digging in the git history. The commit messages seemed to convay a
concern that SIDs which have DT nodes and struct devices but did not
have attached drivers trigger DMA errors to flush out bad stuff.

> Also we're now tantalisingly close to having ARM use regular default domains
> anyway - the full conversion to iommu-dma still depends on significant
> changes to the IOVA allocator, but I'm 99% sure I've got a viable
> intermediate step which at least gets it out of the way from the rest of the
> core API's PoV (probe/release shenanigans etc.).

What I wanted to get to was an arrangement where arm iommu doesn't
leak out everywhere. Get it out of the GPU drivers and related, get it
out of the iommu drivers.

That seems like a necessary step before even considering replacing it
entirely.

I was going to take a break and see what your of_xlate changes look
like first :)

> that frankly the number of users of arm-smmu with 32-bit mainline kernels
> is, to within experimental error, 0, so honestly it's not worth complicating
> core code with this.

Okay! We won't worry about it for smmuv3 either then

Thanks,
Jason

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

Re: [PATCH 0/7] Convert SMMU to domain_alloc_paging()

[Jason Gunthorpe]

On Thu, Oct 05, 2023 at 03:28:11PM -0300, Jason Gunthorpe wrote:
> Add the global statics for IDENTITY and BLOCKED to the SMMU driver and
> change to use domain_alloc_paging(). This allows SMMU to finalize the
> domain during allocation.
> 
> This is a bit more urgent as I noticed while looking at Nicolin's patches
> that the disable_bypass module parameter no longer works. The system will
> boot fine but the domains will be set to IDENTITY now. To fix this the
> core code must request BLOCKED domains when using ARM DMA ops with this
> module option set.
> 
> This series fixes SMMU, which seems like the more important one of the
> two. SMMUv3 needs a similar repair, but it is more complex to get the two
> global static domains into the SMMUv3 driver. As SMMUv3 is primarily an
> ARM64 driver this is less important since the normal DMA API flow already
> substantially establishes blocking domains via empty IOMMU_DOMAIN_DMA
> attachments. Regardless I have addressed SMMUv3 seperately.
> 
> This relies on the first few patches of the dart conversion series:
> 
> https://lore.kernel.org/r/0-v2-bff223cf6409+282-dart_paging_jgg@nvidia.com
> 
> To enable the BLOCKED global static.
> 
> Jason Gunthorpe (7):
>   iommu/arm-smmu: Reorganize arm_smmu_domain_add_master()
>   iommu/arm-smmu: Convert to a global static identity domain
>   iommu/arm-smmu: Implement IOMMU_DOMAIN_BLOCKED
>   iommu/arm-smmu: Pass arm_smmu_domain to arm_smmu_init_domain_context()
>   iommu/arm-smmu: Convert to domain_alloc_paging()
>   iommu: Compute dev_iommu->require_direct sooner
>   iommu: Restore SMMU "disable_bypass"

This is still pending, it doesn't need rebasing on v6.7

The DART patches were merges so all the dependencies are in v6.7 now.

Jason

Re: [PATCH 0/7] Convert SMMU to domain_alloc_paging()

[Jason Gunthorpe]

On Thu, Oct 05, 2023 at 03:28:11PM -0300, Jason Gunthorpe wrote:
> Add the global statics for IDENTITY and BLOCKED to the SMMU driver and
> change to use domain_alloc_paging(). This allows SMMU to finalize the
> domain during allocation.
> 
> This is a bit more urgent as I noticed while looking at Nicolin's patches
> that the disable_bypass module parameter no longer works. The system will
> boot fine but the domains will be set to IDENTITY now. To fix this the
> core code must request BLOCKED domains when using ARM DMA ops with this
> module option set.
> 
> This series fixes SMMU, which seems like the more important one of the
> two. SMMUv3 needs a similar repair, but it is more complex to get the two
> global static domains into the SMMUv3 driver. As SMMUv3 is primarily an
> ARM64 driver this is less important since the normal DMA API flow already
> substantially establishes blocking domains via empty IOMMU_DOMAIN_DMA
> attachments. Regardless I have addressed SMMUv3 seperately.
> 
> This relies on the first few patches of the dart conversion series:
> 
> https://lore.kernel.org/r/0-v2-bff223cf6409+282-dart_paging_jgg@nvidia.com
> 
> To enable the BLOCKED global static.
> 
> Jason Gunthorpe (7):
>   iommu/arm-smmu: Reorganize arm_smmu_domain_add_master()
>   iommu/arm-smmu: Convert to a global static identity domain
>   iommu/arm-smmu: Implement IOMMU_DOMAIN_BLOCKED
>   iommu/arm-smmu: Pass arm_smmu_domain to arm_smmu_init_domain_context()
>   iommu/arm-smmu: Convert to domain_alloc_paging()
>   iommu: Compute dev_iommu->require_direct sooner
>   iommu: Restore SMMU "disable_bypass"

This is still pending, it doesn't need rebasing on v6.7

The DART patches were merges so all the dependencies are in v6.7 now.

Jason

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

Re: [PATCH 0/7] Convert SMMU to domain_alloc_paging()

[Joerg Roedel]

On Wed, Nov 29, 2023 at 08:49:05PM -0400, Jason Gunthorpe wrote:
> This is still pending, it doesn't need rebasing on v6.7
> 
> The DART patches were merges so all the dependencies are in v6.7 now.

This is mostly ARM-SMMU, so I leave it to Will and Robin to
decide/pick-up.

Regards,

	Joerg

Re: [PATCH 0/7] Convert SMMU to domain_alloc_paging()

[Joerg Roedel]

On Wed, Nov 29, 2023 at 08:49:05PM -0400, Jason Gunthorpe wrote:
> This is still pending, it doesn't need rebasing on v6.7
> 
> The DART patches were merges so all the dependencies are in v6.7 now.

This is mostly ARM-SMMU, so I leave it to Will and Robin to
decide/pick-up.

Regards,

	Joerg

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

Re: [PATCH 0/7] Convert SMMU to domain_alloc_paging()

[Jason Gunthorpe]

On Mon, Dec 11, 2023 at 03:14:07PM +0100, Joerg Roedel wrote:
> On Wed, Nov 29, 2023 at 08:49:05PM -0400, Jason Gunthorpe wrote:
> > This is still pending, it doesn't need rebasing on v6.7
> > 
> > The DART patches were merges so all the dependencies are in v6.7 now.
> 
> This is mostly ARM-SMMU, so I leave it to Will and Robin to
> decide/pick-up.

Let's have it in this kernel please.

There was a v2 two months ago that got no further comments:

https://lore.kernel.org/all/0-v2-c86cc8c2230e+160bb-smmu_newapi_jgg@nvidia.com/

Jason

Re: [PATCH 0/7] Convert SMMU to domain_alloc_paging()

[Jason Gunthorpe]

On Mon, Dec 11, 2023 at 03:14:07PM +0100, Joerg Roedel wrote:
> On Wed, Nov 29, 2023 at 08:49:05PM -0400, Jason Gunthorpe wrote:
> > This is still pending, it doesn't need rebasing on v6.7
> > 
> > The DART patches were merges so all the dependencies are in v6.7 now.
> 
> This is mostly ARM-SMMU, so I leave it to Will and Robin to
> decide/pick-up.

Let's have it in this kernel please.

There was a v2 two months ago that got no further comments:

https://lore.kernel.org/all/0-v2-c86cc8c2230e+160bb-smmu_newapi_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 0/7] Convert SMMU to domain_alloc_paging()

[Will Deacon]

On Mon, Dec 11, 2023 at 03:14:07PM +0100, Joerg Roedel wrote:
> On Wed, Nov 29, 2023 at 08:49:05PM -0400, Jason Gunthorpe wrote:
> > This is still pending, it doesn't need rebasing on v6.7
> > 
> > The DART patches were merges so all the dependencies are in v6.7 now.
> 
> This is mostly ARM-SMMU, so I leave it to Will and Robin to
> decide/pick-up.

Cheers, Joerg. It's on my list of stuff to look at.

Will

Re: [PATCH 0/7] Convert SMMU to domain_alloc_paging()

[Will Deacon]

On Mon, Dec 11, 2023 at 03:14:07PM +0100, Joerg Roedel wrote:
> On Wed, Nov 29, 2023 at 08:49:05PM -0400, Jason Gunthorpe wrote:
> > This is still pending, it doesn't need rebasing on v6.7
> > 
> > The DART patches were merges so all the dependencies are in v6.7 now.
> 
> This is mostly ARM-SMMU, so I leave it to Will and Robin to
> decide/pick-up.

Cheers, Joerg. It's on my list of stuff to look at.

Will

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