RE: [Xen-devel] [PATCH v4] 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@xxxxxxxx]
> Sent: Wednesday, February 18, 2015 5:35 PM
> To: Wang, Xiaoming
> Cc: chris@xxxxxxxxxxxxxxxxxx; david.vrabel@xxxxxxxxxx;
> lauraa@xxxxxxxxxxxxxx; heiko.carstens@xxxxxxxxxx; linux@xxxxxxxxxxx;
> Liu, Chuansheng; Zhang, Dongxing; takahiro.akashi@xxxxxxxxxx;
> akpm@xxxxxxxxxxxxxxxxxxxx; linux-mips@xxxxxxxxxxxxxx; ralf@linux-
> mips.org; xen-devel@xxxxxxxxxxxxxxxxxxxx; boris.ostrovsky@xxxxxxxxxx;
> konrad.wilk@xxxxxxxxxx; d.kasatkin@xxxxxxxxxxx; pebolle@xxxxxxxxxx;
> linux-kernel@xxxxxxxxxxxxxxx
> Subject: RE: [Xen-devel] [PATCH v4] modify the IO_TLB_SEGSIZE and
> IO_TLB_DEFAULT_SIZE configurable as flexible requirement about SW-
> IOMMU.
> 
> >>> On 18.02.15 at 10:09, <xiaoming.wang@xxxxxxxxx> wrote:
> >> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> >> Sent: Tuesday, February 17, 2015 6:09 PM
> >> >>> On 17.02.15 at 07:51, <xiaoming.wang@xxxxxxxxx> wrote:
> >> > --- a/Documentation/kernel-parameters.txt
> >> > +++ b/Documentation/kernel-parameters.txt
> >> > @@ -3438,10 +3438,12 @@ 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.
> >>
> >> This makes no sense - the new numbers added aren't position
> >> independent (nor were the previous <int> and "force").
> >>
> > Use ","  can separate them one by one.
> > We do it at lib/swiotlb.c
> 
> Right, but the documentation above doesn't say so.
> 
OK, I will add some comments on next patch version.
> >> Also you are (supposedly) removing all uses of IO_TLB_DEFAULT_SIZE,
> >> yet you don't seem to remove the definition itself.
> >>
> > I have change all uses of IO_TLB_DEFAULT_SIZE to io_tlb_default_size
> > in lib/swiotlb.c
> 
> Then are there any left elsewhere? If not, again - why don't you remove the
> definition of IO_TLB_DEFAULT_SIZE?
> 
There hasn't any IO_TLB_DEFAULT_SIZE left.
I check the code IO_TLB_DEFAULT_SIZE only used in lib/swiotlb.c.
And I have removed the definition of IO_TLB_DEFAULT_SIZE, in my patch

@@ -120,15 +146,13 @@ unsigned long swiotlb_nr_tbl(void)  }  EXPORT_SYMBOL_GPL(swiotlb_nr_tbl);
 
-/* default to 64MB */
-#define IO_TLB_DEFAULT_SIZE (64UL<<20)

> >> Finally - are arbitrary numbers really okay for the newly added
> >> command line options? I.e. shouldn't you add some checking of their
> validity?
> >>
> > I have validity these code is OK.
> > Example:
> > BOARD_KERNEL_CMDLINE += swiotlb=, ,512,268435456 Io_tlb_segsize has
> > been changed from 128 to 512 Io_tlb_default_size has been changed from
> > 64M to 268435456  (256M)
> 
> I specifically said "arbitrary numbers", which in particular includes zero and
> non-power-of-2 values. If there are any restrictions on which numbers can
> validly be passed here (and it very much looks like there are), such
> restrictions should be enforced imo.
> 
OK, we will validate for these variables' value in next patch version.
> Jan
Xiaoming
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/