[PATCH v5] modify the IO_TLB_SEGSIZE and IO_TLB_DEFAULT_SIZE configurable as flexible requirement about SW-IOMMU.

[PATCH v5] modify the IO_TLB_SEGSIZE and IO_TLB_DEFAULT_SIZE configurable as flexible requirement about SW-IOMMU.

[Wang Xiaoming]

The maximum of SW-IOMMU is limited to 2^11*128 = 256K.
And the size of IO_TLB_DEFAULT_SIZE is limited to (64UL<<20) 64M now.
While in different platform and different requirement this seems improper.
So modifing the IO_TLB_SEGSIZE to io_tlb_segsize and IO_TLB_DEFAULT_SIZE
to io_tlb_default_size which can configure by kernel cmdline.
This can meet different requirement.

Signed-off-by: Chuansheng Liu <chuansheng.liu@intel.com>
Signed-off-by: Zhang Dongxing <dongxing.zhang@intel.com>
Signed-off-by: Wang Xiaoming <xiaoming.wang@intel.com>
---
patch v1 make this change at Kconfig
which needs to edit the .config manually.
https://lkml.org/lkml/2015/1/25/571

patch v2 only change IO_TLB_SEGSIZE configurable.
https://lkml.org/lkml/2015/2/5/812

patch v3 parsing io_tlb_segsize and 
io_tlb_default_size independently.
https://lkml.org/lkml/2015/2/15/217

patch v4 hasn't validated the data from
command line.
https://lkml.org/lkml/2015/2/17/114

 Documentation/kernel-parameters.txt  |    9 ++++-
 arch/mips/cavium-octeon/dma-octeon.c |    2 +-
 arch/mips/netlogic/common/nlm-dma.c  |    2 +-
 drivers/xen/swiotlb-xen.c            |    6 +--
 include/linux/swiotlb.h              |    8 +---
 lib/swiotlb.c                        |   68 +++++++++++++++++++++++++---------
 6 files changed, 65 insertions(+), 30 deletions(-)

diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
index 4df73da..1f50e86 100644
--- a/Documentation/kernel-parameters.txt
+++ b/Documentation/kernel-parameters.txt
@@ -3438,10 +3438,17 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
 			it if 0 is given (See Documentation/cgroups/memory.txt)
 
 	swiotlb=	[ARM,IA-64,PPC,MIPS,X86]
-			Format: { <int> | force }
+			Format: { <int> | force | <int> | <int>}
 			<int> -- Number of I/O TLB slabs
 			force -- force using of bounce buffers even if they
 			         wouldn't be automatically used by the kernel
+			<int> -- Maximum allowable number of contiguous slabs to map
+			<int> -- The size of SW-MMU mapped.
+			Using "," to separate them one by one.
+			Example:
+			BOARD_KERNEL_CMDLINE += swiotlb=32768,force,512,268435456
+			io_tlb_nslabs=32768, swiotlb_force=1, 
+			io_tlb_segsize=512, io_tlb_default_size=268435456
 
 	switches=	[HW,M68k]
 
diff --git a/arch/mips/cavium-octeon/dma-octeon.c b/arch/mips/cavium-octeon/dma-octeon.c
index 3778655..a521af6 100644
--- a/arch/mips/cavium-octeon/dma-octeon.c
+++ b/arch/mips/cavium-octeon/dma-octeon.c
@@ -312,7 +312,7 @@ void __init plat_swiotlb_setup(void)
 		swiotlbsize = 64 * (1<<20);
 #endif
 	swiotlb_nslabs = swiotlbsize >> IO_TLB_SHIFT;
-	swiotlb_nslabs = ALIGN(swiotlb_nslabs, IO_TLB_SEGSIZE);
+	swiotlb_nslabs = ALIGN(swiotlb_nslabs, io_tlb_segsize);
 	swiotlbsize = swiotlb_nslabs << IO_TLB_SHIFT;
 
 	octeon_swiotlb = alloc_bootmem_low_pages(swiotlbsize);
diff --git a/arch/mips/netlogic/common/nlm-dma.c b/arch/mips/netlogic/common/nlm-dma.c
index f3d4ae8..eeffa8f 100644
--- a/arch/mips/netlogic/common/nlm-dma.c
+++ b/arch/mips/netlogic/common/nlm-dma.c
@@ -99,7 +99,7 @@ void __init plat_swiotlb_setup(void)
 
 	swiotlbsize = 1 << 20; /* 1 MB for now */
 	swiotlb_nslabs = swiotlbsize >> IO_TLB_SHIFT;
-	swiotlb_nslabs = ALIGN(swiotlb_nslabs, IO_TLB_SEGSIZE);
+	swiotlb_nslabs = ALIGN(swiotlb_nslabs, io_tlb_segsize);
 	swiotlbsize = swiotlb_nslabs << IO_TLB_SHIFT;
 
 	nlm_swiotlb = alloc_bootmem_low_pages(swiotlbsize);
diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c
index 810ad41..3b3e9fe 100644
--- a/drivers/xen/swiotlb-xen.c
+++ b/drivers/xen/swiotlb-xen.c
@@ -164,11 +164,11 @@ xen_swiotlb_fixup(void *buf, size_t size, unsigned long nslabs)
 	dma_addr_t dma_handle;
 	phys_addr_t p = virt_to_phys(buf);
 
-	dma_bits = get_order(IO_TLB_SEGSIZE << IO_TLB_SHIFT) + PAGE_SHIFT;
+	dma_bits = get_order(io_tlb_segsize << IO_TLB_SHIFT) + PAGE_SHIFT;
 
 	i = 0;
 	do {
-		int slabs = min(nslabs - i, (unsigned long)IO_TLB_SEGSIZE);
+		int slabs = min(nslabs - i, (unsigned long)io_tlb_segsize);
 
 		do {
 			rc = xen_create_contiguous_region(
@@ -187,7 +187,7 @@ static unsigned long xen_set_nslabs(unsigned long nr_tbl)
 {
 	if (!nr_tbl) {
 		xen_io_tlb_nslabs = (64 * 1024 * 1024 >> IO_TLB_SHIFT);
-		xen_io_tlb_nslabs = ALIGN(xen_io_tlb_nslabs, IO_TLB_SEGSIZE);
+		xen_io_tlb_nslabs = ALIGN(xen_io_tlb_nslabs, io_tlb_segsize);
 	} else
 		xen_io_tlb_nslabs = nr_tbl;
 
diff --git a/include/linux/swiotlb.h b/include/linux/swiotlb.h
index e7a018e..13506db 100644
--- a/include/linux/swiotlb.h
+++ b/include/linux/swiotlb.h
@@ -8,13 +8,7 @@ struct dma_attrs;
 struct scatterlist;
 
 extern int swiotlb_force;
-
-/*
- * Maximum allowable number of contiguous slabs to map,
- * must be a power of 2.  What is the appropriate value ?
- * The complexity of {map,unmap}_single is linearly dependent on this value.
- */
-#define IO_TLB_SEGSIZE	128
+extern int io_tlb_segsize;
 
 /*
  * log of the size of each IO TLB slab.  The number of slabs is command line
diff --git a/lib/swiotlb.c b/lib/swiotlb.c
index 4abda07..3b71afd 100644
--- a/lib/swiotlb.c
+++ b/lib/swiotlb.c
@@ -56,6 +56,24 @@
 int swiotlb_force;
 
 /*
+ * default to 128
+ * Maximum allowable number of contiguous slabs to map,
+ * must be a power of 2.  What is the appropriate value ?
+ * define io_tlb_segsize as a parameter
+ * which can be changed dynamically in config file for special usage.
+ * The complexity of {map,unmap}_single is linearly dependent on this value.
+ */
+#define IO_TLB_SEGSIZE 128
+int io_tlb_segsize = IO_TLB_SEGSIZE;
+
+/* default to 64MB 
+ * define io_tlb_default_size as a parameter
+ * which can be changed dynamically in config file for special usage.
+ */
+#define IO_TLB_DEFAULT_SIZE (64UL<<20)
+static unsigned long io_tlb_default_size = IO_TLB_DEFAULT_SIZE;
+
+/*
  * Used to do a quick range check in swiotlb_tbl_unmap_single and
  * swiotlb_tbl_sync_single_*, to see if the memory was in fact allocated by this
  * API.
@@ -101,13 +119,32 @@ setup_io_tlb_npages(char *str)
 {
 	if (isdigit(*str)) {
 		io_tlb_nslabs = simple_strtoul(str, &str, 0);
-		/* avoid tail segment of size < IO_TLB_SEGSIZE */
-		io_tlb_nslabs = ALIGN(io_tlb_nslabs, IO_TLB_SEGSIZE);
 	}
 	if (*str == ',')
 		++str;
-	if (!strcmp(str, "force"))
+	if (!strncmp(str, "force", 5)) {
 		swiotlb_force = 1;
+		str += 5;
+	}
+	if (*str == ',')
+		++str;
+	if (isdigit(*str)) {
+		int n = 0;
+		io_tlb_segsize = simple_strtoul(str, &str, 0);
+		io_tlb_segsize = ALIGN(io_tlb_segsize, IO_TLB_SEGSIZE);
+		while ((io_tlb_segsize - 1) >> n)
+			n++;
+		io_tlb_segsize = (1 << n);
+	}
+	if (*str == ',')
+		++str;
+	if (isdigit(*str)) {
+		io_tlb_default_size = simple_strtoul(str, &str, 0);
+		io_tlb_default_size = ALIGN(io_tlb_default_size, IO_TLB_DEFAULT_SIZE);
+	}
+
+	/* avoid tail segment of size < io_tlb_segsize */
+	io_tlb_nslabs = ALIGN(io_tlb_nslabs, io_tlb_segsize);
 
 	return 0;
 }
@@ -120,15 +157,13 @@ unsigned long swiotlb_nr_tbl(void)
 }
 EXPORT_SYMBOL_GPL(swiotlb_nr_tbl);
 
-/* default to 64MB */
-#define IO_TLB_DEFAULT_SIZE (64UL<<20)
 unsigned long swiotlb_size_or_default(void)
 {
 	unsigned long size;
 
 	size = io_tlb_nslabs << IO_TLB_SHIFT;
 
-	return size ? size : (IO_TLB_DEFAULT_SIZE);
+	return size ? size : (io_tlb_default_size);
 }
 
 /* Note that this doesn't work with highmem page */
@@ -183,7 +218,7 @@ int __init swiotlb_init_with_tbl(char *tlb, unsigned long nslabs, int verbose)
 
 	/*
 	 * Allocate and initialize the free list array.  This array is used
-	 * to find contiguous free memory regions of size up to IO_TLB_SEGSIZE
+	 * to find contiguous free memory regions of size up to io_tlb_segsize
 	 * between io_tlb_start and io_tlb_end.
 	 */
 	io_tlb_list = memblock_virt_alloc(
@@ -193,7 +228,7 @@ int __init swiotlb_init_with_tbl(char *tlb, unsigned long nslabs, int verbose)
 				PAGE_ALIGN(io_tlb_nslabs * sizeof(phys_addr_t)),
 				PAGE_SIZE);
 	for (i = 0; i < io_tlb_nslabs; i++) {
-		io_tlb_list[i] = IO_TLB_SEGSIZE - OFFSET(i, IO_TLB_SEGSIZE);
+		io_tlb_list[i] = io_tlb_segsize - OFFSET(i, io_tlb_segsize);
 		io_tlb_orig_addr[i] = INVALID_PHYS_ADDR;
 	}
 	io_tlb_index = 0;
@@ -211,13 +246,12 @@ int __init swiotlb_init_with_tbl(char *tlb, unsigned long nslabs, int verbose)
 void  __init
 swiotlb_init(int verbose)
 {
-	size_t default_size = IO_TLB_DEFAULT_SIZE;
 	unsigned char *vstart;
 	unsigned long bytes;
 
 	if (!io_tlb_nslabs) {
-		io_tlb_nslabs = (default_size >> IO_TLB_SHIFT);
-		io_tlb_nslabs = ALIGN(io_tlb_nslabs, IO_TLB_SEGSIZE);
+		io_tlb_nslabs = (io_tlb_default_size >> IO_TLB_SHIFT);
+		io_tlb_nslabs = ALIGN(io_tlb_nslabs, io_tlb_segsize);
 	}
 
 	bytes = io_tlb_nslabs << IO_TLB_SHIFT;
@@ -249,7 +283,7 @@ swiotlb_late_init_with_default_size(size_t default_size)
 
 	if (!io_tlb_nslabs) {
 		io_tlb_nslabs = (default_size >> IO_TLB_SHIFT);
-		io_tlb_nslabs = ALIGN(io_tlb_nslabs, IO_TLB_SEGSIZE);
+		io_tlb_nslabs = ALIGN(io_tlb_nslabs, io_tlb_segsize);
 	}
 
 	/*
@@ -308,7 +342,7 @@ swiotlb_late_init_with_tbl(char *tlb, unsigned long nslabs)
 
 	/*
 	 * Allocate and initialize the free list array.  This array is used
-	 * to find contiguous free memory regions of size up to IO_TLB_SEGSIZE
+	 * to find contiguous free memory regions of size up to io_tlb_segsize
 	 * between io_tlb_start and io_tlb_end.
 	 */
 	io_tlb_list = (unsigned int *)__get_free_pages(GFP_KERNEL,
@@ -324,7 +358,7 @@ swiotlb_late_init_with_tbl(char *tlb, unsigned long nslabs)
 		goto cleanup4;
 
 	for (i = 0; i < io_tlb_nslabs; i++) {
-		io_tlb_list[i] = IO_TLB_SEGSIZE - OFFSET(i, IO_TLB_SEGSIZE);
+		io_tlb_list[i] = io_tlb_segsize - OFFSET(i, io_tlb_segsize);
 		io_tlb_orig_addr[i] = INVALID_PHYS_ADDR;
 	}
 	io_tlb_index = 0;
@@ -493,7 +527,7 @@ phys_addr_t swiotlb_tbl_map_single(struct device *hwdev,
 
 			for (i = index; i < (int) (index + nslots); i++)
 				io_tlb_list[i] = 0;
-			for (i = index - 1; (OFFSET(i, IO_TLB_SEGSIZE) != IO_TLB_SEGSIZE - 1) && io_tlb_list[i]; i--)
+			for (i = index - 1; (OFFSET(i, io_tlb_segsize) != io_tlb_segsize - 1) && io_tlb_list[i]; i--)
 				io_tlb_list[i] = ++count;
 			tlb_addr = io_tlb_start + (index << IO_TLB_SHIFT);
 
@@ -571,7 +605,7 @@ void swiotlb_tbl_unmap_single(struct device *hwdev, phys_addr_t tlb_addr,
 	 */
 	spin_lock_irqsave(&io_tlb_lock, flags);
 	{
-		count = ((index + nslots) < ALIGN(index + 1, IO_TLB_SEGSIZE) ?
+		count = ((index + nslots) < ALIGN(index + 1, io_tlb_segsize) ?
 			 io_tlb_list[index + nslots] : 0);
 		/*
 		 * Step 1: return the slots to the free list, merging the
@@ -585,7 +619,7 @@ void swiotlb_tbl_unmap_single(struct device *hwdev, phys_addr_t tlb_addr,
 		 * Step 2: merge the returned slots with the preceding slots,
 		 * if available (non zero)
 		 */
-		for (i = index - 1; (OFFSET(i, IO_TLB_SEGSIZE) != IO_TLB_SEGSIZE -1) && io_tlb_list[i]; i--)
+		for (i = index - 1; (OFFSET(i, io_tlb_segsize) != io_tlb_segsize -1) && io_tlb_list[i]; i--)
 			io_tlb_list[i] = ++count;
 	}
 	spin_unlock_irqrestore(&io_tlb_lock, flags);
-- 
1.7.9.5

Re: [PATCH v5] modify the IO_TLB_SEGSIZE and IO_TLB_DEFAULT_SIZE configurable as flexible requirement about SW-IOMMU.

[Konrad Rzeszutek Wilk]

On Tue, Mar 03, 2015 at 04:11:09PM +0800, Wang Xiaoming wrote:
> The maximum of SW-IOMMU is limited to 2^11*128 = 256K.
> And the size of IO_TLB_DEFAULT_SIZE is limited to (64UL<<20) 64M now.
> While in different platform and different requirement this seems improper.
> So modifing the IO_TLB_SEGSIZE to io_tlb_segsize and IO_TLB_DEFAULT_SIZE
> to io_tlb_default_size which can configure by kernel cmdline.
> This can meet different requirement.
> 
> Signed-off-by: Chuansheng Liu <chuansheng.liu@intel.com>
> Signed-off-by: Zhang Dongxing <dongxing.zhang@intel.com>
> Signed-off-by: Wang Xiaoming <xiaoming.wang@intel.com>
> ---
> patch v1 make this change at Kconfig
> which needs to edit the .config manually.
> https://lkml.org/lkml/2015/1/25/571
> 
> patch v2 only change IO_TLB_SEGSIZE configurable.
> https://lkml.org/lkml/2015/2/5/812
> 
> patch v3 parsing io_tlb_segsize and 
> io_tlb_default_size independently.
> https://lkml.org/lkml/2015/2/15/217
> 
> patch v4 hasn't validated the data from
> command line.

Thank you for redoing this per review.

> https://lkml.org/lkml/2015/2/17/114
> 
>  Documentation/kernel-parameters.txt  |    9 ++++-
>  arch/mips/cavium-octeon/dma-octeon.c |    2 +-
>  arch/mips/netlogic/common/nlm-dma.c  |    2 +-
>  drivers/xen/swiotlb-xen.c            |    6 +--
>  include/linux/swiotlb.h              |    8 +---
>  lib/swiotlb.c                        |   68 +++++++++++++++++++++++++---------
>  6 files changed, 65 insertions(+), 30 deletions(-)
> 
> diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
> index 4df73da..1f50e86 100644
> --- a/Documentation/kernel-parameters.txt
> +++ b/Documentation/kernel-parameters.txt
> @@ -3438,10 +3438,17 @@ bytes respectively. Such letter suffixes can also be entirely omitted.
>  			it if 0 is given (See Documentation/cgroups/memory.txt)
>  
>  	swiotlb=	[ARM,IA-64,PPC,MIPS,X86]
> -			Format: { <int> | force }
> +			Format: { <int> | force | <int> | <int>}
                                                        ,

s/|/,/

>  			<int> -- Number of I/O TLB slabs
>  			force -- force using of bounce buffers even if they
>  			         wouldn't be automatically used by the kernel
> +			<int> -- Maximum allowable number of contiguous slabs to map
> +			<int> -- The size of SW-MMU mapped.
> +			Using "," to separate them one by one.

"Use ',' to seperate them."

> +			Example:
> +			BOARD_KERNEL_CMDLINE += swiotlb=32768,force,512,268435456
> +			io_tlb_nslabs=32768, swiotlb_force=1, 
> +			io_tlb_segsize=512, io_tlb_default_size=268435456

I think you can remove the example - and just have it in the C code.

>  
>  	switches=	[HW,M68k]
>  
> diff --git a/arch/mips/cavium-octeon/dma-octeon.c b/arch/mips/cavium-octeon/dma-octeon.c
> index 3778655..a521af6 100644
> --- a/arch/mips/cavium-octeon/dma-octeon.c
> +++ b/arch/mips/cavium-octeon/dma-octeon.c
> @@ -312,7 +312,7 @@ void __init plat_swiotlb_setup(void)
>  		swiotlbsize = 64 * (1<<20);
>  #endif
>  	swiotlb_nslabs = swiotlbsize >> IO_TLB_SHIFT;
> -	swiotlb_nslabs = ALIGN(swiotlb_nslabs, IO_TLB_SEGSIZE);
> +	swiotlb_nslabs = ALIGN(swiotlb_nslabs, io_tlb_segsize);
>  	swiotlbsize = swiotlb_nslabs << IO_TLB_SHIFT;
>  
>  	octeon_swiotlb = alloc_bootmem_low_pages(swiotlbsize);
> diff --git a/arch/mips/netlogic/common/nlm-dma.c b/arch/mips/netlogic/common/nlm-dma.c
> index f3d4ae8..eeffa8f 100644
> --- a/arch/mips/netlogic/common/nlm-dma.c
> +++ b/arch/mips/netlogic/common/nlm-dma.c
> @@ -99,7 +99,7 @@ void __init plat_swiotlb_setup(void)
>  
>  	swiotlbsize = 1 << 20; /* 1 MB for now */
>  	swiotlb_nslabs = swiotlbsize >> IO_TLB_SHIFT;
> -	swiotlb_nslabs = ALIGN(swiotlb_nslabs, IO_TLB_SEGSIZE);
> +	swiotlb_nslabs = ALIGN(swiotlb_nslabs, io_tlb_segsize);
>  	swiotlbsize = swiotlb_nslabs << IO_TLB_SHIFT;
>  
>  	nlm_swiotlb = alloc_bootmem_low_pages(swiotlbsize);
> diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c
> index 810ad41..3b3e9fe 100644
> --- a/drivers/xen/swiotlb-xen.c
> +++ b/drivers/xen/swiotlb-xen.c
> @@ -164,11 +164,11 @@ xen_swiotlb_fixup(void *buf, size_t size, unsigned long nslabs)
>  	dma_addr_t dma_handle;
>  	phys_addr_t p = virt_to_phys(buf);
>  
> -	dma_bits = get_order(IO_TLB_SEGSIZE << IO_TLB_SHIFT) + PAGE_SHIFT;
> +	dma_bits = get_order(io_tlb_segsize << IO_TLB_SHIFT) + PAGE_SHIFT;
>  
>  	i = 0;
>  	do {
> -		int slabs = min(nslabs - i, (unsigned long)IO_TLB_SEGSIZE);
> +		int slabs = min(nslabs - i, (unsigned long)io_tlb_segsize);
>  
>  		do {
>  			rc = xen_create_contiguous_region(
> @@ -187,7 +187,7 @@ static unsigned long xen_set_nslabs(unsigned long nr_tbl)
>  {
>  	if (!nr_tbl) {
>  		xen_io_tlb_nslabs = (64 * 1024 * 1024 >> IO_TLB_SHIFT);
> -		xen_io_tlb_nslabs = ALIGN(xen_io_tlb_nslabs, IO_TLB_SEGSIZE);
> +		xen_io_tlb_nslabs = ALIGN(xen_io_tlb_nslabs, io_tlb_segsize);
>  	} else
>  		xen_io_tlb_nslabs = nr_tbl;
>  
> diff --git a/include/linux/swiotlb.h b/include/linux/swiotlb.h
> index e7a018e..13506db 100644
> --- a/include/linux/swiotlb.h
> +++ b/include/linux/swiotlb.h
> @@ -8,13 +8,7 @@ struct dma_attrs;
>  struct scatterlist;
>  
>  extern int swiotlb_force;
> -
> -/*
> - * Maximum allowable number of contiguous slabs to map,
> - * must be a power of 2.  What is the appropriate value ?
> - * The complexity of {map,unmap}_single is linearly dependent on this value.
> - */
> -#define IO_TLB_SEGSIZE	128
> +extern int io_tlb_segsize;
>  
>  /*
>   * log of the size of each IO TLB slab.  The number of slabs is command line
> diff --git a/lib/swiotlb.c b/lib/swiotlb.c
> index 4abda07..3b71afd 100644
> --- a/lib/swiotlb.c
> +++ b/lib/swiotlb.c
> @@ -56,6 +56,24 @@
>  int swiotlb_force;
>  
>  /*
> + * default to 128
> + * Maximum allowable number of contiguous slabs to map,
> + * must be a power of 2.  What is the appropriate value ?
> + * define io_tlb_segsize as a parameter
> + * which can be changed dynamically in config file for special usage.
> + * The complexity of {map,unmap}_single is linearly dependent on this value.
> + */
> +#define IO_TLB_SEGSIZE 128

Add the tab back please.

> +int io_tlb_segsize = IO_TLB_SEGSIZE;
> +
> +/* default to 64MB 
> + * define io_tlb_default_size as a parameter
> + * which can be changed dynamically in config file for special usage.

.. or as a parameter during bootup.

> + */
> +#define IO_TLB_DEFAULT_SIZE (64UL<<20)
> +static unsigned long io_tlb_default_size = IO_TLB_DEFAULT_SIZE;
> +
> +/*
>   * Used to do a quick range check in swiotlb_tbl_unmap_single and
>   * swiotlb_tbl_sync_single_*, to see if the memory was in fact allocated by this
>   * API.
> @@ -101,13 +119,32 @@ setup_io_tlb_npages(char *str)
>  {
>  	if (isdigit(*str)) {
>  		io_tlb_nslabs = simple_strtoul(str, &str, 0);
> -		/* avoid tail segment of size < IO_TLB_SEGSIZE */
> -		io_tlb_nslabs = ALIGN(io_tlb_nslabs, IO_TLB_SEGSIZE);
>  	}
>  	if (*str == ',')
>  		++str;
> -	if (!strcmp(str, "force"))
> +	if (!strncmp(str, "force", 5)) {
>  		swiotlb_force = 1;
> +		str += 5;
> +	}

So the format is now:

	Format: { <int> | force | <int> | <int>}

which means I can do
	32,22323,force

Or
	force,32

Or	
	32,force

I think you need to make function be inside a loop
to deal with 'force' being at odd locations.

> +	if (*str == ',')
> +		++str;
> +	if (isdigit(*str)) {
> +		int n = 0;
> +		io_tlb_segsize = simple_strtoul(str, &str, 0);
> +		io_tlb_segsize = ALIGN(io_tlb_segsize, IO_TLB_SEGSIZE);
> +		while ((io_tlb_segsize - 1) >> n)
> +			n++;
> +		io_tlb_segsize = (1 << n);
> +	}
> +	if (*str == ',')
> +		++str;
> +	if (isdigit(*str)) {
> +		io_tlb_default_size = simple_strtoul(str, &str, 0);
> +		io_tlb_default_size = ALIGN(io_tlb_default_size, IO_TLB_DEFAULT_SIZE);
> +	}
> +
> +	/* avoid tail segment of size < io_tlb_segsize */
> +	io_tlb_nslabs = ALIGN(io_tlb_nslabs, io_tlb_segsize);
>  
>  	return 0;
>  }
> @@ -120,15 +157,13 @@ unsigned long swiotlb_nr_tbl(void)
>  }
>  EXPORT_SYMBOL_GPL(swiotlb_nr_tbl);
>  
> -/* default to 64MB */
> -#define IO_TLB_DEFAULT_SIZE (64UL<<20)
>  unsigned long swiotlb_size_or_default(void)
>  {
>  	unsigned long size;
>  
>  	size = io_tlb_nslabs << IO_TLB_SHIFT;
>  
> -	return size ? size : (IO_TLB_DEFAULT_SIZE);
> +	return size ? size : (io_tlb_default_size);
>  }
>  
>  /* Note that this doesn't work with highmem page */
> @@ -183,7 +218,7 @@ int __init swiotlb_init_with_tbl(char *tlb, unsigned long nslabs, int verbose)
>  
>  	/*
>  	 * Allocate and initialize the free list array.  This array is used
> -	 * to find contiguous free memory regions of size up to IO_TLB_SEGSIZE
> +	 * to find contiguous free memory regions of size up to io_tlb_segsize
>  	 * between io_tlb_start and io_tlb_end.
>  	 */
>  	io_tlb_list = memblock_virt_alloc(
> @@ -193,7 +228,7 @@ int __init swiotlb_init_with_tbl(char *tlb, unsigned long nslabs, int verbose)
>  				PAGE_ALIGN(io_tlb_nslabs * sizeof(phys_addr_t)),
>  				PAGE_SIZE);
>  	for (i = 0; i < io_tlb_nslabs; i++) {
> -		io_tlb_list[i] = IO_TLB_SEGSIZE - OFFSET(i, IO_TLB_SEGSIZE);
> +		io_tlb_list[i] = io_tlb_segsize - OFFSET(i, io_tlb_segsize);
>  		io_tlb_orig_addr[i] = INVALID_PHYS_ADDR;
>  	}
>  	io_tlb_index = 0;
> @@ -211,13 +246,12 @@ int __init swiotlb_init_with_tbl(char *tlb, unsigned long nslabs, int verbose)
>  void  __init
>  swiotlb_init(int verbose)
>  {
> -	size_t default_size = IO_TLB_DEFAULT_SIZE;
>  	unsigned char *vstart;
>  	unsigned long bytes;
>  
>  	if (!io_tlb_nslabs) {
> -		io_tlb_nslabs = (default_size >> IO_TLB_SHIFT);
> -		io_tlb_nslabs = ALIGN(io_tlb_nslabs, IO_TLB_SEGSIZE);
> +		io_tlb_nslabs = (io_tlb_default_size >> IO_TLB_SHIFT);
> +		io_tlb_nslabs = ALIGN(io_tlb_nslabs, io_tlb_segsize);
>  	}
>  
>  	bytes = io_tlb_nslabs << IO_TLB_SHIFT;
> @@ -249,7 +283,7 @@ swiotlb_late_init_with_default_size(size_t default_size)
>  
>  	if (!io_tlb_nslabs) {
>  		io_tlb_nslabs = (default_size >> IO_TLB_SHIFT);
> -		io_tlb_nslabs = ALIGN(io_tlb_nslabs, IO_TLB_SEGSIZE);
> +		io_tlb_nslabs = ALIGN(io_tlb_nslabs, io_tlb_segsize);
>  	}
>  
>  	/*
> @@ -308,7 +342,7 @@ swiotlb_late_init_with_tbl(char *tlb, unsigned long nslabs)
>  
>  	/*
>  	 * Allocate and initialize the free list array.  This array is used
> -	 * to find contiguous free memory regions of size up to IO_TLB_SEGSIZE
> +	 * to find contiguous free memory regions of size up to io_tlb_segsize
>  	 * between io_tlb_start and io_tlb_end.
>  	 */
>  	io_tlb_list = (unsigned int *)__get_free_pages(GFP_KERNEL,
> @@ -324,7 +358,7 @@ swiotlb_late_init_with_tbl(char *tlb, unsigned long nslabs)
>  		goto cleanup4;
>  
>  	for (i = 0; i < io_tlb_nslabs; i++) {
> -		io_tlb_list[i] = IO_TLB_SEGSIZE - OFFSET(i, IO_TLB_SEGSIZE);
> +		io_tlb_list[i] = io_tlb_segsize - OFFSET(i, io_tlb_segsize);
>  		io_tlb_orig_addr[i] = INVALID_PHYS_ADDR;
>  	}
>  	io_tlb_index = 0;
> @@ -493,7 +527,7 @@ phys_addr_t swiotlb_tbl_map_single(struct device *hwdev,
>  
>  			for (i = index; i < (int) (index + nslots); i++)
>  				io_tlb_list[i] = 0;
> -			for (i = index - 1; (OFFSET(i, IO_TLB_SEGSIZE) != IO_TLB_SEGSIZE - 1) && io_tlb_list[i]; i--)
> +			for (i = index - 1; (OFFSET(i, io_tlb_segsize) != io_tlb_segsize - 1) && io_tlb_list[i]; i--)
>  				io_tlb_list[i] = ++count;
>  			tlb_addr = io_tlb_start + (index << IO_TLB_SHIFT);
>  
> @@ -571,7 +605,7 @@ void swiotlb_tbl_unmap_single(struct device *hwdev, phys_addr_t tlb_addr,
>  	 */
>  	spin_lock_irqsave(&io_tlb_lock, flags);
>  	{
> -		count = ((index + nslots) < ALIGN(index + 1, IO_TLB_SEGSIZE) ?
> +		count = ((index + nslots) < ALIGN(index + 1, io_tlb_segsize) ?
>  			 io_tlb_list[index + nslots] : 0);
>  		/*
>  		 * Step 1: return the slots to the free list, merging the
> @@ -585,7 +619,7 @@ void swiotlb_tbl_unmap_single(struct device *hwdev, phys_addr_t tlb_addr,
>  		 * Step 2: merge the returned slots with the preceding slots,
>  		 * if available (non zero)
>  		 */
> -		for (i = index - 1; (OFFSET(i, IO_TLB_SEGSIZE) != IO_TLB_SEGSIZE -1) && io_tlb_list[i]; i--)
> +		for (i = index - 1; (OFFSET(i, io_tlb_segsize) != io_tlb_segsize -1) && io_tlb_list[i]; i--)
>  			io_tlb_list[i] = ++count;
>  	}
>  	spin_unlock_irqrestore(&io_tlb_lock, flags);
> -- 
> 1.7.9.5
> 

RE: [PATCH v5] modify the IO_TLB_SEGSIZE and IO_TLB_DEFAULT_SIZE configurable as flexible requirement about SW-IOMMU.

[Wang, Xiaoming]

> -----Original Message-----
> From: Konrad Rzeszutek Wilk [mailto:konrad.wilk@oracle.com]
> Sent: Thursday, March 5, 2015 3:43 AM
> To: Wang, Xiaoming
> Cc: chris@chris-wilson.co.uk; david.vrabel@citrix.com;
> lauraa@codeaurora.org; heiko.carstens@de.ibm.com; linux@horizon.com;
> Liu@aserp2030.oracle.com; Liu, Chuansheng; Zhang@aserp2030.oracle.com;
> Zhang, Dongxing; takahiro.akashi@linaro.org; akpm@linux-foundation.org;
> linux-mips@linux-mips.org; ralf@linux-mips.org; xen-
> devel@lists.xenproject.org; boris.ostrovsky@oracle.com;
> d.kasatkin@samsung.com; pebolle@tiscali.nl; linux-kernel@vger.kernel.org;
> JBeulich@suse.com
> Subject: Re: [PATCH v5] modify the IO_TLB_SEGSIZE and
> IO_TLB_DEFAULT_SIZE configurable as flexible requirement about SW-
> IOMMU.
> 
> On Tue, Mar 03, 2015 at 04:11:09PM +0800, Wang Xiaoming wrote:
> > The maximum of SW-IOMMU is limited to 2^11*128 = 256K.
> > And the size of IO_TLB_DEFAULT_SIZE is limited to (64UL<<20) 64M now.
> > While in different platform and different requirement this seems improper.
> > So modifing the IO_TLB_SEGSIZE to io_tlb_segsize and
> > IO_TLB_DEFAULT_SIZE to io_tlb_default_size which can configure by
> kernel cmdline.
> > This can meet different requirement.
> >
> > Signed-off-by: Chuansheng Liu <chuansheng.liu@intel.com>
> > Signed-off-by: Zhang Dongxing <dongxing.zhang@intel.com>
> > Signed-off-by: Wang Xiaoming <xiaoming.wang@intel.com>
> > ---
> > patch v1 make this change at Kconfig
> > which needs to edit the .config manually.
> > https://lkml.org/lkml/2015/1/25/571
> >
> > patch v2 only change IO_TLB_SEGSIZE configurable.
> > https://lkml.org/lkml/2015/2/5/812
> >
> > patch v3 parsing io_tlb_segsize and
> > io_tlb_default_size independently.
> > https://lkml.org/lkml/2015/2/15/217
> >
> > patch v4 hasn't validated the data from command line.
> 
> Thank you for redoing this per review.
> 
> > https://lkml.org/lkml/2015/2/17/114
> >
> >  Documentation/kernel-parameters.txt  |    9 ++++-
> >  arch/mips/cavium-octeon/dma-octeon.c |    2 +-
> >  arch/mips/netlogic/common/nlm-dma.c  |    2 +-
> >  drivers/xen/swiotlb-xen.c            |    6 +--
> >  include/linux/swiotlb.h              |    8 +---
> >  lib/swiotlb.c                        |   68 +++++++++++++++++++++++++---------
> >  6 files changed, 65 insertions(+), 30 deletions(-)
> >
> > diff --git a/Documentation/kernel-parameters.txt
> > b/Documentation/kernel-parameters.txt
> > index 4df73da..1f50e86 100644
> > --- a/Documentation/kernel-parameters.txt
> > +++ b/Documentation/kernel-parameters.txt
> > @@ -3438,10 +3438,17 @@ bytes respectively. Such letter suffixes can
> also be entirely omitted.
> >  			it if 0 is given (See
> Documentation/cgroups/memory.txt)
> >
> >  	swiotlb=	[ARM,IA-64,PPC,MIPS,X86]
> > -			Format: { <int> | force }
> > +			Format: { <int> | force | <int> | <int>}
>                                                         ,
> 
> s/|/,/
> 
How about change the Format to 
			Format: { <int>,force,<int>,<int>}
Force the parameter input  in consecutive order.
 
> >  			<int> -- Number of I/O TLB slabs
> >  			force -- force using of bounce buffers even if they
> >  			         wouldn't be automatically used by the kernel
> > +			<int> -- Maximum allowable number of contiguous
> slabs to map
> > +			<int> -- The size of SW-MMU mapped.
> > +			Using "," to separate them one by one.
> 
> "Use ',' to seperate them."
> 
> > +			Example:
> > +			BOARD_KERNEL_CMDLINE +=
> swiotlb=32768,force,512,268435456
> > +			io_tlb_nslabs=32768, swiotlb_force=1,
> > +			io_tlb_segsize=512, io_tlb_default_size=268435456
> 
> I think you can remove the example - and just have it in the C code.
> 
> >
> >  	switches=	[HW,M68k]
> >
> > diff --git a/arch/mips/cavium-octeon/dma-octeon.c
> > b/arch/mips/cavium-octeon/dma-octeon.c
> > index 3778655..a521af6 100644
> > --- a/arch/mips/cavium-octeon/dma-octeon.c
> > +++ b/arch/mips/cavium-octeon/dma-octeon.c
> > @@ -312,7 +312,7 @@ void __init plat_swiotlb_setup(void)
> >  		swiotlbsize = 64 * (1<<20);
> >  #endif
> >  	swiotlb_nslabs = swiotlbsize >> IO_TLB_SHIFT;
> > -	swiotlb_nslabs = ALIGN(swiotlb_nslabs, IO_TLB_SEGSIZE);
> > +	swiotlb_nslabs = ALIGN(swiotlb_nslabs, io_tlb_segsize);
> >  	swiotlbsize = swiotlb_nslabs << IO_TLB_SHIFT;
> >
> >  	octeon_swiotlb = alloc_bootmem_low_pages(swiotlbsize);
> > diff --git a/arch/mips/netlogic/common/nlm-dma.c
> > b/arch/mips/netlogic/common/nlm-dma.c
> > index f3d4ae8..eeffa8f 100644
> > --- a/arch/mips/netlogic/common/nlm-dma.c
> > +++ b/arch/mips/netlogic/common/nlm-dma.c
> > @@ -99,7 +99,7 @@ void __init plat_swiotlb_setup(void)
> >
> >  	swiotlbsize = 1 << 20; /* 1 MB for now */
> >  	swiotlb_nslabs = swiotlbsize >> IO_TLB_SHIFT;
> > -	swiotlb_nslabs = ALIGN(swiotlb_nslabs, IO_TLB_SEGSIZE);
> > +	swiotlb_nslabs = ALIGN(swiotlb_nslabs, io_tlb_segsize);
> >  	swiotlbsize = swiotlb_nslabs << IO_TLB_SHIFT;
> >
> >  	nlm_swiotlb = alloc_bootmem_low_pages(swiotlbsize);
> > diff --git a/drivers/xen/swiotlb-xen.c b/drivers/xen/swiotlb-xen.c
> > index 810ad41..3b3e9fe 100644
> > --- a/drivers/xen/swiotlb-xen.c
> > +++ b/drivers/xen/swiotlb-xen.c
> > @@ -164,11 +164,11 @@ xen_swiotlb_fixup(void *buf, size_t size,
> unsigned long nslabs)
> >  	dma_addr_t dma_handle;
> >  	phys_addr_t p = virt_to_phys(buf);
> >
> > -	dma_bits = get_order(IO_TLB_SEGSIZE << IO_TLB_SHIFT) +
> PAGE_SHIFT;
> > +	dma_bits = get_order(io_tlb_segsize << IO_TLB_SHIFT) + PAGE_SHIFT;
> >
> >  	i = 0;
> >  	do {
> > -		int slabs = min(nslabs - i, (unsigned long)IO_TLB_SEGSIZE);
> > +		int slabs = min(nslabs - i, (unsigned long)io_tlb_segsize);
> >
> >  		do {
> >  			rc = xen_create_contiguous_region( @@ -187,7
> +187,7 @@ static
> > unsigned long xen_set_nslabs(unsigned long nr_tbl)  {
> >  	if (!nr_tbl) {
> >  		xen_io_tlb_nslabs = (64 * 1024 * 1024 >> IO_TLB_SHIFT);
> > -		xen_io_tlb_nslabs = ALIGN(xen_io_tlb_nslabs,
> IO_TLB_SEGSIZE);
> > +		xen_io_tlb_nslabs = ALIGN(xen_io_tlb_nslabs, io_tlb_segsize);
> >  	} else
> >  		xen_io_tlb_nslabs = nr_tbl;
> >
> > diff --git a/include/linux/swiotlb.h b/include/linux/swiotlb.h index
> > e7a018e..13506db 100644
> > --- a/include/linux/swiotlb.h
> > +++ b/include/linux/swiotlb.h
> > @@ -8,13 +8,7 @@ struct dma_attrs;
> >  struct scatterlist;
> >
> >  extern int swiotlb_force;
> > -
> > -/*
> > - * Maximum allowable number of contiguous slabs to map,
> > - * must be a power of 2.  What is the appropriate value ?
> > - * The complexity of {map,unmap}_single is linearly dependent on this
> value.
> > - */
> > -#define IO_TLB_SEGSIZE	128
> > +extern int io_tlb_segsize;
> >
> >  /*
> >   * log of the size of each IO TLB slab.  The number of slabs is
> > command line diff --git a/lib/swiotlb.c b/lib/swiotlb.c index
> > 4abda07..3b71afd 100644
> > --- a/lib/swiotlb.c
> > +++ b/lib/swiotlb.c
> > @@ -56,6 +56,24 @@
> >  int swiotlb_force;
> >
> >  /*
> > + * default to 128
> > + * Maximum allowable number of contiguous slabs to map,
> > + * must be a power of 2.  What is the appropriate value ?
> > + * define io_tlb_segsize as a parameter
> > + * which can be changed dynamically in config file for special usage.
> > + * The complexity of {map,unmap}_single is linearly dependent on this
> value.
> > + */
> > +#define IO_TLB_SEGSIZE 128
> 
> Add the tab back please.
> 
> > +int io_tlb_segsize = IO_TLB_SEGSIZE;
> > +
> > +/* default to 64MB
> > + * define io_tlb_default_size as a parameter
> > + * which can be changed dynamically in config file for special usage.
> 
> .. or as a parameter during bootup.
> 
> > + */
> > +#define IO_TLB_DEFAULT_SIZE (64UL<<20) static unsigned long
> > +io_tlb_default_size = IO_TLB_DEFAULT_SIZE;
> > +
> > +/*
> >   * Used to do a quick range check in swiotlb_tbl_unmap_single and
> >   * swiotlb_tbl_sync_single_*, to see if the memory was in fact allocated by
> this
> >   * API.
> > @@ -101,13 +119,32 @@ setup_io_tlb_npages(char *str)  {
> >  	if (isdigit(*str)) {
> >  		io_tlb_nslabs = simple_strtoul(str, &str, 0);
> > -		/* avoid tail segment of size < IO_TLB_SEGSIZE */
> > -		io_tlb_nslabs = ALIGN(io_tlb_nslabs, IO_TLB_SEGSIZE);
> >  	}
> >  	if (*str == ',')
> >  		++str;
> > -	if (!strcmp(str, "force"))
> > +	if (!strncmp(str, "force", 5)) {
> >  		swiotlb_force = 1;
> > +		str += 5;
> > +	}
> 
> So the format is now:
> 
> 	Format: { <int> | force | <int> | <int>}
> 
> which means I can do
> 	32,22323,force
> 
> Or
> 	force,32
> 
> Or
> 	32,force
> 
If I use Format: { <int>,force,<int>,<int>}
32,22323,force can't acceptable.
There are three  <int>  here, if there are out of order, that will cause confuse.
Only 	32,force,32323
Or	32,,32323,2322
Or	,,323222,3232
Are available.

> I think you need to make function be inside a loop to deal with 'force' being
> at odd locations.
> 

> > +	if (*str == ',')
> > +		++str;
> > +	if (isdigit(*str)) {
> > +		int n = 0;
> > +		io_tlb_segsize = simple_strtoul(str, &str, 0);
> > +		io_tlb_segsize = ALIGN(io_tlb_segsize, IO_TLB_SEGSIZE);
> > +		while ((io_tlb_segsize - 1) >> n)
> > +			n++;
> > +		io_tlb_segsize = (1 << n);
> > +	}
> > +	if (*str == ',')
> > +		++str;
> > +	if (isdigit(*str)) {
> > +		io_tlb_default_size = simple_strtoul(str, &str, 0);
> > +		io_tlb_default_size = ALIGN(io_tlb_default_size,
> IO_TLB_DEFAULT_SIZE);
> > +	}
> > +
> > +	/* avoid tail segment of size < io_tlb_segsize */
> > +	io_tlb_nslabs = ALIGN(io_tlb_nslabs, io_tlb_segsize);
> >
Here has a leakage if io_tlb_nslabs hasn't set before it will be set to io_tlb_segsize.
I will fix it in next version.
> >  	return 0;
> >  }
> > @@ -120,15 +157,13 @@ unsigned long swiotlb_nr_tbl(void)  }
> > EXPORT_SYMBOL_GPL(swiotlb_nr_tbl);
> >
> > -/* default to 64MB */
> > -#define IO_TLB_DEFAULT_SIZE (64UL<<20)  unsigned long
> > swiotlb_size_or_default(void)  {
> >  	unsigned long size;
> >
> >  	size = io_tlb_nslabs << IO_TLB_SHIFT;
> >
> > -	return size ? size : (IO_TLB_DEFAULT_SIZE);
> > +	return size ? size : (io_tlb_default_size);
> >  }
> >
> >  /* Note that this doesn't work with highmem page */ @@ -183,7 +218,7
> > @@ int __init swiotlb_init_with_tbl(char *tlb, unsigned long nslabs,
> > int verbose)
> >
> >  	/*
> >  	 * Allocate and initialize the free list array.  This array is used
> > -	 * to find contiguous free memory regions of size up to
> IO_TLB_SEGSIZE
> > +	 * to find contiguous free memory regions of size up to
> > +io_tlb_segsize
> >  	 * between io_tlb_start and io_tlb_end.
> >  	 */
> >  	io_tlb_list = memblock_virt_alloc(
> > @@ -193,7 +228,7 @@ int __init swiotlb_init_with_tbl(char *tlb, unsigned
> long nslabs, int verbose)
> >  				PAGE_ALIGN(io_tlb_nslabs *
> sizeof(phys_addr_t)),
> >  				PAGE_SIZE);
> >  	for (i = 0; i < io_tlb_nslabs; i++) {
> > -		io_tlb_list[i] = IO_TLB_SEGSIZE - OFFSET(i, IO_TLB_SEGSIZE);
> > +		io_tlb_list[i] = io_tlb_segsize - OFFSET(i, io_tlb_segsize);
> >  		io_tlb_orig_addr[i] = INVALID_PHYS_ADDR;
> >  	}
> >  	io_tlb_index = 0;
> > @@ -211,13 +246,12 @@ int __init swiotlb_init_with_tbl(char *tlb,
> > unsigned long nslabs, int verbose)  void  __init  swiotlb_init(int
> > verbose)  {
> > -	size_t default_size = IO_TLB_DEFAULT_SIZE;
> >  	unsigned char *vstart;
> >  	unsigned long bytes;
> >
> >  	if (!io_tlb_nslabs) {
> > -		io_tlb_nslabs = (default_size >> IO_TLB_SHIFT);
> > -		io_tlb_nslabs = ALIGN(io_tlb_nslabs, IO_TLB_SEGSIZE);
> > +		io_tlb_nslabs = (io_tlb_default_size >> IO_TLB_SHIFT);
> > +		io_tlb_nslabs = ALIGN(io_tlb_nslabs, io_tlb_segsize);
> >  	}
> >
> >  	bytes = io_tlb_nslabs << IO_TLB_SHIFT; @@ -249,7 +283,7 @@
> > swiotlb_late_init_with_default_size(size_t default_size)
> >
> >  	if (!io_tlb_nslabs) {
> >  		io_tlb_nslabs = (default_size >> IO_TLB_SHIFT);
> > -		io_tlb_nslabs = ALIGN(io_tlb_nslabs, IO_TLB_SEGSIZE);
> > +		io_tlb_nslabs = ALIGN(io_tlb_nslabs, io_tlb_segsize);
> >  	}
> >
> >  	/*
> > @@ -308,7 +342,7 @@ swiotlb_late_init_with_tbl(char *tlb, unsigned
> > long nslabs)
> >
> >  	/*
> >  	 * Allocate and initialize the free list array.  This array is used
> > -	 * to find contiguous free memory regions of size up to
> IO_TLB_SEGSIZE
> > +	 * to find contiguous free memory regions of size up to
> > +io_tlb_segsize
> >  	 * between io_tlb_start and io_tlb_end.
> >  	 */
> >  	io_tlb_list = (unsigned int *)__get_free_pages(GFP_KERNEL, @@ -
> 324,7
> > +358,7 @@ swiotlb_late_init_with_tbl(char *tlb, unsigned long nslabs)
> >  		goto cleanup4;
> >
> >  	for (i = 0; i < io_tlb_nslabs; i++) {
> > -		io_tlb_list[i] = IO_TLB_SEGSIZE - OFFSET(i, IO_TLB_SEGSIZE);
> > +		io_tlb_list[i] = io_tlb_segsize - OFFSET(i, io_tlb_segsize);
> >  		io_tlb_orig_addr[i] = INVALID_PHYS_ADDR;
> >  	}
> >  	io_tlb_index = 0;
> > @@ -493,7 +527,7 @@ phys_addr_t swiotlb_tbl_map_single(struct device
> > *hwdev,
> >
> >  			for (i = index; i < (int) (index + nslots); i++)
> >  				io_tlb_list[i] = 0;
> > -			for (i = index - 1; (OFFSET(i, IO_TLB_SEGSIZE) !=
> IO_TLB_SEGSIZE - 1) && io_tlb_list[i]; i--)
> > +			for (i = index - 1; (OFFSET(i, io_tlb_segsize) !=
> io_tlb_segsize -
> > +1) && io_tlb_list[i]; i--)
> >  				io_tlb_list[i] = ++count;
> >  			tlb_addr = io_tlb_start + (index << IO_TLB_SHIFT);
> >
> > @@ -571,7 +605,7 @@ void swiotlb_tbl_unmap_single(struct device
> *hwdev, phys_addr_t tlb_addr,
> >  	 */
> >  	spin_lock_irqsave(&io_tlb_lock, flags);
> >  	{
> > -		count = ((index + nslots) < ALIGN(index + 1, IO_TLB_SEGSIZE) ?
> > +		count = ((index + nslots) < ALIGN(index + 1, io_tlb_segsize) ?
> >  			 io_tlb_list[index + nslots] : 0);
> >  		/*
> >  		 * Step 1: return the slots to the free list, merging the @@ -
> 585,7
> > +619,7 @@ void swiotlb_tbl_unmap_single(struct device *hwdev,
> phys_addr_t tlb_addr,
> >  		 * Step 2: merge the returned slots with the preceding slots,
> >  		 * if available (non zero)
> >  		 */
> > -		for (i = index - 1; (OFFSET(i, IO_TLB_SEGSIZE) !=
> IO_TLB_SEGSIZE -1) && io_tlb_list[i]; i--)
> > +		for (i = index - 1; (OFFSET(i, io_tlb_segsize) != io_tlb_segsize
> > +-1) && io_tlb_list[i]; i--)
> >  			io_tlb_list[i] = ++count;
> >  	}
> >  	spin_unlock_irqrestore(&io_tlb_lock, flags);
> > --
> > 1.7.9.5
> >

RE: [PATCH v5] modify the IO_TLB_SEGSIZE and IO_TLB_DEFAULT_SIZE configurable as flexible requirement about SW-IOMMU.

[Jan Beulich]

>>> On 05.03.15 at 04:53, <xiaoming.wang@intel.com> wrote:
>> From: Konrad Rzeszutek Wilk [mailto:konrad.wilk@oracle.com]
>> Sent: Thursday, March 5, 2015 3:43 AM
>> On Tue, Mar 03, 2015 at 04:11:09PM +0800, Wang Xiaoming wrote:
>> > @@ -101,13 +119,32 @@ setup_io_tlb_npages(char *str)  {
>> >  	if (isdigit(*str)) {
>> >  		io_tlb_nslabs = simple_strtoul(str, &str, 0);
>> > -		/* avoid tail segment of size < IO_TLB_SEGSIZE */
>> > -		io_tlb_nslabs = ALIGN(io_tlb_nslabs, IO_TLB_SEGSIZE);
>> >  	}
>> >  	if (*str == ',')
>> >  		++str;
>> > -	if (!strcmp(str, "force"))
>> > +	if (!strncmp(str, "force", 5)) {
>> >  		swiotlb_force = 1;
>> > +		str += 5;
>> > +	}
>> 
>> So the format is now:
>> 
>> 	Format: { <int> | force | <int> | <int>}
>> 
>> which means I can do
>> 	32,22323,force
>> 
>> Or
>> 	force,32
>> 
>> Or
>> 	32,force
>> 
> If I use Format: { <int>,force,<int>,<int>}
> 32,22323,force can't acceptable.
> There are three  <int>  here, if there are out of order, that will cause 
> confuse.
> Only 	32,force,32323
> Or	32,,32323,2322
> Or	,,323222,3232
> Are available.

You need to make sure that all previously valid variants are still
usable, i.e. force alone, a number alone, force,<number> and
<number>,force. How many variants you want to support with
your additions is mostly up to you; I'd recommend permitting force
in any position.

Jan

RE: [PATCH v5] modify the IO_TLB_SEGSIZE and IO_TLB_DEFAULT_SIZE configurable as flexible requirement about SW-IOMMU.

[Wang, Xiaoming]

Dear Jan

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: Thursday, March 5, 2015 4:40 PM
> To: Wang, Xiaoming
> Cc: Liu@aserp2030.oracle.com; Zhang@aserp2030.oracle.com; chris@chris-
> wilson.co.uk; david.vrabel@citrix.com; lauraa@codeaurora.org;
> heiko.carstens@de.ibm.com; linux@horizon.com; Liu, Chuansheng; Zhang,
> Dongxing; takahiro.akashi@linaro.org; akpm@linux-foundation.org; linux-
> mips@linux-mips.org; ralf@linux-mips.org; xen-devel@lists.xenproject.org;
> boris.ostrovsky@oracle.com; Konrad Rzeszutek Wilk;
> d.kasatkin@samsung.com; pebolle@tiscali.nl; linux-kernel@vger.kernel.org
> Subject: RE: [PATCH v5] modify the IO_TLB_SEGSIZE and
> IO_TLB_DEFAULT_SIZE configurable as flexible requirement about SW-
> IOMMU.
> 
> >>> On 05.03.15 at 04:53, <xiaoming.wang@intel.com> wrote:
> >> From: Konrad Rzeszutek Wilk [mailto:konrad.wilk@oracle.com]
> >> Sent: Thursday, March 5, 2015 3:43 AM On Tue, Mar 03, 2015 at
> >> 04:11:09PM +0800, Wang Xiaoming wrote:
> >> > @@ -101,13 +119,32 @@ setup_io_tlb_npages(char *str)  {
> >> >  	if (isdigit(*str)) {
> >> >  		io_tlb_nslabs = simple_strtoul(str, &str, 0);
> >> > -		/* avoid tail segment of size < IO_TLB_SEGSIZE */
> >> > -		io_tlb_nslabs = ALIGN(io_tlb_nslabs, IO_TLB_SEGSIZE);
> >> >  	}
> >> >  	if (*str == ',')
> >> >  		++str;
> >> > -	if (!strcmp(str, "force"))
> >> > +	if (!strncmp(str, "force", 5)) {
> >> >  		swiotlb_force = 1;
> >> > +		str += 5;
> >> > +	}
> >>
> >> So the format is now:
> >>
> >> 	Format: { <int> | force | <int> | <int>}
> >>
> >> which means I can do
> >> 	32,22323,force
> >>
> >> Or
> >> 	force,32
> >>
> >> Or
> >> 	32,force
> >>
> > If I use Format: { <int>,force,<int>,<int>} 32,22323,force can't
> > acceptable.
> > There are three  <int>  here, if there are out of order, that will
> > cause confuse.
> > Only 	32,force,32323
> > Or	32,,32323,2322
> > Or	,,323222,3232
> > Are available.
> 
> You need to make sure that all previously valid variants are still usable, i.e.
> force alone, a number alone, force,<number> and <number>,force. How
> many variants you want to support with your additions is mostly up to you;
> I'd recommend permitting force in any position.
> 
I don't think it's suitable to accept that "force" in any position.
If we defined Format: { <int>,force,<int>,<int>}
"force" must be located in second position. 
And we add comment for each <int> also.
Every position may be defined specifically.

> Jan

RE: [PATCH v5] modify the IO_TLB_SEGSIZE and IO_TLB_DEFAULT_SIZE configurable as flexible requirement about SW-IOMMU.

[Jan Beulich]

>>> On 05.03.15 at 09:52, <xiaoming.wang@intel.com> wrote:
>> From: Jan Beulich [mailto:JBeulich@suse.com]
>> Sent: Thursday, March 5, 2015 4:40 PM
>> >>> On 05.03.15 at 04:53, <xiaoming.wang@intel.com> wrote:
>> >> From: Konrad Rzeszutek Wilk [mailto:konrad.wilk@oracle.com]
>> >> Sent: Thursday, March 5, 2015 3:43 AM On Tue, Mar 03, 2015 at
>> >> 04:11:09PM +0800, Wang Xiaoming wrote:
>> >> > @@ -101,13 +119,32 @@ setup_io_tlb_npages(char *str)  {
>> >> >  	if (isdigit(*str)) {
>> >> >  		io_tlb_nslabs = simple_strtoul(str, &str, 0);
>> >> > -		/* avoid tail segment of size < IO_TLB_SEGSIZE */
>> >> > -		io_tlb_nslabs = ALIGN(io_tlb_nslabs, IO_TLB_SEGSIZE);
>> >> >  	}
>> >> >  	if (*str == ',')
>> >> >  		++str;
>> >> > -	if (!strcmp(str, "force"))
>> >> > +	if (!strncmp(str, "force", 5)) {
>> >> >  		swiotlb_force = 1;
>> >> > +		str += 5;
>> >> > +	}
>> >>
>> >> So the format is now:
>> >>
>> >> 	Format: { <int> | force | <int> | <int>}
>> >>
>> >> which means I can do
>> >> 	32,22323,force
>> >>
>> >> Or
>> >> 	force,32
>> >>
>> >> Or
>> >> 	32,force
>> >>
>> > If I use Format: { <int>,force,<int>,<int>} 32,22323,force can't
>> > acceptable.
>> > There are three  <int>  here, if there are out of order, that will
>> > cause confuse.
>> > Only 	32,force,32323
>> > Or	32,,32323,2322
>> > Or	,,323222,3232
>> > Are available.
>> 
>> You need to make sure that all previously valid variants are still usable, 
> i.e.
>> force alone, a number alone, force,<number> and <number>,force. How
>> many variants you want to support with your additions is mostly up to you;
>> I'd recommend permitting force in any position.
>> 
> I don't think it's suitable to accept that "force" in any position.
> If we defined Format: { <int>,force,<int>,<int>}
> "force" must be located in second position. 
> And we add comment for each <int> also.
> Every position may be defined specifically.

As said, with the old format allowing force in either first or second
position, you have to at least accept that in the new version too.
Accepting force in any position would be a (desirable) courtesy to
the user.

Jan

RE: [PATCH v5] modify the IO_TLB_SEGSIZE and IO_TLB_DEFAULT_SIZE configurable as flexible requirement about SW-IOMMU.

[Wang, Xiaoming]

Dear Jan

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: Thursday, March 5, 2015 5:00 PM
> To: Wang, Xiaoming
> Cc: Liu@aserp2030.oracle.com; Zhang@aserp2030.oracle.com; chris@chris-
> wilson.co.uk; david.vrabel@citrix.com; lauraa@codeaurora.org;
> heiko.carstens@de.ibm.com; linux@horizon.com; Liu, Chuansheng; Zhang,
> Dongxing; takahiro.akashi@linaro.org; akpm@linux-foundation.org; linux-
> mips@linux-mips.org; ralf@linux-mips.org; xen-devel@lists.xenproject.org;
> boris.ostrovsky@oracle.com; Konrad RzeszutekWilk;
> d.kasatkin@samsung.com; pebolle@tiscali.nl; linux-kernel@vger.kernel.org
> Subject: RE: [PATCH v5] modify the IO_TLB_SEGSIZE and
> IO_TLB_DEFAULT_SIZE configurable as flexible requirement about SW-
> IOMMU.
> 
> >>> On 05.03.15 at 09:52, <xiaoming.wang@intel.com> wrote:
> >> From: Jan Beulich [mailto:JBeulich@suse.com]
> >> Sent: Thursday, March 5, 2015 4:40 PM
> >> >>> On 05.03.15 at 04:53, <xiaoming.wang@intel.com> wrote:
> >> >> From: Konrad Rzeszutek Wilk [mailto:konrad.wilk@oracle.com]
> >> >> Sent: Thursday, March 5, 2015 3:43 AM On Tue, Mar 03, 2015 at
> >> >> 04:11:09PM +0800, Wang Xiaoming wrote:
> >> >> > @@ -101,13 +119,32 @@ setup_io_tlb_npages(char *str)  {
> >> >> >  	if (isdigit(*str)) {
> >> >> >  		io_tlb_nslabs = simple_strtoul(str, &str, 0);
> >> >> > -		/* avoid tail segment of size < IO_TLB_SEGSIZE */
> >> >> > -		io_tlb_nslabs = ALIGN(io_tlb_nslabs,
> IO_TLB_SEGSIZE);
> >> >> >  	}
> >> >> >  	if (*str == ',')
> >> >> >  		++str;
> >> >> > -	if (!strcmp(str, "force"))
> >> >> > +	if (!strncmp(str, "force", 5)) {
> >> >> >  		swiotlb_force = 1;
> >> >> > +		str += 5;
> >> >> > +	}
> >> >>
> >> >> So the format is now:
> >> >>
> >> >> 	Format: { <int> | force | <int> | <int>}
> >> >>
> >> >> which means I can do
> >> >> 	32,22323,force
> >> >>
> >> >> Or
> >> >> 	force,32
> >> >>
> >> >> Or
> >> >> 	32,force
> >> >>
> >> > If I use Format: { <int>,force,<int>,<int>} 32,22323,force can't
> >> > acceptable.
> >> > There are three  <int>  here, if there are out of order, that will
> >> > cause confuse.
> >> > Only 	32,force,32323
> >> > Or	32,,32323,2322
> >> > Or	,,323222,3232
> >> > Are available.
> >>
> >> You need to make sure that all previously valid variants are still
> >> usable,
> > i.e.
> >> force alone, a number alone, force,<number> and <number>,force. How
> >> many variants you want to support with your additions is mostly up to
> >> you; I'd recommend permitting force in any position.
> >>
> > I don't think it's suitable to accept that "force" in any position.
> > If we defined Format: { <int>,force,<int>,<int>} "force" must be
> > located in second position.
> > And we add comment for each <int> also.
> > Every position may be defined specifically.
> 
> As said, with the old format allowing force in either first or second position,
> you have to at least accept that in the new version too.
> Accepting force in any position would be a (desirable) courtesy to the user.
> 
I have checked the code and do some test.

First "|" in Format  means *or*  in Documentation/kernel-parameters.txt
For example:
        acpi=           [HW,ACPI,X86]
                        Advanced Configuration and Power Interface
                        Format: { force | off | strict | noirq | rsdt }

        acpi_sci=       [HW,ACPI] ACPI System Control Interrupt trigger mode
                        Format: { level | edge | high | low }

Second the code in lib/swiotlb.c can't realize the function that
"force" in either first or second position with the Format
       swiotlb=        [ARM,IA-64,PPC,MIPS,X86]
                       Format: { <int> | force }
                       <int> -- Number of I/O TLB slabs

I test with parameter
BOARD_KERNEL_CMDLINE += swiotlb=force,500
The result is io_tlb_nslabs=32768, swiotlb_force=0x0,
io_tlb_nslabs=32768 because that if io_tlb_nslabs can't get from 
early_param("swiotlb", setup_io_tlb_npages);
It will be valued at swiotlb_init with default.
So both  "<int>"  and "force" can't recognized with original code below.

static int __init
setup_io_tlb_npages(char *str)
{
        if (isdigit(*str)) {
                io_tlb_nslabs = simple_strtoul(str, &str, 0);
                /* avoid tail segment of size < IO_TLB_SEGSIZE */
                io_tlb_nslabs = ALIGN(io_tlb_nslabs, IO_TLB_SEGSIZE);
        }
        if (*str == ',')
                ++str;
        if (!strcmp(str, "force"))
                swiotlb_force = 1;

        return 0;
}
early_param("swiotlb", setup_io_tlb_npages);

So we can't use Format: { <int> | force | <int> | <int>} any more
as there are four parameters.
Format: { <int>,force,<int>,<int>} is suitable I think.
And fixing  "force" is follow the code design previously in setup_io_tlb_npages.

> Jan

Re: [PATCH v5] modify the IO_TLB_SEGSIZE and IO_TLB_DEFAULT_SIZE configurable as flexible requirement about SW-IOMMU.

[Konrad Rzeszutek Wilk]

. snip..

> Format: { <int>,force,<int>,<int>} is suitable I think.
> And fixing  "force" is follow the code design previously in setup_io_tlb_npages.

It is a bug. It should have been smart enough to deal with the 'force' being
in any order.

If you are willing to make a patch to fix this - either folded into this
patch I am responding to or as a seperate one - that would be most excellent!

However, I can also do it - but my plate is full so it will take me some time
to get to it.

RE: [PATCH v5] modify the IO_TLB_SEGSIZE and IO_TLB_DEFAULT_SIZE configurable as flexible requirement about SW-IOMMU.

[Wang, Xiaoming]

> -----Original Message-----
> From: Konrad Rzeszutek Wilk [mailto:konrad.wilk@oracle.com]
> Sent: Friday, March 6, 2015 11:20 PM
> To: Wang, Xiaoming
> Cc: Jan Beulich; Liu@aserp2030.oracle.com; Zhang@aserp2030.oracle.com;
> chris@chris-wilson.co.uk; david.vrabel@citrix.com; lauraa@codeaurora.org;
> heiko.carstens@de.ibm.com; linux@horizon.com; Liu, Chuansheng; Zhang,
> Dongxing; takahiro.akashi@linaro.org; akpm@linux-foundation.org; linux-
> mips@linux-mips.org; ralf@linux-mips.org; xen-devel@lists.xenproject.org;
> boris.ostrovsky@oracle.com; d.kasatkin@samsung.com; pebolle@tiscali.nl;
> linux-kernel@vger.kernel.org; jkosina@suse.cz
> Subject: Re: [PATCH v5] modify the IO_TLB_SEGSIZE and
> IO_TLB_DEFAULT_SIZE configurable as flexible requirement about SW-
> IOMMU.
> 
> . snip..
> 
> > Format: { <int>,force,<int>,<int>} is suitable I think.
> > And fixing  "force" is follow the code design previously in
> setup_io_tlb_npages.
> 
> It is a bug. It should have been smart enough to deal with the 'force' being in
> any order.
> 
> If you are willing to make a patch to fix this - either folded into this patch I
> am responding to or as a seperate one - that would be most excellent!
> 
OK, I will try to make a patch to deal with the 'force'  in any order.
> However, I can also do it - but my plate is full so it will take me some time to
> get to it.