arm64: mm: bug around swiotlb_dma_ops

arm64: mm: bug around swiotlb_dma_ops

[Nikita Yushchenko]

Hi.

Per Documentation/DMA-API-HOWTO.txt, driver of device capable of 64-bit
DMA addressing, should call dma_set_mask_and_coherent(dev,
DMA_BIT_MASK(64)) and if that succeeds, assume that 64-bit DMA
addressing is available.

This behaves incorrectly on arm64 system (Renesas r8a7795-h3ulcb) here.

- Device (NVME SSD) has it's dev->archdata.dma_ops set to swiotlb_dma_ops.

- swiotlb_dma_ops.dma_supported is set to swiotlb_dma_supported():

int swiotlb_dma_supported(struct device *hwdev, u64 mask)
{
        return phys_to_dma(hwdev, io_tlb_end - 1) <= mask;
}

this definitely returns true for mask=DMA_BIT_MASK(64) since that is
maximum possible 64-bit value.

- Thus device dma_mask is unconditionally updated, and
dma_set_mask_and_coherent() succeeds.

- Later, __swiotlb_map_page() / __swiotlb_map_sg_attr() will consult
this updated mask, and return high addresses as valid DMA addresses.


Thus recommended dma_set_mask_and_coherent() call, instead of checking
if platform supports 64-bit DMA addressing, unconditionally enables
64-bit DMA addressing. In case of device actually can't do DMA to 64-bit
addresses (e.g. because of limitations in PCIe controller), this breaks
things. This is exactly what happens here.


Not sure what is proper fix for this though.

Nikita

arm64: mm: bug around swiotlb_dma_ops

[Arnd Bergmann]

On Thursday, December 15, 2016 7:20:11 PM CET Nikita Yushchenko wrote:
> Hi.
> 
> Per Documentation/DMA-API-HOWTO.txt, driver of device capable of 64-bit
> DMA addressing, should call dma_set_mask_and_coherent(dev,
> DMA_BIT_MASK(64)) and if that succeeds, assume that 64-bit DMA
> addressing is available.
> 
> This behaves incorrectly on arm64 system (Renesas r8a7795-h3ulcb) here.
> 
> - Device (NVME SSD) has it's dev->archdata.dma_ops set to swiotlb_dma_ops.
> 
> - swiotlb_dma_ops.dma_supported is set to swiotlb_dma_supported():
> 
> int swiotlb_dma_supported(struct device *hwdev, u64 mask)
> {
>         return phys_to_dma(hwdev, io_tlb_end - 1) <= mask;
> }
> 
> this definitely returns true for mask=DMA_BIT_MASK(64) since that is
> maximum possible 64-bit value.
> 
> - Thus device dma_mask is unconditionally updated, and
> dma_set_mask_and_coherent() succeeds.
> 
> - Later, __swiotlb_map_page() / __swiotlb_map_sg_attr() will consult
> this updated mask, and return high addresses as valid DMA addresses.
> 
> 
> Thus recommended dma_set_mask_and_coherent() call, instead of checking
> if platform supports 64-bit DMA addressing, unconditionally enables
> 64-bit DMA addressing. In case of device actually can't do DMA to 64-bit
> addresses (e.g. because of limitations in PCIe controller), this breaks
> things. This is exactly what happens here.
> 
> 
> Not sure what is proper fix for this though.

I had prototyped something for this a long time ago. It's probably
wrong or incomplete, but maybe it helps you get closer to a solution.

	Arnd

commit 76c3f31874b0791b4be72cdd64791a64495c3a4a
Author: Arnd Bergmann <arnd@arndb.de>
Date:   Tue Nov 17 14:06:55 2015 +0100

    [EXPERIMENTAL] ARM64: check implement dma_set_mask
    
    Needs work for coherent mask
    
    Signed-off-by: Arnd Bergmann <arnd@arndb.de>

diff --git a/arch/arm64/include/asm/device.h b/arch/arm64/include/asm/device.h
index 243ef256b8c9..a57e7bb10e71 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 290a84f3351f..aa65875c611b 100644
--- a/arch/arm64/mm/dma-mapping.c
+++ b/arch/arm64/mm/dma-mapping.c
@@ -352,6 +352,31 @@ 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,6 +392,7 @@ 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,
 };
 
 static int __init atomic_pool_init(void)
@@ -957,6 +983,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;
+
 	dev->archdata.dma_coherent = coherent;
 	__iommu_setup_dma_ops(dev, dma_base, size, iommu);
 }

arm64: mm: bug around swiotlb_dma_ops

[Nikita Yushchenko]

>> Thus recommended dma_set_mask_and_coherent() call, instead of checking
>> if platform supports 64-bit DMA addressing, unconditionally enables
>> 64-bit DMA addressing. In case of device actually can't do DMA to 64-bit
>> addresses (e.g. because of limitations in PCIe controller), this breaks
>> things. This is exactly what happens here.
>>
> 
> I had prototyped something for this a long time ago. It's probably
> wrong or incomplete, but maybe it helps you get closer to a solution.

With swiotlb, "memory device can DMA to" and "memory drivers should
allocate for DMA" is no longer the same: swiotlb allows drivers to
dma_map any memory, but device still has it's restrictions.

Problem is caused by that swiotlb mixes these two meanings:

- swiotlb's mapping code assumes that masks describe what device is
capable of
  - for dma_mask, this dependency is indirect, via arch's dma_capable(),
which naively uses dma_mask on arm64,
  - for dma_coherent_mask, dependency is coded in common code in
lib/swiotlb.c, in swiotlb_alloc_coherent()

- but swiotlb_dma_supported() assumes that masks describe what memory
driver is allowed to allocate, and unconditionally allows wide masks.

Problem is not arm64 specific. Although arm64 specific workaround is
possible by altering arm64's swiotlb_dma_ops.


Actually overall situation is quite messy.

*) There is no memory allocation API that can enforce arbitrary range
restrictions. At memory allocation level, only GFP_* flags are
available. Thus DMA allocators have to speculate (play with with GFP_DMA
/ GFP_DMA32 flags, fail requests if actual allocated memory does not
match mask).

*) Although phys_to_dma() / dma_to_phys() take struct device argument
and thus can potentially do device-specific translations, there is no
infrastructure to do bridge-specific translation. For example, RCAR PCIe
controller can define several windows if host memory for inbound PCIe
transactions, that can be configured via device tree - but won't work at
runtime.

*) The way how arch_setup_dma_ops() is called for PCI devices on
platforms using device tree, does not pass whatever host bridge specific
information. Call chain is via of_dma_configure(), that consults
dma-ranges of controller's node's parent, not controller node itself.

*) Format of dma-ranges used by several PCIe host bridges is NOT the
same as format that of_dma_configure() expects. Thus fixing node used in
of_dma_configure() call won't help, unless binding is changed, which
required fixing multiple drivers.

*) Generally speaking, usage of masks to define range limitations looks
obsolete. It was ok to describe limited DMA address width in individual
devices, but does not fit well with modern architectures with bridges /
translations/ iommus / whatever.


If trying to avoid big changes and only fixing particular problem with
particular device not working on arm64, I think best way is to
alter__swiotlb_dma_supported() in arch/arm64/mm/dma-mapping.c to detect
and decline (with -EIO) mask that is unsupported by device connection.
This will cover both dma_mask and coherent_dma_mask.

This has to be combined with some way to explicitly extract information
about limitations. Checking device parent's dma masks won't work unless
somebody bothers to populate them.


Nikita

arm64: mm: bug around swiotlb_dma_ops

[Nikita Yushchenko]

> If trying to avoid big changes and only fixing particular problem with
> particular device not working on arm64, I think best way is to
> alter__swiotlb_dma_supported() in arch/arm64/mm/dma-mapping.c to detect
> and decline (with -EIO) mask that is unsupported by device connection.
> This will cover both dma_mask and coherent_dma_mask.
> 
> This has to be combined with some way to explicitly extract information
> about limitations. Checking device parent's dma masks won't work unless
> somebody bothers to populate them.

I'm trying to bring together a patch doing this, will post that when ready