[PATCH 1/2] iommu/dma: Implement dma_{map,unmap}_resource()

[PATCH 1/2] iommu/dma: Implement dma_{map,unmap}_resource()

[Robin Murphy]

With the new dma_{map,unmap}_resource() functions added to the DMA API
for the benefit of cases like slave DMA, add suitable implementations to
the arsenal of our generic layer. Since cache maintenance should not be
a concern, these can both be standalone versions without the need for
architecture-specific wrappers.

CC: Joerg Roedel <joro-zLv9SwRftAIdnm+yROfE0A@public.gmane.org>
Signed-off-by: Robin Murphy <robin.murphy-5wv7dgnIgG8@public.gmane.org>
---

Since patch 2 has a build dependency on this one, they should probably
go together through either the arm64 tree or the iommu tree, but I can't
make up my mind which one seems more appropriate...

Robin.

 drivers/iommu/dma-iommu.c | 13 +++++++++++++
 include/linux/dma-iommu.h |  4 ++++
 2 files changed, 17 insertions(+)

diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
index c5ab8667e6f2..50acd71915db 100644
--- a/drivers/iommu/dma-iommu.c
+++ b/drivers/iommu/dma-iommu.c
@@ -624,6 +624,19 @@ void iommu_dma_unmap_sg(struct device *dev, struct scatterlist *sg, int nents,
 	__iommu_dma_unmap(iommu_get_domain_for_dev(dev), sg_dma_address(sg));
 }
 
+dma_addr_t iommu_dma_map_resource(struct device *dev, phys_addr_t phys,
+		size_t size, enum dma_data_direction dir, unsigned long attrs)
+{
+	return iommu_dma_map_page(dev, phys_to_page(phys), offset_in_page(phys),
+			size, dma_direction_to_prot(dir, false) | IOMMU_MMIO);
+}
+
+void iommu_dma_unmap_resource(struct device *dev, dma_addr_t handle,
+		size_t size, enum dma_data_direction dir, unsigned long attrs)
+{
+	__iommu_dma_unmap(iommu_get_domain_for_dev(dev), handle);
+}
+
 int iommu_dma_supported(struct device *dev, u64 mask)
 {
 	/*
diff --git a/include/linux/dma-iommu.h b/include/linux/dma-iommu.h
index 32c589062bd9..7f7e9a7e3839 100644
--- a/include/linux/dma-iommu.h
+++ b/include/linux/dma-iommu.h
@@ -61,6 +61,10 @@ void iommu_dma_unmap_page(struct device *dev, dma_addr_t handle, size_t size,
 		enum dma_data_direction dir, unsigned long attrs);
 void iommu_dma_unmap_sg(struct device *dev, struct scatterlist *sg, int nents,
 		enum dma_data_direction dir, unsigned long attrs);
+dma_addr_t iommu_dma_map_resource(struct device *dev, phys_addr_t phys,
+		size_t size, enum dma_data_direction dir, unsigned long attrs);
+void iommu_dma_unmap_resource(struct device *dev, dma_addr_t handle,
+		size_t size, enum dma_data_direction dir, unsigned long attrs);
 int iommu_dma_supported(struct device *dev, u64 mask);
 int iommu_dma_mapping_error(struct device *dev, dma_addr_t dma_addr);
 
-- 
1.9.1

[PATCH 1/2] iommu/dma: Implement dma_{map,unmap}_resource()

[Robin Murphy]

With the new dma_{map,unmap}_resource() functions added to the DMA API
for the benefit of cases like slave DMA, add suitable implementations to
the arsenal of our generic layer. Since cache maintenance should not be
a concern, these can both be standalone versions without the need for
architecture-specific wrappers.

CC: Joerg Roedel <joro@8bytes.org>
Signed-off-by: Robin Murphy <robin.murphy@arm.com>
---

Since patch 2 has a build dependency on this one, they should probably
go together through either the arm64 tree or the iommu tree, but I can't
make up my mind which one seems more appropriate...

Robin.

 drivers/iommu/dma-iommu.c | 13 +++++++++++++
 include/linux/dma-iommu.h |  4 ++++
 2 files changed, 17 insertions(+)

diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
index c5ab8667e6f2..50acd71915db 100644
--- a/drivers/iommu/dma-iommu.c
+++ b/drivers/iommu/dma-iommu.c
@@ -624,6 +624,19 @@ void iommu_dma_unmap_sg(struct device *dev, struct scatterlist *sg, int nents,
 	__iommu_dma_unmap(iommu_get_domain_for_dev(dev), sg_dma_address(sg));
 }
 
+dma_addr_t iommu_dma_map_resource(struct device *dev, phys_addr_t phys,
+		size_t size, enum dma_data_direction dir, unsigned long attrs)
+{
+	return iommu_dma_map_page(dev, phys_to_page(phys), offset_in_page(phys),
+			size, dma_direction_to_prot(dir, false) | IOMMU_MMIO);
+}
+
+void iommu_dma_unmap_resource(struct device *dev, dma_addr_t handle,
+		size_t size, enum dma_data_direction dir, unsigned long attrs)
+{
+	__iommu_dma_unmap(iommu_get_domain_for_dev(dev), handle);
+}
+
 int iommu_dma_supported(struct device *dev, u64 mask)
 {
 	/*
diff --git a/include/linux/dma-iommu.h b/include/linux/dma-iommu.h
index 32c589062bd9..7f7e9a7e3839 100644
--- a/include/linux/dma-iommu.h
+++ b/include/linux/dma-iommu.h
@@ -61,6 +61,10 @@ void iommu_dma_unmap_page(struct device *dev, dma_addr_t handle, size_t size,
 		enum dma_data_direction dir, unsigned long attrs);
 void iommu_dma_unmap_sg(struct device *dev, struct scatterlist *sg, int nents,
 		enum dma_data_direction dir, unsigned long attrs);
+dma_addr_t iommu_dma_map_resource(struct device *dev, phys_addr_t phys,
+		size_t size, enum dma_data_direction dir, unsigned long attrs);
+void iommu_dma_unmap_resource(struct device *dev, dma_addr_t handle,
+		size_t size, enum dma_data_direction dir, unsigned long attrs);
 int iommu_dma_supported(struct device *dev, u64 mask);
 int iommu_dma_mapping_error(struct device *dev, dma_addr_t dma_addr);
 
-- 
1.9.1

[PATCH 2/2] arm64: Wire up iommu_dma_{map, unmap}_resource()

[Robin Murphy]

With no coherency to worry about, just plug'em straight in.

CC: Catalin Marinas <catalin.marinas-5wv7dgnIgG8@public.gmane.org>
CC: Will Deacon <will.deacon-5wv7dgnIgG8@public.gmane.org>
Signed-off-by: Robin Murphy <robin.murphy-5wv7dgnIgG8@public.gmane.org>
---
 arch/arm64/mm/dma-mapping.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/arm64/mm/dma-mapping.c b/arch/arm64/mm/dma-mapping.c
index 3f74d0d98de6..5cd0a383b14b 100644
--- a/arch/arm64/mm/dma-mapping.c
+++ b/arch/arm64/mm/dma-mapping.c
@@ -796,6 +796,8 @@ static void __iommu_unmap_sg_attrs(struct device *dev,
 	.sync_single_for_device = __iommu_sync_single_for_device,
 	.sync_sg_for_cpu = __iommu_sync_sg_for_cpu,
 	.sync_sg_for_device = __iommu_sync_sg_for_device,
+	.map_resource = iommu_dma_map_resource,
+	.unmap_resource = iommu_dma_unmap_resource,
 	.dma_supported = iommu_dma_supported,
 	.mapping_error = iommu_dma_mapping_error,
 };
-- 
1.9.1

[PATCH 2/2] arm64: Wire up iommu_dma_{map, unmap}_resource()

[Robin Murphy]

With no coherency to worry about, just plug'em straight in.

CC: Catalin Marinas <catalin.marinas@arm.com>
CC: Will Deacon <will.deacon@arm.com>
Signed-off-by: Robin Murphy <robin.murphy@arm.com>
---
 arch/arm64/mm/dma-mapping.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/arm64/mm/dma-mapping.c b/arch/arm64/mm/dma-mapping.c
index 3f74d0d98de6..5cd0a383b14b 100644
--- a/arch/arm64/mm/dma-mapping.c
+++ b/arch/arm64/mm/dma-mapping.c
@@ -796,6 +796,8 @@ static void __iommu_unmap_sg_attrs(struct device *dev,
 	.sync_single_for_device = __iommu_sync_single_for_device,
 	.sync_sg_for_cpu = __iommu_sync_sg_for_cpu,
 	.sync_sg_for_device = __iommu_sync_sg_for_device,
+	.map_resource = iommu_dma_map_resource,
+	.unmap_resource = iommu_dma_unmap_resource,
 	.dma_supported = iommu_dma_supported,
 	.mapping_error = iommu_dma_mapping_error,
 };
-- 
1.9.1

Re: [PATCH 1/2] iommu/dma: Implement dma_{map,unmap}_resource()

[Will Deacon]

On Mon, Oct 17, 2016 at 01:05:29PM +0100, Robin Murphy wrote:
> With the new dma_{map,unmap}_resource() functions added to the DMA API
> for the benefit of cases like slave DMA, add suitable implementations to
> the arsenal of our generic layer. Since cache maintenance should not be
> a concern, these can both be standalone versions without the need for
> architecture-specific wrappers.
> 
> CC: Joerg Roedel <joro-zLv9SwRftAIdnm+yROfE0A@public.gmane.org>
> Signed-off-by: Robin Murphy <robin.murphy-5wv7dgnIgG8@public.gmane.org>
> ---
> 
> Since patch 2 has a build dependency on this one, they should probably
> go together through either the arm64 tree or the iommu tree, but I can't
> make up my mind which one seems more appropriate...

I can take it via the smmu tree, if you like. However, comment below.

>  drivers/iommu/dma-iommu.c | 13 +++++++++++++
>  include/linux/dma-iommu.h |  4 ++++
>  2 files changed, 17 insertions(+)
> 
> diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
> index c5ab8667e6f2..50acd71915db 100644
> --- a/drivers/iommu/dma-iommu.c
> +++ b/drivers/iommu/dma-iommu.c
> @@ -624,6 +624,19 @@ void iommu_dma_unmap_sg(struct device *dev, struct scatterlist *sg, int nents,
>  	__iommu_dma_unmap(iommu_get_domain_for_dev(dev), sg_dma_address(sg));
>  }
>  
> +dma_addr_t iommu_dma_map_resource(struct device *dev, phys_addr_t phys,
> +		size_t size, enum dma_data_direction dir, unsigned long attrs)
> +{
> +	return iommu_dma_map_page(dev, phys_to_page(phys), offset_in_page(phys),
> +			size, dma_direction_to_prot(dir, false) | IOMMU_MMIO);
> +}
>
> +void iommu_dma_unmap_resource(struct device *dev, dma_addr_t handle,
> +		size_t size, enum dma_data_direction dir, unsigned long attrs)
> +{
> +	__iommu_dma_unmap(iommu_get_domain_for_dev(dev), handle);
> +}

I think it's better to call iommu_dma_unmap_page instead. That said, are
you sure it's safe to ignore the "size" parameter here? Is it permitted
to unmap part of a region? If not, why does that parameter exist?

Will

[PATCH 1/2] iommu/dma: Implement dma_{map,unmap}_resource()

[Will Deacon]

On Mon, Oct 17, 2016 at 01:05:29PM +0100, Robin Murphy wrote:
> With the new dma_{map,unmap}_resource() functions added to the DMA API
> for the benefit of cases like slave DMA, add suitable implementations to
> the arsenal of our generic layer. Since cache maintenance should not be
> a concern, these can both be standalone versions without the need for
> architecture-specific wrappers.
> 
> CC: Joerg Roedel <joro@8bytes.org>
> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
> ---
> 
> Since patch 2 has a build dependency on this one, they should probably
> go together through either the arm64 tree or the iommu tree, but I can't
> make up my mind which one seems more appropriate...

I can take it via the smmu tree, if you like. However, comment below.

>  drivers/iommu/dma-iommu.c | 13 +++++++++++++
>  include/linux/dma-iommu.h |  4 ++++
>  2 files changed, 17 insertions(+)
> 
> diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
> index c5ab8667e6f2..50acd71915db 100644
> --- a/drivers/iommu/dma-iommu.c
> +++ b/drivers/iommu/dma-iommu.c
> @@ -624,6 +624,19 @@ void iommu_dma_unmap_sg(struct device *dev, struct scatterlist *sg, int nents,
>  	__iommu_dma_unmap(iommu_get_domain_for_dev(dev), sg_dma_address(sg));
>  }
>  
> +dma_addr_t iommu_dma_map_resource(struct device *dev, phys_addr_t phys,
> +		size_t size, enum dma_data_direction dir, unsigned long attrs)
> +{
> +	return iommu_dma_map_page(dev, phys_to_page(phys), offset_in_page(phys),
> +			size, dma_direction_to_prot(dir, false) | IOMMU_MMIO);
> +}
>
> +void iommu_dma_unmap_resource(struct device *dev, dma_addr_t handle,
> +		size_t size, enum dma_data_direction dir, unsigned long attrs)
> +{
> +	__iommu_dma_unmap(iommu_get_domain_for_dev(dev), handle);
> +}

I think it's better to call iommu_dma_unmap_page instead. That said, are
you sure it's safe to ignore the "size" parameter here? Is it permitted
to unmap part of a region? If not, why does that parameter exist?

Will

Re: [PATCH 1/2] iommu/dma: Implement dma_{map,unmap}_resource()

[Robin Murphy]

On 19/10/16 15:02, Will Deacon wrote:
> On Mon, Oct 17, 2016 at 01:05:29PM +0100, Robin Murphy wrote:
>> With the new dma_{map,unmap}_resource() functions added to the DMA API
>> for the benefit of cases like slave DMA, add suitable implementations to
>> the arsenal of our generic layer. Since cache maintenance should not be
>> a concern, these can both be standalone versions without the need for
>> architecture-specific wrappers.
>>
>> CC: Joerg Roedel <joro-zLv9SwRftAIdnm+yROfE0A@public.gmane.org>
>> Signed-off-by: Robin Murphy <robin.murphy-5wv7dgnIgG8@public.gmane.org>
>> ---
>>
>> Since patch 2 has a build dependency on this one, they should probably
>> go together through either the arm64 tree or the iommu tree, but I can't
>> make up my mind which one seems more appropriate...
> 
> I can take it via the smmu tree, if you like. However, comment below.

I'm surprised that didn't occur to me - makes sense, thanks.

>>  drivers/iommu/dma-iommu.c | 13 +++++++++++++
>>  include/linux/dma-iommu.h |  4 ++++
>>  2 files changed, 17 insertions(+)
>>
>> diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
>> index c5ab8667e6f2..50acd71915db 100644
>> --- a/drivers/iommu/dma-iommu.c
>> +++ b/drivers/iommu/dma-iommu.c
>> @@ -624,6 +624,19 @@ void iommu_dma_unmap_sg(struct device *dev, struct scatterlist *sg, int nents,
>>  	__iommu_dma_unmap(iommu_get_domain_for_dev(dev), sg_dma_address(sg));
>>  }
>>  
>> +dma_addr_t iommu_dma_map_resource(struct device *dev, phys_addr_t phys,
>> +		size_t size, enum dma_data_direction dir, unsigned long attrs)
>> +{
>> +	return iommu_dma_map_page(dev, phys_to_page(phys), offset_in_page(phys),
>> +			size, dma_direction_to_prot(dir, false) | IOMMU_MMIO);
>> +}
>>
>> +void iommu_dma_unmap_resource(struct device *dev, dma_addr_t handle,
>> +		size_t size, enum dma_data_direction dir, unsigned long attrs)
>> +{
>> +	__iommu_dma_unmap(iommu_get_domain_for_dev(dev), handle);
>> +}
> 
> I think it's better to call iommu_dma_unmap_page instead. That said, are
> you sure it's safe to ignore the "size" parameter here? Is it permitted
> to unmap part of a region? If not, why does that parameter exist?

Actually, "#define iommu_dma_unmap_resource iommu_dma_unmap_page" in the
header would make life even simpler. Or is that too evil?

Glossing over the size parameter is a detail of this particular
implementation. Equivalently to iommu_dma_unmap_page(), it's there
(along with dir and attrs) to match the signature of
dma_unmap_resource(), so that arch code doesn't need yet another wrapper
for its .unmap_resource callback in dma_ops (see patch 2). As it
happens, ignoring the size is effectively extra-safe in the sense that
we're enforcing the API and not even trusting the caller - since we have
to look up the iova to free it, and that iova records the size
originally mapped, we simply unmap that original size because it makes
for simpler code and can't be wrong. See the __iommu_dma_unmap()
implementation itself.

Robin.

> 
> Will
> 

[PATCH 1/2] iommu/dma: Implement dma_{map,unmap}_resource()

[Robin Murphy]

On 19/10/16 15:02, Will Deacon wrote:
> On Mon, Oct 17, 2016 at 01:05:29PM +0100, Robin Murphy wrote:
>> With the new dma_{map,unmap}_resource() functions added to the DMA API
>> for the benefit of cases like slave DMA, add suitable implementations to
>> the arsenal of our generic layer. Since cache maintenance should not be
>> a concern, these can both be standalone versions without the need for
>> architecture-specific wrappers.
>>
>> CC: Joerg Roedel <joro@8bytes.org>
>> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
>> ---
>>
>> Since patch 2 has a build dependency on this one, they should probably
>> go together through either the arm64 tree or the iommu tree, but I can't
>> make up my mind which one seems more appropriate...
> 
> I can take it via the smmu tree, if you like. However, comment below.

I'm surprised that didn't occur to me - makes sense, thanks.

>>  drivers/iommu/dma-iommu.c | 13 +++++++++++++
>>  include/linux/dma-iommu.h |  4 ++++
>>  2 files changed, 17 insertions(+)
>>
>> diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
>> index c5ab8667e6f2..50acd71915db 100644
>> --- a/drivers/iommu/dma-iommu.c
>> +++ b/drivers/iommu/dma-iommu.c
>> @@ -624,6 +624,19 @@ void iommu_dma_unmap_sg(struct device *dev, struct scatterlist *sg, int nents,
>>  	__iommu_dma_unmap(iommu_get_domain_for_dev(dev), sg_dma_address(sg));
>>  }
>>  
>> +dma_addr_t iommu_dma_map_resource(struct device *dev, phys_addr_t phys,
>> +		size_t size, enum dma_data_direction dir, unsigned long attrs)
>> +{
>> +	return iommu_dma_map_page(dev, phys_to_page(phys), offset_in_page(phys),
>> +			size, dma_direction_to_prot(dir, false) | IOMMU_MMIO);
>> +}
>>
>> +void iommu_dma_unmap_resource(struct device *dev, dma_addr_t handle,
>> +		size_t size, enum dma_data_direction dir, unsigned long attrs)
>> +{
>> +	__iommu_dma_unmap(iommu_get_domain_for_dev(dev), handle);
>> +}
> 
> I think it's better to call iommu_dma_unmap_page instead. That said, are
> you sure it's safe to ignore the "size" parameter here? Is it permitted
> to unmap part of a region? If not, why does that parameter exist?

Actually, "#define iommu_dma_unmap_resource iommu_dma_unmap_page" in the
header would make life even simpler. Or is that too evil?

Glossing over the size parameter is a detail of this particular
implementation. Equivalently to iommu_dma_unmap_page(), it's there
(along with dir and attrs) to match the signature of
dma_unmap_resource(), so that arch code doesn't need yet another wrapper
for its .unmap_resource callback in dma_ops (see patch 2). As it
happens, ignoring the size is effectively extra-safe in the sense that
we're enforcing the API and not even trusting the caller - since we have
to look up the iova to free it, and that iova records the size
originally mapped, we simply unmap that original size because it makes
for simpler code and can't be wrong. See the __iommu_dma_unmap()
implementation itself.

Robin.

> 
> Will
>