[PATCH v2 0/3] dma-mapping: introduce new dma unmap and sync variants

[PATCH v2 0/3] dma-mapping: introduce new dma unmap and sync variants

[Laurentiu Tudor]

From: Laurentiu Tudor <laurentiu.tudor@nxp.com>

This series introduces a few new dma unmap and sync api variants that,
on top of what the originals do, return the virtual address
corresponding to the input dma address. In order to do that a new dma
map op is added, .get_virt_addr that takes the input dma address and
returns the virtual address backing it up.
The second patch adds an implementation for this new dma map op in the
generic iommu dma glue code and wires it in.
The third patch updates the dpaa2-eth driver to use the new apis.

Context: https://lkml.org/lkml/2019/5/31/684

Changes in v2
 * use "dma_unmap_*_desc" names (Robin)
 * return va/page instead of phys addr (Robin)

Changes since RFC
 * completely changed the approach: added unmap and sync variants that
   return the phys addr instead of adding a new dma to phys conversion
   function

Laurentiu Tudor (3):
  dma-mapping: introduce new dma unmap and sync api variants
  iommu/dma: wire-up new dma map op .get_virt_addr
  dpaa2_eth: use new unmap and sync dma api variants

 drivers/iommu/dma-iommu.c                     | 16 ++++++
 .../net/ethernet/freescale/dpaa2/dpaa2-eth.c  | 40 +++++---------
 .../net/ethernet/freescale/dpaa2/dpaa2-eth.h  |  1 -
 include/linux/dma-mapping.h                   | 55 +++++++++++++++++++
 4 files changed, 86 insertions(+), 26 deletions(-)

-- 
2.17.1

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

[PATCH v2 1/3] dma-mapping: introduce new dma unmap and sync api variants

[Laurentiu Tudor]

From: Laurentiu Tudor <laurentiu.tudor@nxp.com>

Introduce a few new dma unmap and sync variants that, on top of the
original variants, return the virtual address corresponding to the
input dma address.
In order to implement this a new dma map op is added and used:
    void *get_virt_addr(dev, dma_handle);
It does the actual conversion of an input dma address to the output
virtual address.

Signed-off-by: Laurentiu Tudor <laurentiu.tudor@nxp.com>
---
 include/linux/dma-mapping.h | 55 +++++++++++++++++++++++++++++++++++++
 1 file changed, 55 insertions(+)

diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h
index 4a1c4fca475a..ae7bb8a84b9d 100644
--- a/include/linux/dma-mapping.h
+++ b/include/linux/dma-mapping.h
@@ -132,6 +132,7 @@ struct dma_map_ops {
 	u64 (*get_required_mask)(struct device *dev);
 	size_t (*max_mapping_size)(struct device *dev);
 	unsigned long (*get_merge_boundary)(struct device *dev);
+	void *(*get_virt_addr)(struct device *dev, dma_addr_t dma_handle);
 };
 
 #define DMA_MAPPING_ERROR		(~(dma_addr_t)0)
@@ -304,6 +305,21 @@ static inline void dma_unmap_page_attrs(struct device *dev, dma_addr_t addr,
 	debug_dma_unmap_page(dev, addr, size, dir);
 }
 
+static inline struct page *
+dma_unmap_page_attrs_desc(struct device *dev, dma_addr_t addr, size_t size,
+			  enum dma_data_direction dir, unsigned long attrs)
+{
+	const struct dma_map_ops *ops = get_dma_ops(dev);
+	void *ptr = NULL;
+
+	if (ops && ops->get_virt_addr)
+		ptr = ops->get_virt_addr(dev, addr);
+
+	dma_unmap_page_attrs(dev, addr, size, dir, attrs);
+
+	return ptr ? virt_to_page(ptr) : NULL;
+}
+
 /*
  * dma_maps_sg_attrs returns 0 on error and > 0 on success.
  * It should never return a value < 0.
@@ -390,6 +406,21 @@ static inline void dma_sync_single_for_cpu(struct device *dev, dma_addr_t addr,
 	debug_dma_sync_single_for_cpu(dev, addr, size, dir);
 }
 
+static inline void *
+dma_sync_single_for_cpu_desc(struct device *dev, dma_addr_t addr, size_t size,
+			     enum dma_data_direction dir)
+{
+	const struct dma_map_ops *ops = get_dma_ops(dev);
+	void *ptr = NULL;
+
+	if (ops && ops->get_virt_addr)
+		ptr = ops->get_virt_addr(dev, addr);
+
+	dma_sync_single_for_cpu(dev, addr, size, dir);
+
+	return ptr;
+}
+
 static inline void dma_sync_single_for_device(struct device *dev,
 					      dma_addr_t addr, size_t size,
 					      enum dma_data_direction dir)
@@ -500,6 +531,12 @@ static inline void dma_sync_single_for_cpu(struct device *dev, dma_addr_t addr,
 		size_t size, enum dma_data_direction dir)
 {
 }
+
+static inline void *
+dma_sync_single_for_cpu_desc(struct device *dev, dma_addr_t addr, size_t size,
+			     enum dma_data_direction dir)
+{
+}
 static inline void dma_sync_single_for_device(struct device *dev,
 		dma_addr_t addr, size_t size, enum dma_data_direction dir)
 {
@@ -594,6 +631,21 @@ static inline void dma_unmap_single_attrs(struct device *dev, dma_addr_t addr,
 	return dma_unmap_page_attrs(dev, addr, size, dir, attrs);
 }
 
+static inline void *
+dma_unmap_single_attrs_desc(struct device *dev, dma_addr_t addr, size_t size,
+			    enum dma_data_direction dir, unsigned long attrs)
+{
+	const struct dma_map_ops *ops = get_dma_ops(dev);
+	void *ptr = NULL;
+
+	if (ops && ops->get_virt_addr)
+		ptr = ops->get_virt_addr(dev, addr);
+
+	dma_unmap_single_attrs(dev, addr, size, dir, attrs);
+
+	return ptr;
+}
+
 static inline void dma_sync_single_range_for_cpu(struct device *dev,
 		dma_addr_t addr, unsigned long offset, size_t size,
 		enum dma_data_direction dir)
@@ -610,10 +662,13 @@ static inline void dma_sync_single_range_for_device(struct device *dev,
 
 #define dma_map_single(d, a, s, r) dma_map_single_attrs(d, a, s, r, 0)
 #define dma_unmap_single(d, a, s, r) dma_unmap_single_attrs(d, a, s, r, 0)
+#define dma_unmap_single_desc(d, a, s, r) \
+		dma_unmap_single_attrs_desc(d, a, s, r, 0)
 #define dma_map_sg(d, s, n, r) dma_map_sg_attrs(d, s, n, r, 0)
 #define dma_unmap_sg(d, s, n, r) dma_unmap_sg_attrs(d, s, n, r, 0)
 #define dma_map_page(d, p, o, s, r) dma_map_page_attrs(d, p, o, s, r, 0)
 #define dma_unmap_page(d, a, s, r) dma_unmap_page_attrs(d, a, s, r, 0)
+#define dma_unmap_page_desc(d, a, s, r) dma_unmap_page_attrs_desc(d, a, s, r, 0)
 #define dma_get_sgtable(d, t, v, h, s) dma_get_sgtable_attrs(d, t, v, h, s, 0)
 #define dma_mmap_coherent(d, v, c, h, s) dma_mmap_attrs(d, v, c, h, s, 0)
 
-- 
2.17.1

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v2 1/3] dma-mapping: introduce new dma unmap and sync api variants

[hch]

On Thu, Oct 24, 2019 at 12:41:41PM +0000, Laurentiu Tudor wrote:
> From: Laurentiu Tudor <laurentiu.tudor@nxp.com>
> 
> Introduce a few new dma unmap and sync variants that, on top of the
> original variants, return the virtual address corresponding to the
> input dma address.
> In order to implement this a new dma map op is added and used:
>     void *get_virt_addr(dev, dma_handle);
> It does the actual conversion of an input dma address to the output
> virtual address.

We'll definitively need an implementation for dma-direct at least as
well.  Also as said previously we need a dma_can_unmap_by_dma_addr()
or similar helper that tells the driver beforehand if this works, so
that the driver can either use a sub-optimal workaround or fail the
probe if this functionality isn't implemented.
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

RE: [PATCH v2 1/3] dma-mapping: introduce new dma unmap and sync api variants

[Laurentiu Tudor]

> -----Original Message-----
> From: hch@lst.de <hch@lst.de>
> Sent: Monday, October 28, 2019 2:38 PM
> 
> On Thu, Oct 24, 2019 at 12:41:41PM +0000, Laurentiu Tudor wrote:
> > From: Laurentiu Tudor <laurentiu.tudor@nxp.com>
> >
> > Introduce a few new dma unmap and sync variants that, on top of the
> > original variants, return the virtual address corresponding to the
> > input dma address.
> > In order to implement this a new dma map op is added and used:
> >     void *get_virt_addr(dev, dma_handle);
> > It does the actual conversion of an input dma address to the output
> > virtual address.
> 
> We'll definitively need an implementation for dma-direct at least as
> well.  Also as said previously we need a dma_can_unmap_by_dma_addr()
> or similar helper that tells the driver beforehand if this works, so
> that the driver can either use a sub-optimal workaround or fail the
> probe if this functionality isn't implemented.

Alright. On top of that I need to make this work on booke ppc as we have one driver that runs both on arm and ppc and will use these APIs.

---
Best Regards, Laurentiu
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v2 1/3] dma-mapping: introduce new dma unmap and sync api variants

[Robin Murphy]

On 24/10/2019 13:41, Laurentiu Tudor wrote:
> From: Laurentiu Tudor <laurentiu.tudor@nxp.com>
> 
> Introduce a few new dma unmap and sync variants that, on top of the
> original variants, return the virtual address corresponding to the
> input dma address.
> In order to implement this a new dma map op is added and used:
>      void *get_virt_addr(dev, dma_handle);
> It does the actual conversion of an input dma address to the output
> virtual address.

At this point, I think it might be better to just change the prototype 
of the .unmap_page/.sync_single_for_cpu callbacks themselves. In cases 
where .get_virt_addr would be non-trivial, it's most likely duplicating 
work that the relevant callback has to do anyway (i.e. where the virtual 
and/or physical address is needed internally for a cache maintenance or 
bounce buffer operation). It would also help avoid any possible 
ambiguity about whether .get_virt_addr returns the VA corresponding 
dma_handle (if one exists) rather than the VA of the buffer *mapped to* 
dma_handle, which for a bounce-buffering implementation would be 
different, and the one you actually need - a naive 
phys_to_virt(dma_to_phys(dma_handle)) would lead you to the wrong place 
(in fact it looks like DPAA2 would currently go wrong with 
"swiotlb=force" and the SMMU disabled or in passthrough).

One question there is whether we'd want careful special-casing to avoid 
introducing overhead where unmap/sync are currently complete no-ops, or 
whether an extra phys_to_virt() or so in those paths would be tolerable.

> Signed-off-by: Laurentiu Tudor <laurentiu.tudor@nxp.com>
> ---
>   include/linux/dma-mapping.h | 55 +++++++++++++++++++++++++++++++++++++
>   1 file changed, 55 insertions(+)
> 
> diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h
> index 4a1c4fca475a..ae7bb8a84b9d 100644
> --- a/include/linux/dma-mapping.h
> +++ b/include/linux/dma-mapping.h
> @@ -132,6 +132,7 @@ struct dma_map_ops {
>   	u64 (*get_required_mask)(struct device *dev);
>   	size_t (*max_mapping_size)(struct device *dev);
>   	unsigned long (*get_merge_boundary)(struct device *dev);
> +	void *(*get_virt_addr)(struct device *dev, dma_addr_t dma_handle);
>   };
>   
>   #define DMA_MAPPING_ERROR		(~(dma_addr_t)0)
> @@ -304,6 +305,21 @@ static inline void dma_unmap_page_attrs(struct device *dev, dma_addr_t addr,
>   	debug_dma_unmap_page(dev, addr, size, dir);
>   }
>   
> +static inline struct page *
> +dma_unmap_page_attrs_desc(struct device *dev, dma_addr_t addr, size_t size,
> +			  enum dma_data_direction dir, unsigned long attrs)
> +{
> +	const struct dma_map_ops *ops = get_dma_ops(dev);
> +	void *ptr = NULL;
> +
> +	if (ops && ops->get_virt_addr)
> +		ptr = ops->get_virt_addr(dev, addr);

Note that this doesn't work for dma-direct, but for the sake of arm64 at 
least it almost certainly wants to.

Robin.

> +	dma_unmap_page_attrs(dev, addr, size, dir, attrs);
> +
> +	return ptr ? virt_to_page(ptr) : NULL;
> +}
> +
>   /*
>    * dma_maps_sg_attrs returns 0 on error and > 0 on success.
>    * It should never return a value < 0.
> @@ -390,6 +406,21 @@ static inline void dma_sync_single_for_cpu(struct device *dev, dma_addr_t addr,
>   	debug_dma_sync_single_for_cpu(dev, addr, size, dir);
>   }
>   
> +static inline void *
> +dma_sync_single_for_cpu_desc(struct device *dev, dma_addr_t addr, size_t size,
> +			     enum dma_data_direction dir)
> +{
> +	const struct dma_map_ops *ops = get_dma_ops(dev);
> +	void *ptr = NULL;
> +
> +	if (ops && ops->get_virt_addr)
> +		ptr = ops->get_virt_addr(dev, addr);
> +
> +	dma_sync_single_for_cpu(dev, addr, size, dir);
> +
> +	return ptr;
> +}
> +
>   static inline void dma_sync_single_for_device(struct device *dev,
>   					      dma_addr_t addr, size_t size,
>   					      enum dma_data_direction dir)
> @@ -500,6 +531,12 @@ static inline void dma_sync_single_for_cpu(struct device *dev, dma_addr_t addr,
>   		size_t size, enum dma_data_direction dir)
>   {
>   }
> +
> +static inline void *
> +dma_sync_single_for_cpu_desc(struct device *dev, dma_addr_t addr, size_t size,
> +			     enum dma_data_direction dir)
> +{
> +}
>   static inline void dma_sync_single_for_device(struct device *dev,
>   		dma_addr_t addr, size_t size, enum dma_data_direction dir)
>   {
> @@ -594,6 +631,21 @@ static inline void dma_unmap_single_attrs(struct device *dev, dma_addr_t addr,
>   	return dma_unmap_page_attrs(dev, addr, size, dir, attrs);
>   }
>   
> +static inline void *
> +dma_unmap_single_attrs_desc(struct device *dev, dma_addr_t addr, size_t size,
> +			    enum dma_data_direction dir, unsigned long attrs)
> +{
> +	const struct dma_map_ops *ops = get_dma_ops(dev);
> +	void *ptr = NULL;
> +
> +	if (ops && ops->get_virt_addr)
> +		ptr = ops->get_virt_addr(dev, addr);
> +
> +	dma_unmap_single_attrs(dev, addr, size, dir, attrs);
> +
> +	return ptr;
> +}
> +
>   static inline void dma_sync_single_range_for_cpu(struct device *dev,
>   		dma_addr_t addr, unsigned long offset, size_t size,
>   		enum dma_data_direction dir)
> @@ -610,10 +662,13 @@ static inline void dma_sync_single_range_for_device(struct device *dev,
>   
>   #define dma_map_single(d, a, s, r) dma_map_single_attrs(d, a, s, r, 0)
>   #define dma_unmap_single(d, a, s, r) dma_unmap_single_attrs(d, a, s, r, 0)
> +#define dma_unmap_single_desc(d, a, s, r) \
> +		dma_unmap_single_attrs_desc(d, a, s, r, 0)
>   #define dma_map_sg(d, s, n, r) dma_map_sg_attrs(d, s, n, r, 0)
>   #define dma_unmap_sg(d, s, n, r) dma_unmap_sg_attrs(d, s, n, r, 0)
>   #define dma_map_page(d, p, o, s, r) dma_map_page_attrs(d, p, o, s, r, 0)
>   #define dma_unmap_page(d, a, s, r) dma_unmap_page_attrs(d, a, s, r, 0)
> +#define dma_unmap_page_desc(d, a, s, r) dma_unmap_page_attrs_desc(d, a, s, r, 0)
>   #define dma_get_sgtable(d, t, v, h, s) dma_get_sgtable_attrs(d, t, v, h, s, 0)
>   #define dma_mmap_coherent(d, v, c, h, s) dma_mmap_attrs(d, v, c, h, s, 0)
>   
> 
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v2 1/3] dma-mapping: introduce new dma unmap and sync api variants

[Laurentiu Tudor]

On 28.10.2019 15:42, Robin Murphy wrote:
> On 24/10/2019 13:41, Laurentiu Tudor wrote:
>> From: Laurentiu Tudor <laurentiu.tudor@nxp.com>
>>
>> Introduce a few new dma unmap and sync variants that, on top of the
>> original variants, return the virtual address corresponding to the
>> input dma address.
>> In order to implement this a new dma map op is added and used:
>>      void *get_virt_addr(dev, dma_handle);
>> It does the actual conversion of an input dma address to the output
>> virtual address.
> 
> At this point, I think it might be better to just change the prototype 
> of the .unmap_page/.sync_single_for_cpu callbacks themselves. 

I could give this a try. At a first sight, looks like it will be quite 
an intrusive change.

> In cases 
> where .get_virt_addr would be non-trivial, it's most likely duplicating 
> work that the relevant callback has to do anyway (i.e. where the virtual 
> and/or physical address is needed internally for a cache maintenance or 
> bounce buffer operation). It would also help avoid any possible 
> ambiguity about whether .get_virt_addr returns the VA corresponding 
> dma_handle (if one exists) rather than the VA of the buffer *mapped to* 
> dma_handle, which for a bounce-buffering implementation would be 
> different, and the one you actually need - a naive 
> phys_to_virt(dma_to_phys(dma_handle)) would lead you to the wrong place 


> (in fact it looks like DPAA2 would currently go wrong with 
> "swiotlb=force" and the SMMU disabled or in passthrough).

Yes, most likely.

> One question there is whether we'd want careful special-casing to avoid 
> introducing overhead where unmap/sync are currently complete no-ops, or 
> whether an extra phys_to_virt() or so in those paths would be tolerable.
> 
>> Signed-off-by: Laurentiu Tudor <laurentiu.tudor@nxp.com>
>> ---
>>   include/linux/dma-mapping.h | 55 +++++++++++++++++++++++++++++++++++++
>>   1 file changed, 55 insertions(+)
>>
>> diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h
>> index 4a1c4fca475a..ae7bb8a84b9d 100644
>> --- a/include/linux/dma-mapping.h
>> +++ b/include/linux/dma-mapping.h
>> @@ -132,6 +132,7 @@ struct dma_map_ops {
>>       u64 (*get_required_mask)(struct device *dev);
>>       size_t (*max_mapping_size)(struct device *dev);
>>       unsigned long (*get_merge_boundary)(struct device *dev);
>> +    void *(*get_virt_addr)(struct device *dev, dma_addr_t dma_handle);
>>   };
>>   #define DMA_MAPPING_ERROR        (~(dma_addr_t)0)
>> @@ -304,6 +305,21 @@ static inline void dma_unmap_page_attrs(struct 
>> device *dev, dma_addr_t addr,
>>       debug_dma_unmap_page(dev, addr, size, dir);
>>   }
>> +static inline struct page *
>> +dma_unmap_page_attrs_desc(struct device *dev, dma_addr_t addr, size_t 
>> size,
>> +              enum dma_data_direction dir, unsigned long attrs)
>> +{
>> +    const struct dma_map_ops *ops = get_dma_ops(dev);
>> +    void *ptr = NULL;
>> +
>> +    if (ops && ops->get_virt_addr)
>> +        ptr = ops->get_virt_addr(dev, addr);
> 
> Note that this doesn't work for dma-direct, but for the sake of arm64 at 
> least it almost certainly wants to.
> 

Will take care of it.

---
Best Regards, Laurentiu

> 
>> +    dma_unmap_page_attrs(dev, addr, size, dir, attrs);
>> +
>> +    return ptr ? virt_to_page(ptr) : NULL;
>> +}
>> +
>>   /*
>>    * dma_maps_sg_attrs returns 0 on error and > 0 on success.
>>    * It should never return a value < 0.
>> @@ -390,6 +406,21 @@ static inline void dma_sync_single_for_cpu(struct 
>> device *dev, dma_addr_t addr,
>>       debug_dma_sync_single_for_cpu(dev, addr, size, dir);
>>   }
>> +static inline void *
>> +dma_sync_single_for_cpu_desc(struct device *dev, dma_addr_t addr, 
>> size_t size,
>> +                 enum dma_data_direction dir)
>> +{
>> +    const struct dma_map_ops *ops = get_dma_ops(dev);
>> +    void *ptr = NULL;
>> +
>> +    if (ops && ops->get_virt_addr)
>> +        ptr = ops->get_virt_addr(dev, addr);
>> +
>> +    dma_sync_single_for_cpu(dev, addr, size, dir);
>> +
>> +    return ptr;
>> +}
>> +
>>   static inline void dma_sync_single_for_device(struct device *dev,
>>                             dma_addr_t addr, size_t size,
>>                             enum dma_data_direction dir)
>> @@ -500,6 +531,12 @@ static inline void dma_sync_single_for_cpu(struct 
>> device *dev, dma_addr_t addr,
>>           size_t size, enum dma_data_direction dir)
>>   {
>>   }
>> +
>> +static inline void *
>> +dma_sync_single_for_cpu_desc(struct device *dev, dma_addr_t addr, 
>> size_t size,
>> +                 enum dma_data_direction dir)
>> +{
>> +}
>>   static inline void dma_sync_single_for_device(struct device *dev,
>>           dma_addr_t addr, size_t size, enum dma_data_direction dir)
>>   {
>> @@ -594,6 +631,21 @@ static inline void dma_unmap_single_attrs(struct 
>> device *dev, dma_addr_t addr,
>>       return dma_unmap_page_attrs(dev, addr, size, dir, attrs);
>>   }
>> +static inline void *
>> +dma_unmap_single_attrs_desc(struct device *dev, dma_addr_t addr, 
>> size_t size,
>> +                enum dma_data_direction dir, unsigned long attrs)
>> +{
>> +    const struct dma_map_ops *ops = get_dma_ops(dev);
>> +    void *ptr = NULL;
>> +
>> +    if (ops && ops->get_virt_addr)
>> +        ptr = ops->get_virt_addr(dev, addr);
>> +
>> +    dma_unmap_single_attrs(dev, addr, size, dir, attrs);
>> +
>> +    return ptr;
>> +}
>> +
>>   static inline void dma_sync_single_range_for_cpu(struct device *dev,
>>           dma_addr_t addr, unsigned long offset, size_t size,
>>           enum dma_data_direction dir)
>> @@ -610,10 +662,13 @@ static inline void 
>> dma_sync_single_range_for_device(struct device *dev,
>>   #define dma_map_single(d, a, s, r) dma_map_single_attrs(d, a, s, r, 0)
>>   #define dma_unmap_single(d, a, s, r) dma_unmap_single_attrs(d, a, s, 
>> r, 0)
>> +#define dma_unmap_single_desc(d, a, s, r) \
>> +        dma_unmap_single_attrs_desc(d, a, s, r, 0)
>>   #define dma_map_sg(d, s, n, r) dma_map_sg_attrs(d, s, n, r, 0)
>>   #define dma_unmap_sg(d, s, n, r) dma_unmap_sg_attrs(d, s, n, r, 0)
>>   #define dma_map_page(d, p, o, s, r) dma_map_page_attrs(d, p, o, s, 
>> r, 0)
>>   #define dma_unmap_page(d, a, s, r) dma_unmap_page_attrs(d, a, s, r, 0)
>> +#define dma_unmap_page_desc(d, a, s, r) dma_unmap_page_attrs_desc(d, 
>> a, s, r, 0)
>>   #define dma_get_sgtable(d, t, v, h, s) dma_get_sgtable_attrs(d, t, 
>> v, h, s, 0)
>>   #define dma_mmap_coherent(d, v, c, h, s) dma_mmap_attrs(d, v, c, h, 
>> s, 0)
>>
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v2 1/3] dma-mapping: introduce new dma unmap and sync api variants

[Laurentiu Tudor]

Hi Robin,

On 28.10.2019 15:42, Robin Murphy wrote:
> On 24/10/2019 13:41, Laurentiu Tudor wrote:
>> From: Laurentiu Tudor <laurentiu.tudor@nxp.com>
>>
>> Introduce a few new dma unmap and sync variants that, on top of the
>> original variants, return the virtual address corresponding to the
>> input dma address.
>> In order to implement this a new dma map op is added and used:
>>      void *get_virt_addr(dev, dma_handle);
>> It does the actual conversion of an input dma address to the output
>> virtual address.
> 
> At this point, I think it might be better to just change the prototype 
> of the .unmap_page/.sync_single_for_cpu callbacks themselves. In cases 
> where .get_virt_addr would be non-trivial, it's most likely duplicating 
> work that the relevant callback has to do anyway (i.e. where the virtual 
> and/or physical address is needed internally for a cache maintenance or 
> bounce buffer operation). 

Looking in the generic dma-iommu, I didn't see any mean of freely 
getting the pa or va bqcking the iova so I can't think of a way of doing 
this without adding a call to iommu_iova_to_phys() somewhere in the 
unmap op implementation. Obviously, this would come with an overhead 
that will probably upset people.
At the moment I can't think at an option other than the initial one, 
that is adding the .get_virt_addr op. Please let me know your opinions 
on this.

---
Thanks & Best Regards, Laurentiu

> It would also help avoid any possible 
> ambiguity about whether .get_virt_addr returns the VA corresponding 
> dma_handle (if one exists) rather than the VA of the buffer *mapped to* 
> dma_handle, which for a bounce-buffering implementation would be 
> different, and the one you actually need - a naive 
> phys_to_virt(dma_to_phys(dma_handle)) would lead you to the wrong place 
> (in fact it looks like DPAA2 would currently go wrong with 
> "swiotlb=force" and the SMMU disabled or in passthrough).
> 
> One question there is whether we'd want careful special-casing to avoid 
> introducing overhead where unmap/sync are currently complete no-ops, or 
> whether an extra phys_to_virt() or so in those paths would be tolerable.
> 
>> Signed-off-by: Laurentiu Tudor <laurentiu.tudor@nxp.com>
>> ---
>>   include/linux/dma-mapping.h | 55 +++++++++++++++++++++++++++++++++++++
>>   1 file changed, 55 insertions(+)
>>
>> diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h
>> index 4a1c4fca475a..ae7bb8a84b9d 100644
>> --- a/include/linux/dma-mapping.h
>> +++ b/include/linux/dma-mapping.h
>> @@ -132,6 +132,7 @@ struct dma_map_ops {
>>       u64 (*get_required_mask)(struct device *dev);
>>       size_t (*max_mapping_size)(struct device *dev);
>>       unsigned long (*get_merge_boundary)(struct device *dev);
>> +    void *(*get_virt_addr)(struct device *dev, dma_addr_t dma_handle);
>>   };
>>   #define DMA_MAPPING_ERROR        (~(dma_addr_t)0)
>> @@ -304,6 +305,21 @@ static inline void dma_unmap_page_attrs(struct 
>> device *dev, dma_addr_t addr,
>>       debug_dma_unmap_page(dev, addr, size, dir);
>>   }
>> +static inline struct page *
>> +dma_unmap_page_attrs_desc(struct device *dev, dma_addr_t addr, size_t 
>> size,
>> +              enum dma_data_direction dir, unsigned long attrs)
>> +{
>> +    const struct dma_map_ops *ops = get_dma_ops(dev);
>> +    void *ptr = NULL;
>> +
>> +    if (ops && ops->get_virt_addr)
>> +        ptr = ops->get_virt_addr(dev, addr);
> 
> Note that this doesn't work for dma-direct, but for the sake of arm64 at 
> least it almost certainly wants to.
> 
> Robin.
> 
>> +    dma_unmap_page_attrs(dev, addr, size, dir, attrs);
>> +
>> +    return ptr ? virt_to_page(ptr) : NULL;
>> +}
>> +
>>   /*
>>    * dma_maps_sg_attrs returns 0 on error and > 0 on success.
>>    * It should never return a value < 0.
>> @@ -390,6 +406,21 @@ static inline void dma_sync_single_for_cpu(struct 
>> device *dev, dma_addr_t addr,
>>       debug_dma_sync_single_for_cpu(dev, addr, size, dir);
>>   }
>> +static inline void *
>> +dma_sync_single_for_cpu_desc(struct device *dev, dma_addr_t addr, 
>> size_t size,
>> +                 enum dma_data_direction dir)
>> +{
>> +    const struct dma_map_ops *ops = get_dma_ops(dev);
>> +    void *ptr = NULL;
>> +
>> +    if (ops && ops->get_virt_addr)
>> +        ptr = ops->get_virt_addr(dev, addr);
>> +
>> +    dma_sync_single_for_cpu(dev, addr, size, dir);
>> +
>> +    return ptr;
>> +}
>> +
>>   static inline void dma_sync_single_for_device(struct device *dev,
>>                             dma_addr_t addr, size_t size,
>>                             enum dma_data_direction dir)
>> @@ -500,6 +531,12 @@ static inline void dma_sync_single_for_cpu(struct 
>> device *dev, dma_addr_t addr,
>>           size_t size, enum dma_data_direction dir)
>>   {
>>   }
>> +
>> +static inline void *
>> +dma_sync_single_for_cpu_desc(struct device *dev, dma_addr_t addr, 
>> size_t size,
>> +                 enum dma_data_direction dir)
>> +{
>> +}
>>   static inline void dma_sync_single_for_device(struct device *dev,
>>           dma_addr_t addr, size_t size, enum dma_data_direction dir)
>>   {
>> @@ -594,6 +631,21 @@ static inline void dma_unmap_single_attrs(struct 
>> device *dev, dma_addr_t addr,
>>       return dma_unmap_page_attrs(dev, addr, size, dir, attrs);
>>   }
>> +static inline void *
>> +dma_unmap_single_attrs_desc(struct device *dev, dma_addr_t addr, 
>> size_t size,
>> +                enum dma_data_direction dir, unsigned long attrs)
>> +{
>> +    const struct dma_map_ops *ops = get_dma_ops(dev);
>> +    void *ptr = NULL;
>> +
>> +    if (ops && ops->get_virt_addr)
>> +        ptr = ops->get_virt_addr(dev, addr);
>> +
>> +    dma_unmap_single_attrs(dev, addr, size, dir, attrs);
>> +
>> +    return ptr;
>> +}
>> +
>>   static inline void dma_sync_single_range_for_cpu(struct device *dev,
>>           dma_addr_t addr, unsigned long offset, size_t size,
>>           enum dma_data_direction dir)
>> @@ -610,10 +662,13 @@ static inline void 
>> dma_sync_single_range_for_device(struct device *dev,
>>   #define dma_map_single(d, a, s, r) dma_map_single_attrs(d, a, s, r, 0)
>>   #define dma_unmap_single(d, a, s, r) dma_unmap_single_attrs(d, a, s, 
>> r, 0)
>> +#define dma_unmap_single_desc(d, a, s, r) \
>> +        dma_unmap_single_attrs_desc(d, a, s, r, 0)
>>   #define dma_map_sg(d, s, n, r) dma_map_sg_attrs(d, s, n, r, 0)
>>   #define dma_unmap_sg(d, s, n, r) dma_unmap_sg_attrs(d, s, n, r, 0)
>>   #define dma_map_page(d, p, o, s, r) dma_map_page_attrs(d, p, o, s, 
>> r, 0)
>>   #define dma_unmap_page(d, a, s, r) dma_unmap_page_attrs(d, a, s, r, 0)
>> +#define dma_unmap_page_desc(d, a, s, r) dma_unmap_page_attrs_desc(d, 
>> a, s, r, 0)
>>   #define dma_get_sgtable(d, t, v, h, s) dma_get_sgtable_attrs(d, t, 
>> v, h, s, 0)
>>   #define dma_mmap_coherent(d, v, c, h, s) dma_mmap_attrs(d, v, c, h, 
>> s, 0)
>>
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v2 1/3] dma-mapping: introduce new dma unmap and sync api variants

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 6190 bytes --]

Hi Laurentiu,

I love your patch! Yet something to improve:

[auto build test ERROR on net-next/master]
[also build test ERROR on v5.4-rc5 next-20191029]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Laurentiu-Tudor/dma-mapping-introduce-new-dma-unmap-and-sync-variants/20191027-173418
base:   https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git 503a64635d5ef7351657c78ad77f8b5ff658d5fc
config: um-x86_64_defconfig (attached as .config)
compiler: gcc-7 (Debian 7.4.0-14) 7.4.0
reproduce:
        # save the attached .config to linux build tree
        make ARCH=um SUBARCH=x86_64

If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   In file included from include/linux/skbuff.h:31:0,
                    from include/net/net_namespace.h:38,
                    from include/linux/inet.h:42,
                    from include/linux/sunrpc/msg_prot.h:204,
                    from include/linux/sunrpc/auth.h:16,
                    from include/linux/nfs_fs.h:31,
                    from init/do_mounts.c:23:
   include/linux/dma-mapping.h: In function 'dma_sync_single_for_cpu_desc':
   include/linux/dma-mapping.h:539:1: warning: no return statement in function returning non-void [-Wreturn-type]
    }
    ^
   include/linux/dma-mapping.h: In function 'dma_unmap_single_attrs_desc':
>> include/linux/dma-mapping.h:638:34: error: implicit declaration of function 'get_dma_ops'; did you mean 'get_mm_rss'? [-Werror=implicit-function-declaration]
     const struct dma_map_ops *ops = get_dma_ops(dev);
                                     ^~~~~~~~~~~
                                     get_mm_rss
   include/linux/dma-mapping.h:638:34: warning: initialization makes pointer from integer without a cast [-Wint-conversion]
   cc1: some warnings being treated as errors

vim +638 include/linux/dma-mapping.h

   534	
   535	static inline void *
   536	dma_sync_single_for_cpu_desc(struct device *dev, dma_addr_t addr, size_t size,
   537				     enum dma_data_direction dir)
   538	{
 > 539	}
   540	static inline void dma_sync_single_for_device(struct device *dev,
   541			dma_addr_t addr, size_t size, enum dma_data_direction dir)
   542	{
   543	}
   544	static inline void dma_sync_sg_for_cpu(struct device *dev,
   545			struct scatterlist *sg, int nelems, enum dma_data_direction dir)
   546	{
   547	}
   548	static inline void dma_sync_sg_for_device(struct device *dev,
   549			struct scatterlist *sg, int nelems, enum dma_data_direction dir)
   550	{
   551	}
   552	static inline int dma_mapping_error(struct device *dev, dma_addr_t dma_addr)
   553	{
   554		return -ENOMEM;
   555	}
   556	static inline void *dma_alloc_attrs(struct device *dev, size_t size,
   557			dma_addr_t *dma_handle, gfp_t flag, unsigned long attrs)
   558	{
   559		return NULL;
   560	}
   561	static void dma_free_attrs(struct device *dev, size_t size, void *cpu_addr,
   562			dma_addr_t dma_handle, unsigned long attrs)
   563	{
   564	}
   565	static inline void *dmam_alloc_attrs(struct device *dev, size_t size,
   566			dma_addr_t *dma_handle, gfp_t gfp, unsigned long attrs)
   567	{
   568		return NULL;
   569	}
   570	static inline void dmam_free_coherent(struct device *dev, size_t size,
   571			void *vaddr, dma_addr_t dma_handle)
   572	{
   573	}
   574	static inline void dma_cache_sync(struct device *dev, void *vaddr, size_t size,
   575			enum dma_data_direction dir)
   576	{
   577	}
   578	static inline int dma_get_sgtable_attrs(struct device *dev,
   579			struct sg_table *sgt, void *cpu_addr, dma_addr_t dma_addr,
   580			size_t size, unsigned long attrs)
   581	{
   582		return -ENXIO;
   583	}
   584	static inline int dma_mmap_attrs(struct device *dev, struct vm_area_struct *vma,
   585			void *cpu_addr, dma_addr_t dma_addr, size_t size,
   586			unsigned long attrs)
   587	{
   588		return -ENXIO;
   589	}
   590	static inline bool dma_can_mmap(struct device *dev)
   591	{
   592		return false;
   593	}
   594	static inline int dma_supported(struct device *dev, u64 mask)
   595	{
   596		return 0;
   597	}
   598	static inline int dma_set_mask(struct device *dev, u64 mask)
   599	{
   600		return -EIO;
   601	}
   602	static inline int dma_set_coherent_mask(struct device *dev, u64 mask)
   603	{
   604		return -EIO;
   605	}
   606	static inline u64 dma_get_required_mask(struct device *dev)
   607	{
   608		return 0;
   609	}
   610	static inline size_t dma_max_mapping_size(struct device *dev)
   611	{
   612		return 0;
   613	}
   614	static inline unsigned long dma_get_merge_boundary(struct device *dev)
   615	{
   616		return 0;
   617	}
   618	#endif /* CONFIG_HAS_DMA */
   619	
   620	static inline dma_addr_t dma_map_single_attrs(struct device *dev, void *ptr,
   621			size_t size, enum dma_data_direction dir, unsigned long attrs)
   622	{
   623		debug_dma_map_single(dev, ptr, size);
   624		return dma_map_page_attrs(dev, virt_to_page(ptr), offset_in_page(ptr),
   625				size, dir, attrs);
   626	}
   627	
   628	static inline void dma_unmap_single_attrs(struct device *dev, dma_addr_t addr,
   629			size_t size, enum dma_data_direction dir, unsigned long attrs)
   630	{
   631		return dma_unmap_page_attrs(dev, addr, size, dir, attrs);
   632	}
   633	
   634	static inline void *
   635	dma_unmap_single_attrs_desc(struct device *dev, dma_addr_t addr, size_t size,
   636				    enum dma_data_direction dir, unsigned long attrs)
   637	{
 > 638		const struct dma_map_ops *ops = get_dma_ops(dev);
   639		void *ptr = NULL;
   640	
   641		if (ops && ops->get_virt_addr)
   642			ptr = ops->get_virt_addr(dev, addr);
   643	
   644		dma_unmap_single_attrs(dev, addr, size, dir, attrs);
   645	
   646		return ptr;
   647	}
   648	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 8323 bytes --]

[-- Attachment #3: Type: text/plain, Size: 156 bytes --]

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

[PATCH v2 2/3] iommu/dma: wire-up new dma map op .get_virt_addr

[Laurentiu Tudor]

From: Laurentiu Tudor <laurentiu.tudor@nxp.com>

Add an implementation of the newly introduced dma map op in the
generic DMA IOMMU generic glue layer and wire it up.

Signed-off-by: Laurentiu Tudor <laurentiu.tudor@nxp.com>
---
 drivers/iommu/dma-iommu.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
index f321279baf9e..15e76232d697 100644
--- a/drivers/iommu/dma-iommu.c
+++ b/drivers/iommu/dma-iommu.c
@@ -1091,6 +1091,21 @@ static unsigned long iommu_dma_get_merge_boundary(struct device *dev)
 	return (1UL << __ffs(domain->pgsize_bitmap)) - 1;
 }
 
+static void *iommu_dma_get_virt_addr(struct device *dev, dma_addr_t dma_handle)
+{
+	struct iommu_domain *domain = iommu_get_domain_for_dev(dev);
+
+	if (domain) {
+		phys_addr_t phys;
+
+		phys = iommu_iova_to_phys(domain, dma_handle);
+		if (phys)
+			return phys_to_virt(phys);
+	}
+
+	return NULL;
+}
+
 static const struct dma_map_ops iommu_dma_ops = {
 	.alloc			= iommu_dma_alloc,
 	.free			= iommu_dma_free,
@@ -1107,6 +1122,7 @@ static const struct dma_map_ops iommu_dma_ops = {
 	.map_resource		= iommu_dma_map_resource,
 	.unmap_resource		= iommu_dma_unmap_resource,
 	.get_merge_boundary	= iommu_dma_get_merge_boundary,
+	.get_virt_addr		= iommu_dma_get_virt_addr,
 };
 
 /*
-- 
2.17.1

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v2 2/3] iommu/dma: wire-up new dma map op .get_virt_addr

[Robin Murphy]

On 24/10/2019 13:41, Laurentiu Tudor wrote:
> From: Laurentiu Tudor <laurentiu.tudor@nxp.com>
> 
> Add an implementation of the newly introduced dma map op in the
> generic DMA IOMMU generic glue layer and wire it up.
> 
> Signed-off-by: Laurentiu Tudor <laurentiu.tudor@nxp.com>
> ---
>   drivers/iommu/dma-iommu.c | 16 ++++++++++++++++
>   1 file changed, 16 insertions(+)
> 
> diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
> index f321279baf9e..15e76232d697 100644
> --- a/drivers/iommu/dma-iommu.c
> +++ b/drivers/iommu/dma-iommu.c
> @@ -1091,6 +1091,21 @@ static unsigned long iommu_dma_get_merge_boundary(struct device *dev)
>   	return (1UL << __ffs(domain->pgsize_bitmap)) - 1;
>   }
>   
> +static void *iommu_dma_get_virt_addr(struct device *dev, dma_addr_t dma_handle)
> +{
> +	struct iommu_domain *domain = iommu_get_domain_for_dev(dev);

Note that we'd generally use the iommu_get_dma_domain() fastpath...

> +
> +	if (domain) {

...wherein we can also assume that the device having iommu_dma_ops 
assigned at all implies that a DMA ops domain is in place.

Robin.

> +		phys_addr_t phys;
> +
> +		phys = iommu_iova_to_phys(domain, dma_handle);
> +		if (phys)
> +			return phys_to_virt(phys);
> +	}
> +
> +	return NULL;
> +}
> +
>   static const struct dma_map_ops iommu_dma_ops = {
>   	.alloc			= iommu_dma_alloc,
>   	.free			= iommu_dma_free,
> @@ -1107,6 +1122,7 @@ static const struct dma_map_ops iommu_dma_ops = {
>   	.map_resource		= iommu_dma_map_resource,
>   	.unmap_resource		= iommu_dma_unmap_resource,
>   	.get_merge_boundary	= iommu_dma_get_merge_boundary,
> +	.get_virt_addr		= iommu_dma_get_virt_addr,
>   };
>   
>   /*
> 
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

[PATCH v2 3/3] dpaa2_eth: use new unmap and sync dma api variants

[Laurentiu Tudor]

From: Laurentiu Tudor <laurentiu.tudor@nxp.com>

Convert this driver to usage of the newly introduced dma unmap and
sync DMA APIs. This will get rid of the unsupported direct usage of
iommu_iova_to_phys() API.

Signed-off-by: Laurentiu Tudor <laurentiu.tudor@nxp.com>
---
 .../net/ethernet/freescale/dpaa2/dpaa2-eth.c  | 40 +++++++------------
 .../net/ethernet/freescale/dpaa2/dpaa2-eth.h  |  1 -
 2 files changed, 15 insertions(+), 26 deletions(-)

diff --git a/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c b/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c
index 19379bae0144..8c3391e6e598 100644
--- a/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c
+++ b/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c
@@ -29,16 +29,6 @@ MODULE_LICENSE("Dual BSD/GPL");
 MODULE_AUTHOR("Freescale Semiconductor, Inc");
 MODULE_DESCRIPTION("Freescale DPAA2 Ethernet Driver");
 
-static void *dpaa2_iova_to_virt(struct iommu_domain *domain,
-				dma_addr_t iova_addr)
-{
-	phys_addr_t phys_addr;
-
-	phys_addr = domain ? iommu_iova_to_phys(domain, iova_addr) : iova_addr;
-
-	return phys_to_virt(phys_addr);
-}
-
 static void validate_rx_csum(struct dpaa2_eth_priv *priv,
 			     u32 fd_status,
 			     struct sk_buff *skb)
@@ -85,9 +75,10 @@ static void free_rx_fd(struct dpaa2_eth_priv *priv,
 	sgt = vaddr + dpaa2_fd_get_offset(fd);
 	for (i = 1; i < DPAA2_ETH_MAX_SG_ENTRIES; i++) {
 		addr = dpaa2_sg_get_addr(&sgt[i]);
-		sg_vaddr = dpaa2_iova_to_virt(priv->iommu_domain, addr);
-		dma_unmap_page(dev, addr, DPAA2_ETH_RX_BUF_SIZE,
-			       DMA_BIDIRECTIONAL);
+		sg_vaddr = page_to_virt
+				(dma_unmap_page_desc(dev, addr,
+						    DPAA2_ETH_RX_BUF_SIZE,
+						    DMA_BIDIRECTIONAL));
 
 		free_pages((unsigned long)sg_vaddr, 0);
 		if (dpaa2_sg_is_final(&sgt[i]))
@@ -143,9 +134,10 @@ static struct sk_buff *build_frag_skb(struct dpaa2_eth_priv *priv,
 
 		/* Get the address and length from the S/G entry */
 		sg_addr = dpaa2_sg_get_addr(sge);
-		sg_vaddr = dpaa2_iova_to_virt(priv->iommu_domain, sg_addr);
-		dma_unmap_page(dev, sg_addr, DPAA2_ETH_RX_BUF_SIZE,
-			       DMA_BIDIRECTIONAL);
+		sg_vaddr = page_to_virt
+				(dma_unmap_page_desc(dev, sg_addr,
+						    DPAA2_ETH_RX_BUF_SIZE,
+						    DMA_BIDIRECTIONAL));
 
 		sg_length = dpaa2_sg_get_len(sge);
 
@@ -210,9 +202,9 @@ static void free_bufs(struct dpaa2_eth_priv *priv, u64 *buf_array, int count)
 	int i;
 
 	for (i = 0; i < count; i++) {
-		vaddr = dpaa2_iova_to_virt(priv->iommu_domain, buf_array[i]);
-		dma_unmap_page(dev, buf_array[i], DPAA2_ETH_RX_BUF_SIZE,
-			       DMA_BIDIRECTIONAL);
+		vaddr = page_to_virt(dma_unmap_page_desc(dev, buf_array[i],
+							 DPAA2_ETH_RX_BUF_SIZE,
+							 DMA_BIDIRECTIONAL));
 		free_pages((unsigned long)vaddr, 0);
 	}
 }
@@ -369,9 +361,8 @@ static void dpaa2_eth_rx(struct dpaa2_eth_priv *priv,
 	/* Tracing point */
 	trace_dpaa2_rx_fd(priv->net_dev, fd);
 
-	vaddr = dpaa2_iova_to_virt(priv->iommu_domain, addr);
-	dma_sync_single_for_cpu(dev, addr, DPAA2_ETH_RX_BUF_SIZE,
-				DMA_BIDIRECTIONAL);
+	vaddr = dma_sync_single_for_cpu_desc(dev, addr, DPAA2_ETH_RX_BUF_SIZE,
+					     DMA_BIDIRECTIONAL);
 
 	fas = dpaa2_get_fas(vaddr, false);
 	prefetch(fas);
@@ -682,7 +673,8 @@ static void free_tx_fd(const struct dpaa2_eth_priv *priv,
 	u32 fd_len = dpaa2_fd_get_len(fd);
 
 	fd_addr = dpaa2_fd_get_addr(fd);
-	buffer_start = dpaa2_iova_to_virt(priv->iommu_domain, fd_addr);
+	buffer_start = dma_sync_single_for_cpu_desc(dev, fd_addr, sizeof(*swa),
+						    DMA_BIDIRECTIONAL);
 	swa = (struct dpaa2_eth_swa *)buffer_start;
 
 	if (fd_format == dpaa2_fd_single) {
@@ -3448,8 +3440,6 @@ static int dpaa2_eth_probe(struct fsl_mc_device *dpni_dev)
 	priv = netdev_priv(net_dev);
 	priv->net_dev = net_dev;
 
-	priv->iommu_domain = iommu_get_domain_for_dev(dev);
-
 	/* Obtain a MC portal */
 	err = fsl_mc_portal_allocate(dpni_dev, FSL_MC_IO_ATOMIC_CONTEXT_PORTAL,
 				     &priv->mc_io);
diff --git a/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.h b/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.h
index 8a0e65b3267f..4e5183617ebd 100644
--- a/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.h
+++ b/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.h
@@ -374,7 +374,6 @@ struct dpaa2_eth_priv {
 
 	struct fsl_mc_device *dpbp_dev;
 	u16 bpid;
-	struct iommu_domain *iommu_domain;
 
 	bool tx_tstamp; /* Tx timestamping enabled */
 	bool rx_tstamp; /* Rx timestamping enabled */
-- 
2.17.1

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v2 3/3] dpaa2_eth: use new unmap and sync dma api variants

[Jonathan Lemon]

On 24 Oct 2019, at 5:41, Laurentiu Tudor wrote:

> From: Laurentiu Tudor <laurentiu.tudor@nxp.com>
>
> Convert this driver to usage of the newly introduced dma unmap and
> sync DMA APIs. This will get rid of the unsupported direct usage of
> iommu_iova_to_phys() API.
>
> Signed-off-by: Laurentiu Tudor <laurentiu.tudor@nxp.com>
> ---
>  .../net/ethernet/freescale/dpaa2/dpaa2-eth.c  | 40 
> +++++++------------
>  .../net/ethernet/freescale/dpaa2/dpaa2-eth.h  |  1 -
>  2 files changed, 15 insertions(+), 26 deletions(-)
>
> diff --git a/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c 
> b/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c
> index 19379bae0144..8c3391e6e598 100644
> --- a/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c
> +++ b/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c
> @@ -29,16 +29,6 @@ MODULE_LICENSE("Dual BSD/GPL");
>  MODULE_AUTHOR("Freescale Semiconductor, Inc");
>  MODULE_DESCRIPTION("Freescale DPAA2 Ethernet Driver");
>
> -static void *dpaa2_iova_to_virt(struct iommu_domain *domain,
> -				dma_addr_t iova_addr)
> -{
> -	phys_addr_t phys_addr;
> -
> -	phys_addr = domain ? iommu_iova_to_phys(domain, iova_addr) : 
> iova_addr;
> -
> -	return phys_to_virt(phys_addr);
> -}
> -
>  static void validate_rx_csum(struct dpaa2_eth_priv *priv,
>  			     u32 fd_status,
>  			     struct sk_buff *skb)
> @@ -85,9 +75,10 @@ static void free_rx_fd(struct dpaa2_eth_priv *priv,
>  	sgt = vaddr + dpaa2_fd_get_offset(fd);
>  	for (i = 1; i < DPAA2_ETH_MAX_SG_ENTRIES; i++) {
>  		addr = dpaa2_sg_get_addr(&sgt[i]);
> -		sg_vaddr = dpaa2_iova_to_virt(priv->iommu_domain, addr);
> -		dma_unmap_page(dev, addr, DPAA2_ETH_RX_BUF_SIZE,
> -			       DMA_BIDIRECTIONAL);
> +		sg_vaddr = page_to_virt
> +				(dma_unmap_page_desc(dev, addr,
> +						    DPAA2_ETH_RX_BUF_SIZE,
> +						    DMA_BIDIRECTIONAL));

This is doing virt -> page -> virt.  Why not just have the new
function return the VA corresponding to the addr, which would
match the other functions?
-- 
Jonathan


>
>  		free_pages((unsigned long)sg_vaddr, 0);
>  		if (dpaa2_sg_is_final(&sgt[i]))
> @@ -143,9 +134,10 @@ static struct sk_buff *build_frag_skb(struct 
> dpaa2_eth_priv *priv,
>
>  		/* Get the address and length from the S/G entry */
>  		sg_addr = dpaa2_sg_get_addr(sge);
> -		sg_vaddr = dpaa2_iova_to_virt(priv->iommu_domain, sg_addr);
> -		dma_unmap_page(dev, sg_addr, DPAA2_ETH_RX_BUF_SIZE,
> -			       DMA_BIDIRECTIONAL);
> +		sg_vaddr = page_to_virt
> +				(dma_unmap_page_desc(dev, sg_addr,
> +						    DPAA2_ETH_RX_BUF_SIZE,
> +						    DMA_BIDIRECTIONAL));
>
>  		sg_length = dpaa2_sg_get_len(sge);
>
> @@ -210,9 +202,9 @@ static void free_bufs(struct dpaa2_eth_priv *priv, 
> u64 *buf_array, int count)
>  	int i;
>
>  	for (i = 0; i < count; i++) {
> -		vaddr = dpaa2_iova_to_virt(priv->iommu_domain, buf_array[i]);
> -		dma_unmap_page(dev, buf_array[i], DPAA2_ETH_RX_BUF_SIZE,
> -			       DMA_BIDIRECTIONAL);
> +		vaddr = page_to_virt(dma_unmap_page_desc(dev, buf_array[i],
> +							 DPAA2_ETH_RX_BUF_SIZE,
> +							 DMA_BIDIRECTIONAL));
>  		free_pages((unsigned long)vaddr, 0);
>  	}
>  }
> @@ -369,9 +361,8 @@ static void dpaa2_eth_rx(struct dpaa2_eth_priv 
> *priv,
>  	/* Tracing point */
>  	trace_dpaa2_rx_fd(priv->net_dev, fd);
>
> -	vaddr = dpaa2_iova_to_virt(priv->iommu_domain, addr);
> -	dma_sync_single_for_cpu(dev, addr, DPAA2_ETH_RX_BUF_SIZE,
> -				DMA_BIDIRECTIONAL);
> +	vaddr = dma_sync_single_for_cpu_desc(dev, addr, 
> DPAA2_ETH_RX_BUF_SIZE,
> +					     DMA_BIDIRECTIONAL);
>
>  	fas = dpaa2_get_fas(vaddr, false);
>  	prefetch(fas);
> @@ -682,7 +673,8 @@ static void free_tx_fd(const struct dpaa2_eth_priv 
> *priv,
>  	u32 fd_len = dpaa2_fd_get_len(fd);
>
>  	fd_addr = dpaa2_fd_get_addr(fd);
> -	buffer_start = dpaa2_iova_to_virt(priv->iommu_domain, fd_addr);
> +	buffer_start = dma_sync_single_for_cpu_desc(dev, fd_addr, 
> sizeof(*swa),
> +						    DMA_BIDIRECTIONAL);
>  	swa = (struct dpaa2_eth_swa *)buffer_start;
>
>  	if (fd_format == dpaa2_fd_single) {
> @@ -3448,8 +3440,6 @@ static int dpaa2_eth_probe(struct fsl_mc_device 
> *dpni_dev)
>  	priv = netdev_priv(net_dev);
>  	priv->net_dev = net_dev;
>
> -	priv->iommu_domain = iommu_get_domain_for_dev(dev);
> -
>  	/* Obtain a MC portal */
>  	err = fsl_mc_portal_allocate(dpni_dev, 
> FSL_MC_IO_ATOMIC_CONTEXT_PORTAL,
>  				     &priv->mc_io);
> diff --git a/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.h 
> b/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.h
> index 8a0e65b3267f..4e5183617ebd 100644
> --- a/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.h
> +++ b/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.h
> @@ -374,7 +374,6 @@ struct dpaa2_eth_priv {
>
>  	struct fsl_mc_device *dpbp_dev;
>  	u16 bpid;
> -	struct iommu_domain *iommu_domain;
>
>  	bool tx_tstamp; /* Tx timestamping enabled */
>  	bool rx_tstamp; /* Rx timestamping enabled */
> -- 
> 2.17.1
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v2 3/3] dpaa2_eth: use new unmap and sync dma api variants

[Laurentiu Tudor]

Hi Jonathan,

On 25.10.2019 19:12, Jonathan Lemon wrote:
> 
> 
> On 24 Oct 2019, at 5:41, Laurentiu Tudor wrote:
> 
>> From: Laurentiu Tudor <laurentiu.tudor@nxp.com>
>>
>> Convert this driver to usage of the newly introduced dma unmap and
>> sync DMA APIs. This will get rid of the unsupported direct usage of
>> iommu_iova_to_phys() API.
>>
>> Signed-off-by: Laurentiu Tudor <laurentiu.tudor@nxp.com>
>> ---
>>  .../net/ethernet/freescale/dpaa2/dpaa2-eth.c  | 40 +++++++------------
>>  .../net/ethernet/freescale/dpaa2/dpaa2-eth.h  |  1 -
>>  2 files changed, 15 insertions(+), 26 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c 
>> b/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c
>> index 19379bae0144..8c3391e6e598 100644
>> --- a/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c
>> +++ b/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c
>> @@ -29,16 +29,6 @@ MODULE_LICENSE("Dual BSD/GPL");
>>  MODULE_AUTHOR("Freescale Semiconductor, Inc");
>>  MODULE_DESCRIPTION("Freescale DPAA2 Ethernet Driver");
>>
>> -static void *dpaa2_iova_to_virt(struct iommu_domain *domain,
>> -                dma_addr_t iova_addr)
>> -{
>> -    phys_addr_t phys_addr;
>> -
>> -    phys_addr = domain ? iommu_iova_to_phys(domain, iova_addr) : 
>> iova_addr;
>> -
>> -    return phys_to_virt(phys_addr);
>> -}
>> -
>>  static void validate_rx_csum(struct dpaa2_eth_priv *priv,
>>                   u32 fd_status,
>>                   struct sk_buff *skb)
>> @@ -85,9 +75,10 @@ static void free_rx_fd(struct dpaa2_eth_priv *priv,
>>      sgt = vaddr + dpaa2_fd_get_offset(fd);
>>      for (i = 1; i < DPAA2_ETH_MAX_SG_ENTRIES; i++) {
>>          addr = dpaa2_sg_get_addr(&sgt[i]);
>> -        sg_vaddr = dpaa2_iova_to_virt(priv->iommu_domain, addr);
>> -        dma_unmap_page(dev, addr, DPAA2_ETH_RX_BUF_SIZE,
>> -                   DMA_BIDIRECTIONAL);
>> +        sg_vaddr = page_to_virt
>> +                (dma_unmap_page_desc(dev, addr,
>> +                            DPAA2_ETH_RX_BUF_SIZE,
>> +                            DMA_BIDIRECTIONAL));
> 
> This is doing virt -> page -> virt.  Why not just have the new
> function return the VA corresponding to the addr, which would
> match the other functions?

I'd really like that as it would get rid of the page_to_virt() calls but 
it will break the symmetry with the dma_map_page() API. I'll let the 
maintainers decide.

---
Best Regards, Laurentiu
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v2 3/3] dpaa2_eth: use new unmap and sync dma api variants

[hch]

On Mon, Oct 28, 2019 at 10:55:05AM +0000, Laurentiu Tudor wrote:
> >> @@ -85,9 +75,10 @@ static void free_rx_fd(struct dpaa2_eth_priv *priv,
> >>      sgt = vaddr + dpaa2_fd_get_offset(fd);
> >>      for (i = 1; i < DPAA2_ETH_MAX_SG_ENTRIES; i++) {
> >>          addr = dpaa2_sg_get_addr(&sgt[i]);
> >> -        sg_vaddr = dpaa2_iova_to_virt(priv->iommu_domain, addr);
> >> -        dma_unmap_page(dev, addr, DPAA2_ETH_RX_BUF_SIZE,
> >> -                   DMA_BIDIRECTIONAL);
> >> +        sg_vaddr = page_to_virt
> >> +                (dma_unmap_page_desc(dev, addr,
> >> +                            DPAA2_ETH_RX_BUF_SIZE,
> >> +                            DMA_BIDIRECTIONAL));
> > 
> > This is doing virt -> page -> virt.  Why not just have the new
> > function return the VA corresponding to the addr, which would
> > match the other functions?
> 
> I'd really like that as it would get rid of the page_to_virt() calls but 
> it will break the symmetry with the dma_map_page() API. I'll let the 
> maintainers decide.

It would be symmetric with dma_map_single, though.  Maybe we need
both variants?
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v2 3/3] dpaa2_eth: use new unmap and sync dma api variants

[Laurentiu Tudor]

On 28.10.2019 13:38, hch@lst.de wrote:
> On Mon, Oct 28, 2019 at 10:55:05AM +0000, Laurentiu Tudor wrote:
>>>> @@ -85,9 +75,10 @@ static void free_rx_fd(struct dpaa2_eth_priv *priv,
>>>>       sgt = vaddr + dpaa2_fd_get_offset(fd);
>>>>       for (i = 1; i < DPAA2_ETH_MAX_SG_ENTRIES; i++) {
>>>>           addr = dpaa2_sg_get_addr(&sgt[i]);
>>>> -        sg_vaddr = dpaa2_iova_to_virt(priv->iommu_domain, addr);
>>>> -        dma_unmap_page(dev, addr, DPAA2_ETH_RX_BUF_SIZE,
>>>> -                   DMA_BIDIRECTIONAL);
>>>> +        sg_vaddr = page_to_virt
>>>> +                (dma_unmap_page_desc(dev, addr,
>>>> +                            DPAA2_ETH_RX_BUF_SIZE,
>>>> +                            DMA_BIDIRECTIONAL));
>>>
>>> This is doing virt -> page -> virt.  Why not just have the new
>>> function return the VA corresponding to the addr, which would
>>> match the other functions?
>>
>> I'd really like that as it would get rid of the page_to_virt() calls but
>> it will break the symmetry with the dma_map_page() API. I'll let the
>> maintainers decide.
> 
> It would be symmetric with dma_map_single, though.  Maybe we need
> both variants?

Patch 1/3 also adds an dma_unmap_single_desc(). Would it be legal to 
just use dma_unmap_single_desc() in the driver even if the driver does 
it's mappings with dma_map_page()?

---
Best Regards, Laurentiu
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu