[RFC PATCH 0/3] dma-mapping: introduce a new dma api

[RFC PATCH 0/3] dma-mapping: introduce a new dma api

[Laurentiu Tudor]

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

This series introduces a new dma api called dma_addr_to_phys_addr()
that converts a dma address to the corresponding physical address.
It consists in a new dma map op and the wrapper api, both added
in the first patch. The second patch adds an implementation in the
iommu dma glue code and wires it up. The third patch updates a driver
to use the new api.

Note: Originally i wanted to use the simpler api name dma_to_phys()
but it's already used in the dma direct apis. It would be great if
there would be a solution (unifying them?) to use this nicer naming.

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

Laurentiu Tudor (3):
  dma-mapping: introduce a new dma api dma_addr_to_phys_addr()
  iommu/dma: wire-up new dma op dma_addr_to_phys_addr()
  dpaa2_eth: use dma_addr_to_phys_addr() new dma api

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

-- 
2.17.1

[RFC PATCH 1/3] dma-mapping: introduce a new dma api dma_addr_to_phys_addr()

[Laurentiu Tudor]

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

Introduce a new dma map op called dma_addr_to_phys_addr() that converts
a dma address to the physical address backing it up and add wrapper for
it.

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

diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h
index 4a1c4fca475a..5965d159c9a9 100644
--- a/include/linux/dma-mapping.h
+++ b/include/linux/dma-mapping.h
@@ -132,6 +132,8 @@ 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);
+	phys_addr_t (*dma_addr_to_phys_addr)(struct device *dev,
+					     dma_addr_t dma_handle);
 };
 
 #define DMA_MAPPING_ERROR		(~(dma_addr_t)0)
@@ -442,6 +444,19 @@ static inline int dma_mapping_error(struct device *dev, dma_addr_t dma_addr)
 	return 0;
 }
 
+static inline phys_addr_t dma_addr_to_phys_addr(struct device *dev,
+						dma_addr_t dma_handle)
+{
+	const struct dma_map_ops *ops = get_dma_ops(dev);
+
+	if (dma_is_direct(ops))
+		return (phys_addr_t)dma_handle;
+	else if (ops->dma_addr_to_phys_addr)
+		return ops->dma_addr_to_phys_addr(dev, dma_handle);
+
+	return 0;
+}
+
 void *dma_alloc_attrs(struct device *dev, size_t size, dma_addr_t *dma_handle,
 		gfp_t flag, unsigned long attrs);
 void dma_free_attrs(struct device *dev, size_t size, void *cpu_addr,
@@ -578,6 +593,12 @@ static inline unsigned long dma_get_merge_boundary(struct device *dev)
 {
 	return 0;
 }
+
+static inline phys_addr_t dma_addr_to_phys_addr(struct device *dev,
+						dma_addr_t dma_handle)
+{
+	return 0;
+}
 #endif /* CONFIG_HAS_DMA */
 
 static inline dma_addr_t dma_map_single_attrs(struct device *dev, void *ptr,
-- 
2.17.1

[RFC PATCH 2/3] iommu/dma: wire-up new dma op dma_addr_to_phys_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 | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
index f321279baf9e..0d2856793ecd 100644
--- a/drivers/iommu/dma-iommu.c
+++ b/drivers/iommu/dma-iommu.c
@@ -1091,6 +1091,17 @@ static unsigned long iommu_dma_get_merge_boundary(struct device *dev)
 	return (1UL << __ffs(domain->pgsize_bitmap)) - 1;
 }
 
+static phys_addr_t iommu_dma_dma_addr_to_phys_addr(struct device *dev,
+						   dma_addr_t dma_handle)
+{
+	struct iommu_domain *domain = iommu_get_domain_for_dev(dev);
+
+	if (domain)
+		return iommu_iova_to_phys(domain, dma_handle);
+
+	return 0;
+}
+
 static const struct dma_map_ops iommu_dma_ops = {
 	.alloc			= iommu_dma_alloc,
 	.free			= iommu_dma_free,
@@ -1107,6 +1118,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,
+	.dma_addr_to_phys_addr	= iommu_dma_dma_addr_to_phys_addr,
 };
 
 /*
-- 
2.17.1

[RFC PATCH 3/3] dpaa2_eth: use dma_addr_to_phys_addr() new dma api

[Laurentiu Tudor]

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

Convert this driver to usage of the newly introduced
dma_addr_to_phys_addr() DMA API. 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  | 21 +++++++------------
 .../net/ethernet/freescale/dpaa2/dpaa2-eth.h  |  1 -
 2 files changed, 7 insertions(+), 15 deletions(-)

diff --git a/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c b/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c
index 19379bae0144..7332b91ca3a2 100644
--- a/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c
+++ b/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c
@@ -29,14 +29,9 @@ 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)
+static void *dpaa2_iova_to_virt(struct device *dev, dma_addr_t dma_handle)
 {
-	phys_addr_t phys_addr;
-
-	phys_addr = domain ? iommu_iova_to_phys(domain, iova_addr) : iova_addr;
-
-	return phys_to_virt(phys_addr);
+	return phys_to_virt(dma_addr_to_phys_addr(dev, dma_handle));
 }
 
 static void validate_rx_csum(struct dpaa2_eth_priv *priv,
@@ -85,7 +80,7 @@ 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);
+		sg_vaddr = dpaa2_iova_to_virt(dev, addr);
 		dma_unmap_page(dev, addr, DPAA2_ETH_RX_BUF_SIZE,
 			       DMA_BIDIRECTIONAL);
 
@@ -143,7 +138,7 @@ 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);
+		sg_vaddr = dpaa2_iova_to_virt(dev, sg_addr);
 		dma_unmap_page(dev, sg_addr, DPAA2_ETH_RX_BUF_SIZE,
 			       DMA_BIDIRECTIONAL);
 
@@ -210,7 +205,7 @@ 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]);
+		vaddr = dpaa2_iova_to_virt(dev, buf_array[i]);
 		dma_unmap_page(dev, buf_array[i], DPAA2_ETH_RX_BUF_SIZE,
 			       DMA_BIDIRECTIONAL);
 		free_pages((unsigned long)vaddr, 0);
@@ -369,7 +364,7 @@ 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);
+	vaddr = dpaa2_iova_to_virt(dev, addr);
 	dma_sync_single_for_cpu(dev, addr, DPAA2_ETH_RX_BUF_SIZE,
 				DMA_BIDIRECTIONAL);
 
@@ -682,7 +677,7 @@ 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 = dpaa2_iova_to_virt(dev, fd_addr);
 	swa = (struct dpaa2_eth_swa *)buffer_start;
 
 	if (fd_format == dpaa2_fd_single) {
@@ -3448,8 +3443,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

Re: [RFC PATCH 1/3] dma-mapping: introduce a new dma api dma_addr_to_phys_addr()

[Robin Murphy]

On 22/10/2019 13:55, Laurentiu Tudor wrote:
> From: Laurentiu Tudor <laurentiu.tudor@nxp.com>
> 
> Introduce a new dma map op called dma_addr_to_phys_addr() that converts
> a dma address to the physical address backing it up and add wrapper for
> it.

I'd really love it if there was a name which could encapsulate that this 
is *only* for extreme special cases of constrained descriptors/pagetable 
entries/etc. where there's simply no practical way to keep track of a 
CPU address alongside the DMA address, and the only option is this 
potentially-arbitrarily-complex operation (I mean, on some systems it 
may end up taking locks and poking hardware).

Either way it's tricky - much as I don't like adding an interface which 
is ripe for drivers to misuse, I also really don't want hacks like 
bdf95923086f shoved into other APIs to compensate, so on balance I'd 
probably consider this proposal ever so slightly the lesser evil.

> Signed-off-by: Laurentiu Tudor <laurentiu.tudor@nxp.com>
> ---
>   include/linux/dma-mapping.h | 21 +++++++++++++++++++++
>   1 file changed, 21 insertions(+)
> 
> diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h
> index 4a1c4fca475a..5965d159c9a9 100644
> --- a/include/linux/dma-mapping.h
> +++ b/include/linux/dma-mapping.h
> @@ -132,6 +132,8 @@ 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);
> +	phys_addr_t (*dma_addr_to_phys_addr)(struct device *dev,
> +					     dma_addr_t dma_handle);

I'd be inclined to name the internal callback something a bit snappier 
like .get_phys_addr.

>   };
>   
>   #define DMA_MAPPING_ERROR		(~(dma_addr_t)0)
> @@ -442,6 +444,19 @@ static inline int dma_mapping_error(struct device *dev, dma_addr_t dma_addr)
>   	return 0;
>   }
>   
> +static inline phys_addr_t dma_addr_to_phys_addr(struct device *dev,
> +						dma_addr_t dma_handle)
> +{
> +	const struct dma_map_ops *ops = get_dma_ops(dev);
> +
> +	if (dma_is_direct(ops))
> +		return (phys_addr_t)dma_handle;

Well that's not right, is it - remember why you had that namespace 
collision? ;)

Robin.

> +	else if (ops->dma_addr_to_phys_addr)
> +		return ops->dma_addr_to_phys_addr(dev, dma_handle);
> +
> +	return 0;
> +}
> +
>   void *dma_alloc_attrs(struct device *dev, size_t size, dma_addr_t *dma_handle,
>   		gfp_t flag, unsigned long attrs);
>   void dma_free_attrs(struct device *dev, size_t size, void *cpu_addr,
> @@ -578,6 +593,12 @@ static inline unsigned long dma_get_merge_boundary(struct device *dev)
>   {
>   	return 0;
>   }
> +
> +static inline phys_addr_t dma_addr_to_phys_addr(struct device *dev,
> +						dma_addr_t dma_handle)
> +{
> +	return 0;
> +}
>   #endif /* CONFIG_HAS_DMA */
>   
>   static inline dma_addr_t dma_map_single_attrs(struct device *dev, void *ptr,
> 

Re: [RFC PATCH 1/3] dma-mapping: introduce a new dma api dma_addr_to_phys_addr()

[Laurentiu Tudor]

On 22.10.2019 16:25, Robin Murphy wrote:
> On 22/10/2019 13:55, Laurentiu Tudor wrote:
>> From: Laurentiu Tudor <laurentiu.tudor@nxp.com>
>>
>> Introduce a new dma map op called dma_addr_to_phys_addr() that converts
>> a dma address to the physical address backing it up and add wrapper for
>> it.
> 
> I'd really love it if there was a name which could encapsulate that this 
> is *only* for extreme special cases of constrained descriptors/pagetable 
> entries/etc. where there's simply no practical way to keep track of a 
> CPU address alongside the DMA address, and the only option is this 
> potentially-arbitrarily-complex operation (I mean, on some systems it 
> may end up taking locks and poking hardware).
> 
> Either way it's tricky - much as I don't like adding an interface which 
> is ripe for drivers to misuse, I also really don't want hacks like 
> bdf95923086f shoved into other APIs to compensate, so on balance I'd 
> probably consider this proposal ever so slightly the lesser evil.
> 
>> Signed-off-by: Laurentiu Tudor <laurentiu.tudor@nxp.com>
>> ---
>>   include/linux/dma-mapping.h | 21 +++++++++++++++++++++
>>   1 file changed, 21 insertions(+)
>>
>> diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h
>> index 4a1c4fca475a..5965d159c9a9 100644
>> --- a/include/linux/dma-mapping.h
>> +++ b/include/linux/dma-mapping.h
>> @@ -132,6 +132,8 @@ 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);
>> +    phys_addr_t (*dma_addr_to_phys_addr)(struct device *dev,
>> +                         dma_addr_t dma_handle);
> 
> I'd be inclined to name the internal callback something a bit snappier 
> like .get_phys_addr.

Alright. Want me to also rename the wrapper to something like 
dma_get_phys_addr()? Sounds a bit nicer to me.

>>   };
>>   #define DMA_MAPPING_ERROR        (~(dma_addr_t)0)
>> @@ -442,6 +444,19 @@ static inline int dma_mapping_error(struct device 
>> *dev, dma_addr_t dma_addr)
>>       return 0;
>>   }
>> +static inline phys_addr_t dma_addr_to_phys_addr(struct device *dev,
>> +                        dma_addr_t dma_handle)
>> +{
>> +    const struct dma_map_ops *ops = get_dma_ops(dev);
>> +
>> +    if (dma_is_direct(ops))
>> +        return (phys_addr_t)dma_handle;
> 
> Well that's not right, is it - remember why you had that namespace 
> collision? ;)
> 

Ugh, correct. Don't know what I was thinking. Will rework the check.

---
Thanks & Best Regards, Laurentiu

Re: [RFC PATCH 1/3] dma-mapping: introduce a new dma api dma_addr_to_phys_addr()

[Laurentiu Tudor]

Hi Robin,

On 22.10.2019 16:25, Robin Murphy wrote:
> On 22/10/2019 13:55, Laurentiu Tudor wrote:
>> From: Laurentiu Tudor <laurentiu.tudor@nxp.com>
>>
>> Introduce a new dma map op called dma_addr_to_phys_addr() that converts
>> a dma address to the physical address backing it up and add wrapper for
>> it.
> 
> I'd really love it if there was a name which could encapsulate that this 
> is *only* for extreme special cases of constrained descriptors/pagetable 
> entries/etc. where there's simply no practical way to keep track of a 
> CPU address alongside the DMA address, and the only option is this 
> potentially-arbitrarily-complex operation (I mean, on some systems it 
> may end up taking locks and poking hardware).
> 
> Either way it's tricky - much as I don't like adding an interface which 
> is ripe for drivers to misuse, I also really don't want hacks like 
> bdf95923086f shoved into other APIs to compensate, so on balance I'd 
> probably consider this proposal ever so slightly the lesser evil.

We had an internal discussion over these points you are raising and 
Madalin (cc-ed) came up with another idea: instead of adding this prone 
to misuse api how about experimenting with a new dma unmap and dma sync 
variants that would return the physical address by calling the newly 
introduced dma map op. Something along these lines:
  * phys_addr_t dma_unmap_page_ret_phys(...)
  * phys_addr_t dma_unmap_single_ret_phys(...)
  * phys_addr_t dma_sync_single_for_cpu_ret_phys(...)
I'm thinking that this proposal should reduce the risks opened by the 
initial variant.
Please let me know what you think.

---
Thanks & Best Regards, Laurentiu

Re: [RFC PATCH 1/3] dma-mapping: introduce a new dma api dma_addr_to_phys_addr()

[hch]

On Wed, Oct 23, 2019 at 11:53:41AM +0000, Laurentiu Tudor wrote:
> We had an internal discussion over these points you are raising and 
> Madalin (cc-ed) came up with another idea: instead of adding this prone 
> to misuse api how about experimenting with a new dma unmap and dma sync 
> variants that would return the physical address by calling the newly 
> introduced dma map op. Something along these lines:
>   * phys_addr_t dma_unmap_page_ret_phys(...)
>   * phys_addr_t dma_unmap_single_ret_phys(...)
>   * phys_addr_t dma_sync_single_for_cpu_ret_phys(...)
> I'm thinking that this proposal should reduce the risks opened by the 
> initial variant.
> Please let me know what you think.

I'm not sure what the ret is supposed to mean, but I generally like
that idea better.  We also need to make sure there is an easy way
to figure out if these APIs are available, as they generally aren't
for any non-IOMMU API IOMMU drivers.

Re: [RFC PATCH 1/3] dma-mapping: introduce a new dma api dma_addr_to_phys_addr()

[Laurentiu Tudor]

On 24.10.2019 05:01, hch@lst.de wrote:
> On Wed, Oct 23, 2019 at 11:53:41AM +0000, Laurentiu Tudor wrote:
>> We had an internal discussion over these points you are raising and
>> Madalin (cc-ed) came up with another idea: instead of adding this prone
>> to misuse api how about experimenting with a new dma unmap and dma sync
>> variants that would return the physical address by calling the newly
>> introduced dma map op. Something along these lines:
>>    * phys_addr_t dma_unmap_page_ret_phys(...)
>>    * phys_addr_t dma_unmap_single_ret_phys(...)
>>    * phys_addr_t dma_sync_single_for_cpu_ret_phys(...)
>> I'm thinking that this proposal should reduce the risks opened by the
>> initial variant.
>> Please let me know what you think.
> 
> I'm not sure what the ret is supposed to mean, but I generally like
> that idea better.  

It was supposed to be short for "return" but given that I'm not good at 
naming stuff I'll just drop it.

> We also need to make sure there is an easy way
> to figure out if these APIs are available, as they generally aren't
> for any non-IOMMU API IOMMU drivers.

I was really hoping to manage making them as generic as possible but 
anyway, I'll start working on a PoC and see how it turns out. This will 
probably happen sometime next next week as the following week I'll be 
traveling to a conference.

---
Best Regards, Laurentiu

Re: [RFC PATCH 1/3] dma-mapping: introduce a new dma api dma_addr_to_phys_addr()

[Robin Murphy]

On 2019-10-24 8:49 am, Laurentiu Tudor wrote:
> 
> 
> On 24.10.2019 05:01, hch@lst.de wrote:
>> On Wed, Oct 23, 2019 at 11:53:41AM +0000, Laurentiu Tudor wrote:
>>> We had an internal discussion over these points you are raising and
>>> Madalin (cc-ed) came up with another idea: instead of adding this prone
>>> to misuse api how about experimenting with a new dma unmap and dma sync
>>> variants that would return the physical address by calling the newly
>>> introduced dma map op. Something along these lines:
>>>     * phys_addr_t dma_unmap_page_ret_phys(...)
>>>     * phys_addr_t dma_unmap_single_ret_phys(...)
>>>     * phys_addr_t dma_sync_single_for_cpu_ret_phys(...)
>>> I'm thinking that this proposal should reduce the risks opened by the
>>> initial variant.
>>> Please let me know what you think.
>>
>> I'm not sure what the ret is supposed to mean, but I generally like
>> that idea better.
> 
> It was supposed to be short for "return" but given that I'm not good at
> naming stuff I'll just drop it.

Hmm, how about something like "dma_unmap_*_desc" for the context of the 
mapped DMA address also being used as a descriptor token?

>> We also need to make sure there is an easy way
>> to figure out if these APIs are available, as they generally aren't
>> for any non-IOMMU API IOMMU drivers.
> 
> I was really hoping to manage making them as generic as possible but
> anyway, I'll start working on a PoC and see how it turns out. This will
> probably happen sometime next next week as the following week I'll be
> traveling to a conference.

AFAICS, even a full implementation of these APIs would have to be 
capable of returning an indication that there is no valid physical 
address - e.g. if unmap is called with a bogus DMA address that was 
never mapped. At that point there'sseemingly no problem just 
implementing the trivial case on top of any existing unmap/sync 
callbacks for everyone. I'd imagine that drivers which want this aren't 
likely to run on the older architectures where the weird IOMMUs live, so 
they could probably just always treat failure as unexpected and fatal 
either way.

In fact, I'm now wondering whether it's likely to be common that users 
want the physical address specifically, or whether it would make sense 
to return the original VA/page, both for symmetry with the corresponding 
map calls and for the ease of being able to return NULL when necessary.

Robin.

Re: [RFC PATCH 1/3] dma-mapping: introduce a new dma api dma_addr_to_phys_addr()

[Laurentiu Tudor]

On 24.10.2019 14:04, Robin Murphy wrote:
> On 2019-10-24 8:49 am, Laurentiu Tudor wrote:
>>
>>
>> On 24.10.2019 05:01, hch@lst.de wrote:
>>> On Wed, Oct 23, 2019 at 11:53:41AM +0000, Laurentiu Tudor wrote:
>>>> We had an internal discussion over these points you are raising and
>>>> Madalin (cc-ed) came up with another idea: instead of adding this prone
>>>> to misuse api how about experimenting with a new dma unmap and dma sync
>>>> variants that would return the physical address by calling the newly
>>>> introduced dma map op. Something along these lines:
>>>>     * phys_addr_t dma_unmap_page_ret_phys(...)
>>>>     * phys_addr_t dma_unmap_single_ret_phys(...)
>>>>     * phys_addr_t dma_sync_single_for_cpu_ret_phys(...)
>>>> I'm thinking that this proposal should reduce the risks opened by the
>>>> initial variant.
>>>> Please let me know what you think.
>>>
>>> I'm not sure what the ret is supposed to mean, but I generally like
>>> that idea better.
>>
>> It was supposed to be short for "return" but given that I'm not good at
>> naming stuff I'll just drop it.
> 
> Hmm, how about something like "dma_unmap_*_desc" for the context of the 
> mapped DMA address also being used as a descriptor token?

Alright.

>>> We also need to make sure there is an easy way
>>> to figure out if these APIs are available, as they generally aren't
>>> for any non-IOMMU API IOMMU drivers.
>>
>> I was really hoping to manage making them as generic as possible but
>> anyway, I'll start working on a PoC and see how it turns out. This will
>> probably happen sometime next next week as the following week I'll be
>> traveling to a conference.
> 
> AFAICS, even a full implementation of these APIs would have to be 
> capable of returning an indication that there is no valid physical 
> address - e.g. if unmap is called with a bogus DMA address that was 
> never mapped. At that point there'sseemingly no problem just 
> implementing the trivial case on top of any existing unmap/sync 
> callbacks for everyone. I'd imagine that drivers which want this aren't 
> likely to run on the older architectures where the weird IOMMUs live, so 
> they could probably just always treat failure as unexpected and fatal 
> either way.
> 
> In fact, I'm now wondering whether it's likely to be common that users 
> want the physical address specifically, or whether it would make sense 
> to return the original VA/page, both for symmetry with the corresponding 
> map calls and for the ease of being able to return NULL when necessary.

That's sounds wonderful as it should make the code leaner in the drivers.

---
Best Regards, Laurentiu