[v4,5/6] iommu/dma: Simplify calls to iommu_setup_dma_ops()

Message ID 20210610075130.67517-6-jean-philippe@linaro.org
State New
Headers show
Series
  • Add support for ACPI VIOT
Related show

Commit Message

Jean-Philippe Brucker June 10, 2021, 7:51 a.m.
dma-iommu uses the address bounds described in domain->geometry during
IOVA allocation. The address size parameters of iommu_setup_dma_ops()
are useful for describing additional limits set by the platform
firmware, but aren't needed for drivers that call this function from
probe_finalize(). The base parameter can be zero because dma-iommu
already removes the first IOVA page, and the limit parameter can be
U64_MAX because it's only checked against the domain geometry. Simplify
calls to iommu_setup_dma_ops().

Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>

---
 drivers/iommu/amd/iommu.c   |  9 +--------
 drivers/iommu/dma-iommu.c   |  4 +++-
 drivers/iommu/intel/iommu.c | 10 +---------
 3 files changed, 5 insertions(+), 18 deletions(-)

-- 
2.31.1

Comments

Eric Auger June 16, 2021, 3:50 p.m. | #1
Hi Jean,

On 6/10/21 9:51 AM, Jean-Philippe Brucker wrote:
> dma-iommu uses the address bounds described in domain->geometry during

> IOVA allocation. The address size parameters of iommu_setup_dma_ops()

> are useful for describing additional limits set by the platform

> firmware, but aren't needed for drivers that call this function from

> probe_finalize(). The base parameter can be zero because dma-iommu

> already removes the first IOVA page, and the limit parameter can be

> U64_MAX because it's only checked against the domain geometry. Simplify

> calls to iommu_setup_dma_ops().

>

> Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>

Reviewed-by: Eric Auger <eric.auger@redhat.com>


Eric
> ---

>  drivers/iommu/amd/iommu.c   |  9 +--------

>  drivers/iommu/dma-iommu.c   |  4 +++-

>  drivers/iommu/intel/iommu.c | 10 +---------

>  3 files changed, 5 insertions(+), 18 deletions(-)

>

> diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c

> index 94b96d81fcfd..d3123bc05c08 100644

> --- a/drivers/iommu/amd/iommu.c

> +++ b/drivers/iommu/amd/iommu.c

> @@ -1708,14 +1708,7 @@ static struct iommu_device *amd_iommu_probe_device(struct device *dev)

>  

>  static void amd_iommu_probe_finalize(struct device *dev)

>  {

> -	struct iommu_domain *domain;

> -

> -	/* Domains are initialized for this device - have a look what we ended up with */

> -	domain = iommu_get_domain_for_dev(dev);

> -	if (domain->type == IOMMU_DOMAIN_DMA)

> -		iommu_setup_dma_ops(dev, IOVA_START_PFN << PAGE_SHIFT, U64_MAX);

> -	else

> -		set_dma_ops(dev, NULL);

> +	iommu_setup_dma_ops(dev, 0, U64_MAX);

>  }

>  

>  static void amd_iommu_release_device(struct device *dev)

> diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c

> index c62e19bed302..175f8eaeb5b3 100644

> --- a/drivers/iommu/dma-iommu.c

> +++ b/drivers/iommu/dma-iommu.c

> @@ -1322,7 +1322,9 @@ void iommu_setup_dma_ops(struct device *dev, u64 dma_base, u64 dma_limit)

>  	if (domain->type == IOMMU_DOMAIN_DMA) {

>  		if (iommu_dma_init_domain(domain, dma_base, dma_limit, dev))

>  			goto out_err;

> -		dev->dma_ops = &iommu_dma_ops;

> +		set_dma_ops(dev, &iommu_dma_ops);

> +	} else {

> +		set_dma_ops(dev, NULL);

>  	}

>  

>  	return;

> diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c

> index 85f18342603c..8d866940692a 100644

> --- a/drivers/iommu/intel/iommu.c

> +++ b/drivers/iommu/intel/iommu.c

> @@ -5165,15 +5165,7 @@ static void intel_iommu_release_device(struct device *dev)

>  

>  static void intel_iommu_probe_finalize(struct device *dev)

>  {

> -	dma_addr_t base = IOVA_START_PFN << VTD_PAGE_SHIFT;

> -	struct iommu_domain *domain = iommu_get_domain_for_dev(dev);

> -	struct dmar_domain *dmar_domain = to_dmar_domain(domain);

> -

> -	if (domain && domain->type == IOMMU_DOMAIN_DMA)

> -		iommu_setup_dma_ops(dev, base,

> -				    __DOMAIN_MAX_ADDR(dmar_domain->gaw));

> -	else

> -		set_dma_ops(dev, NULL);

> +	iommu_setup_dma_ops(dev, 0, U64_MAX);

>  }

>  

>  static void intel_iommu_get_resv_regions(struct device *device,
Robin Murphy June 16, 2021, 5:02 p.m. | #2
On 2021-06-10 08:51, Jean-Philippe Brucker wrote:
> dma-iommu uses the address bounds described in domain->geometry during

> IOVA allocation. The address size parameters of iommu_setup_dma_ops()

> are useful for describing additional limits set by the platform

> firmware, but aren't needed for drivers that call this function from

> probe_finalize(). The base parameter can be zero because dma-iommu

> already removes the first IOVA page, and the limit parameter can be

> U64_MAX because it's only checked against the domain geometry. Simplify

> calls to iommu_setup_dma_ops().

> 

> Signed-off-by: Jean-Philippe Brucker <jean-philippe@linaro.org>

> ---

>   drivers/iommu/amd/iommu.c   |  9 +--------

>   drivers/iommu/dma-iommu.c   |  4 +++-

>   drivers/iommu/intel/iommu.c | 10 +---------

>   3 files changed, 5 insertions(+), 18 deletions(-)

> 

> diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c

> index 94b96d81fcfd..d3123bc05c08 100644

> --- a/drivers/iommu/amd/iommu.c

> +++ b/drivers/iommu/amd/iommu.c

> @@ -1708,14 +1708,7 @@ static struct iommu_device *amd_iommu_probe_device(struct device *dev)

>   

>   static void amd_iommu_probe_finalize(struct device *dev)

>   {

> -	struct iommu_domain *domain;

> -

> -	/* Domains are initialized for this device - have a look what we ended up with */

> -	domain = iommu_get_domain_for_dev(dev);

> -	if (domain->type == IOMMU_DOMAIN_DMA)

> -		iommu_setup_dma_ops(dev, IOVA_START_PFN << PAGE_SHIFT, U64_MAX);

> -	else

> -		set_dma_ops(dev, NULL);

> +	iommu_setup_dma_ops(dev, 0, U64_MAX);

>   }

>   

>   static void amd_iommu_release_device(struct device *dev)

> diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c

> index c62e19bed302..175f8eaeb5b3 100644

> --- a/drivers/iommu/dma-iommu.c

> +++ b/drivers/iommu/dma-iommu.c

> @@ -1322,7 +1322,9 @@ void iommu_setup_dma_ops(struct device *dev, u64 dma_base, u64 dma_limit)

>   	if (domain->type == IOMMU_DOMAIN_DMA) {

>   		if (iommu_dma_init_domain(domain, dma_base, dma_limit, dev))

>   			goto out_err;

> -		dev->dma_ops = &iommu_dma_ops;

> +		set_dma_ops(dev, &iommu_dma_ops);

> +	} else {

> +		set_dma_ops(dev, NULL);


I'm not keen on moving this here, since iommu-dma only knows that its 
own ops are right for devices it *is* managing; it can't assume any 
particular ops are appropriate for devices it isn't. The idea here is 
that arch_setup_dma_ops() may have already set the appropriate ops for 
the non-IOMMU case, so if the default domain type is passthrough then we 
leave those in place.

For example, I do still plan to revisit my conversion of arch/arm 
someday, at which point I'd have to undo this for that reason.

Simplifying the base and size arguments is of course fine, but TBH I'd 
say rip the whole bloody lot out of the arch_setup_dma_ops() flow now. 
It's a considerable faff passing them around for nothing but a tenuous 
sanity check in iommu_dma_init_domain(), and now that dev->dma_range_map 
is a common thing we should expect that to give us any relevant 
limitations if we even still care.

That said, those are all things which can be fixed up later if the 
series is otherwise ready to go and there's still a chance of landing it 
for 5.14. If you do have any other reason to respin, then I think the 
x86 probe_finalize functions simply want an unconditional 
set_dma_ops(dev, NULL) before the iommu_setup_dma_ops() call.

Cheers,
Robin.

>   	}

>   

>   	return;

> diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c

> index 85f18342603c..8d866940692a 100644

> --- a/drivers/iommu/intel/iommu.c

> +++ b/drivers/iommu/intel/iommu.c

> @@ -5165,15 +5165,7 @@ static void intel_iommu_release_device(struct device *dev)

>   

>   static void intel_iommu_probe_finalize(struct device *dev)

>   {

> -	dma_addr_t base = IOVA_START_PFN << VTD_PAGE_SHIFT;

> -	struct iommu_domain *domain = iommu_get_domain_for_dev(dev);

> -	struct dmar_domain *dmar_domain = to_dmar_domain(domain);

> -

> -	if (domain && domain->type == IOMMU_DOMAIN_DMA)

> -		iommu_setup_dma_ops(dev, base,

> -				    __DOMAIN_MAX_ADDR(dmar_domain->gaw));

> -	else

> -		set_dma_ops(dev, NULL);

> +	iommu_setup_dma_ops(dev, 0, U64_MAX);

>   }

>   

>   static void intel_iommu_get_resv_regions(struct device *device,

>
Jean-Philippe Brucker June 18, 2021, 10:50 a.m. | #3
On Wed, Jun 16, 2021 at 06:02:39PM +0100, Robin Murphy wrote:
> > diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c

> > index c62e19bed302..175f8eaeb5b3 100644

> > --- a/drivers/iommu/dma-iommu.c

> > +++ b/drivers/iommu/dma-iommu.c

> > @@ -1322,7 +1322,9 @@ void iommu_setup_dma_ops(struct device *dev, u64 dma_base, u64 dma_limit)

> >   	if (domain->type == IOMMU_DOMAIN_DMA) {

> >   		if (iommu_dma_init_domain(domain, dma_base, dma_limit, dev))

> >   			goto out_err;

> > -		dev->dma_ops = &iommu_dma_ops;

> > +		set_dma_ops(dev, &iommu_dma_ops);

> > +	} else {

> > +		set_dma_ops(dev, NULL);

> 

> I'm not keen on moving this here, since iommu-dma only knows that its own

> ops are right for devices it *is* managing; it can't assume any particular

> ops are appropriate for devices it isn't. The idea here is that

> arch_setup_dma_ops() may have already set the appropriate ops for the

> non-IOMMU case, so if the default domain type is passthrough then we leave

> those in place.

> 

> For example, I do still plan to revisit my conversion of arch/arm someday,

> at which point I'd have to undo this for that reason.


Makes sense, I'll remove this bit.

> Simplifying the base and size arguments is of course fine, but TBH I'd say

> rip the whole bloody lot out of the arch_setup_dma_ops() flow now. It's a

> considerable faff passing them around for nothing but a tenuous sanity check

> in iommu_dma_init_domain(), and now that dev->dma_range_map is a common

> thing we should expect that to give us any relevant limitations if we even

> still care.


So I started working on this but it gets too bulky for a preparatory
patch. Dropping the parameters from arch_setup_dma_ops() seems especially
complicated because arm32 does need the size parameter for IOMMU mappings
and that value falls back to the bus DMA mask or U32_MAX in the absence of
dma-ranges. I could try to dig into this for a separate series.

Even only dropping the parameters from iommu_setup_dma_ops() isn't
completely trivial (8 files changed, 55 insertions(+), 36 deletions(-)
because we still need the lower IOVA limit from dma_range_map), so I'd
rather send it separately and have it sit in -next for a while.

Thanks,
Jean

> 

> That said, those are all things which can be fixed up later if the series is

> otherwise ready to go and there's still a chance of landing it for 5.14. If

> you do have any other reason to respin, then I think the x86 probe_finalize

> functions simply want an unconditional set_dma_ops(dev, NULL) before the

> iommu_setup_dma_ops() call.

> 

> Cheers,

> Robin.

> 

> >   	}

> >   	return;

> > diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c

> > index 85f18342603c..8d866940692a 100644

> > --- a/drivers/iommu/intel/iommu.c

> > +++ b/drivers/iommu/intel/iommu.c

> > @@ -5165,15 +5165,7 @@ static void intel_iommu_release_device(struct device *dev)

> >   static void intel_iommu_probe_finalize(struct device *dev)

> >   {

> > -	dma_addr_t base = IOVA_START_PFN << VTD_PAGE_SHIFT;

> > -	struct iommu_domain *domain = iommu_get_domain_for_dev(dev);

> > -	struct dmar_domain *dmar_domain = to_dmar_domain(domain);

> > -

> > -	if (domain && domain->type == IOMMU_DOMAIN_DMA)

> > -		iommu_setup_dma_ops(dev, base,

> > -				    __DOMAIN_MAX_ADDR(dmar_domain->gaw));

> > -	else

> > -		set_dma_ops(dev, NULL);

> > +	iommu_setup_dma_ops(dev, 0, U64_MAX);

> >   }

> >   static void intel_iommu_get_resv_regions(struct device *device,

> >
Robin Murphy June 18, 2021, 11:19 a.m. | #4
On 2021-06-18 11:50, Jean-Philippe Brucker wrote:
> On Wed, Jun 16, 2021 at 06:02:39PM +0100, Robin Murphy wrote:

>>> diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c

>>> index c62e19bed302..175f8eaeb5b3 100644

>>> --- a/drivers/iommu/dma-iommu.c

>>> +++ b/drivers/iommu/dma-iommu.c

>>> @@ -1322,7 +1322,9 @@ void iommu_setup_dma_ops(struct device *dev, u64 dma_base, u64 dma_limit)

>>>    	if (domain->type == IOMMU_DOMAIN_DMA) {

>>>    		if (iommu_dma_init_domain(domain, dma_base, dma_limit, dev))

>>>    			goto out_err;

>>> -		dev->dma_ops = &iommu_dma_ops;

>>> +		set_dma_ops(dev, &iommu_dma_ops);

>>> +	} else {

>>> +		set_dma_ops(dev, NULL);

>>

>> I'm not keen on moving this here, since iommu-dma only knows that its own

>> ops are right for devices it *is* managing; it can't assume any particular

>> ops are appropriate for devices it isn't. The idea here is that

>> arch_setup_dma_ops() may have already set the appropriate ops for the

>> non-IOMMU case, so if the default domain type is passthrough then we leave

>> those in place.

>>

>> For example, I do still plan to revisit my conversion of arch/arm someday,

>> at which point I'd have to undo this for that reason.

> 

> Makes sense, I'll remove this bit.

> 

>> Simplifying the base and size arguments is of course fine, but TBH I'd say

>> rip the whole bloody lot out of the arch_setup_dma_ops() flow now. It's a

>> considerable faff passing them around for nothing but a tenuous sanity check

>> in iommu_dma_init_domain(), and now that dev->dma_range_map is a common

>> thing we should expect that to give us any relevant limitations if we even

>> still care.

> 

> So I started working on this but it gets too bulky for a preparatory

> patch. Dropping the parameters from arch_setup_dma_ops() seems especially

> complicated because arm32 does need the size parameter for IOMMU mappings

> and that value falls back to the bus DMA mask or U32_MAX in the absence of

> dma-ranges. I could try to dig into this for a separate series.

> 

> Even only dropping the parameters from iommu_setup_dma_ops() isn't

> completely trivial (8 files changed, 55 insertions(+), 36 deletions(-)

> because we still need the lower IOVA limit from dma_range_map), so I'd

> rather send it separately and have it sit in -next for a while.


Oh, sure, I didn't mean to imply that the whole cleanup should be within 
the scope of this series, just that we can shave off as much as we *do* 
need to touch here (which TBH is pretty much what you're doing already), 
and mainly to start taking the attitude that these arguments are now 
superseded and increasingly vestigial.

I expected the cross-arch cleanup to be a bit fiddly, but I'd forgotten 
that arch/arm was still actively using these values, so maybe I can 
revisit this when I pick up my iommu-dma conversion again (I swear it's 
not dead, just resting!)

Cheers,
Robin.

Patch

diff --git a/drivers/iommu/amd/iommu.c b/drivers/iommu/amd/iommu.c
index 94b96d81fcfd..d3123bc05c08 100644
--- a/drivers/iommu/amd/iommu.c
+++ b/drivers/iommu/amd/iommu.c
@@ -1708,14 +1708,7 @@  static struct iommu_device *amd_iommu_probe_device(struct device *dev)
 
 static void amd_iommu_probe_finalize(struct device *dev)
 {
-	struct iommu_domain *domain;
-
-	/* Domains are initialized for this device - have a look what we ended up with */
-	domain = iommu_get_domain_for_dev(dev);
-	if (domain->type == IOMMU_DOMAIN_DMA)
-		iommu_setup_dma_ops(dev, IOVA_START_PFN << PAGE_SHIFT, U64_MAX);
-	else
-		set_dma_ops(dev, NULL);
+	iommu_setup_dma_ops(dev, 0, U64_MAX);
 }
 
 static void amd_iommu_release_device(struct device *dev)
diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
index c62e19bed302..175f8eaeb5b3 100644
--- a/drivers/iommu/dma-iommu.c
+++ b/drivers/iommu/dma-iommu.c
@@ -1322,7 +1322,9 @@  void iommu_setup_dma_ops(struct device *dev, u64 dma_base, u64 dma_limit)
 	if (domain->type == IOMMU_DOMAIN_DMA) {
 		if (iommu_dma_init_domain(domain, dma_base, dma_limit, dev))
 			goto out_err;
-		dev->dma_ops = &iommu_dma_ops;
+		set_dma_ops(dev, &iommu_dma_ops);
+	} else {
+		set_dma_ops(dev, NULL);
 	}
 
 	return;
diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index 85f18342603c..8d866940692a 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -5165,15 +5165,7 @@  static void intel_iommu_release_device(struct device *dev)
 
 static void intel_iommu_probe_finalize(struct device *dev)
 {
-	dma_addr_t base = IOVA_START_PFN << VTD_PAGE_SHIFT;
-	struct iommu_domain *domain = iommu_get_domain_for_dev(dev);
-	struct dmar_domain *dmar_domain = to_dmar_domain(domain);
-
-	if (domain && domain->type == IOMMU_DOMAIN_DMA)
-		iommu_setup_dma_ops(dev, base,
-				    __DOMAIN_MAX_ADDR(dmar_domain->gaw));
-	else
-		set_dma_ops(dev, NULL);
+	iommu_setup_dma_ops(dev, 0, U64_MAX);
 }
 
 static void intel_iommu_get_resv_regions(struct device *device,