[PATCH v2] arm64: do not set dma masks that device connection can't handle

[PATCH v2] arm64: do not set dma masks that device connection can't handle

[Nikita Yushchenko]

It is possible that device is capable of 64-bit DMA addresses, and
device driver tries to set wide DMA mask, but bridge or bus used to
connect device to the system can't handle wide addresses.

With swiotlb, memory above 4G still can be used by drivers for streaming
DMA, but *dev->mask and dev->dma_coherent_mask must still keep values
that hardware handles physically.

This patch enforces that. Based on original version by
Arnd Bergmann <arnd@arndb.de>, extended with coherent mask hadnling.

Signed-off-by: Nikita Yushchenko <nikita.yoush@cogentembedded.com>
CC: Arnd Bergmann <arnd@arndb.de>
---
Changes since v1:
- fixed issues noted by Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
  - save mask, not size
  - remove doube empty line

 arch/arm64/Kconfig              |  3 +++
 arch/arm64/include/asm/device.h |  1 +
 arch/arm64/mm/dma-mapping.c     | 51 +++++++++++++++++++++++++++++++++++++++++
 3 files changed, 55 insertions(+)

diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index 1117421..afb2c08 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -216,6 +216,9 @@ config NEED_DMA_MAP_STATE
 config NEED_SG_DMA_LENGTH
 	def_bool y
 
+config ARCH_HAS_DMA_SET_COHERENT_MASK
+	def_bool y
+
 config SMP
 	def_bool y
 
diff --git a/arch/arm64/include/asm/device.h b/arch/arm64/include/asm/device.h
index 243ef25..a57e7bb 100644
--- a/arch/arm64/include/asm/device.h
+++ b/arch/arm64/include/asm/device.h
@@ -22,6 +22,7 @@ struct dev_archdata {
 	void *iommu;			/* private IOMMU data */
 #endif
 	bool dma_coherent;
+	u64 parent_dma_mask;
 };
 
 struct pdev_archdata {
diff --git a/arch/arm64/mm/dma-mapping.c b/arch/arm64/mm/dma-mapping.c
index e040827..5ab15ce 100644
--- a/arch/arm64/mm/dma-mapping.c
+++ b/arch/arm64/mm/dma-mapping.c
@@ -352,6 +352,30 @@ static int __swiotlb_dma_supported(struct device *hwdev, u64 mask)
 	return 1;
 }
 
+static int __swiotlb_set_dma_mask(struct device *dev, u64 mask)
+{
+	/* device is not DMA capable */
+	if (!dev->dma_mask)
+		return -EIO;
+
+	/* mask is below swiotlb bounce buffer, so fail */
+	if (!swiotlb_dma_supported(dev, mask))
+		return -EIO;
+
+	/*
+	 * because of the swiotlb, we can return success for
+	 * larger masks, but need to ensure that bounce buffers
+	 * are used above parent_dma_mask, so set that as
+	 * the effective mask.
+	 */
+	if (mask > dev->archdata.parent_dma_mask)
+		mask = dev->archdata.parent_dma_mask;
+
+	*dev->dma_mask = mask;
+
+	return 0;
+}
+
 static struct dma_map_ops swiotlb_dma_ops = {
 	.alloc = __dma_alloc,
 	.free = __dma_free,
@@ -367,8 +391,23 @@ static struct dma_map_ops swiotlb_dma_ops = {
 	.sync_sg_for_device = __swiotlb_sync_sg_for_device,
 	.dma_supported = __swiotlb_dma_supported,
 	.mapping_error = swiotlb_dma_mapping_error,
+	.set_dma_mask = __swiotlb_set_dma_mask,
 };
 
+int dma_set_coherent_mask(struct device *dev, u64 mask)
+{
+	if (!dma_supported(dev, mask))
+		return -EIO;
+
+	if (get_dma_ops(dev) == &swiotlb_dma_ops &&
+	    mask > dev->archdata.parent_dma_mask)
+		mask = dev->archdata.parent_dma_mask;
+
+	dev->coherent_dma_mask = mask;
+	return 0;
+}
+EXPORT_SYMBOL(dma_set_coherent_mask);
+
 static int __init atomic_pool_init(void)
 {
 	pgprot_t prot = __pgprot(PROT_NORMAL_NC);
@@ -958,6 +997,18 @@ void arch_setup_dma_ops(struct device *dev, u64 dma_base, u64 size,
 	if (!dev->archdata.dma_ops)
 		dev->archdata.dma_ops = &swiotlb_dma_ops;
 
+	/*
+	 * we don't yet support buses that have a non-zero mapping.
+	 *  Let's hope we won't need it
+	 */
+	WARN_ON(dma_base != 0);
+
+	/*
+	 * Whatever the parent bus can set. A device must not set
+	 * a DMA mask larger than this.
+	 */
+	dev->archdata.parent_dma_mask = size - 1;
+
 	dev->archdata.dma_coherent = coherent;
 	__iommu_setup_dma_ops(dev, dma_base, size, iommu);
 }
-- 
2.1.4

[PATCH v2] arm64: do not set dma masks that device connection can't handle

[Nikita Yushchenko]

It is possible that device is capable of 64-bit DMA addresses, and
device driver tries to set wide DMA mask, but bridge or bus used to
connect device to the system can't handle wide addresses.

With swiotlb, memory above 4G still can be used by drivers for streaming
DMA, but *dev->mask and dev->dma_coherent_mask must still keep values
that hardware handles physically.

This patch enforces that. Based on original version by
Arnd Bergmann <arnd@arndb.de>, extended with coherent mask hadnling.

Signed-off-by: Nikita Yushchenko <nikita.yoush@cogentembedded.com>
CC: Arnd Bergmann <arnd@arndb.de>
---
Changes since v1:
- fixed issues noted by Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
  - save mask, not size
  - remove doube empty line

 arch/arm64/Kconfig              |  3 +++
 arch/arm64/include/asm/device.h |  1 +
 arch/arm64/mm/dma-mapping.c     | 51 +++++++++++++++++++++++++++++++++++++++++
 3 files changed, 55 insertions(+)

diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index 1117421..afb2c08 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -216,6 +216,9 @@ config NEED_DMA_MAP_STATE
 config NEED_SG_DMA_LENGTH
 	def_bool y
 
+config ARCH_HAS_DMA_SET_COHERENT_MASK
+	def_bool y
+
 config SMP
 	def_bool y
 
diff --git a/arch/arm64/include/asm/device.h b/arch/arm64/include/asm/device.h
index 243ef25..a57e7bb 100644
--- a/arch/arm64/include/asm/device.h
+++ b/arch/arm64/include/asm/device.h
@@ -22,6 +22,7 @@ struct dev_archdata {
 	void *iommu;			/* private IOMMU data */
 #endif
 	bool dma_coherent;
+	u64 parent_dma_mask;
 };
 
 struct pdev_archdata {
diff --git a/arch/arm64/mm/dma-mapping.c b/arch/arm64/mm/dma-mapping.c
index e040827..5ab15ce 100644
--- a/arch/arm64/mm/dma-mapping.c
+++ b/arch/arm64/mm/dma-mapping.c
@@ -352,6 +352,30 @@ static int __swiotlb_dma_supported(struct device *hwdev, u64 mask)
 	return 1;
 }
 
+static int __swiotlb_set_dma_mask(struct device *dev, u64 mask)
+{
+	/* device is not DMA capable */
+	if (!dev->dma_mask)
+		return -EIO;
+
+	/* mask is below swiotlb bounce buffer, so fail */
+	if (!swiotlb_dma_supported(dev, mask))
+		return -EIO;
+
+	/*
+	 * because of the swiotlb, we can return success for
+	 * larger masks, but need to ensure that bounce buffers
+	 * are used above parent_dma_mask, so set that as
+	 * the effective mask.
+	 */
+	if (mask > dev->archdata.parent_dma_mask)
+		mask = dev->archdata.parent_dma_mask;
+
+	*dev->dma_mask = mask;
+
+	return 0;
+}
+
 static struct dma_map_ops swiotlb_dma_ops = {
 	.alloc = __dma_alloc,
 	.free = __dma_free,
@@ -367,8 +391,23 @@ static struct dma_map_ops swiotlb_dma_ops = {
 	.sync_sg_for_device = __swiotlb_sync_sg_for_device,
 	.dma_supported = __swiotlb_dma_supported,
 	.mapping_error = swiotlb_dma_mapping_error,
+	.set_dma_mask = __swiotlb_set_dma_mask,
 };
 
+int dma_set_coherent_mask(struct device *dev, u64 mask)
+{
+	if (!dma_supported(dev, mask))
+		return -EIO;
+
+	if (get_dma_ops(dev) == &swiotlb_dma_ops &&
+	    mask > dev->archdata.parent_dma_mask)
+		mask = dev->archdata.parent_dma_mask;
+
+	dev->coherent_dma_mask = mask;
+	return 0;
+}
+EXPORT_SYMBOL(dma_set_coherent_mask);
+
 static int __init atomic_pool_init(void)
 {
 	pgprot_t prot = __pgprot(PROT_NORMAL_NC);
@@ -958,6 +997,18 @@ void arch_setup_dma_ops(struct device *dev, u64 dma_base, u64 size,
 	if (!dev->archdata.dma_ops)
 		dev->archdata.dma_ops = &swiotlb_dma_ops;
 
+	/*
+	 * we don't yet support buses that have a non-zero mapping.
+	 *  Let's hope we won't need it
+	 */
+	WARN_ON(dma_base != 0);
+
+	/*
+	 * Whatever the parent bus can set. A device must not set
+	 * a DMA mask larger than this.
+	 */
+	dev->archdata.parent_dma_mask = size - 1;
+
 	dev->archdata.dma_coherent = coherent;
 	__iommu_setup_dma_ops(dev, dma_base, size, iommu);
 }
-- 
2.1.4

Re: [PATCH v2] arm64: do not set dma masks that device connection can't handle

[Will Deacon]

On Mon, Jan 09, 2017 at 10:30:02AM +0300, Nikita Yushchenko wrote:
> It is possible that device is capable of 64-bit DMA addresses, and
> device driver tries to set wide DMA mask, but bridge or bus used to
> connect device to the system can't handle wide addresses.
> 
> With swiotlb, memory above 4G still can be used by drivers for streaming
> DMA, but *dev->mask and dev->dma_coherent_mask must still keep values
> that hardware handles physically.
> 
> This patch enforces that. Based on original version by
> Arnd Bergmann <arnd@arndb.de>, extended with coherent mask hadnling.
> 
> Signed-off-by: Nikita Yushchenko <nikita.yoush@cogentembedded.com>
> CC: Arnd Bergmann <arnd@arndb.de>
> ---
> Changes since v1:
> - fixed issues noted by Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
>   - save mask, not size
>   - remove doube empty line
> 
>  arch/arm64/Kconfig              |  3 +++
>  arch/arm64/include/asm/device.h |  1 +
>  arch/arm64/mm/dma-mapping.c     | 51 +++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 55 insertions(+)

I still don't think this patch is general enough. The problem you're seeing
with swiotlb seems to be exactly the same problem reported by Feng Kan over
at:

  http://lkml.kernel.org/r/CAL85gmA_SSCwM80TKdkZqEe+S1beWzDEvdki1kpkmUTDRmSP7g@mail.gmail.com

[read on; it was initially thought to be a hardware erratum, but it's
 actually the inability to restrict the DMA mask of the endpoint that's
 the problem]

The point here is that an IOMMU doesn't solve your issue, and the
IOMMU-backed DMA ops need the same treatment. In light of that, it really
feels to me like the DMA masks should be restricted in of_dma_configure
so that the parent mask is taken into account there, rather than hook
into each set of DMA ops to intercept set_dma_mask. We'd still need to
do something to stop dma_set_mask widening the mask if it was restricted
by of_dma_configure, but I think Robin (cc'd) was playing with that.

Will

[PATCH v2] arm64: do not set dma masks that device connection can't handle

[Will Deacon]

On Mon, Jan 09, 2017 at 10:30:02AM +0300, Nikita Yushchenko wrote:
> It is possible that device is capable of 64-bit DMA addresses, and
> device driver tries to set wide DMA mask, but bridge or bus used to
> connect device to the system can't handle wide addresses.
> 
> With swiotlb, memory above 4G still can be used by drivers for streaming
> DMA, but *dev->mask and dev->dma_coherent_mask must still keep values
> that hardware handles physically.
> 
> This patch enforces that. Based on original version by
> Arnd Bergmann <arnd@arndb.de>, extended with coherent mask hadnling.
> 
> Signed-off-by: Nikita Yushchenko <nikita.yoush@cogentembedded.com>
> CC: Arnd Bergmann <arnd@arndb.de>
> ---
> Changes since v1:
> - fixed issues noted by Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
>   - save mask, not size
>   - remove doube empty line
> 
>  arch/arm64/Kconfig              |  3 +++
>  arch/arm64/include/asm/device.h |  1 +
>  arch/arm64/mm/dma-mapping.c     | 51 +++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 55 insertions(+)

I still don't think this patch is general enough. The problem you're seeing
with swiotlb seems to be exactly the same problem reported by Feng Kan over
at:

  http://lkml.kernel.org/r/CAL85gmA_SSCwM80TKdkZqEe+S1beWzDEvdki1kpkmUTDRmSP7g at mail.gmail.com

[read on; it was initially thought to be a hardware erratum, but it's
 actually the inability to restrict the DMA mask of the endpoint that's
 the problem]

The point here is that an IOMMU doesn't solve your issue, and the
IOMMU-backed DMA ops need the same treatment. In light of that, it really
feels to me like the DMA masks should be restricted in of_dma_configure
so that the parent mask is taken into account there, rather than hook
into each set of DMA ops to intercept set_dma_mask. We'd still need to
do something to stop dma_set_mask widening the mask if it was restricted
by of_dma_configure, but I think Robin (cc'd) was playing with that.

Will

Re: [PATCH v2] arm64: do not set dma masks that device connection can't handle

[Nikita Yushchenko]

Hi

> The point here is that an IOMMU doesn't solve your issue, and the
> IOMMU-backed DMA ops need the same treatment. In light of that, it really
> feels to me like the DMA masks should be restricted in of_dma_configure
> so that the parent mask is taken into account there, rather than hook
> into each set of DMA ops to intercept set_dma_mask. We'd still need to
> do something to stop dma_set_mask widening the mask if it was restricted
> by of_dma_configure, but I think Robin (cc'd) was playing with that.

What issue "IOMMU doesn't solve"?

Issue I'm trying to address is - inconsistency within swiotlb
dma_map_ops, where (1) any wide mask is silently accepted, but (2) then
mask is used to decide if bounce buffers are needed or not. This
inconsistency causes NVMe+R-Car cobmo not working (and breaking memory
instead).

I just can't think out what similar issue iommu can have.
Do you mean that in iommu case, mask also must not be set to whatever
wider than initial value? Why? What is the use of mask in iommu case? Is
there any real case when iommu can't address all memory existing in the
system?

NVMe maintainer has just stated that they expect
set_dma_mask(DMA_BIT_MASK(64)) to always succeed, and are going to error
out driver probe if that call fails.  They claim that architecture must
always be able to dma_map() whatever memory existing in the system - via
iommu or swiotlb or whatever. Their direction is to remove bounce
buffers from block and other layers.

With this direction, semantics of dma mask becomes even more
questionable. I'd say dma_mask is candidate for removal (or to move to
swiotlb's or iommu's local area)

Nikita

[PATCH v2] arm64: do not set dma masks that device connection can't handle

[Nikita Yushchenko]

Hi

> The point here is that an IOMMU doesn't solve your issue, and the
> IOMMU-backed DMA ops need the same treatment. In light of that, it really
> feels to me like the DMA masks should be restricted in of_dma_configure
> so that the parent mask is taken into account there, rather than hook
> into each set of DMA ops to intercept set_dma_mask. We'd still need to
> do something to stop dma_set_mask widening the mask if it was restricted
> by of_dma_configure, but I think Robin (cc'd) was playing with that.

What issue "IOMMU doesn't solve"?

Issue I'm trying to address is - inconsistency within swiotlb
dma_map_ops, where (1) any wide mask is silently accepted, but (2) then
mask is used to decide if bounce buffers are needed or not. This
inconsistency causes NVMe+R-Car cobmo not working (and breaking memory
instead).

I just can't think out what similar issue iommu can have.
Do you mean that in iommu case, mask also must not be set to whatever
wider than initial value? Why? What is the use of mask in iommu case? Is
there any real case when iommu can't address all memory existing in the
system?

NVMe maintainer has just stated that they expect
set_dma_mask(DMA_BIT_MASK(64)) to always succeed, and are going to error
out driver probe if that call fails.  They claim that architecture must
always be able to dma_map() whatever memory existing in the system - via
iommu or swiotlb or whatever. Their direction is to remove bounce
buffers from block and other layers.

With this direction, semantics of dma mask becomes even more
questionable. I'd say dma_mask is candidate for removal (or to move to
swiotlb's or iommu's local area)

Nikita

Re: [PATCH v2] arm64: do not set dma masks that device connection can't handle

[Arnd Bergmann]

On Tuesday, January 10, 2017 3:47:25 PM CET Nikita Yushchenko wrote:
> 
> > The point here is that an IOMMU doesn't solve your issue, and the
> > IOMMU-backed DMA ops need the same treatment. In light of that, it really
> > feels to me like the DMA masks should be restricted in of_dma_configure
> > so that the parent mask is taken into account there, rather than hook
> > into each set of DMA ops to intercept set_dma_mask.

of_dma_configure() sets up a 32-bit mask, which is assumed to always work
in the kernel. We can't change it to a larger mask because that would
break drivers that have to restrict devices to 32-bit.

If the bus addressing is narrower than 32 bits however, the initial mask
should probably be limited to whatever the bus supports, but that is not
the problem we are trying to solve here.

> > We'd still need to
> > do something to stop dma_set_mask widening the mask if it was restricted
> > by of_dma_configure, but I think Robin (cc'd) was playing with that.
> 
> What issue "IOMMU doesn't solve"?
> 
> Issue I'm trying to address is - inconsistency within swiotlb
> dma_map_ops, where (1) any wide mask is silently accepted, but (2) then
> mask is used to decide if bounce buffers are needed or not. This
> inconsistency causes NVMe+R-Car cobmo not working (and breaking memory
> instead).

It's not just an inconsistency, it's a known bug that we really
need to fix.

> I just can't think out what similar issue iommu can have.
> Do you mean that in iommu case, mask also must not be set to whatever
> wider than initial value? Why? What is the use of mask in iommu case? Is
> there any real case when iommu can't address all memory existing in the
> system?

I think the problem that Will is referring to is when the IOMMU has
a virtual address space that is wider than the DMA mask of the device:
In this case, dma_map_single() might return a dma_addr_t that is not
reachable by the device.

I'd consider that a separate bug that needs to be worked around in
the IOMMU code.

> NVMe maintainer has just stated that they expect
> set_dma_mask(DMA_BIT_MASK(64)) to always succeed, and are going to error
> out driver probe if that call fails.  They claim that architecture must
> always be able to dma_map() whatever memory existing in the system - via
> iommu or swiotlb or whatever. Their direction is to remove bounce
> buffers from block and other layers.
> 
> With this direction, semantics of dma mask becomes even more
> questionable. I'd say dma_mask is candidate for removal (or to move to
> swiotlb's or iommu's local area)

Removing dma_mask is not realistic any time soon, there are too many things
in the kernel that use it for one thing or another, so any changes here
have to be done really carefully. We definitely need the mask to support
architectures without swiotlb.

	Arnd

[PATCH v2] arm64: do not set dma masks that device connection can't handle

[Arnd Bergmann]

On Tuesday, January 10, 2017 3:47:25 PM CET Nikita Yushchenko wrote:
> 
> > The point here is that an IOMMU doesn't solve your issue, and the
> > IOMMU-backed DMA ops need the same treatment. In light of that, it really
> > feels to me like the DMA masks should be restricted in of_dma_configure
> > so that the parent mask is taken into account there, rather than hook
> > into each set of DMA ops to intercept set_dma_mask.

of_dma_configure() sets up a 32-bit mask, which is assumed to always work
in the kernel. We can't change it to a larger mask because that would
break drivers that have to restrict devices to 32-bit.

If the bus addressing is narrower than 32 bits however, the initial mask
should probably be limited to whatever the bus supports, but that is not
the problem we are trying to solve here.

> > We'd still need to
> > do something to stop dma_set_mask widening the mask if it was restricted
> > by of_dma_configure, but I think Robin (cc'd) was playing with that.
> 
> What issue "IOMMU doesn't solve"?
> 
> Issue I'm trying to address is - inconsistency within swiotlb
> dma_map_ops, where (1) any wide mask is silently accepted, but (2) then
> mask is used to decide if bounce buffers are needed or not. This
> inconsistency causes NVMe+R-Car cobmo not working (and breaking memory
> instead).

It's not just an inconsistency, it's a known bug that we really
need to fix.

> I just can't think out what similar issue iommu can have.
> Do you mean that in iommu case, mask also must not be set to whatever
> wider than initial value? Why? What is the use of mask in iommu case? Is
> there any real case when iommu can't address all memory existing in the
> system?

I think the problem that Will is referring to is when the IOMMU has
a virtual address space that is wider than the DMA mask of the device:
In this case, dma_map_single() might return a dma_addr_t that is not
reachable by the device.

I'd consider that a separate bug that needs to be worked around in
the IOMMU code.

> NVMe maintainer has just stated that they expect
> set_dma_mask(DMA_BIT_MASK(64)) to always succeed, and are going to error
> out driver probe if that call fails.  They claim that architecture must
> always be able to dma_map() whatever memory existing in the system - via
> iommu or swiotlb or whatever. Their direction is to remove bounce
> buffers from block and other layers.
> 
> With this direction, semantics of dma mask becomes even more
> questionable. I'd say dma_mask is candidate for removal (or to move to
> swiotlb's or iommu's local area)

Removing dma_mask is not realistic any time soon, there are too many things
in the kernel that use it for one thing or another, so any changes here
have to be done really carefully. We definitely need the mask to support
architectures without swiotlb.

	Arnd

Re: [PATCH v2] arm64: do not set dma masks that device connection can't handle

[Robin Murphy]

On 10/01/17 12:47, Nikita Yushchenko wrote:
> Hi
> 
>> The point here is that an IOMMU doesn't solve your issue, and the
>> IOMMU-backed DMA ops need the same treatment. In light of that, it really
>> feels to me like the DMA masks should be restricted in of_dma_configure
>> so that the parent mask is taken into account there, rather than hook
>> into each set of DMA ops to intercept set_dma_mask. We'd still need to
>> do something to stop dma_set_mask widening the mask if it was restricted
>> by of_dma_configure, but I think Robin (cc'd) was playing with that.
> 
> What issue "IOMMU doesn't solve"?
> 
> Issue I'm trying to address is - inconsistency within swiotlb
> dma_map_ops, where (1) any wide mask is silently accepted, but (2) then
> mask is used to decide if bounce buffers are needed or not. This
> inconsistency causes NVMe+R-Car cobmo not working (and breaking memory
> instead).

The fundamental underlying problem is the "any wide mask is silently
accepted" part, and that applies equally to IOMMU ops as well.

> I just can't think out what similar issue iommu can have.
> Do you mean that in iommu case, mask also must not be set to whatever
> wider than initial value? Why? What is the use of mask in iommu case? Is
> there any real case when iommu can't address all memory existing in the
> system?

There's a very subtle misunderstanding there - the DMA mask does not
describe the memory a device can address, it describes the range of
addresses the device is capable of generating. Yes, in the non-IOMMU
case they are equivalent, but once you put an IOMMU in between, the
problem is merely shifted from "what range of physical addresses can
this device access" to "what range of IOVAs is valid to give to this
device" - the fact that those IOVAs can map to any underlying physical
address only obviates the need for any bouncing at the memory end; it
doesn't remove the fact that the device has a hardware addressing
limitation which needs to be accommodated.

The thread Will linked to describes that equivalent version of your
problem - the IOMMU gives the device 48-bit addresses which get
erroneously truncated because it doesn't know that only 42 bits are
actually wired up. That situation still requires the device's DMA mask
to correctly describe its addressing capability just as yours does.

> NVMe maintainer has just stated that they expect
> set_dma_mask(DMA_BIT_MASK(64)) to always succeed, and are going to error
> out driver probe if that call fails.  They claim that architecture must
> always be able to dma_map() whatever memory existing in the system - via
> iommu or swiotlb or whatever. Their direction is to remove bounce
> buffers from block and other layers.

I have to say I'm in full agreement with that - having subsystems do
their own bouncing is generally redundant, although there are some
edge-cases like MMC_BLOCK_BOUNCE (which is primarily about aligning and
coalescing buffers than working around DMA limitations per se).

> With this direction, semantics of dma mask becomes even more
> questionable. I'd say dma_mask is candidate for removal (or to move to
> swiotlb's or iommu's local area)

We still need a way for drivers to communicate a device's probed
addressing capability to SWIOTLB, so there's always going to have to be
*some* sort of public interface. Personally, the change in semantics I'd
like to see is to make dma_set_mask() only fail if DMA is entirely
disallowed - in the normal case it would always succeed, but the DMA API
implementation would be permitted to set a smaller mask than requested
(this is effectively what the x86 IOMMU ops do already). The significant
work that would require, though, is changing all the drivers currently
using this sort of pattern:

	if (!dma_set_mask(dev, DMA_BIT_MASK(64))
		/* put device into 64-bit mode */
	else if (!dma_set_mask(dev, DMA_BIT_MASK(32))
		/* put device into 32-bit mode */
	else
		/* error */

to something like this:

	if (!dma_set_mask(dev, DMA_BIT_MASK(64))
		/* error */
	if (dma_get_mask(dev) > DMA_BIT_MASK(32))
		/* put device into 64-bit mode */
	else
		/* put device into 32-bit mode */

Which would be a pretty major job.

Robin.

[PATCH v2] arm64: do not set dma masks that device connection can't handle

[Robin Murphy]

On 10/01/17 12:47, Nikita Yushchenko wrote:
> Hi
> 
>> The point here is that an IOMMU doesn't solve your issue, and the
>> IOMMU-backed DMA ops need the same treatment. In light of that, it really
>> feels to me like the DMA masks should be restricted in of_dma_configure
>> so that the parent mask is taken into account there, rather than hook
>> into each set of DMA ops to intercept set_dma_mask. We'd still need to
>> do something to stop dma_set_mask widening the mask if it was restricted
>> by of_dma_configure, but I think Robin (cc'd) was playing with that.
> 
> What issue "IOMMU doesn't solve"?
> 
> Issue I'm trying to address is - inconsistency within swiotlb
> dma_map_ops, where (1) any wide mask is silently accepted, but (2) then
> mask is used to decide if bounce buffers are needed or not. This
> inconsistency causes NVMe+R-Car cobmo not working (and breaking memory
> instead).

The fundamental underlying problem is the "any wide mask is silently
accepted" part, and that applies equally to IOMMU ops as well.

> I just can't think out what similar issue iommu can have.
> Do you mean that in iommu case, mask also must not be set to whatever
> wider than initial value? Why? What is the use of mask in iommu case? Is
> there any real case when iommu can't address all memory existing in the
> system?

There's a very subtle misunderstanding there - the DMA mask does not
describe the memory a device can address, it describes the range of
addresses the device is capable of generating. Yes, in the non-IOMMU
case they are equivalent, but once you put an IOMMU in between, the
problem is merely shifted from "what range of physical addresses can
this device access" to "what range of IOVAs is valid to give to this
device" - the fact that those IOVAs can map to any underlying physical
address only obviates the need for any bouncing at the memory end; it
doesn't remove the fact that the device has a hardware addressing
limitation which needs to be accommodated.

The thread Will linked to describes that equivalent version of your
problem - the IOMMU gives the device 48-bit addresses which get
erroneously truncated because it doesn't know that only 42 bits are
actually wired up. That situation still requires the device's DMA mask
to correctly describe its addressing capability just as yours does.

> NVMe maintainer has just stated that they expect
> set_dma_mask(DMA_BIT_MASK(64)) to always succeed, and are going to error
> out driver probe if that call fails.  They claim that architecture must
> always be able to dma_map() whatever memory existing in the system - via
> iommu or swiotlb or whatever. Their direction is to remove bounce
> buffers from block and other layers.

I have to say I'm in full agreement with that - having subsystems do
their own bouncing is generally redundant, although there are some
edge-cases like MMC_BLOCK_BOUNCE (which is primarily about aligning and
coalescing buffers than working around DMA limitations per se).

> With this direction, semantics of dma mask becomes even more
> questionable. I'd say dma_mask is candidate for removal (or to move to
> swiotlb's or iommu's local area)

We still need a way for drivers to communicate a device's probed
addressing capability to SWIOTLB, so there's always going to have to be
*some* sort of public interface. Personally, the change in semantics I'd
like to see is to make dma_set_mask() only fail if DMA is entirely
disallowed - in the normal case it would always succeed, but the DMA API
implementation would be permitted to set a smaller mask than requested
(this is effectively what the x86 IOMMU ops do already). The significant
work that would require, though, is changing all the drivers currently
using this sort of pattern:

	if (!dma_set_mask(dev, DMA_BIT_MASK(64))
		/* put device into 64-bit mode */
	else if (!dma_set_mask(dev, DMA_BIT_MASK(32))
		/* put device into 32-bit mode */
	else
		/* error */

to something like this:

	if (!dma_set_mask(dev, DMA_BIT_MASK(64))
		/* error */
	if (dma_get_mask(dev) > DMA_BIT_MASK(32))
		/* put device into 64-bit mode */
	else
		/* put device into 32-bit mode */

Which would be a pretty major job.

Robin.

Re: [PATCH v2] arm64: do not set dma masks that device connection can't handle

[Arnd Bergmann]

On Tuesday, January 10, 2017 1:25:12 PM CET Robin Murphy wrote:
> On 10/01/17 12:47, Nikita Yushchenko wrote:
> >> The point here is that an IOMMU doesn't solve your issue, and the
> >> IOMMU-backed DMA ops need the same treatment. In light of that, it really
> >> feels to me like the DMA masks should be restricted in of_dma_configure
> >> so that the parent mask is taken into account there, rather than hook
> >> into each set of DMA ops to intercept set_dma_mask. We'd still need to
> >> do something to stop dma_set_mask widening the mask if it was restricted
> >> by of_dma_configure, but I think Robin (cc'd) was playing with that.
> > 
> > What issue "IOMMU doesn't solve"?
> > 
> > Issue I'm trying to address is - inconsistency within swiotlb
> > dma_map_ops, where (1) any wide mask is silently accepted, but (2) then
> > mask is used to decide if bounce buffers are needed or not. This
> > inconsistency causes NVMe+R-Car cobmo not working (and breaking memory
> > instead).
> 
> The fundamental underlying problem is the "any wide mask is silently
> accepted" part, and that applies equally to IOMMU ops as well.

It's a much rarer problem for the IOMMU case though, because it only
impacts devices that are restricted to addressing of less than 32-bits.

If you have an IOMMU enabled, the dma-mapping interface does not care
if the device can do wider than 32 bit addressing, as it will never
hand out IOVAs above 0xffffffff.

> > I just can't think out what similar issue iommu can have.
> > Do you mean that in iommu case, mask also must not be set to whatever
> > wider than initial value? Why? What is the use of mask in iommu case? Is
> > there any real case when iommu can't address all memory existing in the
> > system?
> 
> There's a very subtle misunderstanding there - the DMA mask does not
> describe the memory a device can address, it describes the range of
> addresses the device is capable of generating. Yes, in the non-IOMMU
> case they are equivalent, but once you put an IOMMU in between, the
> problem is merely shifted from "what range of physical addresses can
> this device access" to "what range of IOVAs is valid to give to this
> device" - the fact that those IOVAs can map to any underlying physical
> address only obviates the need for any bouncing at the memory end; it
> doesn't remove the fact that the device has a hardware addressing
> limitation which needs to be accommodated.
> 
> The thread Will linked to describes that equivalent version of your
> problem - the IOMMU gives the device 48-bit addresses which get
> erroneously truncated because it doesn't know that only 42 bits are
> actually wired up. That situation still requires the device's DMA mask
> to correctly describe its addressing capability just as yours does.

That problem should only impact virtual machines which have a guest
bus address space covering more than 42 bits of physical RAM, whereas
the problem we have with swiotlb is for the dma-mapping interface.

> > With this direction, semantics of dma mask becomes even more
> > questionable. I'd say dma_mask is candidate for removal (or to move to
> > swiotlb's or iommu's local area)
> 
> We still need a way for drivers to communicate a device's probed
> addressing capability to SWIOTLB, so there's always going to have to be
> *some* sort of public interface. Personally, the change in semantics I'd
> like to see is to make dma_set_mask() only fail if DMA is entirely
> disallowed - in the normal case it would always succeed, but the DMA API
> implementation would be permitted to set a smaller mask than requested
> (this is effectively what the x86 IOMMU ops do already).

With swiotlb enabled, it only needs to fail if the mask does not contain
the swiotlb bounce buffer area, either because the start of RAM is outside
of the mask, or the bounce area has been allocated at the end of ZONE_DMA
and the mask is smaller than ZONE_DMA.

	Arnd

[PATCH v2] arm64: do not set dma masks that device connection can't handle

[Arnd Bergmann]

On Tuesday, January 10, 2017 1:25:12 PM CET Robin Murphy wrote:
> On 10/01/17 12:47, Nikita Yushchenko wrote:
> >> The point here is that an IOMMU doesn't solve your issue, and the
> >> IOMMU-backed DMA ops need the same treatment. In light of that, it really
> >> feels to me like the DMA masks should be restricted in of_dma_configure
> >> so that the parent mask is taken into account there, rather than hook
> >> into each set of DMA ops to intercept set_dma_mask. We'd still need to
> >> do something to stop dma_set_mask widening the mask if it was restricted
> >> by of_dma_configure, but I think Robin (cc'd) was playing with that.
> > 
> > What issue "IOMMU doesn't solve"?
> > 
> > Issue I'm trying to address is - inconsistency within swiotlb
> > dma_map_ops, where (1) any wide mask is silently accepted, but (2) then
> > mask is used to decide if bounce buffers are needed or not. This
> > inconsistency causes NVMe+R-Car cobmo not working (and breaking memory
> > instead).
> 
> The fundamental underlying problem is the "any wide mask is silently
> accepted" part, and that applies equally to IOMMU ops as well.

It's a much rarer problem for the IOMMU case though, because it only
impacts devices that are restricted to addressing of less than 32-bits.

If you have an IOMMU enabled, the dma-mapping interface does not care
if the device can do wider than 32 bit addressing, as it will never
hand out IOVAs above 0xffffffff.

> > I just can't think out what similar issue iommu can have.
> > Do you mean that in iommu case, mask also must not be set to whatever
> > wider than initial value? Why? What is the use of mask in iommu case? Is
> > there any real case when iommu can't address all memory existing in the
> > system?
> 
> There's a very subtle misunderstanding there - the DMA mask does not
> describe the memory a device can address, it describes the range of
> addresses the device is capable of generating. Yes, in the non-IOMMU
> case they are equivalent, but once you put an IOMMU in between, the
> problem is merely shifted from "what range of physical addresses can
> this device access" to "what range of IOVAs is valid to give to this
> device" - the fact that those IOVAs can map to any underlying physical
> address only obviates the need for any bouncing at the memory end; it
> doesn't remove the fact that the device has a hardware addressing
> limitation which needs to be accommodated.
> 
> The thread Will linked to describes that equivalent version of your
> problem - the IOMMU gives the device 48-bit addresses which get
> erroneously truncated because it doesn't know that only 42 bits are
> actually wired up. That situation still requires the device's DMA mask
> to correctly describe its addressing capability just as yours does.

That problem should only impact virtual machines which have a guest
bus address space covering more than 42 bits of physical RAM, whereas
the problem we have with swiotlb is for the dma-mapping interface.

> > With this direction, semantics of dma mask becomes even more
> > questionable. I'd say dma_mask is candidate for removal (or to move to
> > swiotlb's or iommu's local area)
> 
> We still need a way for drivers to communicate a device's probed
> addressing capability to SWIOTLB, so there's always going to have to be
> *some* sort of public interface. Personally, the change in semantics I'd
> like to see is to make dma_set_mask() only fail if DMA is entirely
> disallowed - in the normal case it would always succeed, but the DMA API
> implementation would be permitted to set a smaller mask than requested
> (this is effectively what the x86 IOMMU ops do already).

With swiotlb enabled, it only needs to fail if the mask does not contain
the swiotlb bounce buffer area, either because the start of RAM is outside
of the mask, or the bounce area has been allocated at the end of ZONE_DMA
and the mask is smaller than ZONE_DMA.

	Arnd

[PATCH] arm64: avoid increasing DMA masks above what hardware supports

[Nikita Yushchenko]

There are cases when device supports wide DMA addresses wider than
device's connection supports.

In this case driver sets DMA mask based on knowledge of device
capabilities. That must succeed to allow drivers to initialize.

However, swiotlb or iommu still need knowledge about actual device
capabilities. To avoid breakage, actual mask must not be set wider than
device connection allows.

Signed-off-by: Nikita Yushchenko <nikita.yoush@cogentembedded.com>
CC: Arnd Bergmann <arnd@arndb.de>
CC: Robin Murphy <robin.murphy@arm.com>
CC: Will Deacon <will.deacon@arm.com>
---
 arch/arm64/Kconfig                   |  3 +++
 arch/arm64/include/asm/device.h      |  1 +
 arch/arm64/include/asm/dma-mapping.h |  3 +++
 arch/arm64/mm/dma-mapping.c          | 43 ++++++++++++++++++++++++++++++++++++
 4 files changed, 50 insertions(+)

diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index 1117421..afb2c08 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -216,6 +216,9 @@ config NEED_DMA_MAP_STATE
 config NEED_SG_DMA_LENGTH
 	def_bool y
 
+config ARCH_HAS_DMA_SET_COHERENT_MASK
+	def_bool y
+
 config SMP
 	def_bool y
 
diff --git a/arch/arm64/include/asm/device.h b/arch/arm64/include/asm/device.h
index 243ef25..a57e7bb 100644
--- a/arch/arm64/include/asm/device.h
+++ b/arch/arm64/include/asm/device.h
@@ -22,6 +22,7 @@ struct dev_archdata {
 	void *iommu;			/* private IOMMU data */
 #endif
 	bool dma_coherent;
+	u64 parent_dma_mask;
 };
 
 struct pdev_archdata {
diff --git a/arch/arm64/include/asm/dma-mapping.h b/arch/arm64/include/asm/dma-mapping.h
index ccea82c..eab36d2 100644
--- a/arch/arm64/include/asm/dma-mapping.h
+++ b/arch/arm64/include/asm/dma-mapping.h
@@ -51,6 +51,9 @@ void arch_setup_dma_ops(struct device *dev, u64 dma_base, u64 size,
 			const struct iommu_ops *iommu, bool coherent);
 #define arch_setup_dma_ops	arch_setup_dma_ops
 
+#define HAVE_ARCH_DMA_SET_MASK 1
+extern int dma_set_mask(struct device *dev, u64 dma_mask);
+
 #ifdef CONFIG_IOMMU_DMA
 void arch_teardown_dma_ops(struct device *dev);
 #define arch_teardown_dma_ops	arch_teardown_dma_ops
diff --git a/arch/arm64/mm/dma-mapping.c b/arch/arm64/mm/dma-mapping.c
index e040827..7b1bb87 100644
--- a/arch/arm64/mm/dma-mapping.c
+++ b/arch/arm64/mm/dma-mapping.c
@@ -203,6 +203,37 @@ static void __dma_free(struct device *dev, size_t size,
 	__dma_free_coherent(dev, size, swiotlb_addr, dma_handle, attrs);
 }
 
+int dma_set_mask(struct device *dev, u64 dma_mask)
+{
+	const struct dma_map_ops *ops = get_dma_ops(dev);
+
+	if (mask > dev->archdata.parent_dma_mask)
+		mask = dev->archdata.parent_dma_mask;
+
+	if (ops->set_dma_mask)
+		return ops->set_dma_mask(dev, mask);
+
+	if (!dev->dma_mask || !dma_supported(dev, mask))
+		return -EIO;
+
+	*dev->dma_mask = mask;
+	return 0;
+}
+EXPORT_SYMBOL(dma_set_mask);
+
+int dma_set_coherent_mask(struct device *dev, u64 mask)
+{
+	if (mask > dev->archdata.parent_dma_mask)
+		mask = dev->archdata.parent_dma_mask;
+
+	if (!dma_supported(dev, mask))
+		return -EIO;
+
+	dev->coherent_dma_mask = mask;
+	return 0;
+}
+EXPORT_SYMBOL(dma_set_coherent_mask);
+
 static dma_addr_t __swiotlb_map_page(struct device *dev, struct page *page,
 				     unsigned long offset, size_t size,
 				     enum dma_data_direction dir,
@@ -958,6 +989,18 @@ void arch_setup_dma_ops(struct device *dev, u64 dma_base, u64 size,
 	if (!dev->archdata.dma_ops)
 		dev->archdata.dma_ops = &swiotlb_dma_ops;
 
+	/*
+	 * we don't yet support buses that have a non-zero mapping.
+	 *  Let's hope we won't need it
+	 */
+	WARN_ON(dma_base != 0);
+
+	/*
+	 * Whatever the parent bus can set. A device must not set
+	 * a DMA mask larger than this.
+	 */
+	dev->archdata.parent_dma_mask = size - 1;
+
 	dev->archdata.dma_coherent = coherent;
 	__iommu_setup_dma_ops(dev, dma_base, size, iommu);
 }
-- 
2.1.4

[PATCH] arm64: avoid increasing DMA masks above what hardware supports

[Nikita Yushchenko]

There are cases when device supports wide DMA addresses wider than
device's connection supports.

In this case driver sets DMA mask based on knowledge of device
capabilities. That must succeed to allow drivers to initialize.

However, swiotlb or iommu still need knowledge about actual device
capabilities. To avoid breakage, actual mask must not be set wider than
device connection allows.

Signed-off-by: Nikita Yushchenko <nikita.yoush@cogentembedded.com>
CC: Arnd Bergmann <arnd@arndb.de>
CC: Robin Murphy <robin.murphy@arm.com>
CC: Will Deacon <will.deacon@arm.com>
---
 arch/arm64/Kconfig                   |  3 +++
 arch/arm64/include/asm/device.h      |  1 +
 arch/arm64/include/asm/dma-mapping.h |  3 +++
 arch/arm64/mm/dma-mapping.c          | 43 ++++++++++++++++++++++++++++++++++++
 4 files changed, 50 insertions(+)

diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index 1117421..afb2c08 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -216,6 +216,9 @@ config NEED_DMA_MAP_STATE
 config NEED_SG_DMA_LENGTH
 	def_bool y
 
+config ARCH_HAS_DMA_SET_COHERENT_MASK
+	def_bool y
+
 config SMP
 	def_bool y
 
diff --git a/arch/arm64/include/asm/device.h b/arch/arm64/include/asm/device.h
index 243ef25..a57e7bb 100644
--- a/arch/arm64/include/asm/device.h
+++ b/arch/arm64/include/asm/device.h
@@ -22,6 +22,7 @@ struct dev_archdata {
 	void *iommu;			/* private IOMMU data */
 #endif
 	bool dma_coherent;
+	u64 parent_dma_mask;
 };
 
 struct pdev_archdata {
diff --git a/arch/arm64/include/asm/dma-mapping.h b/arch/arm64/include/asm/dma-mapping.h
index ccea82c..eab36d2 100644
--- a/arch/arm64/include/asm/dma-mapping.h
+++ b/arch/arm64/include/asm/dma-mapping.h
@@ -51,6 +51,9 @@ void arch_setup_dma_ops(struct device *dev, u64 dma_base, u64 size,
 			const struct iommu_ops *iommu, bool coherent);
 #define arch_setup_dma_ops	arch_setup_dma_ops
 
+#define HAVE_ARCH_DMA_SET_MASK 1
+extern int dma_set_mask(struct device *dev, u64 dma_mask);
+
 #ifdef CONFIG_IOMMU_DMA
 void arch_teardown_dma_ops(struct device *dev);
 #define arch_teardown_dma_ops	arch_teardown_dma_ops
diff --git a/arch/arm64/mm/dma-mapping.c b/arch/arm64/mm/dma-mapping.c
index e040827..7b1bb87 100644
--- a/arch/arm64/mm/dma-mapping.c
+++ b/arch/arm64/mm/dma-mapping.c
@@ -203,6 +203,37 @@ static void __dma_free(struct device *dev, size_t size,
 	__dma_free_coherent(dev, size, swiotlb_addr, dma_handle, attrs);
 }
 
+int dma_set_mask(struct device *dev, u64 dma_mask)
+{
+	const struct dma_map_ops *ops = get_dma_ops(dev);
+
+	if (mask > dev->archdata.parent_dma_mask)
+		mask = dev->archdata.parent_dma_mask;
+
+	if (ops->set_dma_mask)
+		return ops->set_dma_mask(dev, mask);
+
+	if (!dev->dma_mask || !dma_supported(dev, mask))
+		return -EIO;
+
+	*dev->dma_mask = mask;
+	return 0;
+}
+EXPORT_SYMBOL(dma_set_mask);
+
+int dma_set_coherent_mask(struct device *dev, u64 mask)
+{
+	if (mask > dev->archdata.parent_dma_mask)
+		mask = dev->archdata.parent_dma_mask;
+
+	if (!dma_supported(dev, mask))
+		return -EIO;
+
+	dev->coherent_dma_mask = mask;
+	return 0;
+}
+EXPORT_SYMBOL(dma_set_coherent_mask);
+
 static dma_addr_t __swiotlb_map_page(struct device *dev, struct page *page,
 				     unsigned long offset, size_t size,
 				     enum dma_data_direction dir,
@@ -958,6 +989,18 @@ void arch_setup_dma_ops(struct device *dev, u64 dma_base, u64 size,
 	if (!dev->archdata.dma_ops)
 		dev->archdata.dma_ops = &swiotlb_dma_ops;
 
+	/*
+	 * we don't yet support buses that have a non-zero mapping.
+	 *  Let's hope we won't need it
+	 */
+	WARN_ON(dma_base != 0);
+
+	/*
+	 * Whatever the parent bus can set. A device must not set
+	 * a DMA mask larger than this.
+	 */
+	dev->archdata.parent_dma_mask = size - 1;
+
 	dev->archdata.dma_coherent = coherent;
 	__iommu_setup_dma_ops(dev, dma_base, size, iommu);
 }
-- 
2.1.4

Re: [PATCH v2] arm64: do not set dma masks that device connection can't handle

[Nikita Yushchenko]

>> What issue "IOMMU doesn't solve"?
>>
>> Issue I'm trying to address is - inconsistency within swiotlb
>> dma_map_ops, where (1) any wide mask is silently accepted, but (2) then
>> mask is used to decide if bounce buffers are needed or not. This
>> inconsistency causes NVMe+R-Car cobmo not working (and breaking memory
>> instead).
> 
> The fundamental underlying problem is the "any wide mask is silently
> accepted" part, and that applies equally to IOMMU ops as well.

Is just posted version better?

It should cover iommu case as well.

Nikita

[PATCH v2] arm64: do not set dma masks that device connection can't handle

[Nikita Yushchenko]

>> What issue "IOMMU doesn't solve"?
>>
>> Issue I'm trying to address is - inconsistency within swiotlb
>> dma_map_ops, where (1) any wide mask is silently accepted, but (2) then
>> mask is used to decide if bounce buffers are needed or not. This
>> inconsistency causes NVMe+R-Car cobmo not working (and breaking memory
>> instead).
> 
> The fundamental underlying problem is the "any wide mask is silently
> accepted" part, and that applies equally to IOMMU ops as well.

Is just posted version better?

It should cover iommu case as well.

Nikita

Re: [PATCH v2] arm64: do not set dma masks that device connection can't handle

[Robin Murphy]

On 10/01/17 13:42, Arnd Bergmann wrote:
> On Tuesday, January 10, 2017 1:25:12 PM CET Robin Murphy wrote:
>> On 10/01/17 12:47, Nikita Yushchenko wrote:
>>>> The point here is that an IOMMU doesn't solve your issue, and the
>>>> IOMMU-backed DMA ops need the same treatment. In light of that, it really
>>>> feels to me like the DMA masks should be restricted in of_dma_configure
>>>> so that the parent mask is taken into account there, rather than hook
>>>> into each set of DMA ops to intercept set_dma_mask. We'd still need to
>>>> do something to stop dma_set_mask widening the mask if it was restricted
>>>> by of_dma_configure, but I think Robin (cc'd) was playing with that.
>>>
>>> What issue "IOMMU doesn't solve"?
>>>
>>> Issue I'm trying to address is - inconsistency within swiotlb
>>> dma_map_ops, where (1) any wide mask is silently accepted, but (2) then
>>> mask is used to decide if bounce buffers are needed or not. This
>>> inconsistency causes NVMe+R-Car cobmo not working (and breaking memory
>>> instead).
>>
>> The fundamental underlying problem is the "any wide mask is silently
>> accepted" part, and that applies equally to IOMMU ops as well.
> 
> It's a much rarer problem for the IOMMU case though, because it only
> impacts devices that are restricted to addressing of less than 32-bits.
> 
> If you have an IOMMU enabled, the dma-mapping interface does not care
> if the device can do wider than 32 bit addressing, as it will never
> hand out IOVAs above 0xffffffff.

I can assure you that it will - we constrain allocations to the
intersection of the IOMMU domain aperture (normally the IOMMU's physical
input address width) and the given device's DMA mask. If both of those
are >32 bits then >32-bit IOVAs will fall out. For the arm64/common
implementation I have prototyped a copy of the x86 optimisation which
always first tries to get 32-bit IOVAs for PCI devices, but even then it
can start returning higher addresses if the 32-bit space fills up.

>>> I just can't think out what similar issue iommu can have.
>>> Do you mean that in iommu case, mask also must not be set to whatever
>>> wider than initial value? Why? What is the use of mask in iommu case? Is
>>> there any real case when iommu can't address all memory existing in the
>>> system?
>>
>> There's a very subtle misunderstanding there - the DMA mask does not
>> describe the memory a device can address, it describes the range of
>> addresses the device is capable of generating. Yes, in the non-IOMMU
>> case they are equivalent, but once you put an IOMMU in between, the
>> problem is merely shifted from "what range of physical addresses can
>> this device access" to "what range of IOVAs is valid to give to this
>> device" - the fact that those IOVAs can map to any underlying physical
>> address only obviates the need for any bouncing at the memory end; it
>> doesn't remove the fact that the device has a hardware addressing
>> limitation which needs to be accommodated.
>>
>> The thread Will linked to describes that equivalent version of your
>> problem - the IOMMU gives the device 48-bit addresses which get
>> erroneously truncated because it doesn't know that only 42 bits are
>> actually wired up. That situation still requires the device's DMA mask
>> to correctly describe its addressing capability just as yours does.
> 
> That problem should only impact virtual machines which have a guest
> bus address space covering more than 42 bits of physical RAM, whereas
> the problem we have with swiotlb is for the dma-mapping interface.

As above, it impacts DMA API use for anything whose addressing
capability is narrower than the IOMMU's reported input size and whose
driver is able to blindly set a too-big DMA mask. It just happens to be
the case that the stars line up on most systems, and for 32-bit devices
who keep the default DMA mask.

I actually have a third variation of this problem involving a PCI root
complex which *could* drive full-width (40-bit) addresses, but won't,
due to the way its PCI<->AXI interface is programmed. That would require
even more complicated dma-ranges handling to describe the windows of
valid physical addresses which it *will* pass, so I'm not pressing the
issue - let's just get the basic DMA mask case fixed first.

>>> With this direction, semantics of dma mask becomes even more
>>> questionable. I'd say dma_mask is candidate for removal (or to move to
>>> swiotlb's or iommu's local area)
>>
>> We still need a way for drivers to communicate a device's probed
>> addressing capability to SWIOTLB, so there's always going to have to be
>> *some* sort of public interface. Personally, the change in semantics I'd
>> like to see is to make dma_set_mask() only fail if DMA is entirely
>> disallowed - in the normal case it would always succeed, but the DMA API
>> implementation would be permitted to set a smaller mask than requested
>> (this is effectively what the x86 IOMMU ops do already).
> 
> With swiotlb enabled, it only needs to fail if the mask does not contain
> the swiotlb bounce buffer area, either because the start of RAM is outside
> of the mask, or the bounce area has been allocated at the end of ZONE_DMA
> and the mask is smaller than ZONE_DMA.

Agreed, I'd managed to overlook that specific case, but I'd be inclined
to consider "impossible" a subset of "disallowed" still :)

Robin.

[PATCH v2] arm64: do not set dma masks that device connection can't handle

[Robin Murphy]

On 10/01/17 13:42, Arnd Bergmann wrote:
> On Tuesday, January 10, 2017 1:25:12 PM CET Robin Murphy wrote:
>> On 10/01/17 12:47, Nikita Yushchenko wrote:
>>>> The point here is that an IOMMU doesn't solve your issue, and the
>>>> IOMMU-backed DMA ops need the same treatment. In light of that, it really
>>>> feels to me like the DMA masks should be restricted in of_dma_configure
>>>> so that the parent mask is taken into account there, rather than hook
>>>> into each set of DMA ops to intercept set_dma_mask. We'd still need to
>>>> do something to stop dma_set_mask widening the mask if it was restricted
>>>> by of_dma_configure, but I think Robin (cc'd) was playing with that.
>>>
>>> What issue "IOMMU doesn't solve"?
>>>
>>> Issue I'm trying to address is - inconsistency within swiotlb
>>> dma_map_ops, where (1) any wide mask is silently accepted, but (2) then
>>> mask is used to decide if bounce buffers are needed or not. This
>>> inconsistency causes NVMe+R-Car cobmo not working (and breaking memory
>>> instead).
>>
>> The fundamental underlying problem is the "any wide mask is silently
>> accepted" part, and that applies equally to IOMMU ops as well.
> 
> It's a much rarer problem for the IOMMU case though, because it only
> impacts devices that are restricted to addressing of less than 32-bits.
> 
> If you have an IOMMU enabled, the dma-mapping interface does not care
> if the device can do wider than 32 bit addressing, as it will never
> hand out IOVAs above 0xffffffff.

I can assure you that it will - we constrain allocations to the
intersection of the IOMMU domain aperture (normally the IOMMU's physical
input address width) and the given device's DMA mask. If both of those
are >32 bits then >32-bit IOVAs will fall out. For the arm64/common
implementation I have prototyped a copy of the x86 optimisation which
always first tries to get 32-bit IOVAs for PCI devices, but even then it
can start returning higher addresses if the 32-bit space fills up.

>>> I just can't think out what similar issue iommu can have.
>>> Do you mean that in iommu case, mask also must not be set to whatever
>>> wider than initial value? Why? What is the use of mask in iommu case? Is
>>> there any real case when iommu can't address all memory existing in the
>>> system?
>>
>> There's a very subtle misunderstanding there - the DMA mask does not
>> describe the memory a device can address, it describes the range of
>> addresses the device is capable of generating. Yes, in the non-IOMMU
>> case they are equivalent, but once you put an IOMMU in between, the
>> problem is merely shifted from "what range of physical addresses can
>> this device access" to "what range of IOVAs is valid to give to this
>> device" - the fact that those IOVAs can map to any underlying physical
>> address only obviates the need for any bouncing at the memory end; it
>> doesn't remove the fact that the device has a hardware addressing
>> limitation which needs to be accommodated.
>>
>> The thread Will linked to describes that equivalent version of your
>> problem - the IOMMU gives the device 48-bit addresses which get
>> erroneously truncated because it doesn't know that only 42 bits are
>> actually wired up. That situation still requires the device's DMA mask
>> to correctly describe its addressing capability just as yours does.
> 
> That problem should only impact virtual machines which have a guest
> bus address space covering more than 42 bits of physical RAM, whereas
> the problem we have with swiotlb is for the dma-mapping interface.

As above, it impacts DMA API use for anything whose addressing
capability is narrower than the IOMMU's reported input size and whose
driver is able to blindly set a too-big DMA mask. It just happens to be
the case that the stars line up on most systems, and for 32-bit devices
who keep the default DMA mask.

I actually have a third variation of this problem involving a PCI root
complex which *could* drive full-width (40-bit) addresses, but won't,
due to the way its PCI<->AXI interface is programmed. That would require
even more complicated dma-ranges handling to describe the windows of
valid physical addresses which it *will* pass, so I'm not pressing the
issue - let's just get the basic DMA mask case fixed first.

>>> With this direction, semantics of dma mask becomes even more
>>> questionable. I'd say dma_mask is candidate for removal (or to move to
>>> swiotlb's or iommu's local area)
>>
>> We still need a way for drivers to communicate a device's probed
>> addressing capability to SWIOTLB, so there's always going to have to be
>> *some* sort of public interface. Personally, the change in semantics I'd
>> like to see is to make dma_set_mask() only fail if DMA is entirely
>> disallowed - in the normal case it would always succeed, but the DMA API
>> implementation would be permitted to set a smaller mask than requested
>> (this is effectively what the x86 IOMMU ops do already).
> 
> With swiotlb enabled, it only needs to fail if the mask does not contain
> the swiotlb bounce buffer area, either because the start of RAM is outside
> of the mask, or the bounce area has been allocated at the end of ZONE_DMA
> and the mask is smaller than ZONE_DMA.

Agreed, I'd managed to overlook that specific case, but I'd be inclined
to consider "impossible" a subset of "disallowed" still :)

Robin.

Re: [PATCH v2] arm64: do not set dma masks that device connection can't handle

[Christoph Hellwig]

On Tue, Jan 10, 2017 at 03:47:25PM +0300, Nikita Yushchenko wrote:
> With this direction, semantics of dma mask becomes even more
> questionable. I'd say dma_mask is candidate for removal (or to move to
> swiotlb's or iommu's local area)

We need the dma mask so that the device can advertise what addresses
the device supports.  Many old devices only support 32-bit DMA addressing,
and some less common ones just 24-bit or other weird ones.

[PATCH v2] arm64: do not set dma masks that device connection can't handle

[Christoph Hellwig]

On Tue, Jan 10, 2017 at 03:47:25PM +0300, Nikita Yushchenko wrote:
> With this direction, semantics of dma mask becomes even more
> questionable. I'd say dma_mask is candidate for removal (or to move to
> swiotlb's or iommu's local area)

We need the dma mask so that the device can advertise what addresses
the device supports.  Many old devices only support 32-bit DMA addressing,
and some less common ones just 24-bit or other weird ones.

Re: [PATCH v2] arm64: do not set dma masks that device connection can't handle

[Christoph Hellwig]

On Tue, Jan 10, 2017 at 01:25:12PM +0000, Robin Murphy wrote:
> We still need a way for drivers to communicate a device's probed
> addressing capability to SWIOTLB, so there's always going to have to be
> *some* sort of public interface. Personally, the change in semantics I'd
> like to see is to make dma_set_mask() only fail if DMA is entirely
> disallowed - in the normal case it would always succeed, but the DMA API
> implementation would be permitted to set a smaller mask than requested
> (this is effectively what the x86 IOMMU ops do already).

Yes, this sounds reasonable.

> The significant
> work that would require, though, is changing all the drivers currently
> using this sort of pattern:
> 
> 	if (!dma_set_mask(dev, DMA_BIT_MASK(64))
> 		/* put device into 64-bit mode */
> 	else if (!dma_set_mask(dev, DMA_BIT_MASK(32))
> 		/* put device into 32-bit mode */
> 	else
> 		/* error */

While we have this pattern in a lot of places it's already rather
pointless on most architectures as the first dma_set_mask call
won't ever fail for the most common dma_ops implementations.

> to something like this:
> 
> 	if (!dma_set_mask(dev, DMA_BIT_MASK(64))
> 		/* error */
> 	if (dma_get_mask(dev) > DMA_BIT_MASK(32))
> 		/* put device into 64-bit mode */
> 	else
> 		/* put device into 32-bit mode */
> 
> Which would be a pretty major job.

I don't think it's too bad.  Also for many modern devices there is no
need to put the device into a specific mode.  It's mostly a historic
issue from the PCI/PCI-X days with the less efficient DAC addressing
scheme.

[PATCH v2] arm64: do not set dma masks that device connection can't handle

[Christoph Hellwig]

On Tue, Jan 10, 2017 at 01:25:12PM +0000, Robin Murphy wrote:
> We still need a way for drivers to communicate a device's probed
> addressing capability to SWIOTLB, so there's always going to have to be
> *some* sort of public interface. Personally, the change in semantics I'd
> like to see is to make dma_set_mask() only fail if DMA is entirely
> disallowed - in the normal case it would always succeed, but the DMA API
> implementation would be permitted to set a smaller mask than requested
> (this is effectively what the x86 IOMMU ops do already).

Yes, this sounds reasonable.

> The significant
> work that would require, though, is changing all the drivers currently
> using this sort of pattern:
> 
> 	if (!dma_set_mask(dev, DMA_BIT_MASK(64))
> 		/* put device into 64-bit mode */
> 	else if (!dma_set_mask(dev, DMA_BIT_MASK(32))
> 		/* put device into 32-bit mode */
> 	else
> 		/* error */

While we have this pattern in a lot of places it's already rather
pointless on most architectures as the first dma_set_mask call
won't ever fail for the most common dma_ops implementations.

> to something like this:
> 
> 	if (!dma_set_mask(dev, DMA_BIT_MASK(64))
> 		/* error */
> 	if (dma_get_mask(dev) > DMA_BIT_MASK(32))
> 		/* put device into 64-bit mode */
> 	else
> 		/* put device into 32-bit mode */
> 
> Which would be a pretty major job.

I don't think it's too bad.  Also for many modern devices there is no
need to put the device into a specific mode.  It's mostly a historic
issue from the PCI/PCI-X days with the less efficient DAC addressing
scheme.

Re: [PATCH v2] arm64: do not set dma masks that device connection can't handle

[Christoph Hellwig]

On Tue, Jan 10, 2017 at 02:42:23PM +0100, Arnd Bergmann wrote:
> It's a much rarer problem for the IOMMU case though, because it only
> impacts devices that are restricted to addressing of less than 32-bits.
> 
> If you have an IOMMU enabled, the dma-mapping interface does not care
> if the device can do wider than 32 bit addressing, as it will never
> hand out IOVAs above 0xffffffff.

That's absolutely not the case.  IOMMUs can and do generate addresses
larger than 32-bit.  Also various platforms have modes where an IOMMU
can be used when <= 32-bit addresses are used and bypassed if full 64-bit
addressing is supported and I/O isolation is not explicitly requested.

[PATCH v2] arm64: do not set dma masks that device connection can't handle

[Christoph Hellwig]

On Tue, Jan 10, 2017 at 02:42:23PM +0100, Arnd Bergmann wrote:
> It's a much rarer problem for the IOMMU case though, because it only
> impacts devices that are restricted to addressing of less than 32-bits.
> 
> If you have an IOMMU enabled, the dma-mapping interface does not care
> if the device can do wider than 32 bit addressing, as it will never
> hand out IOVAs above 0xffffffff.

That's absolutely not the case.  IOMMUs can and do generate addresses
larger than 32-bit.  Also various platforms have modes where an IOMMU
can be used when <= 32-bit addresses are used and bypassed if full 64-bit
addressing is supported and I/O isolation is not explicitly requested.

Re: [PATCH v2] arm64: do not set dma masks that device connection can't handle

[Arnd Bergmann]

On Tuesday, January 10, 2017 2:16:57 PM CET Robin Murphy wrote:
> On 10/01/17 13:42, Arnd Bergmann wrote:
> > On Tuesday, January 10, 2017 1:25:12 PM CET Robin Murphy wrote:
> >> On 10/01/17 12:47, Nikita Yushchenko wrote:
> >>>> The point here is that an IOMMU doesn't solve your issue, and the
> >>>> IOMMU-backed DMA ops need the same treatment. In light of that, it really
> >>>> feels to me like the DMA masks should be restricted in of_dma_configure
> >>>> so that the parent mask is taken into account there, rather than hook
> >>>> into each set of DMA ops to intercept set_dma_mask. We'd still need to
> >>>> do something to stop dma_set_mask widening the mask if it was restricted
> >>>> by of_dma_configure, but I think Robin (cc'd) was playing with that.
> >>>
> >>> What issue "IOMMU doesn't solve"?
> >>>
> >>> Issue I'm trying to address is - inconsistency within swiotlb
> >>> dma_map_ops, where (1) any wide mask is silently accepted, but (2) then
> >>> mask is used to decide if bounce buffers are needed or not. This
> >>> inconsistency causes NVMe+R-Car cobmo not working (and breaking memory
> >>> instead).
> >>
> >> The fundamental underlying problem is the "any wide mask is silently
> >> accepted" part, and that applies equally to IOMMU ops as well.
> > 
> > It's a much rarer problem for the IOMMU case though, because it only
> > impacts devices that are restricted to addressing of less than 32-bits.
> > 
> > If you have an IOMMU enabled, the dma-mapping interface does not care
> > if the device can do wider than 32 bit addressing, as it will never
> > hand out IOVAs above 0xffffffff.
> 
> I can assure you that it will - we constrain allocations to the
> intersection of the IOMMU domain aperture (normally the IOMMU's physical
> input address width) and the given device's DMA mask. If both of those
> are >32 bits then >32-bit IOVAs will fall out. For the arm64/common
> implementation I have prototyped a copy of the x86 optimisation which
> always first tries to get 32-bit IOVAs for PCI devices, but even then it
> can start returning higher addresses if the 32-bit space fills up.

Ok, got it. I have to admit that most of my knowledge about the internals
of IOMMUs is from PowerPC of a few years ago, which couldn't do this at
all. I agree that we need to do the same thing on swiotlb and iommu then.

> >> The thread Will linked to describes that equivalent version of your
> >> problem - the IOMMU gives the device 48-bit addresses which get
> >> erroneously truncated because it doesn't know that only 42 bits are
> >> actually wired up. That situation still requires the device's DMA mask
> >> to correctly describe its addressing capability just as yours does.
> > 
> > That problem should only impact virtual machines which have a guest
> > bus address space covering more than 42 bits of physical RAM, whereas
> > the problem we have with swiotlb is for the dma-mapping interface.
> > 
> I actually have a third variation of this problem involving a PCI root
> complex which *could* drive full-width (40-bit) addresses, but won't,
> due to the way its PCI<->AXI interface is programmed. That would require
> even more complicated dma-ranges handling to describe the windows of
> valid physical addresses which it *will* pass, so I'm not pressing the
> issue - let's just get the basic DMA mask case fixed first.

Can you describe this a little more? We should at least try to not
make it harder to solve the next problem while solving this one,
so I'd like to understand the exact limitation you are hitting there.

	Arnd

[PATCH v2] arm64: do not set dma masks that device connection can't handle

[Arnd Bergmann]

On Tuesday, January 10, 2017 2:16:57 PM CET Robin Murphy wrote:
> On 10/01/17 13:42, Arnd Bergmann wrote:
> > On Tuesday, January 10, 2017 1:25:12 PM CET Robin Murphy wrote:
> >> On 10/01/17 12:47, Nikita Yushchenko wrote:
> >>>> The point here is that an IOMMU doesn't solve your issue, and the
> >>>> IOMMU-backed DMA ops need the same treatment. In light of that, it really
> >>>> feels to me like the DMA masks should be restricted in of_dma_configure
> >>>> so that the parent mask is taken into account there, rather than hook
> >>>> into each set of DMA ops to intercept set_dma_mask. We'd still need to
> >>>> do something to stop dma_set_mask widening the mask if it was restricted
> >>>> by of_dma_configure, but I think Robin (cc'd) was playing with that.
> >>>
> >>> What issue "IOMMU doesn't solve"?
> >>>
> >>> Issue I'm trying to address is - inconsistency within swiotlb
> >>> dma_map_ops, where (1) any wide mask is silently accepted, but (2) then
> >>> mask is used to decide if bounce buffers are needed or not. This
> >>> inconsistency causes NVMe+R-Car cobmo not working (and breaking memory
> >>> instead).
> >>
> >> The fundamental underlying problem is the "any wide mask is silently
> >> accepted" part, and that applies equally to IOMMU ops as well.
> > 
> > It's a much rarer problem for the IOMMU case though, because it only
> > impacts devices that are restricted to addressing of less than 32-bits.
> > 
> > If you have an IOMMU enabled, the dma-mapping interface does not care
> > if the device can do wider than 32 bit addressing, as it will never
> > hand out IOVAs above 0xffffffff.
> 
> I can assure you that it will - we constrain allocations to the
> intersection of the IOMMU domain aperture (normally the IOMMU's physical
> input address width) and the given device's DMA mask. If both of those
> are >32 bits then >32-bit IOVAs will fall out. For the arm64/common
> implementation I have prototyped a copy of the x86 optimisation which
> always first tries to get 32-bit IOVAs for PCI devices, but even then it
> can start returning higher addresses if the 32-bit space fills up.

Ok, got it. I have to admit that most of my knowledge about the internals
of IOMMUs is from PowerPC of a few years ago, which couldn't do this at
all. I agree that we need to do the same thing on swiotlb and iommu then.

> >> The thread Will linked to describes that equivalent version of your
> >> problem - the IOMMU gives the device 48-bit addresses which get
> >> erroneously truncated because it doesn't know that only 42 bits are
> >> actually wired up. That situation still requires the device's DMA mask
> >> to correctly describe its addressing capability just as yours does.
> > 
> > That problem should only impact virtual machines which have a guest
> > bus address space covering more than 42 bits of physical RAM, whereas
> > the problem we have with swiotlb is for the dma-mapping interface.
> > 
> I actually have a third variation of this problem involving a PCI root
> complex which *could* drive full-width (40-bit) addresses, but won't,
> due to the way its PCI<->AXI interface is programmed. That would require
> even more complicated dma-ranges handling to describe the windows of
> valid physical addresses which it *will* pass, so I'm not pressing the
> issue - let's just get the basic DMA mask case fixed first.

Can you describe this a little more? We should at least try to not
make it harder to solve the next problem while solving this one,
so I'd like to understand the exact limitation you are hitting there.

	Arnd

Re: [PATCH] arm64: avoid increasing DMA masks above what hardware supports

[Robin Murphy]

On 10/01/17 14:00, Nikita Yushchenko wrote:
> There are cases when device supports wide DMA addresses wider than
> device's connection supports.
> 
> In this case driver sets DMA mask based on knowledge of device
> capabilities. That must succeed to allow drivers to initialize.
> 
> However, swiotlb or iommu still need knowledge about actual device
> capabilities. To avoid breakage, actual mask must not be set wider than
> device connection allows.
> 
> Signed-off-by: Nikita Yushchenko <nikita.yoush@cogentembedded.com>
> CC: Arnd Bergmann <arnd@arndb.de>
> CC: Robin Murphy <robin.murphy@arm.com>
> CC: Will Deacon <will.deacon@arm.com>
> ---
>  arch/arm64/Kconfig                   |  3 +++
>  arch/arm64/include/asm/device.h      |  1 +
>  arch/arm64/include/asm/dma-mapping.h |  3 +++
>  arch/arm64/mm/dma-mapping.c          | 43 ++++++++++++++++++++++++++++++++++++
>  4 files changed, 50 insertions(+)
> 
> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> index 1117421..afb2c08 100644
> --- a/arch/arm64/Kconfig
> +++ b/arch/arm64/Kconfig
> @@ -216,6 +216,9 @@ config NEED_DMA_MAP_STATE
>  config NEED_SG_DMA_LENGTH
>  	def_bool y
>  
> +config ARCH_HAS_DMA_SET_COHERENT_MASK
> +	def_bool y
> +
>  config SMP
>  	def_bool y
>  
> diff --git a/arch/arm64/include/asm/device.h b/arch/arm64/include/asm/device.h
> index 243ef25..a57e7bb 100644
> --- a/arch/arm64/include/asm/device.h
> +++ b/arch/arm64/include/asm/device.h
> @@ -22,6 +22,7 @@ struct dev_archdata {
>  	void *iommu;			/* private IOMMU data */
>  #endif
>  	bool dma_coherent;
> +	u64 parent_dma_mask;
>  };
>  
>  struct pdev_archdata {
> diff --git a/arch/arm64/include/asm/dma-mapping.h b/arch/arm64/include/asm/dma-mapping.h
> index ccea82c..eab36d2 100644
> --- a/arch/arm64/include/asm/dma-mapping.h
> +++ b/arch/arm64/include/asm/dma-mapping.h
> @@ -51,6 +51,9 @@ void arch_setup_dma_ops(struct device *dev, u64 dma_base, u64 size,
>  			const struct iommu_ops *iommu, bool coherent);
>  #define arch_setup_dma_ops	arch_setup_dma_ops
>  
> +#define HAVE_ARCH_DMA_SET_MASK 1
> +extern int dma_set_mask(struct device *dev, u64 dma_mask);
> +
>  #ifdef CONFIG_IOMMU_DMA
>  void arch_teardown_dma_ops(struct device *dev);
>  #define arch_teardown_dma_ops	arch_teardown_dma_ops
> diff --git a/arch/arm64/mm/dma-mapping.c b/arch/arm64/mm/dma-mapping.c
> index e040827..7b1bb87 100644
> --- a/arch/arm64/mm/dma-mapping.c
> +++ b/arch/arm64/mm/dma-mapping.c
> @@ -203,6 +203,37 @@ static void __dma_free(struct device *dev, size_t size,
>  	__dma_free_coherent(dev, size, swiotlb_addr, dma_handle, attrs);
>  }
>  
> +int dma_set_mask(struct device *dev, u64 dma_mask)
> +{
> +	const struct dma_map_ops *ops = get_dma_ops(dev);
> +
> +	if (mask > dev->archdata.parent_dma_mask)
> +		mask = dev->archdata.parent_dma_mask;
> +
> +	if (ops->set_dma_mask)
> +		return ops->set_dma_mask(dev, mask);
> +
> +	if (!dev->dma_mask || !dma_supported(dev, mask))
> +		return -EIO;
> +
> +	*dev->dma_mask = mask;
> +	return 0;
> +}
> +EXPORT_SYMBOL(dma_set_mask);
> +
> +int dma_set_coherent_mask(struct device *dev, u64 mask)
> +{
> +	if (mask > dev->archdata.parent_dma_mask)
> +		mask = dev->archdata.parent_dma_mask;
> +
> +	if (!dma_supported(dev, mask))
> +		return -EIO;
> +
> +	dev->coherent_dma_mask = mask;
> +	return 0;
> +}
> +EXPORT_SYMBOL(dma_set_coherent_mask);
> +
>  static dma_addr_t __swiotlb_map_page(struct device *dev, struct page *page,
>  				     unsigned long offset, size_t size,
>  				     enum dma_data_direction dir,
> @@ -958,6 +989,18 @@ void arch_setup_dma_ops(struct device *dev, u64 dma_base, u64 size,
>  	if (!dev->archdata.dma_ops)
>  		dev->archdata.dma_ops = &swiotlb_dma_ops;
>  
> +	/*
> +	 * we don't yet support buses that have a non-zero mapping.
> +	 *  Let's hope we won't need it
> +	 */
> +	WARN_ON(dma_base != 0);

I believe we now accomodate the bus remap bits on BCM2837 as a DMA
offset, so unfortunately I think this is no longer true.

> +	/*
> +	 * Whatever the parent bus can set. A device must not set
> +	 * a DMA mask larger than this.
> +	 */
> +	dev->archdata.parent_dma_mask = size - 1;

This will effectively constrain *all* DMA masks to be 32-bit, since for
99% of devices we're going to see a size derived from the default mask
passed in here. I worry that that's liable to lead to performance and
stability regressions (now that the block layer can apparently generate
sufficient readahead to ovflow a typical SWIOTLB bounce buffer[1]).
Whilst DT users would be able to mitigate that by putting explicit
"dma-ranges" properties on every device node, it's less clear what we'd
do for ACPI.

I reckon the easiest way forward would be to pass in some flag to
arch_setup_dma_ops to indicate whether it's an explicitly-configured
range or not - then simply initialising parent_dma_mask to ~0 for the
default case *should* keep things working as before.

Robin.

[1]:https://www.mail-archive.com/virtualization@lists.linux-foundation.org/msg26532.html

> +
>  	dev->archdata.dma_coherent = coherent;
>  	__iommu_setup_dma_ops(dev, dma_base, size, iommu);
>  }
> 

[PATCH] arm64: avoid increasing DMA masks above what hardware supports

[Robin Murphy]

On 10/01/17 14:00, Nikita Yushchenko wrote:
> There are cases when device supports wide DMA addresses wider than
> device's connection supports.
> 
> In this case driver sets DMA mask based on knowledge of device
> capabilities. That must succeed to allow drivers to initialize.
> 
> However, swiotlb or iommu still need knowledge about actual device
> capabilities. To avoid breakage, actual mask must not be set wider than
> device connection allows.
> 
> Signed-off-by: Nikita Yushchenko <nikita.yoush@cogentembedded.com>
> CC: Arnd Bergmann <arnd@arndb.de>
> CC: Robin Murphy <robin.murphy@arm.com>
> CC: Will Deacon <will.deacon@arm.com>
> ---
>  arch/arm64/Kconfig                   |  3 +++
>  arch/arm64/include/asm/device.h      |  1 +
>  arch/arm64/include/asm/dma-mapping.h |  3 +++
>  arch/arm64/mm/dma-mapping.c          | 43 ++++++++++++++++++++++++++++++++++++
>  4 files changed, 50 insertions(+)
> 
> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> index 1117421..afb2c08 100644
> --- a/arch/arm64/Kconfig
> +++ b/arch/arm64/Kconfig
> @@ -216,6 +216,9 @@ config NEED_DMA_MAP_STATE
>  config NEED_SG_DMA_LENGTH
>  	def_bool y
>  
> +config ARCH_HAS_DMA_SET_COHERENT_MASK
> +	def_bool y
> +
>  config SMP
>  	def_bool y
>  
> diff --git a/arch/arm64/include/asm/device.h b/arch/arm64/include/asm/device.h
> index 243ef25..a57e7bb 100644
> --- a/arch/arm64/include/asm/device.h
> +++ b/arch/arm64/include/asm/device.h
> @@ -22,6 +22,7 @@ struct dev_archdata {
>  	void *iommu;			/* private IOMMU data */
>  #endif
>  	bool dma_coherent;
> +	u64 parent_dma_mask;
>  };
>  
>  struct pdev_archdata {
> diff --git a/arch/arm64/include/asm/dma-mapping.h b/arch/arm64/include/asm/dma-mapping.h
> index ccea82c..eab36d2 100644
> --- a/arch/arm64/include/asm/dma-mapping.h
> +++ b/arch/arm64/include/asm/dma-mapping.h
> @@ -51,6 +51,9 @@ void arch_setup_dma_ops(struct device *dev, u64 dma_base, u64 size,
>  			const struct iommu_ops *iommu, bool coherent);
>  #define arch_setup_dma_ops	arch_setup_dma_ops
>  
> +#define HAVE_ARCH_DMA_SET_MASK 1
> +extern int dma_set_mask(struct device *dev, u64 dma_mask);
> +
>  #ifdef CONFIG_IOMMU_DMA
>  void arch_teardown_dma_ops(struct device *dev);
>  #define arch_teardown_dma_ops	arch_teardown_dma_ops
> diff --git a/arch/arm64/mm/dma-mapping.c b/arch/arm64/mm/dma-mapping.c
> index e040827..7b1bb87 100644
> --- a/arch/arm64/mm/dma-mapping.c
> +++ b/arch/arm64/mm/dma-mapping.c
> @@ -203,6 +203,37 @@ static void __dma_free(struct device *dev, size_t size,
>  	__dma_free_coherent(dev, size, swiotlb_addr, dma_handle, attrs);
>  }
>  
> +int dma_set_mask(struct device *dev, u64 dma_mask)
> +{
> +	const struct dma_map_ops *ops = get_dma_ops(dev);
> +
> +	if (mask > dev->archdata.parent_dma_mask)
> +		mask = dev->archdata.parent_dma_mask;
> +
> +	if (ops->set_dma_mask)
> +		return ops->set_dma_mask(dev, mask);
> +
> +	if (!dev->dma_mask || !dma_supported(dev, mask))
> +		return -EIO;
> +
> +	*dev->dma_mask = mask;
> +	return 0;
> +}
> +EXPORT_SYMBOL(dma_set_mask);
> +
> +int dma_set_coherent_mask(struct device *dev, u64 mask)
> +{
> +	if (mask > dev->archdata.parent_dma_mask)
> +		mask = dev->archdata.parent_dma_mask;
> +
> +	if (!dma_supported(dev, mask))
> +		return -EIO;
> +
> +	dev->coherent_dma_mask = mask;
> +	return 0;
> +}
> +EXPORT_SYMBOL(dma_set_coherent_mask);
> +
>  static dma_addr_t __swiotlb_map_page(struct device *dev, struct page *page,
>  				     unsigned long offset, size_t size,
>  				     enum dma_data_direction dir,
> @@ -958,6 +989,18 @@ void arch_setup_dma_ops(struct device *dev, u64 dma_base, u64 size,
>  	if (!dev->archdata.dma_ops)
>  		dev->archdata.dma_ops = &swiotlb_dma_ops;
>  
> +	/*
> +	 * we don't yet support buses that have a non-zero mapping.
> +	 *  Let's hope we won't need it
> +	 */
> +	WARN_ON(dma_base != 0);

I believe we now accomodate the bus remap bits on BCM2837 as a DMA
offset, so unfortunately I think this is no longer true.

> +	/*
> +	 * Whatever the parent bus can set. A device must not set
> +	 * a DMA mask larger than this.
> +	 */
> +	dev->archdata.parent_dma_mask = size - 1;

This will effectively constrain *all* DMA masks to be 32-bit, since for
99% of devices we're going to see a size derived from the default mask
passed in here. I worry that that's liable to lead to performance and
stability regressions (now that the block layer can apparently generate
sufficient readahead to ovflow a typical SWIOTLB bounce buffer[1]).
Whilst DT users would be able to mitigate that by putting explicit
"dma-ranges" properties on every device node, it's less clear what we'd
do for ACPI.

I reckon the easiest way forward would be to pass in some flag to
arch_setup_dma_ops to indicate whether it's an explicitly-configured
range or not - then simply initialising parent_dma_mask to ~0 for the
default case *should* keep things working as before.

Robin.

[1]:https://www.mail-archive.com/virtualization at lists.linux-foundation.org/msg26532.html

> +
>  	dev->archdata.dma_coherent = coherent;
>  	__iommu_setup_dma_ops(dev, dma_base, size, iommu);
>  }
> 

Re: [PATCH] arm64: avoid increasing DMA masks above what hardware supports

[Nikita Yushchenko]

>> +	/*
>> +	 * we don't yet support buses that have a non-zero mapping.
>> +	 *  Let's hope we won't need it
>> +	 */
>> +	WARN_ON(dma_base != 0);
> 
> I believe we now accomodate the bus remap bits on BCM2837 as a DMA
> offset, so unfortunately I think this is no longer true.

Arnd, this check is from you. Any updates? Perhaps this check can be
just dropped?

In swiotlb code, dma address (i.e. with offset already applied) is
checked against mask.  Not sure what 'dma_base' means in iommu case.

>> +	/*
>> +	 * Whatever the parent bus can set. A device must not set
>> +	 * a DMA mask larger than this.
>> +	 */
>> +	dev->archdata.parent_dma_mask = size - 1;
> 
> This will effectively constrain *all* DMA masks to be 32-bit, since for
> 99% of devices we're going to see a size derived from the default mask
> passed in here. I worry that that's liable to lead to performance and
> stability regressions

That was exactly my concern when I first tried to address this issue. My
first attempt was to alter very locally exact configuration where
problem shows, while ensuring that everything else stays as is. See
https://lkml.org/lkml/2016/12/29/218

But looks like people want a generic solution.

> I reckon the easiest way forward would be to pass in some flag to
> arch_setup_dma_ops to indicate whether it's an explicitly-configured
> range or not - then simply initialising parent_dma_mask to ~0 for the
> default case *should* keep things working as before.

Currently only arm, arm64 and mips define arch_setup_dma_ops().
Mips version only checks 'coherent' argument, 'size' is used only by arm
and arm64.

Maybe move setting the default from caller to callee?
I.e. pass size=0 if no explicit information exists, and let architecture
handle that?

[PATCH] arm64: avoid increasing DMA masks above what hardware supports

[Nikita Yushchenko]

>> +	/*
>> +	 * we don't yet support buses that have a non-zero mapping.
>> +	 *  Let's hope we won't need it
>> +	 */
>> +	WARN_ON(dma_base != 0);
> 
> I believe we now accomodate the bus remap bits on BCM2837 as a DMA
> offset, so unfortunately I think this is no longer true.

Arnd, this check is from you. Any updates? Perhaps this check can be
just dropped?

In swiotlb code, dma address (i.e. with offset already applied) is
checked against mask.  Not sure what 'dma_base' means in iommu case.

>> +	/*
>> +	 * Whatever the parent bus can set. A device must not set
>> +	 * a DMA mask larger than this.
>> +	 */
>> +	dev->archdata.parent_dma_mask = size - 1;
> 
> This will effectively constrain *all* DMA masks to be 32-bit, since for
> 99% of devices we're going to see a size derived from the default mask
> passed in here. I worry that that's liable to lead to performance and
> stability regressions

That was exactly my concern when I first tried to address this issue. My
first attempt was to alter very locally exact configuration where
problem shows, while ensuring that everything else stays as is. See
https://lkml.org/lkml/2016/12/29/218

But looks like people want a generic solution.

> I reckon the easiest way forward would be to pass in some flag to
> arch_setup_dma_ops to indicate whether it's an explicitly-configured
> range or not - then simply initialising parent_dma_mask to ~0 for the
> default case *should* keep things working as before.

Currently only arm, arm64 and mips define arch_setup_dma_ops().
Mips version only checks 'coherent' argument, 'size' is used only by arm
and arm64.

Maybe move setting the default from caller to callee?
I.e. pass size=0 if no explicit information exists, and let architecture
handle that?

Re: [PATCH] arm64: avoid increasing DMA masks above what hardware supports

[Robin Murphy]

On 11/01/17 07:59, Nikita Yushchenko wrote:
>>> +	/*
>>> +	 * we don't yet support buses that have a non-zero mapping.
>>> +	 *  Let's hope we won't need it
>>> +	 */
>>> +	WARN_ON(dma_base != 0);
>>
>> I believe we now accomodate the bus remap bits on BCM2837 as a DMA
>> offset, so unfortunately I think this is no longer true.
> 
> Arnd, this check is from you. Any updates? Perhaps this check can be
> just dropped?
> 
> In swiotlb code, dma address (i.e. with offset already applied) is
> checked against mask.  Not sure what 'dma_base' means in iommu case.
> 
>>> +	/*
>>> +	 * Whatever the parent bus can set. A device must not set
>>> +	 * a DMA mask larger than this.
>>> +	 */
>>> +	dev->archdata.parent_dma_mask = size - 1;
>>
>> This will effectively constrain *all* DMA masks to be 32-bit, since for
>> 99% of devices we're going to see a size derived from the default mask
>> passed in here. I worry that that's liable to lead to performance and
>> stability regressions
> 
> That was exactly my concern when I first tried to address this issue. My
> first attempt was to alter very locally exact configuration where
> problem shows, while ensuring that everything else stays as is. See
> https://lkml.org/lkml/2016/12/29/218
> 
> But looks like people want a generic solution.
> 
>> I reckon the easiest way forward would be to pass in some flag to
>> arch_setup_dma_ops to indicate whether it's an explicitly-configured
>> range or not - then simply initialising parent_dma_mask to ~0 for the
>> default case *should* keep things working as before.
> 
> Currently only arm, arm64 and mips define arch_setup_dma_ops().
> Mips version only checks 'coherent' argument, 'size' is used only by arm
> and arm64.
> 
> Maybe move setting the default from caller to callee?
> I.e. pass size=0 if no explicit information exists, and let architecture
> handle that?

Yes, I think that ought to work, although the __iommu_setup_dma_ops()
call will still want a real size reflecting the default mask, so
something like:

if (size == 0) {
	dev->archdata.parent_dma_mask = ~0;
	size = 1ULL << 32;
} else {
	dev->archdata.parent_dma_mask = size - 1;
}

should probably do the trick.

Robin.

[PATCH] arm64: avoid increasing DMA masks above what hardware supports

[Robin Murphy]

On 11/01/17 07:59, Nikita Yushchenko wrote:
>>> +	/*
>>> +	 * we don't yet support buses that have a non-zero mapping.
>>> +	 *  Let's hope we won't need it
>>> +	 */
>>> +	WARN_ON(dma_base != 0);
>>
>> I believe we now accomodate the bus remap bits on BCM2837 as a DMA
>> offset, so unfortunately I think this is no longer true.
> 
> Arnd, this check is from you. Any updates? Perhaps this check can be
> just dropped?
> 
> In swiotlb code, dma address (i.e. with offset already applied) is
> checked against mask.  Not sure what 'dma_base' means in iommu case.
> 
>>> +	/*
>>> +	 * Whatever the parent bus can set. A device must not set
>>> +	 * a DMA mask larger than this.
>>> +	 */
>>> +	dev->archdata.parent_dma_mask = size - 1;
>>
>> This will effectively constrain *all* DMA masks to be 32-bit, since for
>> 99% of devices we're going to see a size derived from the default mask
>> passed in here. I worry that that's liable to lead to performance and
>> stability regressions
> 
> That was exactly my concern when I first tried to address this issue. My
> first attempt was to alter very locally exact configuration where
> problem shows, while ensuring that everything else stays as is. See
> https://lkml.org/lkml/2016/12/29/218
> 
> But looks like people want a generic solution.
> 
>> I reckon the easiest way forward would be to pass in some flag to
>> arch_setup_dma_ops to indicate whether it's an explicitly-configured
>> range or not - then simply initialising parent_dma_mask to ~0 for the
>> default case *should* keep things working as before.
> 
> Currently only arm, arm64 and mips define arch_setup_dma_ops().
> Mips version only checks 'coherent' argument, 'size' is used only by arm
> and arm64.
> 
> Maybe move setting the default from caller to callee?
> I.e. pass size=0 if no explicit information exists, and let architecture
> handle that?

Yes, I think that ought to work, although the __iommu_setup_dma_ops()
call will still want a real size reflecting the default mask, so
something like:

if (size == 0) {
	dev->archdata.parent_dma_mask = ~0;
	size = 1ULL << 32;
} else {
	dev->archdata.parent_dma_mask = size - 1;
}

should probably do the trick.

Robin.

Re: [PATCH v2] arm64: do not set dma masks that device connection can't handle

[Nikita Yushchenko]

> I actually have a third variation of this problem involving a PCI root
> complex which *could* drive full-width (40-bit) addresses, but won't,
> due to the way its PCI<->AXI interface is programmed. That would require
> even more complicated dma-ranges handling to describe the windows of
> valid physical addresses which it *will* pass, so I'm not pressing the
> issue - let's just get the basic DMA mask case fixed first.

R-Car + NVMe is actually not "basic case".

It has PCI<->AXI interface involved.
PCI addresses are 64-bit and controller does handle 64-bit addresses
there. Mapping between PCI addresses and AXI addresses is defined. But
AXI is 32-bit.

SoC has iommu that probably could be used between PCIe module and RAM.
Although AFAIK nobody made that working yet.

Board I work with has 4G of RAM, in 4 banks, located at different parts
of wide address space, and only one of them is below 4G. But if iommu is
capable of translating addresses such that 4 gigabyte banks map to first
4 gigabytes of address space, then all memory will become available for
DMA from PCIe device.

[PATCH v2] arm64: do not set dma masks that device connection can't handle

[Nikita Yushchenko]

> I actually have a third variation of this problem involving a PCI root
> complex which *could* drive full-width (40-bit) addresses, but won't,
> due to the way its PCI<->AXI interface is programmed. That would require
> even more complicated dma-ranges handling to describe the windows of
> valid physical addresses which it *will* pass, so I'm not pressing the
> issue - let's just get the basic DMA mask case fixed first.

R-Car + NVMe is actually not "basic case".

It has PCI<->AXI interface involved.
PCI addresses are 64-bit and controller does handle 64-bit addresses
there. Mapping between PCI addresses and AXI addresses is defined. But
AXI is 32-bit.

SoC has iommu that probably could be used between PCIe module and RAM.
Although AFAIK nobody made that working yet.

Board I work with has 4G of RAM, in 4 banks, located at different parts
of wide address space, and only one of them is below 4G. But if iommu is
capable of translating addresses such that 4 gigabyte banks map to first
4 gigabytes of address space, then all memory will become available for
DMA from PCIe device.

Re: [PATCH] arm64: avoid increasing DMA masks above what hardware supports

[Nikita Yushchenko]

> Yes, I think that ought to work, although the __iommu_setup_dma_ops()
> call will still want a real size reflecting the default mask

I see iommu_dma_ops do not define set_dma_mask.

So what if setup was done for size reflecting one mask and then driver
changes mask?  Will things still operate correctly?

[PATCH] arm64: avoid increasing DMA masks above what hardware supports

[Nikita Yushchenko]

> Yes, I think that ought to work, although the __iommu_setup_dma_ops()
> call will still want a real size reflecting the default mask

I see iommu_dma_ops do not define set_dma_mask.

So what if setup was done for size reflecting one mask and then driver
changes mask?  Will things still operate correctly?

Re: [PATCH] arm64: avoid increasing DMA masks above what hardware supports

[Robin Murphy]

On 11/01/17 13:41, Nikita Yushchenko wrote:
>> Yes, I think that ought to work, although the __iommu_setup_dma_ops()
>> call will still want a real size reflecting the default mask
> 
> I see iommu_dma_ops do not define set_dma_mask.
> 
> So what if setup was done for size reflecting one mask and then driver
> changes mask?  Will things still operate correctly?

We've overridden dma_set_mask() at the function level, so it should
always apply regardless. Besides, none of the arm64 ops implement
.set_dma_mask anyway, so we could possibly drop the references to it
altogether.

Conversely, I suppose we could just implement said callback for
swiotlb_dma_ops and iommu_dma_ops with the parent_dma_mask-checking
function and drop the HAVE_ARCH_DMA_SET_MASK override instead. I'm not
sure which approach is preferable - the latter seems arguably cleaner in
isolation, but would also be less consistent with how the coherent mask
has to be handled. Ho hum.

Robin.

[PATCH] arm64: avoid increasing DMA masks above what hardware supports

[Robin Murphy]

On 11/01/17 13:41, Nikita Yushchenko wrote:
>> Yes, I think that ought to work, although the __iommu_setup_dma_ops()
>> call will still want a real size reflecting the default mask
> 
> I see iommu_dma_ops do not define set_dma_mask.
> 
> So what if setup was done for size reflecting one mask and then driver
> changes mask?  Will things still operate correctly?

We've overridden dma_set_mask() at the function level, so it should
always apply regardless. Besides, none of the arm64 ops implement
.set_dma_mask anyway, so we could possibly drop the references to it
altogether.

Conversely, I suppose we could just implement said callback for
swiotlb_dma_ops and iommu_dma_ops with the parent_dma_mask-checking
function and drop the HAVE_ARCH_DMA_SET_MASK override instead. I'm not
sure which approach is preferable - the latter seems arguably cleaner in
isolation, but would also be less consistent with how the coherent mask
has to be handled. Ho hum.

Robin.

[PATCH] arm64: avoid increasing DMA masks above what hardware supports

[Nikita Yushchenko]

11.01.2017 17:50, Robin Murphy ?????:
> On 11/01/17 13:41, Nikita Yushchenko wrote:
>>> Yes, I think that ought to work, although the __iommu_setup_dma_ops()
>>> call will still want a real size reflecting the default mask
>>
>> I see iommu_dma_ops do not define set_dma_mask.
>>
>> So what if setup was done for size reflecting one mask and then driver
>> changes mask?  Will things still operate correctly?
> 
> We've overridden dma_set_mask() at the function level, so it should
> always apply regardless. Besides, none of the arm64 ops implement
> .set_dma_mask anyway, so we could possibly drop the references to it
> altogether.
> 
> Conversely, I suppose we could just implement said callback for
> swiotlb_dma_ops and iommu_dma_ops with the parent_dma_mask-checking
> function and drop the HAVE_ARCH_DMA_SET_MASK override instead. I'm not
> sure which approach is preferable - the latter seems arguably cleaner in
> isolation, but would also be less consistent with how the coherent mask
> has to be handled. Ho hum.

I mean, before patch is applied.

In the current mainline codebase, arm64 iommu does setup dependent on
[default] dma_mask, but does not anyhow react on dma mask change.

I don't know much details about arm64 iommu, but from distant view this
combination looks incorrect:
- if behavior of this hardware should depend on dma_mask of device, then
it should handle mask change,
- if behavior of this hardware should not depend on dma_mask of device,
then what for to pass size to it's setup?

Re: [PATCH v2] arm64: do not set dma masks that device connection can't handle

[Arnd Bergmann]

On Wednesday, January 11, 2017 3:37:22 PM CET Nikita Yushchenko wrote:
> > I actually have a third variation of this problem involving a PCI root
> > complex which *could* drive full-width (40-bit) addresses, but won't,
> > due to the way its PCI<->AXI interface is programmed. That would require
> > even more complicated dma-ranges handling to describe the windows of
> > valid physical addresses which it *will* pass, so I'm not pressing the
> > issue - let's just get the basic DMA mask case fixed first.
> 
> R-Car + NVMe is actually not "basic case".
> 
> It has PCI<->AXI interface involved.
> PCI addresses are 64-bit and controller does handle 64-bit addresses
> there. Mapping between PCI addresses and AXI addresses is defined. But
> AXI is 32-bit.
> 
> SoC has iommu that probably could be used between PCIe module and RAM.
> Although AFAIK nobody made that working yet.
> 
> Board I work with has 4G of RAM, in 4 banks, located at different parts
> of wide address space, and only one of them is below 4G. But if iommu is
> capable of translating addresses such that 4 gigabyte banks map to first
> 4 gigabytes of address space, then all memory will become available for
> DMA from PCIe device.

You can in theory handle this by defining your own platform specific
dma_map_ops, as we used to do in the old days. Unfortunately, the modern
way of using the generic IOVA allocation can't handle really it, so it's
unclear if the work that would be necessary to support it (and the long
term maintenance cost) outweigh the benefits.

The more likely option here is to try harder to get the IOMMU working
(or show that it's impossible but make sure the next chip gets it right).

	Arnd

[PATCH v2] arm64: do not set dma masks that device connection can't handle

[Arnd Bergmann]

On Wednesday, January 11, 2017 3:37:22 PM CET Nikita Yushchenko wrote:
> > I actually have a third variation of this problem involving a PCI root
> > complex which *could* drive full-width (40-bit) addresses, but won't,
> > due to the way its PCI<->AXI interface is programmed. That would require
> > even more complicated dma-ranges handling to describe the windows of
> > valid physical addresses which it *will* pass, so I'm not pressing the
> > issue - let's just get the basic DMA mask case fixed first.
> 
> R-Car + NVMe is actually not "basic case".
> 
> It has PCI<->AXI interface involved.
> PCI addresses are 64-bit and controller does handle 64-bit addresses
> there. Mapping between PCI addresses and AXI addresses is defined. But
> AXI is 32-bit.
> 
> SoC has iommu that probably could be used between PCIe module and RAM.
> Although AFAIK nobody made that working yet.
> 
> Board I work with has 4G of RAM, in 4 banks, located at different parts
> of wide address space, and only one of them is below 4G. But if iommu is
> capable of translating addresses such that 4 gigabyte banks map to first
> 4 gigabytes of address space, then all memory will become available for
> DMA from PCIe device.

You can in theory handle this by defining your own platform specific
dma_map_ops, as we used to do in the old days. Unfortunately, the modern
way of using the generic IOVA allocation can't handle really it, so it's
unclear if the work that would be necessary to support it (and the long
term maintenance cost) outweigh the benefits.

The more likely option here is to try harder to get the IOMMU working
(or show that it's impossible but make sure the next chip gets it right).

	Arnd

Re: [PATCH] arm64: avoid increasing DMA masks above what hardware supports

[Robin Murphy]

On 11/01/17 16:03, Nikita Yushchenko wrote:
> 
> 
> 11.01.2017 17:50, Robin Murphy пишет:
>> On 11/01/17 13:41, Nikita Yushchenko wrote:
>>>> Yes, I think that ought to work, although the __iommu_setup_dma_ops()
>>>> call will still want a real size reflecting the default mask
>>>
>>> I see iommu_dma_ops do not define set_dma_mask.
>>>
>>> So what if setup was done for size reflecting one mask and then driver
>>> changes mask?  Will things still operate correctly?
>>
>> We've overridden dma_set_mask() at the function level, so it should
>> always apply regardless. Besides, none of the arm64 ops implement
>> .set_dma_mask anyway, so we could possibly drop the references to it
>> altogether.
>>
>> Conversely, I suppose we could just implement said callback for
>> swiotlb_dma_ops and iommu_dma_ops with the parent_dma_mask-checking
>> function and drop the HAVE_ARCH_DMA_SET_MASK override instead. I'm not
>> sure which approach is preferable - the latter seems arguably cleaner in
>> isolation, but would also be less consistent with how the coherent mask
>> has to be handled. Ho hum.
> 
> I mean, before patch is applied.
> 
> In the current mainline codebase, arm64 iommu does setup dependent on
> [default] dma_mask, but does not anyhow react on dma mask change.
> 
> I don't know much details about arm64 iommu, but from distant view this
> combination looks incorrect:
> - if behavior of this hardware should depend on dma_mask of device, then
> it should handle mask change,
> - if behavior of this hardware should not depend on dma_mask of device,
> then what for to pass size to it's setup?

Ah, right, I did indeed misunderstand. The IOMMU code is happy with the
DMA masks changing, since it always checks the appropriate one at the
point of IOVA allocation. The initial size given to
__iommu_setup_dma_ops() really just sets up a caching threshold in the
IOVA domain - if there is an explicitly-configured mask, then it's
generally good to set the limit based on that, but otherwise the default
32-bit limit is fine even if the driver subsequently alters the device's
mask(s) before making DMA API calls. Your patch won't actually change
any behaviour in this regard, as long as it still passes a 4GB size by
default.

Robin.

[PATCH] arm64: avoid increasing DMA masks above what hardware supports

[Robin Murphy]

On 11/01/17 16:03, Nikita Yushchenko wrote:
> 
> 
> 11.01.2017 17:50, Robin Murphy ?????:
>> On 11/01/17 13:41, Nikita Yushchenko wrote:
>>>> Yes, I think that ought to work, although the __iommu_setup_dma_ops()
>>>> call will still want a real size reflecting the default mask
>>>
>>> I see iommu_dma_ops do not define set_dma_mask.
>>>
>>> So what if setup was done for size reflecting one mask and then driver
>>> changes mask?  Will things still operate correctly?
>>
>> We've overridden dma_set_mask() at the function level, so it should
>> always apply regardless. Besides, none of the arm64 ops implement
>> .set_dma_mask anyway, so we could possibly drop the references to it
>> altogether.
>>
>> Conversely, I suppose we could just implement said callback for
>> swiotlb_dma_ops and iommu_dma_ops with the parent_dma_mask-checking
>> function and drop the HAVE_ARCH_DMA_SET_MASK override instead. I'm not
>> sure which approach is preferable - the latter seems arguably cleaner in
>> isolation, but would also be less consistent with how the coherent mask
>> has to be handled. Ho hum.
> 
> I mean, before patch is applied.
> 
> In the current mainline codebase, arm64 iommu does setup dependent on
> [default] dma_mask, but does not anyhow react on dma mask change.
> 
> I don't know much details about arm64 iommu, but from distant view this
> combination looks incorrect:
> - if behavior of this hardware should depend on dma_mask of device, then
> it should handle mask change,
> - if behavior of this hardware should not depend on dma_mask of device,
> then what for to pass size to it's setup?

Ah, right, I did indeed misunderstand. The IOMMU code is happy with the
DMA masks changing, since it always checks the appropriate one at the
point of IOVA allocation. The initial size given to
__iommu_setup_dma_ops() really just sets up a caching threshold in the
IOVA domain - if there is an explicitly-configured mask, then it's
generally good to set the limit based on that, but otherwise the default
32-bit limit is fine even if the driver subsequently alters the device's
mask(s) before making DMA API calls. Your patch won't actually change
any behaviour in this regard, as long as it still passes a 4GB size by
default.

Robin.

Re: [PATCH v2] arm64: do not set dma masks that device connection can't handle

[Robin Murphy]

On 11/01/17 12:37, Nikita Yushchenko wrote:
>> I actually have a third variation of this problem involving a PCI root
>> complex which *could* drive full-width (40-bit) addresses, but won't,
>> due to the way its PCI<->AXI interface is programmed. That would require
>> even more complicated dma-ranges handling to describe the windows of
>> valid physical addresses which it *will* pass, so I'm not pressing the
>> issue - let's just get the basic DMA mask case fixed first.
> 
> R-Car + NVMe is actually not "basic case".

I meant "basic" in terms of what needs to be done in Linux - simply
preventing device drivers from overwriting the DT-configured DMA mask
will make everything work as well as well as it possibly can on R-Car,
both with or without the IOMMU, since apparently all you need is to
ensure a PCI device never gets given a DMA address above 4GB. The
situation where PCI devices *can* DMA to all of physical memory, but
can't use arbitrary addresses *outside* it - which only becomes a
problem with an IOMMU - is considerably trickier.

> It has PCI<->AXI interface involved.
> PCI addresses are 64-bit and controller does handle 64-bit addresses
> there. Mapping between PCI addresses and AXI addresses is defined. But
> AXI is 32-bit.
> 
> SoC has iommu that probably could be used between PCIe module and RAM.
> Although AFAIK nobody made that working yet.
> 
> Board I work with has 4G of RAM, in 4 banks, located at different parts
> of wide address space, and only one of them is below 4G. But if iommu is
> capable of translating addresses such that 4 gigabyte banks map to first
> 4 gigabytes of address space, then all memory will become available for
> DMA from PCIe device.

The aforementioned situation on Juno is similar yet different - the PLDA
XR3 root complex uses an address-based lookup table to translate
outgoing PCI memory space transactions to AXI bus addresses with the
appropriate attributes, in power-of-two-sized regions. The firmware
configures 3 LUT entries - 2GB at 0x8000_0000 and 8GB at 0x8_8000_0000
with cache-coherent attributes to cover the DRAM areas, plus a small one
with device attributes covering the GICv2m MSI frame. The issue is that
there is no "no match" translation, so any transaction not within one of
those regions never makes it out of the root complex at all.

That's fine in normal operation, as there's nothing outside those
regions in the physical memory map a PCI device should be accessing
anyway, but turning on the SMMU is another matter - since the IOVA
allocator runs top-down, a PCI device with a 64-bit DMA mask will do a
dma_map or dma_alloc, get the physical page mapped to an IOVA up around
FF_FFFF_F000 (the SMMU will constrain things to the system bus width of
40 bits), then try to access that address and get a termination straight
back from the RC. Similarly, A KVM guest which wants to place its memory
at arbitrary locations and expect device passthrough to work is going to
have a bad time.

I don't know if it's feasible to have the firmware set the LUT up
differently, as that might lead to other problems when not using the
SMMU, and/or just require far more than the 8 available LUT entries
(assuming they have to be non-overlapping - I'm not 100% sure and
documentation is sparse). Thus it seems appropriate to describe the
currently valid PCI-AXI translations with dma-ranges, but then we'd have
multiple entries - last time I looked Linux simply ignores all but the
last one in that case - which can't be combined into a simple bitmask,
so I'm not entirely sure where to go from there. Especially as so far it
seems to be a problem exclusive to one not-widely-available ageing
early-access development platform...

It happens that limiting all PCI DMA masks to 32 bits would bodge around
this problem thanks to the current IOVA allocator behaviour, but that's
pretty yuck, and would force unnecessary bouncing for the non-SMMU case.
My other hack to carve up IOVA domains to reserve all addresses not
matching memblocks is hardly any more realistic, hence why the SMMU is
in the Juno DT in a change-it-at-your-own-peril "disabled" state ;)

Robin.

[PATCH v2] arm64: do not set dma masks that device connection can't handle

[Robin Murphy]

On 11/01/17 12:37, Nikita Yushchenko wrote:
>> I actually have a third variation of this problem involving a PCI root
>> complex which *could* drive full-width (40-bit) addresses, but won't,
>> due to the way its PCI<->AXI interface is programmed. That would require
>> even more complicated dma-ranges handling to describe the windows of
>> valid physical addresses which it *will* pass, so I'm not pressing the
>> issue - let's just get the basic DMA mask case fixed first.
> 
> R-Car + NVMe is actually not "basic case".

I meant "basic" in terms of what needs to be done in Linux - simply
preventing device drivers from overwriting the DT-configured DMA mask
will make everything work as well as well as it possibly can on R-Car,
both with or without the IOMMU, since apparently all you need is to
ensure a PCI device never gets given a DMA address above 4GB. The
situation where PCI devices *can* DMA to all of physical memory, but
can't use arbitrary addresses *outside* it - which only becomes a
problem with an IOMMU - is considerably trickier.

> It has PCI<->AXI interface involved.
> PCI addresses are 64-bit and controller does handle 64-bit addresses
> there. Mapping between PCI addresses and AXI addresses is defined. But
> AXI is 32-bit.
> 
> SoC has iommu that probably could be used between PCIe module and RAM.
> Although AFAIK nobody made that working yet.
> 
> Board I work with has 4G of RAM, in 4 banks, located at different parts
> of wide address space, and only one of them is below 4G. But if iommu is
> capable of translating addresses such that 4 gigabyte banks map to first
> 4 gigabytes of address space, then all memory will become available for
> DMA from PCIe device.

The aforementioned situation on Juno is similar yet different - the PLDA
XR3 root complex uses an address-based lookup table to translate
outgoing PCI memory space transactions to AXI bus addresses with the
appropriate attributes, in power-of-two-sized regions. The firmware
configures 3 LUT entries - 2GB at 0x8000_0000 and 8GB at 0x8_8000_0000
with cache-coherent attributes to cover the DRAM areas, plus a small one
with device attributes covering the GICv2m MSI frame. The issue is that
there is no "no match" translation, so any transaction not within one of
those regions never makes it out of the root complex at all.

That's fine in normal operation, as there's nothing outside those
regions in the physical memory map a PCI device should be accessing
anyway, but turning on the SMMU is another matter - since the IOVA
allocator runs top-down, a PCI device with a 64-bit DMA mask will do a
dma_map or dma_alloc, get the physical page mapped to an IOVA up around
FF_FFFF_F000 (the SMMU will constrain things to the system bus width of
40 bits), then try to access that address and get a termination straight
back from the RC. Similarly, A KVM guest which wants to place its memory
at arbitrary locations and expect device passthrough to work is going to
have a bad time.

I don't know if it's feasible to have the firmware set the LUT up
differently, as that might lead to other problems when not using the
SMMU, and/or just require far more than the 8 available LUT entries
(assuming they have to be non-overlapping - I'm not 100% sure and
documentation is sparse). Thus it seems appropriate to describe the
currently valid PCI-AXI translations with dma-ranges, but then we'd have
multiple entries - last time I looked Linux simply ignores all but the
last one in that case - which can't be combined into a simple bitmask,
so I'm not entirely sure where to go from there. Especially as so far it
seems to be a problem exclusive to one not-widely-available ageing
early-access development platform...

It happens that limiting all PCI DMA masks to 32 bits would bodge around
this problem thanks to the current IOVA allocator behaviour, but that's
pretty yuck, and would force unnecessary bouncing for the non-SMMU case.
My other hack to carve up IOVA domains to reserve all addresses not
matching memblocks is hardly any more realistic, hence why the SMMU is
in the Juno DT in a change-it-at-your-own-peril "disabled" state ;)

Robin.

[PATCH 0/2] arm64: fix handling of DMA masks wider than bus supports

[Nikita Yushchenko]

> I reckon the easiest way forward would be to pass in some flag to
> arch_setup_dma_ops to indicate whether it's an explicitly-configured
> range or not - then simply initialising parent_dma_mask to ~0 for the
> default case *should* keep things working as before.

Tried to do that.

---

Nikita Yushchenko (2):
  dma-mapping: let arch know origin of dma range passed to arch_setup_dma_ops()
  arm64: avoid increasing DMA masks above what hardware supports

 arch/arm/include/asm/dma-mapping.h             |  1 +
 arch/arm/mm/dma-mapping.c                      |  3 +-
 arch/arm64/Kconfig                             |  3 ++
 arch/arm64/include/asm/device.h                |  1 +
 arch/arm64/include/asm/dma-mapping.h           |  6 +++-
 arch/arm64/mm/dma-mapping.c                    | 43 +++++++++++++++++++++++++-
 arch/mips/include/asm/dma-mapping.h            |  3 +-
 drivers/acpi/scan.c                            |  2 +-
 drivers/iommu/rockchip-iommu.c                 |  2 +-
 drivers/net/ethernet/freescale/dpaa/dpaa_eth.c |  2 +-
 drivers/of/device.c                            |  5 ++-
 drivers/staging/fsl-mc/bus/fsl-mc-bus.c        |  2 +-
 12 files changed, 64 insertions(+), 9 deletions(-)

-- 
2.1.4

[PATCH 0/2] arm64: fix handling of DMA masks wider than bus supports

[Nikita Yushchenko]

> I reckon the easiest way forward would be to pass in some flag to
> arch_setup_dma_ops to indicate whether it's an explicitly-configured
> range or not - then simply initialising parent_dma_mask to ~0 for the
> default case *should* keep things working as before.

Tried to do that.

---

Nikita Yushchenko (2):
  dma-mapping: let arch know origin of dma range passed to arch_setup_dma_ops()
  arm64: avoid increasing DMA masks above what hardware supports

 arch/arm/include/asm/dma-mapping.h             |  1 +
 arch/arm/mm/dma-mapping.c                      |  3 +-
 arch/arm64/Kconfig                             |  3 ++
 arch/arm64/include/asm/device.h                |  1 +
 arch/arm64/include/asm/dma-mapping.h           |  6 +++-
 arch/arm64/mm/dma-mapping.c                    | 43 +++++++++++++++++++++++++-
 arch/mips/include/asm/dma-mapping.h            |  3 +-
 drivers/acpi/scan.c                            |  2 +-
 drivers/iommu/rockchip-iommu.c                 |  2 +-
 drivers/net/ethernet/freescale/dpaa/dpaa_eth.c |  2 +-
 drivers/of/device.c                            |  5 ++-
 drivers/staging/fsl-mc/bus/fsl-mc-bus.c        |  2 +-
 12 files changed, 64 insertions(+), 9 deletions(-)

-- 
2.1.4

[PATCH 1/2] dma-mapping: let arch know origin of dma range passed to arch_setup_dma_ops()

[Nikita Yushchenko]

There are cases when device is capable of wide DMA mask (and driver
issues corresponding dma_set_mask() call), but bus device sits on can't
support wide address. Example: NVMe device behind PCIe controller
sitting on 32-bit SoC bus.

To support such case, architecture needs information about such
limitations. Such information can originate from dma-ranges property
in device tree, and is passed to architecture via arch_setup_dma_ops()
call.

Problem is that in wide majority of cases, no dma range is defined.
E.g. ACPI has no means to define it. Thus default range (usually
full 32-bit range, i.e. 4G starting at zero address) is passed instead.

If architecture enforce this range, all setups currently using
wide DMA addresses without explicitly defining support for that via
device tree will break. This is bad, especially for ACPI based
platforms.

To avoid that, this patch adds additional boolean argument to
arch_setup_dma_ops() to show if range originates from authorative source
and thus should be enforced, or is just a guess and should be handled as
such.

Signed-off-by: Nikita Yushchenko <nikita.yoush@cogentembedded.com>
---
 arch/arm/include/asm/dma-mapping.h             | 1 +
 arch/arm/mm/dma-mapping.c                      | 3 ++-
 arch/arm64/include/asm/dma-mapping.h           | 3 ++-
 arch/arm64/mm/dma-mapping.c                    | 3 ++-
 arch/mips/include/asm/dma-mapping.h            | 3 ++-
 drivers/acpi/scan.c                            | 2 +-
 drivers/iommu/rockchip-iommu.c                 | 2 +-
 drivers/net/ethernet/freescale/dpaa/dpaa_eth.c | 2 +-
 drivers/of/device.c                            | 5 ++++-
 drivers/staging/fsl-mc/bus/fsl-mc-bus.c        | 2 +-
 10 files changed, 17 insertions(+), 9 deletions(-)

diff --git a/arch/arm/include/asm/dma-mapping.h b/arch/arm/include/asm/dma-mapping.h
index bf02dbd..2a3863e 100644
--- a/arch/arm/include/asm/dma-mapping.h
+++ b/arch/arm/include/asm/dma-mapping.h
@@ -117,6 +117,7 @@ static inline unsigned long dma_max_pfn(struct device *dev)
 
 #define arch_setup_dma_ops arch_setup_dma_ops
 extern void arch_setup_dma_ops(struct device *dev, u64 dma_base, u64 size,
+			       bool enforce_range,
 			       const struct iommu_ops *iommu, bool coherent);
 
 #define arch_teardown_dma_ops arch_teardown_dma_ops
diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c
index ab77100..b8b11f8 100644
--- a/arch/arm/mm/dma-mapping.c
+++ b/arch/arm/mm/dma-mapping.c
@@ -2380,7 +2380,8 @@ static struct dma_map_ops *arm_get_dma_map_ops(bool coherent)
 }
 
 void arch_setup_dma_ops(struct device *dev, u64 dma_base, u64 size,
-			const struct iommu_ops *iommu, bool coherent)
+			bool enforce_range, const struct iommu_ops *iommu,
+			bool coherent)
 {
 	struct dma_map_ops *dma_ops;
 
diff --git a/arch/arm64/include/asm/dma-mapping.h b/arch/arm64/include/asm/dma-mapping.h
index ccea82c..ae1c23f 100644
--- a/arch/arm64/include/asm/dma-mapping.h
+++ b/arch/arm64/include/asm/dma-mapping.h
@@ -48,7 +48,8 @@ static inline struct dma_map_ops *get_dma_ops(struct device *dev)
 }
 
 void arch_setup_dma_ops(struct device *dev, u64 dma_base, u64 size,
-			const struct iommu_ops *iommu, bool coherent);
+			bool enforce_range, const struct iommu_ops *iommu,
+			bool coherent);
 #define arch_setup_dma_ops	arch_setup_dma_ops
 
 #ifdef CONFIG_IOMMU_DMA
diff --git a/arch/arm64/mm/dma-mapping.c b/arch/arm64/mm/dma-mapping.c
index e040827..ea295f1 100644
--- a/arch/arm64/mm/dma-mapping.c
+++ b/arch/arm64/mm/dma-mapping.c
@@ -953,7 +953,8 @@ static void __iommu_setup_dma_ops(struct device *dev, u64 dma_base, u64 size,
 #endif  /* CONFIG_IOMMU_DMA */
 
 void arch_setup_dma_ops(struct device *dev, u64 dma_base, u64 size,
-			const struct iommu_ops *iommu, bool coherent)
+			bool enforce_range, const struct iommu_ops *iommu,
+			bool coherent)
 {
 	if (!dev->archdata.dma_ops)
 		dev->archdata.dma_ops = &swiotlb_dma_ops;
diff --git a/arch/mips/include/asm/dma-mapping.h b/arch/mips/include/asm/dma-mapping.h
index 7aa71b9..6af4d87 100644
--- a/arch/mips/include/asm/dma-mapping.h
+++ b/arch/mips/include/asm/dma-mapping.h
@@ -34,7 +34,8 @@ extern void dma_cache_sync(struct device *dev, void *vaddr, size_t size,
 
 #define arch_setup_dma_ops arch_setup_dma_ops
 static inline void arch_setup_dma_ops(struct device *dev, u64 dma_base,
-				      u64 size, const struct iommu_ops *iommu,
+				      u64 size, bool enforce_range,
+				      const struct iommu_ops *iommu,
 				      bool coherent)
 {
 #ifdef CONFIG_DMA_PERDEV_COHERENT
diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
index 1926918..dea17a5 100644
--- a/drivers/acpi/scan.c
+++ b/drivers/acpi/scan.c
@@ -1385,7 +1385,7 @@ void acpi_dma_configure(struct device *dev, enum dev_dma_attr attr)
 	 * Assume dma valid range starts at 0 and covers the whole
 	 * coherent_dma_mask.
 	 */
-	arch_setup_dma_ops(dev, 0, dev->coherent_dma_mask + 1, iommu,
+	arch_setup_dma_ops(dev, 0, dev->coherent_dma_mask + 1, false, iommu,
 			   attr == DEV_DMA_COHERENT);
 }
 EXPORT_SYMBOL_GPL(acpi_dma_configure);
diff --git a/drivers/iommu/rockchip-iommu.c b/drivers/iommu/rockchip-iommu.c
index 9afcbf7..0995ab3 100644
--- a/drivers/iommu/rockchip-iommu.c
+++ b/drivers/iommu/rockchip-iommu.c
@@ -1096,7 +1096,7 @@ static int rk_iommu_domain_probe(struct platform_device *pdev)
 		return -ENOMEM;
 
 	/* Set dma_ops for dev, otherwise it would be dummy_dma_ops */
-	arch_setup_dma_ops(dev, 0, DMA_BIT_MASK(32), NULL, false);
+	arch_setup_dma_ops(dev, 0, DMA_BIT_MASK(32), false, NULL, false);
 
 	dma_set_max_seg_size(dev, DMA_BIT_MASK(32));
 	dma_coerce_mask_and_coherent(dev, DMA_BIT_MASK(32));
diff --git a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
index c9b7ad6..19f70d8 100644
--- a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
+++ b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
@@ -2533,7 +2533,7 @@ static int dpaa_eth_probe(struct platform_device *pdev)
 	priv->buf_layout[TX].priv_data_size = DPAA_TX_PRIV_DATA_SIZE; /* Tx */
 
 	/* device used for DMA mapping */
-	arch_setup_dma_ops(dev, 0, 0, NULL, false);
+	arch_setup_dma_ops(dev, 0, 0, false, NULL, false);
 	err = dma_coerce_mask_and_coherent(dev, DMA_BIT_MASK(40));
 	if (err) {
 		dev_err(dev, "dma_coerce_mask_and_coherent() failed\n");
diff --git a/drivers/of/device.c b/drivers/of/device.c
index fd5cfad..1cc2115 100644
--- a/drivers/of/device.c
+++ b/drivers/of/device.c
@@ -89,6 +89,7 @@ void of_dma_configure(struct device *dev, struct device_node *np)
 	bool coherent;
 	unsigned long offset;
 	const struct iommu_ops *iommu;
+	bool enforce_range = false;
 
 	/*
 	 * Set default coherent_dma_mask to 32 bit.  Drivers are expected to
@@ -126,6 +127,8 @@ void of_dma_configure(struct device *dev, struct device_node *np)
 			return;
 		}
 		dev_dbg(dev, "dma_pfn_offset(%#08lx)\n", offset);
+
+		enforce_range = true;
 	}
 
 	dev->dma_pfn_offset = offset;
@@ -147,7 +150,7 @@ void of_dma_configure(struct device *dev, struct device_node *np)
 	dev_dbg(dev, "device is%sbehind an iommu\n",
 		iommu ? " " : " not ");
 
-	arch_setup_dma_ops(dev, dma_addr, size, iommu, coherent);
+	arch_setup_dma_ops(dev, dma_addr, size, enforce_range, iommu, coherent);
 }
 EXPORT_SYMBOL_GPL(of_dma_configure);
 
diff --git a/drivers/staging/fsl-mc/bus/fsl-mc-bus.c b/drivers/staging/fsl-mc/bus/fsl-mc-bus.c
index 5ac373c..480b644 100644
--- a/drivers/staging/fsl-mc/bus/fsl-mc-bus.c
+++ b/drivers/staging/fsl-mc/bus/fsl-mc-bus.c
@@ -540,7 +540,7 @@ int fsl_mc_device_add(struct dprc_obj_desc *obj_desc,
 
 	/* Objects are coherent, unless 'no shareability' flag set. */
 	if (!(obj_desc->flags & DPRC_OBJ_FLAG_NO_MEM_SHAREABILITY))
-		arch_setup_dma_ops(&mc_dev->dev, 0, 0, NULL, true);
+		arch_setup_dma_ops(&mc_dev->dev, 0, 0, false, NULL, true);
 
 	/*
 	 * The device-specific probe callback will get invoked by device_add()
-- 
2.1.4

[PATCH 1/2] dma-mapping: let arch know origin of dma range passed to arch_setup_dma_ops()

[Nikita Yushchenko]

There are cases when device is capable of wide DMA mask (and driver
issues corresponding dma_set_mask() call), but bus device sits on can't
support wide address. Example: NVMe device behind PCIe controller
sitting on 32-bit SoC bus.

To support such case, architecture needs information about such
limitations. Such information can originate from dma-ranges property
in device tree, and is passed to architecture via arch_setup_dma_ops()
call.

Problem is that in wide majority of cases, no dma range is defined.
E.g. ACPI has no means to define it. Thus default range (usually
full 32-bit range, i.e. 4G starting at zero address) is passed instead.

If architecture enforce this range, all setups currently using
wide DMA addresses without explicitly defining support for that via
device tree will break. This is bad, especially for ACPI based
platforms.

To avoid that, this patch adds additional boolean argument to
arch_setup_dma_ops() to show if range originates from authorative source
and thus should be enforced, or is just a guess and should be handled as
such.

Signed-off-by: Nikita Yushchenko <nikita.yoush@cogentembedded.com>
---
 arch/arm/include/asm/dma-mapping.h             | 1 +
 arch/arm/mm/dma-mapping.c                      | 3 ++-
 arch/arm64/include/asm/dma-mapping.h           | 3 ++-
 arch/arm64/mm/dma-mapping.c                    | 3 ++-
 arch/mips/include/asm/dma-mapping.h            | 3 ++-
 drivers/acpi/scan.c                            | 2 +-
 drivers/iommu/rockchip-iommu.c                 | 2 +-
 drivers/net/ethernet/freescale/dpaa/dpaa_eth.c | 2 +-
 drivers/of/device.c                            | 5 ++++-
 drivers/staging/fsl-mc/bus/fsl-mc-bus.c        | 2 +-
 10 files changed, 17 insertions(+), 9 deletions(-)

diff --git a/arch/arm/include/asm/dma-mapping.h b/arch/arm/include/asm/dma-mapping.h
index bf02dbd..2a3863e 100644
--- a/arch/arm/include/asm/dma-mapping.h
+++ b/arch/arm/include/asm/dma-mapping.h
@@ -117,6 +117,7 @@ static inline unsigned long dma_max_pfn(struct device *dev)
 
 #define arch_setup_dma_ops arch_setup_dma_ops
 extern void arch_setup_dma_ops(struct device *dev, u64 dma_base, u64 size,
+			       bool enforce_range,
 			       const struct iommu_ops *iommu, bool coherent);
 
 #define arch_teardown_dma_ops arch_teardown_dma_ops
diff --git a/arch/arm/mm/dma-mapping.c b/arch/arm/mm/dma-mapping.c
index ab77100..b8b11f8 100644
--- a/arch/arm/mm/dma-mapping.c
+++ b/arch/arm/mm/dma-mapping.c
@@ -2380,7 +2380,8 @@ static struct dma_map_ops *arm_get_dma_map_ops(bool coherent)
 }
 
 void arch_setup_dma_ops(struct device *dev, u64 dma_base, u64 size,
-			const struct iommu_ops *iommu, bool coherent)
+			bool enforce_range, const struct iommu_ops *iommu,
+			bool coherent)
 {
 	struct dma_map_ops *dma_ops;
 
diff --git a/arch/arm64/include/asm/dma-mapping.h b/arch/arm64/include/asm/dma-mapping.h
index ccea82c..ae1c23f 100644
--- a/arch/arm64/include/asm/dma-mapping.h
+++ b/arch/arm64/include/asm/dma-mapping.h
@@ -48,7 +48,8 @@ static inline struct dma_map_ops *get_dma_ops(struct device *dev)
 }
 
 void arch_setup_dma_ops(struct device *dev, u64 dma_base, u64 size,
-			const struct iommu_ops *iommu, bool coherent);
+			bool enforce_range, const struct iommu_ops *iommu,
+			bool coherent);
 #define arch_setup_dma_ops	arch_setup_dma_ops
 
 #ifdef CONFIG_IOMMU_DMA
diff --git a/arch/arm64/mm/dma-mapping.c b/arch/arm64/mm/dma-mapping.c
index e040827..ea295f1 100644
--- a/arch/arm64/mm/dma-mapping.c
+++ b/arch/arm64/mm/dma-mapping.c
@@ -953,7 +953,8 @@ static void __iommu_setup_dma_ops(struct device *dev, u64 dma_base, u64 size,
 #endif  /* CONFIG_IOMMU_DMA */
 
 void arch_setup_dma_ops(struct device *dev, u64 dma_base, u64 size,
-			const struct iommu_ops *iommu, bool coherent)
+			bool enforce_range, const struct iommu_ops *iommu,
+			bool coherent)
 {
 	if (!dev->archdata.dma_ops)
 		dev->archdata.dma_ops = &swiotlb_dma_ops;
diff --git a/arch/mips/include/asm/dma-mapping.h b/arch/mips/include/asm/dma-mapping.h
index 7aa71b9..6af4d87 100644
--- a/arch/mips/include/asm/dma-mapping.h
+++ b/arch/mips/include/asm/dma-mapping.h
@@ -34,7 +34,8 @@ extern void dma_cache_sync(struct device *dev, void *vaddr, size_t size,
 
 #define arch_setup_dma_ops arch_setup_dma_ops
 static inline void arch_setup_dma_ops(struct device *dev, u64 dma_base,
-				      u64 size, const struct iommu_ops *iommu,
+				      u64 size, bool enforce_range,
+				      const struct iommu_ops *iommu,
 				      bool coherent)
 {
 #ifdef CONFIG_DMA_PERDEV_COHERENT
diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
index 1926918..dea17a5 100644
--- a/drivers/acpi/scan.c
+++ b/drivers/acpi/scan.c
@@ -1385,7 +1385,7 @@ void acpi_dma_configure(struct device *dev, enum dev_dma_attr attr)
 	 * Assume dma valid range starts at 0 and covers the whole
 	 * coherent_dma_mask.
 	 */
-	arch_setup_dma_ops(dev, 0, dev->coherent_dma_mask + 1, iommu,
+	arch_setup_dma_ops(dev, 0, dev->coherent_dma_mask + 1, false, iommu,
 			   attr == DEV_DMA_COHERENT);
 }
 EXPORT_SYMBOL_GPL(acpi_dma_configure);
diff --git a/drivers/iommu/rockchip-iommu.c b/drivers/iommu/rockchip-iommu.c
index 9afcbf7..0995ab3 100644
--- a/drivers/iommu/rockchip-iommu.c
+++ b/drivers/iommu/rockchip-iommu.c
@@ -1096,7 +1096,7 @@ static int rk_iommu_domain_probe(struct platform_device *pdev)
 		return -ENOMEM;
 
 	/* Set dma_ops for dev, otherwise it would be dummy_dma_ops */
-	arch_setup_dma_ops(dev, 0, DMA_BIT_MASK(32), NULL, false);
+	arch_setup_dma_ops(dev, 0, DMA_BIT_MASK(32), false, NULL, false);
 
 	dma_set_max_seg_size(dev, DMA_BIT_MASK(32));
 	dma_coerce_mask_and_coherent(dev, DMA_BIT_MASK(32));
diff --git a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
index c9b7ad6..19f70d8 100644
--- a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
+++ b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
@@ -2533,7 +2533,7 @@ static int dpaa_eth_probe(struct platform_device *pdev)
 	priv->buf_layout[TX].priv_data_size = DPAA_TX_PRIV_DATA_SIZE; /* Tx */
 
 	/* device used for DMA mapping */
-	arch_setup_dma_ops(dev, 0, 0, NULL, false);
+	arch_setup_dma_ops(dev, 0, 0, false, NULL, false);
 	err = dma_coerce_mask_and_coherent(dev, DMA_BIT_MASK(40));
 	if (err) {
 		dev_err(dev, "dma_coerce_mask_and_coherent() failed\n");
diff --git a/drivers/of/device.c b/drivers/of/device.c
index fd5cfad..1cc2115 100644
--- a/drivers/of/device.c
+++ b/drivers/of/device.c
@@ -89,6 +89,7 @@ void of_dma_configure(struct device *dev, struct device_node *np)
 	bool coherent;
 	unsigned long offset;
 	const struct iommu_ops *iommu;
+	bool enforce_range = false;
 
 	/*
 	 * Set default coherent_dma_mask to 32 bit.  Drivers are expected to
@@ -126,6 +127,8 @@ void of_dma_configure(struct device *dev, struct device_node *np)
 			return;
 		}
 		dev_dbg(dev, "dma_pfn_offset(%#08lx)\n", offset);
+
+		enforce_range = true;
 	}
 
 	dev->dma_pfn_offset = offset;
@@ -147,7 +150,7 @@ void of_dma_configure(struct device *dev, struct device_node *np)
 	dev_dbg(dev, "device is%sbehind an iommu\n",
 		iommu ? " " : " not ");
 
-	arch_setup_dma_ops(dev, dma_addr, size, iommu, coherent);
+	arch_setup_dma_ops(dev, dma_addr, size, enforce_range, iommu, coherent);
 }
 EXPORT_SYMBOL_GPL(of_dma_configure);
 
diff --git a/drivers/staging/fsl-mc/bus/fsl-mc-bus.c b/drivers/staging/fsl-mc/bus/fsl-mc-bus.c
index 5ac373c..480b644 100644
--- a/drivers/staging/fsl-mc/bus/fsl-mc-bus.c
+++ b/drivers/staging/fsl-mc/bus/fsl-mc-bus.c
@@ -540,7 +540,7 @@ int fsl_mc_device_add(struct dprc_obj_desc *obj_desc,
 
 	/* Objects are coherent, unless 'no shareability' flag set. */
 	if (!(obj_desc->flags & DPRC_OBJ_FLAG_NO_MEM_SHAREABILITY))
-		arch_setup_dma_ops(&mc_dev->dev, 0, 0, NULL, true);
+		arch_setup_dma_ops(&mc_dev->dev, 0, 0, false, NULL, true);
 
 	/*
 	 * The device-specific probe callback will get invoked by device_add()
-- 
2.1.4

[PATCH 2/2] arm64: avoid increasing DMA masks above what hardware supports

[Nikita Yushchenko]

There are cases when device supports wide DMA addresses wider than
device's connection supports.

In this case driver sets DMA mask based on knowledge of device
capabilities. That must succeed to allow drivers to initialize.

However, swiotlb or iommu still need knowledge about actual device
capabilities. To avoid breakage, actual mask must not be set wider than
device connection allows.

Signed-off-by: Nikita Yushchenko <nikita.yoush@cogentembedded.com>
CC: Arnd Bergmann <arnd@arndb.de>
CC: Robin Murphy <robin.murphy@arm.com>
CC: Will Deacon <will.deacon@arm.com>
---
 arch/arm64/Kconfig                   |  3 +++
 arch/arm64/include/asm/device.h      |  1 +
 arch/arm64/include/asm/dma-mapping.h |  3 +++
 arch/arm64/mm/dma-mapping.c          | 40 ++++++++++++++++++++++++++++++++++++
 4 files changed, 47 insertions(+)

diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index 1117421..afb2c08 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -216,6 +216,9 @@ config NEED_DMA_MAP_STATE
 config NEED_SG_DMA_LENGTH
 	def_bool y
 
+config ARCH_HAS_DMA_SET_COHERENT_MASK
+	def_bool y
+
 config SMP
 	def_bool y
 
diff --git a/arch/arm64/include/asm/device.h b/arch/arm64/include/asm/device.h
index 243ef25..a57e7bb 100644
--- a/arch/arm64/include/asm/device.h
+++ b/arch/arm64/include/asm/device.h
@@ -22,6 +22,7 @@ struct dev_archdata {
 	void *iommu;			/* private IOMMU data */
 #endif
 	bool dma_coherent;
+	u64 parent_dma_mask;
 };
 
 struct pdev_archdata {
diff --git a/arch/arm64/include/asm/dma-mapping.h b/arch/arm64/include/asm/dma-mapping.h
index ae1c23f..46b53e6 100644
--- a/arch/arm64/include/asm/dma-mapping.h
+++ b/arch/arm64/include/asm/dma-mapping.h
@@ -52,6 +52,9 @@ void arch_setup_dma_ops(struct device *dev, u64 dma_base, u64 size,
 			bool coherent);
 #define arch_setup_dma_ops	arch_setup_dma_ops
 
+#define HAVE_ARCH_DMA_SET_MASK 1
+extern int dma_set_mask(struct device *dev, u64 dma_mask);
+
 #ifdef CONFIG_IOMMU_DMA
 void arch_teardown_dma_ops(struct device *dev);
 #define arch_teardown_dma_ops	arch_teardown_dma_ops
diff --git a/arch/arm64/mm/dma-mapping.c b/arch/arm64/mm/dma-mapping.c
index ea295f1..31b96fd 100644
--- a/arch/arm64/mm/dma-mapping.c
+++ b/arch/arm64/mm/dma-mapping.c
@@ -203,6 +203,37 @@ static void __dma_free(struct device *dev, size_t size,
 	__dma_free_coherent(dev, size, swiotlb_addr, dma_handle, attrs);
 }
 
+int dma_set_mask(struct device *dev, u64 dma_mask)
+{
+	const struct dma_map_ops *ops = get_dma_ops(dev);
+
+	if (mask > dev->archdata.parent_dma_mask)
+		mask = dev->archdata.parent_dma_mask;
+
+	if (ops->set_dma_mask)
+		return ops->set_dma_mask(dev, mask);
+
+	if (!dev->dma_mask || !dma_supported(dev, mask))
+		return -EIO;
+
+	*dev->dma_mask = mask;
+	return 0;
+}
+EXPORT_SYMBOL(dma_set_mask);
+
+int dma_set_coherent_mask(struct device *dev, u64 mask)
+{
+	if (mask > dev->archdata.parent_dma_mask)
+		mask = dev->archdata.parent_dma_mask;
+
+	if (!dma_supported(dev, mask))
+		return -EIO;
+
+	dev->coherent_dma_mask = mask;
+	return 0;
+}
+EXPORT_SYMBOL(dma_set_coherent_mask);
+
 static dma_addr_t __swiotlb_map_page(struct device *dev, struct page *page,
 				     unsigned long offset, size_t size,
 				     enum dma_data_direction dir,
@@ -959,6 +990,15 @@ void arch_setup_dma_ops(struct device *dev, u64 dma_base, u64 size,
 	if (!dev->archdata.dma_ops)
 		dev->archdata.dma_ops = &swiotlb_dma_ops;
 
+	/*
+	 * Whatever the parent bus can set. A device must not set
+	 * a DMA mask larger than this.
+	 */
+	if (enforce_range)
+		dev->archdata.parent_dma_mask = size - 1;
+	else
+		dev->archdata.parent_dma_mask = DMA_BIT_MASK(64);
+
 	dev->archdata.dma_coherent = coherent;
 	__iommu_setup_dma_ops(dev, dma_base, size, iommu);
 }
-- 
2.1.4

[PATCH 2/2] arm64: avoid increasing DMA masks above what hardware supports

[Nikita Yushchenko]

There are cases when device supports wide DMA addresses wider than
device's connection supports.

In this case driver sets DMA mask based on knowledge of device
capabilities. That must succeed to allow drivers to initialize.

However, swiotlb or iommu still need knowledge about actual device
capabilities. To avoid breakage, actual mask must not be set wider than
device connection allows.

Signed-off-by: Nikita Yushchenko <nikita.yoush@cogentembedded.com>
CC: Arnd Bergmann <arnd@arndb.de>
CC: Robin Murphy <robin.murphy@arm.com>
CC: Will Deacon <will.deacon@arm.com>
---
 arch/arm64/Kconfig                   |  3 +++
 arch/arm64/include/asm/device.h      |  1 +
 arch/arm64/include/asm/dma-mapping.h |  3 +++
 arch/arm64/mm/dma-mapping.c          | 40 ++++++++++++++++++++++++++++++++++++
 4 files changed, 47 insertions(+)

diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index 1117421..afb2c08 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -216,6 +216,9 @@ config NEED_DMA_MAP_STATE
 config NEED_SG_DMA_LENGTH
 	def_bool y
 
+config ARCH_HAS_DMA_SET_COHERENT_MASK
+	def_bool y
+
 config SMP
 	def_bool y
 
diff --git a/arch/arm64/include/asm/device.h b/arch/arm64/include/asm/device.h
index 243ef25..a57e7bb 100644
--- a/arch/arm64/include/asm/device.h
+++ b/arch/arm64/include/asm/device.h
@@ -22,6 +22,7 @@ struct dev_archdata {
 	void *iommu;			/* private IOMMU data */
 #endif
 	bool dma_coherent;
+	u64 parent_dma_mask;
 };
 
 struct pdev_archdata {
diff --git a/arch/arm64/include/asm/dma-mapping.h b/arch/arm64/include/asm/dma-mapping.h
index ae1c23f..46b53e6 100644
--- a/arch/arm64/include/asm/dma-mapping.h
+++ b/arch/arm64/include/asm/dma-mapping.h
@@ -52,6 +52,9 @@ void arch_setup_dma_ops(struct device *dev, u64 dma_base, u64 size,
 			bool coherent);
 #define arch_setup_dma_ops	arch_setup_dma_ops
 
+#define HAVE_ARCH_DMA_SET_MASK 1
+extern int dma_set_mask(struct device *dev, u64 dma_mask);
+
 #ifdef CONFIG_IOMMU_DMA
 void arch_teardown_dma_ops(struct device *dev);
 #define arch_teardown_dma_ops	arch_teardown_dma_ops
diff --git a/arch/arm64/mm/dma-mapping.c b/arch/arm64/mm/dma-mapping.c
index ea295f1..31b96fd 100644
--- a/arch/arm64/mm/dma-mapping.c
+++ b/arch/arm64/mm/dma-mapping.c
@@ -203,6 +203,37 @@ static void __dma_free(struct device *dev, size_t size,
 	__dma_free_coherent(dev, size, swiotlb_addr, dma_handle, attrs);
 }
 
+int dma_set_mask(struct device *dev, u64 dma_mask)
+{
+	const struct dma_map_ops *ops = get_dma_ops(dev);
+
+	if (mask > dev->archdata.parent_dma_mask)
+		mask = dev->archdata.parent_dma_mask;
+
+	if (ops->set_dma_mask)
+		return ops->set_dma_mask(dev, mask);
+
+	if (!dev->dma_mask || !dma_supported(dev, mask))
+		return -EIO;
+
+	*dev->dma_mask = mask;
+	return 0;
+}
+EXPORT_SYMBOL(dma_set_mask);
+
+int dma_set_coherent_mask(struct device *dev, u64 mask)
+{
+	if (mask > dev->archdata.parent_dma_mask)
+		mask = dev->archdata.parent_dma_mask;
+
+	if (!dma_supported(dev, mask))
+		return -EIO;
+
+	dev->coherent_dma_mask = mask;
+	return 0;
+}
+EXPORT_SYMBOL(dma_set_coherent_mask);
+
 static dma_addr_t __swiotlb_map_page(struct device *dev, struct page *page,
 				     unsigned long offset, size_t size,
 				     enum dma_data_direction dir,
@@ -959,6 +990,15 @@ void arch_setup_dma_ops(struct device *dev, u64 dma_base, u64 size,
 	if (!dev->archdata.dma_ops)
 		dev->archdata.dma_ops = &swiotlb_dma_ops;
 
+	/*
+	 * Whatever the parent bus can set. A device must not set
+	 * a DMA mask larger than this.
+	 */
+	if (enforce_range)
+		dev->archdata.parent_dma_mask = size - 1;
+	else
+		dev->archdata.parent_dma_mask = DMA_BIT_MASK(64);
+
 	dev->archdata.dma_coherent = coherent;
 	__iommu_setup_dma_ops(dev, dma_base, size, iommu);
 }
-- 
2.1.4

Re: [PATCH 1/2] dma-mapping: let arch know origin of dma range passed to arch_setup_dma_ops()

[Arnd Bergmann]

On Wednesday, January 11, 2017 9:31:51 PM CET Nikita Yushchenko wrote:

> diff --git a/drivers/iommu/rockchip-iommu.c b/drivers/iommu/rockchip-iommu.c
> index 9afcbf7..0995ab3 100644
> --- a/drivers/iommu/rockchip-iommu.c
> +++ b/drivers/iommu/rockchip-iommu.c
> @@ -1096,7 +1096,7 @@ static int rk_iommu_domain_probe(struct platform_device *pdev)
>  		return -ENOMEM;
>  
>  	/* Set dma_ops for dev, otherwise it would be dummy_dma_ops */
> -	arch_setup_dma_ops(dev, 0, DMA_BIT_MASK(32), NULL, false);
> +	arch_setup_dma_ops(dev, 0, DMA_BIT_MASK(32), false, NULL, false);
>  
>  	dma_set_max_seg_size(dev, DMA_BIT_MASK(32));
>  	dma_coerce_mask_and_coherent(dev, DMA_BIT_MASK(32));
> diff --git a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
> index c9b7ad6..19f70d8 100644
> --- a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
> +++ b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
> @@ -2533,7 +2533,7 @@ static int dpaa_eth_probe(struct platform_device *pdev)
>  	priv->buf_layout[TX].priv_data_size = DPAA_TX_PRIV_DATA_SIZE; /* Tx */
>  
>  	/* device used for DMA mapping */
> -	arch_setup_dma_ops(dev, 0, 0, NULL, false);
> +	arch_setup_dma_ops(dev, 0, 0, false, NULL, false);
>  	err = dma_coerce_mask_and_coherent(dev, DMA_BIT_MASK(40));
>  	if (err) {
>  		dev_err(dev, "dma_coerce_mask_and_coherent() failed\n");
> diff --git a/drivers/staging/fsl-mc/bus/fsl-mc-bus.c b/drivers/staging/fsl-mc/bus/fsl-mc-bus.c
> index 5ac373c..480b644 100644
> --- a/drivers/staging/fsl-mc/bus/fsl-mc-bus.c
> +++ b/drivers/staging/fsl-mc/bus/fsl-mc-bus.c
> @@ -540,7 +540,7 @@ int fsl_mc_device_add(struct dprc_obj_desc *obj_desc,
>  
>  	/* Objects are coherent, unless 'no shareability' flag set. */
>  	if (!(obj_desc->flags & DPRC_OBJ_FLAG_NO_MEM_SHAREABILITY))
> -		arch_setup_dma_ops(&mc_dev->dev, 0, 0, NULL, true);
> +		arch_setup_dma_ops(&mc_dev->dev, 0, 0, false, NULL, true);
>  
>  	/*
>  	 * The device-specific probe callback will get invoked by device_add()

Why are these actually calling arch_setup_dma_ops() here in the first
place? Are these all devices that are DMA masters without an OF node?

> diff --git a/drivers/of/device.c b/drivers/of/device.c
> index fd5cfad..1cc2115 100644
> --- a/drivers/of/device.c
> +++ b/drivers/of/device.c
> @@ -89,6 +89,7 @@ void of_dma_configure(struct device *dev, struct device_node *np)
>  	bool coherent;
>  	unsigned long offset;
>  	const struct iommu_ops *iommu;
> +	bool enforce_range = false;
>  
>  	/*
>  	 * Set default coherent_dma_mask to 32 bit.  Drivers are expected to
> @@ -126,6 +127,8 @@ void of_dma_configure(struct device *dev, struct device_node *np)
>  			return;
>  		}
>  		dev_dbg(dev, "dma_pfn_offset(%#08lx)\n", offset);
> +
> +		enforce_range = true;
>  	}
>  
>  	dev->dma_pfn_offset = offset;

Hmm, I think when the dma-ranges are missing, we should either enforce
a 32-bit mask, or disallow DMA completely. It's probably too late for
the latter, I wish we had done this earlier in order to force everyone
on ARM64 to have a valid dma-ranges property for any DMA master.

	Arnd

[PATCH 1/2] dma-mapping: let arch know origin of dma range passed to arch_setup_dma_ops()

[Arnd Bergmann]

On Wednesday, January 11, 2017 9:31:51 PM CET Nikita Yushchenko wrote:

> diff --git a/drivers/iommu/rockchip-iommu.c b/drivers/iommu/rockchip-iommu.c
> index 9afcbf7..0995ab3 100644
> --- a/drivers/iommu/rockchip-iommu.c
> +++ b/drivers/iommu/rockchip-iommu.c
> @@ -1096,7 +1096,7 @@ static int rk_iommu_domain_probe(struct platform_device *pdev)
>  		return -ENOMEM;
>  
>  	/* Set dma_ops for dev, otherwise it would be dummy_dma_ops */
> -	arch_setup_dma_ops(dev, 0, DMA_BIT_MASK(32), NULL, false);
> +	arch_setup_dma_ops(dev, 0, DMA_BIT_MASK(32), false, NULL, false);
>  
>  	dma_set_max_seg_size(dev, DMA_BIT_MASK(32));
>  	dma_coerce_mask_and_coherent(dev, DMA_BIT_MASK(32));
> diff --git a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
> index c9b7ad6..19f70d8 100644
> --- a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
> +++ b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
> @@ -2533,7 +2533,7 @@ static int dpaa_eth_probe(struct platform_device *pdev)
>  	priv->buf_layout[TX].priv_data_size = DPAA_TX_PRIV_DATA_SIZE; /* Tx */
>  
>  	/* device used for DMA mapping */
> -	arch_setup_dma_ops(dev, 0, 0, NULL, false);
> +	arch_setup_dma_ops(dev, 0, 0, false, NULL, false);
>  	err = dma_coerce_mask_and_coherent(dev, DMA_BIT_MASK(40));
>  	if (err) {
>  		dev_err(dev, "dma_coerce_mask_and_coherent() failed\n");
> diff --git a/drivers/staging/fsl-mc/bus/fsl-mc-bus.c b/drivers/staging/fsl-mc/bus/fsl-mc-bus.c
> index 5ac373c..480b644 100644
> --- a/drivers/staging/fsl-mc/bus/fsl-mc-bus.c
> +++ b/drivers/staging/fsl-mc/bus/fsl-mc-bus.c
> @@ -540,7 +540,7 @@ int fsl_mc_device_add(struct dprc_obj_desc *obj_desc,
>  
>  	/* Objects are coherent, unless 'no shareability' flag set. */
>  	if (!(obj_desc->flags & DPRC_OBJ_FLAG_NO_MEM_SHAREABILITY))
> -		arch_setup_dma_ops(&mc_dev->dev, 0, 0, NULL, true);
> +		arch_setup_dma_ops(&mc_dev->dev, 0, 0, false, NULL, true);
>  
>  	/*
>  	 * The device-specific probe callback will get invoked by device_add()

Why are these actually calling arch_setup_dma_ops() here in the first
place? Are these all devices that are DMA masters without an OF node?

> diff --git a/drivers/of/device.c b/drivers/of/device.c
> index fd5cfad..1cc2115 100644
> --- a/drivers/of/device.c
> +++ b/drivers/of/device.c
> @@ -89,6 +89,7 @@ void of_dma_configure(struct device *dev, struct device_node *np)
>  	bool coherent;
>  	unsigned long offset;
>  	const struct iommu_ops *iommu;
> +	bool enforce_range = false;
>  
>  	/*
>  	 * Set default coherent_dma_mask to 32 bit.  Drivers are expected to
> @@ -126,6 +127,8 @@ void of_dma_configure(struct device *dev, struct device_node *np)
>  			return;
>  		}
>  		dev_dbg(dev, "dma_pfn_offset(%#08lx)\n", offset);
> +
> +		enforce_range = true;
>  	}
>  
>  	dev->dma_pfn_offset = offset;

Hmm, I think when the dma-ranges are missing, we should either enforce
a 32-bit mask, or disallow DMA completely. It's probably too late for
the latter, I wish we had done this earlier in order to force everyone
on ARM64 to have a valid dma-ranges property for any DMA master.

	Arnd

Re: [PATCH 2/2] arm64: avoid increasing DMA masks above what hardware supports

[Arnd Bergmann]

On Wednesday, January 11, 2017 9:31:52 PM CET Nikita Yushchenko wrote:
> @@ -959,6 +990,15 @@ void arch_setup_dma_ops(struct device *dev, u64 dma_base, u64 size,
>         if (!dev->archdata.dma_ops)
>                 dev->archdata.dma_ops = &swiotlb_dma_ops;
>  
> +       /*
> +        * Whatever the parent bus can set. A device must not set
> +        * a DMA mask larger than this.
> +        */
> +       if (enforce_range)
> +               dev->archdata.parent_dma_mask = size - 1;
> +       else
> +               dev->archdata.parent_dma_mask = DMA_BIT_MASK(64);
> +
>         dev->archdata.dma_coherent = coherent;
>         __iommu_setup_dma_ops(dev, dma_base, size, iommu);
> 

Could we just pass the mask instead of the size here?

	Arnd

[PATCH 2/2] arm64: avoid increasing DMA masks above what hardware supports

[Arnd Bergmann]

On Wednesday, January 11, 2017 9:31:52 PM CET Nikita Yushchenko wrote:
> @@ -959,6 +990,15 @@ void arch_setup_dma_ops(struct device *dev, u64 dma_base, u64 size,
>         if (!dev->archdata.dma_ops)
>                 dev->archdata.dma_ops = &swiotlb_dma_ops;
>  
> +       /*
> +        * Whatever the parent bus can set. A device must not set
> +        * a DMA mask larger than this.
> +        */
> +       if (enforce_range)
> +               dev->archdata.parent_dma_mask = size - 1;
> +       else
> +               dev->archdata.parent_dma_mask = DMA_BIT_MASK(64);
> +
>         dev->archdata.dma_coherent = coherent;
>         __iommu_setup_dma_ops(dev, dma_base, size, iommu);
> 

Could we just pass the mask instead of the size here?

	Arnd

Re: [PATCH 1/2] dma-mapping: let arch know origin of dma range passed to arch_setup_dma_ops()

[Nikita Yushchenko]

>> diff --git a/drivers/staging/fsl-mc/bus/fsl-mc-bus.c b/drivers/staging/fsl-mc/bus/fsl-mc-bus.c
>> index 5ac373c..480b644 100644
>> --- a/drivers/staging/fsl-mc/bus/fsl-mc-bus.c
>> +++ b/drivers/staging/fsl-mc/bus/fsl-mc-bus.c
>> @@ -540,7 +540,7 @@ int fsl_mc_device_add(struct dprc_obj_desc *obj_desc,
>>  
>>  	/* Objects are coherent, unless 'no shareability' flag set. */
>>  	if (!(obj_desc->flags & DPRC_OBJ_FLAG_NO_MEM_SHAREABILITY))
>> -		arch_setup_dma_ops(&mc_dev->dev, 0, 0, NULL, true);
>> +		arch_setup_dma_ops(&mc_dev->dev, 0, 0, false, NULL, true);
>>  
>>  	/*
>>  	 * The device-specific probe callback will get invoked by device_add()
> 
> Why are these actually calling arch_setup_dma_ops() here in the first
> place? Are these all devices that are DMA masters without an OF node?

I don't know, but that's a different topic. This patch just adds
argument and sets it to false everywhere but in the location when range
should be definitely enforced.

>> @@ -126,6 +127,8 @@ void of_dma_configure(struct device *dev, struct device_node *np)
>>  			return;
>>  		}
>>  		dev_dbg(dev, "dma_pfn_offset(%#08lx)\n", offset);
>> +
>> +		enforce_range = true;
>>  	}
>>  
>>  	dev->dma_pfn_offset = offset;
> 
> Hmm, I think when the dma-ranges are missing, we should either enforce
> a 32-bit mask, or disallow DMA completely. It's probably too late for
> the latter, I wish we had done this earlier in order to force everyone
> on ARM64 to have a valid dma-ranges property for any DMA master.

This can be done over time.

However the very idea of this version of patch is - keep working pieces
as-is, thus for now setting enforce_range to false in case of no defined
dma-ranges is intentional.

What I should re-check is - does rcar dtsi set dma-ranges, and add it if
it does not.

Nikita

[PATCH 1/2] dma-mapping: let arch know origin of dma range passed to arch_setup_dma_ops()

[Nikita Yushchenko]

>> diff --git a/drivers/staging/fsl-mc/bus/fsl-mc-bus.c b/drivers/staging/fsl-mc/bus/fsl-mc-bus.c
>> index 5ac373c..480b644 100644
>> --- a/drivers/staging/fsl-mc/bus/fsl-mc-bus.c
>> +++ b/drivers/staging/fsl-mc/bus/fsl-mc-bus.c
>> @@ -540,7 +540,7 @@ int fsl_mc_device_add(struct dprc_obj_desc *obj_desc,
>>  
>>  	/* Objects are coherent, unless 'no shareability' flag set. */
>>  	if (!(obj_desc->flags & DPRC_OBJ_FLAG_NO_MEM_SHAREABILITY))
>> -		arch_setup_dma_ops(&mc_dev->dev, 0, 0, NULL, true);
>> +		arch_setup_dma_ops(&mc_dev->dev, 0, 0, false, NULL, true);
>>  
>>  	/*
>>  	 * The device-specific probe callback will get invoked by device_add()
> 
> Why are these actually calling arch_setup_dma_ops() here in the first
> place? Are these all devices that are DMA masters without an OF node?

I don't know, but that's a different topic. This patch just adds
argument and sets it to false everywhere but in the location when range
should be definitely enforced.

>> @@ -126,6 +127,8 @@ void of_dma_configure(struct device *dev, struct device_node *np)
>>  			return;
>>  		}
>>  		dev_dbg(dev, "dma_pfn_offset(%#08lx)\n", offset);
>> +
>> +		enforce_range = true;
>>  	}
>>  
>>  	dev->dma_pfn_offset = offset;
> 
> Hmm, I think when the dma-ranges are missing, we should either enforce
> a 32-bit mask, or disallow DMA completely. It's probably too late for
> the latter, I wish we had done this earlier in order to force everyone
> on ARM64 to have a valid dma-ranges property for any DMA master.

This can be done over time.

However the very idea of this version of patch is - keep working pieces
as-is, thus for now setting enforce_range to false in case of no defined
dma-ranges is intentional.

What I should re-check is - does rcar dtsi set dma-ranges, and add it if
it does not.

Nikita

Re: [PATCH 2/2] arm64: avoid increasing DMA masks above what hardware supports

[Nikita Yushchenko]

>> @@ -959,6 +990,15 @@ void arch_setup_dma_ops(struct device *dev, u64 dma_base, u64 size,
>>         if (!dev->archdata.dma_ops)
>>                 dev->archdata.dma_ops = &swiotlb_dma_ops;
>>  
>> +       /*
>> +        * Whatever the parent bus can set. A device must not set
>> +        * a DMA mask larger than this.
>> +        */
>> +       if (enforce_range)
>> +               dev->archdata.parent_dma_mask = size - 1;
>> +       else
>> +               dev->archdata.parent_dma_mask = DMA_BIT_MASK(64);
>> +
>>         dev->archdata.dma_coherent = coherent;
>>         __iommu_setup_dma_ops(dev, dma_base, size, iommu);
>>
> 
> Could we just pass the mask instead of the size here?

We don't want to change API now.

Nikita

[PATCH 2/2] arm64: avoid increasing DMA masks above what hardware supports

[Nikita Yushchenko]

>> @@ -959,6 +990,15 @@ void arch_setup_dma_ops(struct device *dev, u64 dma_base, u64 size,
>>         if (!dev->archdata.dma_ops)
>>                 dev->archdata.dma_ops = &swiotlb_dma_ops;
>>  
>> +       /*
>> +        * Whatever the parent bus can set. A device must not set
>> +        * a DMA mask larger than this.
>> +        */
>> +       if (enforce_range)
>> +               dev->archdata.parent_dma_mask = size - 1;
>> +       else
>> +               dev->archdata.parent_dma_mask = DMA_BIT_MASK(64);
>> +
>>         dev->archdata.dma_coherent = coherent;
>>         __iommu_setup_dma_ops(dev, dma_base, size, iommu);
>>
> 
> Could we just pass the mask instead of the size here?

We don't want to change API now.

Nikita

Re: [PATCH 1/2] dma-mapping: let arch know origin of dma range passed to arch_setup_dma_ops()

[Nikita Yushchenko]

12.01.2017 08:52, Nikita Yushchenko wrote:
>>> diff --git a/drivers/staging/fsl-mc/bus/fsl-mc-bus.c b/drivers/staging/fsl-mc/bus/fsl-mc-bus.c
>>> index 5ac373c..480b644 100644
>>> --- a/drivers/staging/fsl-mc/bus/fsl-mc-bus.c
>>> +++ b/drivers/staging/fsl-mc/bus/fsl-mc-bus.c
>>> @@ -540,7 +540,7 @@ int fsl_mc_device_add(struct dprc_obj_desc *obj_desc,
>>>  
>>>  	/* Objects are coherent, unless 'no shareability' flag set. */
>>>  	if (!(obj_desc->flags & DPRC_OBJ_FLAG_NO_MEM_SHAREABILITY))
>>> -		arch_setup_dma_ops(&mc_dev->dev, 0, 0, NULL, true);
>>> +		arch_setup_dma_ops(&mc_dev->dev, 0, 0, false, NULL, true);
>>>  
>>>  	/*
>>>  	 * The device-specific probe callback will get invoked by device_add()
>>
>> Why are these actually calling arch_setup_dma_ops() here in the first
>> place? Are these all devices that are DMA masters without an OF node?
> 
> I don't know, but that's a different topic. This patch just adds
> argument and sets it to false everywhere but in the location when range
> should be definitely enforced.
> 
>>> @@ -126,6 +127,8 @@ void of_dma_configure(struct device *dev, struct device_node *np)
>>>  			return;
>>>  		}
>>>  		dev_dbg(dev, "dma_pfn_offset(%#08lx)\n", offset);
>>> +
>>> +		enforce_range = true;
>>>  	}
>>>  
>>>  	dev->dma_pfn_offset = offset;
>>
>> Hmm, I think when the dma-ranges are missing, we should either enforce
>> a 32-bit mask, or disallow DMA completely. It's probably too late for
>> the latter, I wish we had done this earlier in order to force everyone
>> on ARM64 to have a valid dma-ranges property for any DMA master.
> 
> This can be done over time.
> 
> However the very idea of this version of patch is - keep working pieces
> as-is, thus for now setting enforce_range to false in case of no defined
> dma-ranges is intentional.

What we can do is - check bus width (as it is defined in DT) and set
enforce_range to true if bus is 32-bit

> What I should re-check is - does rcar dtsi set dma-ranges, and add it if
> it does not.

It does not, will have to add.

In DT bus is defined as 64-bit. But looks like physically it is 32-bit.
Maybe DT needs fixing.

[PATCH 1/2] dma-mapping: let arch know origin of dma range passed to arch_setup_dma_ops()

[Nikita Yushchenko]

12.01.2017 08:52, Nikita Yushchenko wrote:
>>> diff --git a/drivers/staging/fsl-mc/bus/fsl-mc-bus.c b/drivers/staging/fsl-mc/bus/fsl-mc-bus.c
>>> index 5ac373c..480b644 100644
>>> --- a/drivers/staging/fsl-mc/bus/fsl-mc-bus.c
>>> +++ b/drivers/staging/fsl-mc/bus/fsl-mc-bus.c
>>> @@ -540,7 +540,7 @@ int fsl_mc_device_add(struct dprc_obj_desc *obj_desc,
>>>  
>>>  	/* Objects are coherent, unless 'no shareability' flag set. */
>>>  	if (!(obj_desc->flags & DPRC_OBJ_FLAG_NO_MEM_SHAREABILITY))
>>> -		arch_setup_dma_ops(&mc_dev->dev, 0, 0, NULL, true);
>>> +		arch_setup_dma_ops(&mc_dev->dev, 0, 0, false, NULL, true);
>>>  
>>>  	/*
>>>  	 * The device-specific probe callback will get invoked by device_add()
>>
>> Why are these actually calling arch_setup_dma_ops() here in the first
>> place? Are these all devices that are DMA masters without an OF node?
> 
> I don't know, but that's a different topic. This patch just adds
> argument and sets it to false everywhere but in the location when range
> should be definitely enforced.
> 
>>> @@ -126,6 +127,8 @@ void of_dma_configure(struct device *dev, struct device_node *np)
>>>  			return;
>>>  		}
>>>  		dev_dbg(dev, "dma_pfn_offset(%#08lx)\n", offset);
>>> +
>>> +		enforce_range = true;
>>>  	}
>>>  
>>>  	dev->dma_pfn_offset = offset;
>>
>> Hmm, I think when the dma-ranges are missing, we should either enforce
>> a 32-bit mask, or disallow DMA completely. It's probably too late for
>> the latter, I wish we had done this earlier in order to force everyone
>> on ARM64 to have a valid dma-ranges property for any DMA master.
> 
> This can be done over time.
> 
> However the very idea of this version of patch is - keep working pieces
> as-is, thus for now setting enforce_range to false in case of no defined
> dma-ranges is intentional.

What we can do is - check bus width (as it is defined in DT) and set
enforce_range to true if bus is 32-bit

> What I should re-check is - does rcar dtsi set dma-ranges, and add it if
> it does not.

It does not, will have to add.

In DT bus is defined as 64-bit. But looks like physically it is 32-bit.
Maybe DT needs fixing.

Re: [PATCH 1/2] dma-mapping: let arch know origin of dma range passed to arch_setup_dma_ops()

[Will Deacon]

On Thu, Jan 12, 2017 at 08:52:51AM +0300, Nikita Yushchenko wrote:
> >> diff --git a/drivers/staging/fsl-mc/bus/fsl-mc-bus.c b/drivers/staging/fsl-mc/bus/fsl-mc-bus.c
> >> index 5ac373c..480b644 100644
> >> --- a/drivers/staging/fsl-mc/bus/fsl-mc-bus.c
> >> +++ b/drivers/staging/fsl-mc/bus/fsl-mc-bus.c
> >> @@ -540,7 +540,7 @@ int fsl_mc_device_add(struct dprc_obj_desc *obj_desc,
> >>  
> >>  	/* Objects are coherent, unless 'no shareability' flag set. */
> >>  	if (!(obj_desc->flags & DPRC_OBJ_FLAG_NO_MEM_SHAREABILITY))
> >> -		arch_setup_dma_ops(&mc_dev->dev, 0, 0, NULL, true);
> >> +		arch_setup_dma_ops(&mc_dev->dev, 0, 0, false, NULL, true);
> >>  
> >>  	/*
> >>  	 * The device-specific probe callback will get invoked by device_add()
> > 
> > Why are these actually calling arch_setup_dma_ops() here in the first
> > place? Are these all devices that are DMA masters without an OF node?
> 
> I don't know, but that's a different topic. This patch just adds
> argument and sets it to false everywhere but in the location when range
> should be definitely enforced.

I also wouldn't lose any sleep over a staging driver.

Will

[PATCH 1/2] dma-mapping: let arch know origin of dma range passed to arch_setup_dma_ops()

[Will Deacon]

On Thu, Jan 12, 2017 at 08:52:51AM +0300, Nikita Yushchenko wrote:
> >> diff --git a/drivers/staging/fsl-mc/bus/fsl-mc-bus.c b/drivers/staging/fsl-mc/bus/fsl-mc-bus.c
> >> index 5ac373c..480b644 100644
> >> --- a/drivers/staging/fsl-mc/bus/fsl-mc-bus.c
> >> +++ b/drivers/staging/fsl-mc/bus/fsl-mc-bus.c
> >> @@ -540,7 +540,7 @@ int fsl_mc_device_add(struct dprc_obj_desc *obj_desc,
> >>  
> >>  	/* Objects are coherent, unless 'no shareability' flag set. */
> >>  	if (!(obj_desc->flags & DPRC_OBJ_FLAG_NO_MEM_SHAREABILITY))
> >> -		arch_setup_dma_ops(&mc_dev->dev, 0, 0, NULL, true);
> >> +		arch_setup_dma_ops(&mc_dev->dev, 0, 0, false, NULL, true);
> >>  
> >>  	/*
> >>  	 * The device-specific probe callback will get invoked by device_add()
> > 
> > Why are these actually calling arch_setup_dma_ops() here in the first
> > place? Are these all devices that are DMA masters without an OF node?
> 
> I don't know, but that's a different topic. This patch just adds
> argument and sets it to false everywhere but in the location when range
> should be definitely enforced.

I also wouldn't lose any sleep over a staging driver.

Will

Re: [PATCH 1/2] dma-mapping: let arch know origin of dma range passed to arch_setup_dma_ops()

[Arnd Bergmann]

On Thursday, January 12, 2017 12:16:24 PM CET Will Deacon wrote:
> On Thu, Jan 12, 2017 at 08:52:51AM +0300, Nikita Yushchenko wrote:
> > >> diff --git a/drivers/staging/fsl-mc/bus/fsl-mc-bus.c b/drivers/staging/fsl-mc/bus/fsl-mc-bus.c
> > >> index 5ac373c..480b644 100644
> > >> --- a/drivers/staging/fsl-mc/bus/fsl-mc-bus.c
> > >> +++ b/drivers/staging/fsl-mc/bus/fsl-mc-bus.c
> > >> @@ -540,7 +540,7 @@ int fsl_mc_device_add(struct dprc_obj_desc *obj_desc,
> > >>  
> > >>    /* Objects are coherent, unless 'no shareability' flag set. */
> > >>    if (!(obj_desc->flags & DPRC_OBJ_FLAG_NO_MEM_SHAREABILITY))
> > >> -          arch_setup_dma_ops(&mc_dev->dev, 0, 0, NULL, true);
> > >> +          arch_setup_dma_ops(&mc_dev->dev, 0, 0, false, NULL, true);
> > >>  
> > >>    /*
> > >>     * The device-specific probe callback will get invoked by device_add()
> > > 
> > > Why are these actually calling arch_setup_dma_ops() here in the first
> > > place? Are these all devices that are DMA masters without an OF node?
> > 
> > I don't know, but that's a different topic. This patch just adds
> > argument and sets it to false everywhere but in the location when range
> > should be definitely enforced.
> 
> I also wouldn't lose any sleep over a staging driver.

I think this is in the process of being moved out of staging, and
my question was about the other two as well:

drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
drivers/iommu/rockchip-iommu.c

	Arnd

[PATCH 1/2] dma-mapping: let arch know origin of dma range passed to arch_setup_dma_ops()

[Arnd Bergmann]

On Thursday, January 12, 2017 12:16:24 PM CET Will Deacon wrote:
> On Thu, Jan 12, 2017 at 08:52:51AM +0300, Nikita Yushchenko wrote:
> > >> diff --git a/drivers/staging/fsl-mc/bus/fsl-mc-bus.c b/drivers/staging/fsl-mc/bus/fsl-mc-bus.c
> > >> index 5ac373c..480b644 100644
> > >> --- a/drivers/staging/fsl-mc/bus/fsl-mc-bus.c
> > >> +++ b/drivers/staging/fsl-mc/bus/fsl-mc-bus.c
> > >> @@ -540,7 +540,7 @@ int fsl_mc_device_add(struct dprc_obj_desc *obj_desc,
> > >>  
> > >>    /* Objects are coherent, unless 'no shareability' flag set. */
> > >>    if (!(obj_desc->flags & DPRC_OBJ_FLAG_NO_MEM_SHAREABILITY))
> > >> -          arch_setup_dma_ops(&mc_dev->dev, 0, 0, NULL, true);
> > >> +          arch_setup_dma_ops(&mc_dev->dev, 0, 0, false, NULL, true);
> > >>  
> > >>    /*
> > >>     * The device-specific probe callback will get invoked by device_add()
> > > 
> > > Why are these actually calling arch_setup_dma_ops() here in the first
> > > place? Are these all devices that are DMA masters without an OF node?
> > 
> > I don't know, but that's a different topic. This patch just adds
> > argument and sets it to false everywhere but in the location when range
> > should be definitely enforced.
> 
> I also wouldn't lose any sleep over a staging driver.

I think this is in the process of being moved out of staging, and
my question was about the other two as well:

drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
drivers/iommu/rockchip-iommu.c

	Arnd

Re: [PATCH 1/2] dma-mapping: let arch know origin of dma range passed to arch_setup_dma_ops()

[Arnd Bergmann]

On Thursday, January 12, 2017 9:33:32 AM CET Nikita Yushchenko wrote:
> >> Hmm, I think when the dma-ranges are missing, we should either enforce
> >> a 32-bit mask, or disallow DMA completely. It's probably too late for
> >> the latter, I wish we had done this earlier in order to force everyone
> >> on ARM64 to have a valid dma-ranges property for any DMA master.
> > 
> > This can be done over time.
> > 
> > However the very idea of this version of patch is - keep working pieces
> > as-is, thus for now setting enforce_range to false in case of no defined
> > dma-ranges is intentional.
> 
> What we can do is - check bus width (as it is defined in DT) and set
> enforce_range to true if bus is 32-bit
> 
> > What I should re-check is - does rcar dtsi set dma-ranges, and add it if
> > it does not.
> 
> It does not, will have to add.
> 
> In DT bus is defined as 64-bit. But looks like physically it is 32-bit.
> Maybe DT needs fixing.

I think we always assumed that the lack of a dma-ranges property
implied a 32-bit width, as that is the safe fallback as well as the
most common case.

AFAICT, this means you are actually fine on rcar, and all other
platforms will keep working as we enforce it, but might get slowed
down if they relied on the unintended behavior of allowing 64-bit
DMA.

	Arnd

[PATCH 1/2] dma-mapping: let arch know origin of dma range passed to arch_setup_dma_ops()

[Arnd Bergmann]

On Thursday, January 12, 2017 9:33:32 AM CET Nikita Yushchenko wrote:
> >> Hmm, I think when the dma-ranges are missing, we should either enforce
> >> a 32-bit mask, or disallow DMA completely. It's probably too late for
> >> the latter, I wish we had done this earlier in order to force everyone
> >> on ARM64 to have a valid dma-ranges property for any DMA master.
> > 
> > This can be done over time.
> > 
> > However the very idea of this version of patch is - keep working pieces
> > as-is, thus for now setting enforce_range to false in case of no defined
> > dma-ranges is intentional.
> 
> What we can do is - check bus width (as it is defined in DT) and set
> enforce_range to true if bus is 32-bit
> 
> > What I should re-check is - does rcar dtsi set dma-ranges, and add it if
> > it does not.
> 
> It does not, will have to add.
> 
> In DT bus is defined as 64-bit. But looks like physically it is 32-bit.
> Maybe DT needs fixing.

I think we always assumed that the lack of a dma-ranges property
implied a 32-bit width, as that is the safe fallback as well as the
most common case.

AFAICT, this means you are actually fine on rcar, and all other
platforms will keep working as we enforce it, but might get slowed
down if they relied on the unintended behavior of allowing 64-bit
DMA.

	Arnd

Re: [PATCH 1/2] dma-mapping: let arch know origin of dma range passed to arch_setup_dma_ops()

[Nikita Yushchenko]

>>>> Hmm, I think when the dma-ranges are missing, we should either enforce
>>>> a 32-bit mask, or disallow DMA completely. It's probably too late for
>>>> the latter, I wish we had done this earlier in order to force everyone
>>>> on ARM64 to have a valid dma-ranges property for any DMA master.
>>>
>>> This can be done over time.
>>>
>>> However the very idea of this version of patch is - keep working pieces
>>> as-is, thus for now setting enforce_range to false in case of no defined
>>> dma-ranges is intentional.
>>
>> What we can do is - check bus width (as it is defined in DT) and set
>> enforce_range to true if bus is 32-bit
>>
>>> What I should re-check is - does rcar dtsi set dma-ranges, and add it if
>>> it does not.
>>
>> It does not, will have to add.
>>
>> In DT bus is defined as 64-bit. But looks like physically it is 32-bit.
>> Maybe DT needs fixing.
> 
> I think we always assumed that the lack of a dma-ranges property
> implied a 32-bit width, as that is the safe fallback as well as the
> most common case.

Yes we assumed that, but that was combined with blindly accepting wider
dma_mask per driver's request.  Later is being changed.

> AFAICT, this means you are actually fine on rcar, and all other
> platforms will keep working as we enforce it, but might get slowed
> down if they relied on the unintended behavior of allowing 64-bit
> DMA.

Yesterday Robin raised issue that a change starting to enforce default
dma_mask will break existing setups - i.e. those that depend in 64bit
DMA being implicitly supported without manually declaring such support.

In reply to that, I suggested this version of patchset that should keep
existing behavior by default.

I'm fine with both approaches regarding behavior on hw that I don't have
- but I'm not in position to make any decisions on that.

[PATCH 1/2] dma-mapping: let arch know origin of dma range passed to arch_setup_dma_ops()

[Nikita Yushchenko]

>>>> Hmm, I think when the dma-ranges are missing, we should either enforce
>>>> a 32-bit mask, or disallow DMA completely. It's probably too late for
>>>> the latter, I wish we had done this earlier in order to force everyone
>>>> on ARM64 to have a valid dma-ranges property for any DMA master.
>>>
>>> This can be done over time.
>>>
>>> However the very idea of this version of patch is - keep working pieces
>>> as-is, thus for now setting enforce_range to false in case of no defined
>>> dma-ranges is intentional.
>>
>> What we can do is - check bus width (as it is defined in DT) and set
>> enforce_range to true if bus is 32-bit
>>
>>> What I should re-check is - does rcar dtsi set dma-ranges, and add it if
>>> it does not.
>>
>> It does not, will have to add.
>>
>> In DT bus is defined as 64-bit. But looks like physically it is 32-bit.
>> Maybe DT needs fixing.
> 
> I think we always assumed that the lack of a dma-ranges property
> implied a 32-bit width, as that is the safe fallback as well as the
> most common case.

Yes we assumed that, but that was combined with blindly accepting wider
dma_mask per driver's request.  Later is being changed.

> AFAICT, this means you are actually fine on rcar, and all other
> platforms will keep working as we enforce it, but might get slowed
> down if they relied on the unintended behavior of allowing 64-bit
> DMA.

Yesterday Robin raised issue that a change starting to enforce default
dma_mask will break existing setups - i.e. those that depend in 64bit
DMA being implicitly supported without manually declaring such support.

In reply to that, I suggested this version of patchset that should keep
existing behavior by default.

I'm fine with both approaches regarding behavior on hw that I don't have
- but I'm not in position to make any decisions on that.

Re: [PATCH 1/2] dma-mapping: let arch know origin of dma range passed to arch_setup_dma_ops()

[Robin Murphy]

On 12/01/17 13:25, Arnd Bergmann wrote:
> On Thursday, January 12, 2017 12:16:24 PM CET Will Deacon wrote:
>> On Thu, Jan 12, 2017 at 08:52:51AM +0300, Nikita Yushchenko wrote:
>>>>> diff --git a/drivers/staging/fsl-mc/bus/fsl-mc-bus.c b/drivers/staging/fsl-mc/bus/fsl-mc-bus.c
>>>>> index 5ac373c..480b644 100644
>>>>> --- a/drivers/staging/fsl-mc/bus/fsl-mc-bus.c
>>>>> +++ b/drivers/staging/fsl-mc/bus/fsl-mc-bus.c
>>>>> @@ -540,7 +540,7 @@ int fsl_mc_device_add(struct dprc_obj_desc *obj_desc,
>>>>>  
>>>>>    /* Objects are coherent, unless 'no shareability' flag set. */
>>>>>    if (!(obj_desc->flags & DPRC_OBJ_FLAG_NO_MEM_SHAREABILITY))
>>>>> -          arch_setup_dma_ops(&mc_dev->dev, 0, 0, NULL, true);
>>>>> +          arch_setup_dma_ops(&mc_dev->dev, 0, 0, false, NULL, true);
>>>>>  
>>>>>    /*
>>>>>     * The device-specific probe callback will get invoked by device_add()
>>>>
>>>> Why are these actually calling arch_setup_dma_ops() here in the first
>>>> place? Are these all devices that are DMA masters without an OF node?
>>>
>>> I don't know, but that's a different topic. This patch just adds
>>> argument and sets it to false everywhere but in the location when range
>>> should be definitely enforced.
>>
>> I also wouldn't lose any sleep over a staging driver.
> 
> I think this is in the process of being moved out of staging, and
> my question was about the other two as well:

The fsl-mc is actually a sort-of-bus-controller probing and configuring
its (directly DMA-capable) child devices, so is in fact legitimate here.

> drivers/net/ethernet/freescale/dpaa/dpaa_eth.c

That one is completely bogus, and should just go away.

> drivers/iommu/rockchip-iommu.c

That one is part of some ugly trickery involving creating a fake device
to represent multiple separate IOMMU devices. The driver could probably
be reworked to not need it (the Exynos IOMMU handles a similar situation
without such tricks), but it's non-trivial.

Robin.

> 
> 	Arnd
> 

[PATCH 1/2] dma-mapping: let arch know origin of dma range passed to arch_setup_dma_ops()

[Robin Murphy]

On 12/01/17 13:25, Arnd Bergmann wrote:
> On Thursday, January 12, 2017 12:16:24 PM CET Will Deacon wrote:
>> On Thu, Jan 12, 2017 at 08:52:51AM +0300, Nikita Yushchenko wrote:
>>>>> diff --git a/drivers/staging/fsl-mc/bus/fsl-mc-bus.c b/drivers/staging/fsl-mc/bus/fsl-mc-bus.c
>>>>> index 5ac373c..480b644 100644
>>>>> --- a/drivers/staging/fsl-mc/bus/fsl-mc-bus.c
>>>>> +++ b/drivers/staging/fsl-mc/bus/fsl-mc-bus.c
>>>>> @@ -540,7 +540,7 @@ int fsl_mc_device_add(struct dprc_obj_desc *obj_desc,
>>>>>  
>>>>>    /* Objects are coherent, unless 'no shareability' flag set. */
>>>>>    if (!(obj_desc->flags & DPRC_OBJ_FLAG_NO_MEM_SHAREABILITY))
>>>>> -          arch_setup_dma_ops(&mc_dev->dev, 0, 0, NULL, true);
>>>>> +          arch_setup_dma_ops(&mc_dev->dev, 0, 0, false, NULL, true);
>>>>>  
>>>>>    /*
>>>>>     * The device-specific probe callback will get invoked by device_add()
>>>>
>>>> Why are these actually calling arch_setup_dma_ops() here in the first
>>>> place? Are these all devices that are DMA masters without an OF node?
>>>
>>> I don't know, but that's a different topic. This patch just adds
>>> argument and sets it to false everywhere but in the location when range
>>> should be definitely enforced.
>>
>> I also wouldn't lose any sleep over a staging driver.
> 
> I think this is in the process of being moved out of staging, and
> my question was about the other two as well:

The fsl-mc is actually a sort-of-bus-controller probing and configuring
its (directly DMA-capable) child devices, so is in fact legitimate here.

> drivers/net/ethernet/freescale/dpaa/dpaa_eth.c

That one is completely bogus, and should just go away.

> drivers/iommu/rockchip-iommu.c

That one is part of some ugly trickery involving creating a fake device
to represent multiple separate IOMMU devices. The driver could probably
be reworked to not need it (the Exynos IOMMU handles a similar situation
without such tricks), but it's non-trivial.

Robin.

> 
> 	Arnd
> 

Re: [PATCH 2/2] arm64: avoid increasing DMA masks above what hardware supports

[kbuild test robot]

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

Hi Nikita,

[auto build test ERROR on linus/master]
[also build test ERROR on v4.10-rc3 next-20170112]
[cannot apply to arm64/for-next/core]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Nikita-Yushchenko/dma-mapping-let-arch-know-origin-of-dma-range-passed-to-arch_setup_dma_ops/20170113-152733
config: arm64-defconfig (attached as .config)
compiler: aarch64-linux-gnu-gcc (Debian 6.1.1-9) 6.1.1 20160705
reproduce:
        wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=arm64 

All errors (new ones prefixed by >>):

   arch/arm64/mm/dma-mapping.c: In function 'dma_set_mask':
>> arch/arm64/mm/dma-mapping.c:210:6: error: 'mask' undeclared (first use in this function)
     if (mask > dev->archdata.parent_dma_mask)
         ^~~~
   arch/arm64/mm/dma-mapping.c:210:6: note: each undeclared identifier is reported only once for each function it appears in

vim +/mask +210 arch/arm64/mm/dma-mapping.c

   204	}
   205	
   206	int dma_set_mask(struct device *dev, u64 dma_mask)
   207	{
   208		const struct dma_map_ops *ops = get_dma_ops(dev);
   209	
 > 210		if (mask > dev->archdata.parent_dma_mask)
   211			mask = dev->archdata.parent_dma_mask;
   212	
   213		if (ops->set_dma_mask)

---
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: 33713 bytes --]

[PATCH 2/2] arm64: avoid increasing DMA masks above what hardware supports

[kbuild test robot]

Hi Nikita,

[auto build test ERROR on linus/master]
[also build test ERROR on v4.10-rc3 next-20170112]
[cannot apply to arm64/for-next/core]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Nikita-Yushchenko/dma-mapping-let-arch-know-origin-of-dma-range-passed-to-arch_setup_dma_ops/20170113-152733
config: arm64-defconfig (attached as .config)
compiler: aarch64-linux-gnu-gcc (Debian 6.1.1-9) 6.1.1 20160705
reproduce:
        wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=arm64 

All errors (new ones prefixed by >>):

   arch/arm64/mm/dma-mapping.c: In function 'dma_set_mask':
>> arch/arm64/mm/dma-mapping.c:210:6: error: 'mask' undeclared (first use in this function)
     if (mask > dev->archdata.parent_dma_mask)
         ^~~~
   arch/arm64/mm/dma-mapping.c:210:6: note: each undeclared identifier is reported only once for each function it appears in

vim +/mask +210 arch/arm64/mm/dma-mapping.c

   204	}
   205	
   206	int dma_set_mask(struct device *dev, u64 dma_mask)
   207	{
   208		const struct dma_map_ops *ops = get_dma_ops(dev);
   209	
 > 210		if (mask > dev->archdata.parent_dma_mask)
   211			mask = dev->archdata.parent_dma_mask;
   212	
   213		if (ops->set_dma_mask)

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation
-------------- next part --------------
A non-text attachment was scrubbed...
Name: .config.gz
Type: application/gzip
Size: 33713 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20170113/4aebcaaa/attachment-0001.gz>

Re: [PATCH 1/2] dma-mapping: let arch know origin of dma range passed to arch_setup_dma_ops()

[kbuild test robot]

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

Hi Nikita,

[auto build test ERROR on linus/master]
[also build test ERROR on v4.10-rc3 next-20170112]
[cannot apply to arm64/for-next/core]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Nikita-Yushchenko/dma-mapping-let-arch-know-origin-of-dma-range-passed-to-arch_setup_dma_ops/20170113-152733
config: x86_64-randconfig-u0-01131618 (attached as .config)
compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

All errors (new ones prefixed by >>):

   drivers/acpi/scan.c: In function 'acpi_dma_configure':
>> drivers/acpi/scan.c:1388:2: error: too many arguments to function 'arch_setup_dma_ops'
     arch_setup_dma_ops(dev, 0, dev->coherent_dma_mask + 1, false, iommu,
     ^~~~~~~~~~~~~~~~~~
   In file included from drivers/acpi/scan.c:15:0:
   include/linux/dma-mapping.h:611:20: note: declared here
    static inline void arch_setup_dma_ops(struct device *dev, u64 dma_base,
                       ^~~~~~~~~~~~~~~~~~

vim +/arch_setup_dma_ops +1388 drivers/acpi/scan.c

  1382		iommu = iort_iommu_configure(dev);
  1383	
  1384		/*
  1385		 * Assume dma valid range starts at 0 and covers the whole
  1386		 * coherent_dma_mask.
  1387		 */
> 1388		arch_setup_dma_ops(dev, 0, dev->coherent_dma_mask + 1, false, iommu,
  1389				   attr == DEV_DMA_COHERENT);
  1390	}
  1391	EXPORT_SYMBOL_GPL(acpi_dma_configure);

---
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: 25665 bytes --]

[PATCH 1/2] dma-mapping: let arch know origin of dma range passed to arch_setup_dma_ops()

[kbuild test robot]

Hi Nikita,

[auto build test ERROR on linus/master]
[also build test ERROR on v4.10-rc3 next-20170112]
[cannot apply to arm64/for-next/core]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Nikita-Yushchenko/dma-mapping-let-arch-know-origin-of-dma-range-passed-to-arch_setup_dma_ops/20170113-152733
config: x86_64-randconfig-u0-01131618 (attached as .config)
compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

All errors (new ones prefixed by >>):

   drivers/acpi/scan.c: In function 'acpi_dma_configure':
>> drivers/acpi/scan.c:1388:2: error: too many arguments to function 'arch_setup_dma_ops'
     arch_setup_dma_ops(dev, 0, dev->coherent_dma_mask + 1, false, iommu,
     ^~~~~~~~~~~~~~~~~~
   In file included from drivers/acpi/scan.c:15:0:
   include/linux/dma-mapping.h:611:20: note: declared here
    static inline void arch_setup_dma_ops(struct device *dev, u64 dma_base,
                       ^~~~~~~~~~~~~~~~~~

vim +/arch_setup_dma_ops +1388 drivers/acpi/scan.c

  1382		iommu = iort_iommu_configure(dev);
  1383	
  1384		/*
  1385		 * Assume dma valid range starts at 0 and covers the whole
  1386		 * coherent_dma_mask.
  1387		 */
> 1388		arch_setup_dma_ops(dev, 0, dev->coherent_dma_mask + 1, false, iommu,
  1389				   attr == DEV_DMA_COHERENT);
  1390	}
  1391	EXPORT_SYMBOL_GPL(acpi_dma_configure);

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation
-------------- next part --------------
A non-text attachment was scrubbed...
Name: .config.gz
Type: application/gzip
Size: 25665 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20170113/7e91dc43/attachment-0001.gz>