swiotlb: Remove SWIOTLB overflow buffer support

Remove SWIOTLB overflow buffer support and return DMA_ERROR_CODE (a value of zero) to make it consistent with iommu implementation on Intel, AMD, and swiotlb-xen.

Tested only on x86.

Signed-off-by Shuah Khan <[email protected]>

---
 lib/swiotlb.c |   44 ++++++++------------------------------------
 1 file changed, 8 insertions(+), 36 deletions(-)

lib/swiotlb.c +8 -36

index 45bc1f8..7f0a5d1 100644
@@ -15,6 +15,7 @@
  * 05/09/10 linville	Add support for syncing ranges, support syncing for
  *			DMA_BIDIRECTIONAL mappings, miscellaneous cleanup.
  * 08/12/11 beckyb	Add highmem support
+ * 06/12    shuahkhan	Remove io tlb overflow support
  */
 
 #include <linux/cache.h>

@@ -66,13 +67,6 @@ static char *io_tlb_start, *io_tlb_end;
 static unsigned long io_tlb_nslabs;
 
 /*
- * When the IOMMU overflows we return a fallback buffer. This sets the size.
- */
-static unsigned long io_tlb_overflow = 32*1024;
-
-static void *io_tlb_overflow_buffer;
-
-/*
  * This is a free list describing the number of free entries available from
  * each index
  */

@@ -108,7 +102,6 @@ setup_io_tlb_npages(char *str)
 	return 1;
 }
 __setup("swiotlb=", setup_io_tlb_npages);
-/* make io_tlb_overflow tunable too? */
 
 unsigned long swiotlb_nr_tbl(void)
 {

@@ -156,12 +149,6 @@ void __init swiotlb_init_with_tbl(char *tlb, unsigned long nslabs, int verbose)
 	io_tlb_index = 0;
 	io_tlb_orig_addr = alloc_bootmem_pages(PAGE_ALIGN(io_tlb_nslabs * sizeof(phys_addr_t)));
 
-	/*
-	 * Get the overflow emergency buffer
-	 */
-	io_tlb_overflow_buffer = alloc_bootmem_low_pages(PAGE_ALIGN(io_tlb_overflow));
-	if (!io_tlb_overflow_buffer)
-		panic("Cannot allocate SWIOTLB overflow buffer!\n");
 	if (verbose)
 		swiotlb_print_info();
 }

@@ -195,7 +182,8 @@ swiotlb_init_with_default_size(size_t default_size, int verbose)
 void __init
 swiotlb_init(int verbose)
 {
-	swiotlb_init_with_default_size(64 * (1<<20), verbose);	/* default to 64MB */
+	/* default to 64MB */
+	swiotlb_init_with_default_size(64 * (1<<20), verbose);
 }
 
 /*

@@ -264,24 +252,12 @@ swiotlb_late_init_with_default_size(size_t default_size)
 
 	memset(io_tlb_orig_addr, 0, io_tlb_nslabs * sizeof(phys_addr_t));
 
-	/*
-	 * Get the overflow emergency buffer
-	 */
-	io_tlb_overflow_buffer = (void *)__get_free_pages(GFP_DMA,
-	                                          get_order(io_tlb_overflow));
-	if (!io_tlb_overflow_buffer)
-		goto cleanup4;
-
 	swiotlb_print_info();
 
 	late_alloc = 1;
 
 	return 0;
 
-cleanup4:
-	free_pages((unsigned long)io_tlb_orig_addr,
-		   get_order(io_tlb_nslabs * sizeof(phys_addr_t)));
-	io_tlb_orig_addr = NULL;
 cleanup3:
 	free_pages((unsigned long)io_tlb_list, get_order(io_tlb_nslabs *
 	                                                 sizeof(int)));

@@ -297,12 +273,10 @@ cleanup1:
 
 void __init swiotlb_free(void)
 {
-	if (!io_tlb_overflow_buffer)
+	if (!io_tlb_orig_addr)
 		return;
 
 	if (late_alloc) {
-		free_pages((unsigned long)io_tlb_overflow_buffer,
-			   get_order(io_tlb_overflow));
 		free_pages((unsigned long)io_tlb_orig_addr,
 			   get_order(io_tlb_nslabs * sizeof(phys_addr_t)));
 		free_pages((unsigned long)io_tlb_list, get_order(io_tlb_nslabs *

@@ -310,8 +284,6 @@ void __init swiotlb_free(void)
 		free_pages((unsigned long)io_tlb_start,
 			   get_order(io_tlb_nslabs << IO_TLB_SHIFT));
 	} else {
-		free_bootmem_late(__pa(io_tlb_overflow_buffer),
-				  PAGE_ALIGN(io_tlb_overflow));
 		free_bootmem_late(__pa(io_tlb_orig_addr),
 				  PAGE_ALIGN(io_tlb_nslabs * sizeof(phys_addr_t)));
 		free_bootmem_late(__pa(io_tlb_list),

@@ -639,7 +611,7 @@ swiotlb_full(struct device *dev, size_t size, enum dma_data_direction dir,
 	printk(KERN_ERR "DMA: Out of SW-IOMMU space for %zu bytes at "
 	       "device %s\n", size, dev ? dev_name(dev) : "?");
 
-	if (size <= io_tlb_overflow || !do_panic)
+	if (!do_panic)
 		return;
 
 	if (dir == DMA_BIDIRECTIONAL)

@@ -681,7 +653,7 @@ dma_addr_t swiotlb_map_page(struct device *dev, struct page *page,
 	map = map_single(dev, phys, size, dir);
 	if (!map) {
 		swiotlb_full(dev, size, dir, 1);
-		map = io_tlb_overflow_buffer;
+		return DMA_ERROR_CODE;
 	}
 
 	dev_addr = swiotlb_virt_to_bus(dev, map);

@@ -691,7 +663,7 @@ dma_addr_t swiotlb_map_page(struct device *dev, struct page *page,
 	 */
 	if (!dma_capable(dev, dev_addr, size)) {
 		swiotlb_tbl_unmap_single(dev, map, size, dir);
-		dev_addr = swiotlb_virt_to_bus(dev, io_tlb_overflow_buffer);
+		dev_addr = 0;
 	}
 
 	return dev_addr;

@@ -910,7 +882,7 @@ EXPORT_SYMBOL(swiotlb_sync_sg_for_device);
 int
 swiotlb_dma_mapping_error(struct device *hwdev, dma_addr_t dma_addr)
 {
-	return (dma_addr == swiotlb_virt_to_bus(hwdev, io_tlb_overflow_buffer));
+	return !dma_addr;
 }
 EXPORT_SYMBOL(swiotlb_dma_mapping_error);
 

Signature · 1 line

1.7.9.5

Quoted 3 lines · Shuah Khan

Remove SWIOTLB overflow buffer support and return DMA_ERROR_CODE (a value of zero) to make it consistent with iommu implementation on Intel, AMD, and swiotlb-xen.

While this is a good forward step and this needs to be done eventually, you should first send out patches for the drivers that don't check for the DMA_ERROR_CODE when doing mapping. In other words for the drivers that map but don't call dma_mapping_error to check.

When that is fixed and *all the drivers that don't call dma_mapping_error are fixed, then this patch makes sense.

So for right now, NACK.

Quoted 3 lines · Shuah Khan

Remove SWIOTLB overflow buffer support and return DMA_ERROR_CODE (a value of zero) to make it consistent with iommu implementation on Intel, AMD, and swiotlb-xen.

While this is a good forward step and this needs to be done eventually, you should first send out patches for the drivers that don't check for the DMA_ERROR_CODE when doing mapping. In other words for the drivers that map but don't call dma_mapping_error to check.

When that is fixed and *all the drivers that don't call dma_mapping_error are fixed, then this patch makes sense.

Quoted 10 lines · Konrad Rzeszutek Wilk

Quoted 3 lines · Shuah Khan

Remove SWIOTLB overflow buffer support and return DMA_ERROR_CODE (a value of zero) to make it consistent with iommu implementation on Intel, AMD, and swiotlb-xen.

While this is a good forward step and this needs to be done eventually, you should first send out patches for the drivers that don't check for the DMA_ERROR_CODE when doing mapping. In other words for the drivers that map but don't call dma_mapping_error to check.

Quoted 2 lines

When that is fixed and *all the drivers that don't call dma_mapping_error are fixed, then this patch makes sense.

The challenge will be catching all the drivers and have confidence that all of them are covered. I will start looking into this to get a feel for how many drivers needs fixing.

Also, isn't this problem today with other iommu implementations? All of the other iommu implementation don't have support for overflow buffers?

Quoted 11 lines · Shuah Khan

Quoted 10 lines · Konrad Rzeszutek Wilk

Quoted 3 lines · Shuah Khan

Remove SWIOTLB overflow buffer support and return DMA_ERROR_CODE (a value of zero) to make it consistent with iommu implementation on Intel, AMD, and swiotlb-xen.

While this is a good forward step and this needs to be done eventually, you should first send out patches for the drivers that don't check for the DMA_ERROR_CODE when doing mapping. In other words for the drivers that map but don't call dma_mapping_error to check.

Quoted 6 lines

Quoted 2 lines

When that is fixed and *all the drivers that don't call dma_mapping_error are fixed, then this patch makes sense.

The challenge will be catching all the drivers and have confidence that all of them are covered. I will start looking into this to get a feel for how many drivers needs fixing.

I don't know if all is needed. Some of them might be dead or not used at all anymore - who knows? This treasure hunt would give a good idea of which ones are not using the PCI/DMA API right at least.

If it is say, CONFIG_ISA enabled, well, we could #ifdef the overflow buffer in the swiotlb with that option. And then work through fixing up those drivers - except that finding folks with that driver to see if it works .. yuck.. I do have some few ISA cards and some boxes with ISA slots :-)

Quoted 23 lines · Konrad Rzeszutek Wilk

Quoted 11 lines · Shuah Khan

Quoted 10 lines · Konrad Rzeszutek Wilk

Quoted 3 lines · Shuah Khan

Remove SWIOTLB overflow buffer support and return DMA_ERROR_CODE (a value of zero) to make it consistent with iommu implementation on Intel, AMD, and swiotlb-xen.

While this is a good forward step and this needs to be done eventually, you should first send out patches for the drivers that don't check for the DMA_ERROR_CODE when doing mapping. In other words for the drivers that map but don't call dma_mapping_error to check.

Quoted 6 lines

Quoted 2 lines

When that is fixed and *all the drivers that don't call dma_mapping_error are fixed, then this patch makes sense.

The challenge will be catching all the drivers and have confidence that all of them are covered. I will start looking into this to get a feel for how many drivers needs fixing.

I don't know if all is needed. Some of them might be dead or not used at all anymore - who knows? This treasure hunt would give a good idea of which ones are not using the PCI/DMA API right at least.

Quoted 5 lines

If it is say, CONFIG_ISA enabled, well, we could #ifdef the overflow buffer in the swiotlb with that option. And then work through fixing up those drivers - except that finding folks with that driver to see if it works .. yuck.. I do have some few ISA cards and some boxes with ISA slots :-)

Looking at the history CONFIG_ISA was disabled back in 2002 and CONFIG_ISA_DMA_API was disabled in 2004. Sounds like it will be fun finding drivers that could fail.

Disable iotlb overflow support when CONFIG_ISA is enabled. This is the first step towards removing overflow support, to be consistent with other iommu implementations and return DMA_ERROR_CODE. This disabling step is for finding drivers that don't call dma_mapping_error to check for errors returned by the mapping interface. Once drivers are fixed overflow support can be removed.

Tested on x86 with io_tlb_overflow = 32*1024 and io_tlb_overflow = 0.

Signed-off-by Shuah Khan <[email protected]>

---
 lib/swiotlb.c |   65 +++++++++++++++++++++++++++++++++++++++++----------------
 1 file changed, 47 insertions(+), 18 deletions(-)

lib/swiotlb.c +47 -18

index 45bc1f8..f7d285c 100644
@@ -15,6 +15,9 @@
  * 05/09/10 linville	Add support for syncing ranges, support syncing for
  *			DMA_BIDIRECTIONAL mappings, miscellaneous cleanup.
  * 08/12/11 beckyb	Add highmem support
+ * 07/2012  shuahkhan	Disable iotlb overflow support when CONFIG_ISA
+ *			is enabled. Remove it for all configs when drivers
+ *			that don't check for mapping errors are fixed.
  */
 
 #include <linux/cache.h>

@@ -68,7 +71,19 @@ static unsigned long io_tlb_nslabs;
 /*
  * When the IOMMU overflows we return a fallback buffer. This sets the size.
  */
+#if defined(CONFIG_ISA)
+/*
+ * Disable iotlb overflow support when CONFIG_ISA is enabled. This is the
+ * first step towards removing overflow support, to be consistent with other
+ * iommu implementations and return DMA_ERROR_CODE. This disabling step is
+ * for finding drivers that don't call dma_mapping_error to check for errors
+ * returned by the mapping interface. Once drivers are fixed overflow support
+ * can be removed.
+*/
+static unsigned long io_tlb_overflow;
+#else
 static unsigned long io_tlb_overflow = 32*1024;
+#endif
 
 static void *io_tlb_overflow_buffer;
 

@@ -156,12 +171,16 @@ void __init swiotlb_init_with_tbl(char *tlb, unsigned long nslabs, int verbose)
 	io_tlb_index = 0;
 	io_tlb_orig_addr = alloc_bootmem_pages(PAGE_ALIGN(io_tlb_nslabs * sizeof(phys_addr_t)));
 
-	/*
-	 * Get the overflow emergency buffer
-	 */
-	io_tlb_overflow_buffer = alloc_bootmem_low_pages(PAGE_ALIGN(io_tlb_overflow));
-	if (!io_tlb_overflow_buffer)
-		panic("Cannot allocate SWIOTLB overflow buffer!\n");
+	if (io_tlb_overflow) {
+		/*
+		 * Get the overflow emergency buffer
+		 */
+		io_tlb_overflow_buffer = alloc_bootmem_low_pages(
+						PAGE_ALIGN(io_tlb_overflow));
+		if (!io_tlb_overflow_buffer)
+			panic("Cannot allocate SWIOTLB overflow buffer!\n");
+	}
+
 	if (verbose)
 		swiotlb_print_info();
 }

@@ -264,13 +283,15 @@ swiotlb_late_init_with_default_size(size_t default_size)
 
 	memset(io_tlb_orig_addr, 0, io_tlb_nslabs * sizeof(phys_addr_t));
 
-	/*
-	 * Get the overflow emergency buffer
-	 */
-	io_tlb_overflow_buffer = (void *)__get_free_pages(GFP_DMA,
-	                                          get_order(io_tlb_overflow));
-	if (!io_tlb_overflow_buffer)
-		goto cleanup4;
+	if (io_tlb_overflow) {
+		/*
+		 * Get the overflow emergency buffer
+		 */
+		io_tlb_overflow_buffer = (void *)
+			__get_free_pages(GFP_DMA, get_order(io_tlb_overflow));
+		if (!io_tlb_overflow_buffer)
+			goto cleanup4;
+	}
 
 	swiotlb_print_info();
 

@@ -297,12 +318,13 @@ cleanup1:
 
 void __init swiotlb_free(void)
 {
-	if (!io_tlb_overflow_buffer)
+	if (!io_tlb_orig_addr)
 		return;
 
 	if (late_alloc) {
-		free_pages((unsigned long)io_tlb_overflow_buffer,
-			   get_order(io_tlb_overflow));
+		if (io_tlb_overflow_buffer)
+			free_pages((unsigned long)io_tlb_overflow_buffer,
+				   get_order(io_tlb_overflow));
 		free_pages((unsigned long)io_tlb_orig_addr,
 			   get_order(io_tlb_nslabs * sizeof(phys_addr_t)));
 		free_pages((unsigned long)io_tlb_list, get_order(io_tlb_nslabs *

@@ -310,8 +332,9 @@ void __init swiotlb_free(void)
 		free_pages((unsigned long)io_tlb_start,
 			   get_order(io_tlb_nslabs << IO_TLB_SHIFT));
 	} else {
-		free_bootmem_late(__pa(io_tlb_overflow_buffer),
-				  PAGE_ALIGN(io_tlb_overflow));
+		if (io_tlb_overflow_buffer)
+			free_bootmem_late(__pa(io_tlb_overflow_buffer),
+					  PAGE_ALIGN(io_tlb_overflow));
 		free_bootmem_late(__pa(io_tlb_orig_addr),
 				  PAGE_ALIGN(io_tlb_nslabs * sizeof(phys_addr_t)));
 		free_bootmem_late(__pa(io_tlb_list),

@@ -681,6 +704,8 @@ dma_addr_t swiotlb_map_page(struct device *dev, struct page *page,
 	map = map_single(dev, phys, size, dir);
 	if (!map) {
 		swiotlb_full(dev, size, dir, 1);
+		if (!io_tlb_overflow)
+			return DMA_ERROR_CODE;
 		map = io_tlb_overflow_buffer;
 	}
 

@@ -691,6 +716,8 @@ dma_addr_t swiotlb_map_page(struct device *dev, struct page *page,
 	 */
 	if (!dma_capable(dev, dev_addr, size)) {
 		swiotlb_tbl_unmap_single(dev, map, size, dir);
+		if (!io_tlb_overflow)
+			return DMA_ERROR_CODE;
 		dev_addr = swiotlb_virt_to_bus(dev, io_tlb_overflow_buffer);
 	}
 

@@ -910,6 +937,8 @@ EXPORT_SYMBOL(swiotlb_sync_sg_for_device);
 int
 swiotlb_dma_mapping_error(struct device *hwdev, dma_addr_t dma_addr)
 {
+	if (!io_tlb_overflow)
+		return DMA_ERROR_CODE;
 	return (dma_addr == swiotlb_virt_to_bus(hwdev, io_tlb_overflow_buffer));
 }
 EXPORT_SYMBOL(swiotlb_dma_mapping_error);

Signature · 1 line

1.7.9.5

Quoted 6 lines · Shuah Khan

Disable iotlb overflow support when CONFIG_ISA is enabled. This is the first step towards removing overflow support, to be consistent with other iommu implementations and return DMA_ERROR_CODE. This disabling step is for finding drivers that don't call dma_mapping_error to check for errors returned by the mapping interface. Once drivers are fixed overflow support can be removed.

I think I explained myself poorly. We don't want to break old drivers.

If the old drivers expect this behavior (while they should be updated to check the DMA), we need to retain this behavior.

So I think the patch should be done the other way:

Disable IOTLB overflow when CONFIG_ISA is disabled.

But we also need to check whether there are other drivers that don't properly check the DMA address. And if so, add them to this list of must have enabled b/c you might be using this driver.

The first goal is to figure out which of the drivers aren't doing this properly. This should be possible by just grepping for 'dma_map' and seeing which ones don't do the 'dma_check' right after.

Quoted 13 lines · Konrad Rzeszutek Wilk

Quoted 6 lines · Shuah Khan

Disable iotlb overflow support when CONFIG_ISA is enabled. This is the first step towards removing overflow support, to be consistent with other iommu implementations and return DMA_ERROR_CODE. This disabling step is for finding drivers that don't call dma_mapping_error to check for errors returned by the mapping interface. Once drivers are fixed overflow support can be removed.

I think I explained myself poorly. We don't want to break old drivers.

If the old drivers expect this behavior (while they should be updated to check the DMA), we need to retain this behavior.

Quoted 3 lines

So I think the patch should be done the other way:

Disable IOTLB overflow when CONFIG_ISA is disabled.

That makes lot of sense :) I will redo the patch. It might be good to a tunable to control enable/disable behavior. Even though support for this new tunable "swiotlb_overflow" projected to be short lived, it will be lot less painful to disable/enable as needed. Thought and comments?

Quoted 6 lines

But we also need to check whether there are other drivers that don't properly check the DMA address. And if so, add them to this list of must have enabled b/c you might be using this driver.

Sound good. Again having a tunable would help in this case as well.

Quoted 4 lines

The first goal is to figure out which of the drivers aren't doing this properly. This should be possible by just grepping for 'dma_map' and seeing which ones don't do the 'dma_check' right after.

I started some research into this and will continue and provide an update.

Quoted 14 lines · Shuah Khan

Quoted 13 lines · Konrad Rzeszutek Wilk

Quoted 6 lines · Shuah Khan

Disable iotlb overflow support when CONFIG_ISA is enabled. This is the first step towards removing overflow support, to be consistent with other iommu implementations and return DMA_ERROR_CODE. This disabling step is for finding drivers that don't call dma_mapping_error to check for errors returned by the mapping interface. Once drivers are fixed overflow support can be removed.

I think I explained myself poorly. We don't want to break old drivers.

If the old drivers expect this behavior (while they should be updated to check the DMA), we need to retain this behavior.

Quoted 8 lines

Quoted 3 lines

So I think the patch should be done the other way:

Disable IOTLB overflow when CONFIG_ISA is disabled.

That makes lot of sense :) I will redo the patch. It might be good to a tunable to control enable/disable behavior. Even though support for this new tunable "swiotlb_overflow" projected to be short lived, it will be lot less painful to disable/enable as needed. Thought and comments?

Tunnable have a habit of becoming permanant and we really don't want that. The right solution is to drop the overflow - as you have rightly pointed out in the first patch. But the road is blocked by these old drivers that are making it hard.

I would say if you really really want a patch in for 3.6 before the final mega-patch of drivers-that-suck-and-need-to-be-updated-here-is -the-big-patch-that-does-it, then just add a WARN mentioning your name and mine (and LKML) saying to report what you see. That way we can at least get feedback from the field.

And for good measure include in the Documentation/*deprecate-schedule that the overflow functionality is going away when all the drivers that depend on it have been fixed up.

Quoted 14 lines

Quoted 7 lines

But we also need to check whether there are other drivers that don't properly check the DMA address. And if so, add them to this list of must have enabled b/c you might be using this driver.

Sound good. Again having a tunable would help in this case as well.

Quoted 4 lines

The first goal is to figure out which of the drivers aren't doing this properly. This should be possible by just grepping for 'dma_map' and seeing which ones don't do the 'dma_check' right after.

Quoted 2 lines

I started some research into this and will continue and provide an update.

Thank you! It is pretty griddy work so I really appreciate you taking the time to do this. If are at the LinuxCon this year the beers (or coffee if that is your preference) are on me for tackling on this cleanup.

Quoted 4 lines

Quoted 3 lines

The first goal is to figure out which of the drivers aren't doing this properly. This should be possible by just grepping for 'dma_map' and seeing which ones don't do the 'dma_check' right after.