Re: [PATCH 0 of 4] x8-64: Calgary: updates for CONFIG_IOMMU_DEBUG

Re: [PATCH 0 of 4] x8-64: Calgary: updates for CONFIG_IOMMU_DEBUG

[Andi Kleen]

On Thursday 03 August 2006 00:59, Muli Ben-Yehuda wrote:
> Hi Andi,
> 
> A few Calgary updates for CONFIG_IOMMU_DEBUG: print a message when
> CONFIG_IOMMU_DEBUG is on, remove dangerous bringup debugging code and
> only double check our bitmap handling if currently debugging.
> 
> Please apply, patches are againt mainline with all previous Calgary
> patches applied (which should apply cleanly to your tree).

Added thanks.

In the future please be a little bit more verbose in the changelog
department.

_Andi

Re: [PATCH 0 of 4] x8-64: Calgary: updates for CONFIG_IOMMU_DEBUG

[Muli Ben-Yehuda]

On Thu, Aug 03, 2006 at 01:16:48AM +0200, Andi Kleen wrote:
> On Thursday 03 August 2006 00:59, Muli Ben-Yehuda wrote:
> > Hi Andi,
> > 
> > A few Calgary updates for CONFIG_IOMMU_DEBUG: print a message when
> > CONFIG_IOMMU_DEBUG is on, remove dangerous bringup debugging code and
> > only double check our bitmap handling if currently debugging.
> > 
> > Please apply, patches are againt mainline with all previous Calgary
> > patches applied (which should apply cleanly to your tree).
> 
> Added thanks.
> 
> In the future please be a little bit more verbose in the changelog
> department.

The patches themselves have verbose changelog entries, I never know
quite what to write in the introductory message - I guess I'll just
hack mercurial's email extension to get rid of it (unless it's useful
to you?)

Cheers,
Muli

Re: [PATCH 2 of 4] [x86-64] Calgary: only verify the allocation bitmap if CONFIG_IOMMU_DEBUG is on

[Jon Mason]

On Thu, Aug 03, 2006 at 01:59:09AM +0300, Muli Ben-Yehuda wrote:
> 1 files changed, 36 insertions(+), 10 deletions(-)
> arch/x86_64/kernel/pci-calgary.c |   46 +++++++++++++++++++++++++++++---------
> 
> 
> # HG changeset patch
> # User Muli Ben-Yehuda <muli@il.ibm.com>
> # Date 1154558421 -10800
> # Node ID 515131a26b151f1e459642268184d98099e72a6c
> # Parent  9cd758467ce15605504369cf56f790aea8c74748
> [x86-64] Calgary: only verify the allocation bitmap if CONFIG_IOMMU_DEBUG is on
> 
> Introduce new function verify_bit_range(). Define two versions, one
> for CONFIG_IOMMU_DEBUG enabled and one for disabled. Previously we
> were checking that the bitmap was consistent every time we allocated
> or freed an entry in the TCE table, which is good for debugging but
> incurs an unnecessary penalty on non debug builds.
> 
> Signed-off-by: Muli Ben-Yehuda <muli@il.ibm.com>
> Signed-off-by: Jon Mason <jdmason@us.ibm.com>
> 
> diff -r 9cd758467ce1 -r 515131a26b15 arch/x86_64/kernel/pci-calgary.c
> --- a/arch/x86_64/kernel/pci-calgary.c	Thu Aug 03 01:37:12 2006 +0300
> +++ b/arch/x86_64/kernel/pci-calgary.c	Thu Aug 03 01:40:21 2006 +0300
> @@ -133,12 +133,35 @@ static inline void tce_cache_blast_stres
>  {
>  	tce_cache_blast(tbl);
>  }
> +
> +static inline unsigned long verify_bit_range(unsigned long* bitmap,
> +	int expected, unsigned long start, unsigned long end)
> +{
> +	unsigned long idx = start;
> +
> +	BUG_ON(start > end);

This should be ">=".

Thanks,
Jon

> +
> +	while (idx < end) {
> +		if (!!test_bit(idx, bitmap) != expected)
> +			return idx;
> +		++idx;
> +	}
> +
> +	/* all bits have the expected value */
> +	return ~0UL;
> +}
>  #else /* debugging is disabled */
>  int debugging __read_mostly = 0;
>  
>  static inline void tce_cache_blast_stress(struct iommu_table *tbl)
>  {
>  }
> +
> +static inline unsigned long verify_bit_range(unsigned long* bitmap,
> +	int expected, unsigned long start, unsigned long end)
> +{
> +	return ~0UL;
> +}
>  #endif /* CONFIG_IOMMU_DEBUG */
>  
>  static inline unsigned int num_dma_pages(unsigned long dma, unsigned int dmalen)
> @@ -162,6 +185,7 @@ static void iommu_range_reserve(struct i
>  {
>  	unsigned long index;
>  	unsigned long end;
> +	unsigned long badbit;
>  
>  	index = start_addr >> PAGE_SHIFT;
>  
> @@ -173,14 +197,15 @@ static void iommu_range_reserve(struct i
>  	if (end > tbl->it_size) /* don't go off the table */
>  		end = tbl->it_size;
>  
> -	while (index < end) {
> -		if (test_bit(index, tbl->it_map))
> +	badbit = verify_bit_range(tbl->it_map, 0, index, end);
> +	if (badbit != ~0UL) {
> +		if (printk_ratelimit())
>  			printk(KERN_ERR "Calgary: entry already allocated at "
>  			       "0x%lx tbl %p dma 0x%lx npages %u\n",
> -			       index, tbl, start_addr, npages);
> -		++index;
> -	}
> -	set_bit_string(tbl->it_map, start_addr >> PAGE_SHIFT, npages);
> +			       badbit, tbl, start_addr, npages);
> +	}
> +
> +	set_bit_string(tbl->it_map, index, npages);
>  }
>  
>  static unsigned long iommu_range_alloc(struct iommu_table *tbl,
> @@ -247,7 +272,7 @@ static void __iommu_free(struct iommu_ta
>  	unsigned int npages)
>  {
>  	unsigned long entry;
> -	unsigned long i;
> +	unsigned long badbit;
>  
>  	entry = dma_addr >> PAGE_SHIFT;
>  
> @@ -255,11 +280,12 @@ static void __iommu_free(struct iommu_ta
>  
>  	tce_free(tbl, entry, npages);
>  
> -	for (i = 0; i < npages; ++i) {
> -		if (!test_bit(entry + i, tbl->it_map))
> +	badbit = verify_bit_range(tbl->it_map, 1, entry, entry + npages);
> +	if (badbit != ~0UL) {
> +		if (printk_ratelimit())
>  			printk(KERN_ERR "Calgary: bit is off at 0x%lx "
>  			       "tbl %p dma 0x%Lx entry 0x%lx npages %u\n",
> -			       entry + i, tbl, dma_addr, entry, npages);
> +			       badbit, tbl, dma_addr, entry, npages);
>  	}
>  
>  	__clear_bit_string(tbl->it_map, entry, npages);

Re: [PATCH 2 of 4] [x86-64] Calgary: only verify the allocation bitmap if CONFIG_IOMMU_DEBUG is on

[Andi Kleen]

> > +	BUG_ON(start > end);
> 
> This should be ">=".

Changed.

-Andi

Re: [PATCH 2 of 4] [x86-64] Calgary: only verify the allocation bitmap if CONFIG_IOMMU_DEBUG is on

[Muli Ben-Yehuda]

On Wed, Aug 02, 2006 at 10:57:24PM -0500, Jon Mason wrote:

> > diff -r 9cd758467ce1 -r 515131a26b15 arch/x86_64/kernel/pci-calgary.c
> > --- a/arch/x86_64/kernel/pci-calgary.c	Thu Aug 03 01:37:12 2006 +0300
> > +++ b/arch/x86_64/kernel/pci-calgary.c	Thu Aug 03 01:40:21 2006 +0300
> > @@ -133,12 +133,35 @@ static inline void tce_cache_blast_stres
> >  {
> >  	tce_cache_blast(tbl);
> >  }
> > +
> > +static inline unsigned long verify_bit_range(unsigned long* bitmap,
> > +	int expected, unsigned long start, unsigned long end)
> > +{
> > +	unsigned long idx = start;
> > +
> > +	BUG_ON(start > end);
> 
> This should be ">=".

I considered both options and decided that since this is debug code,
we might as well be permissive and not trip when start == end (because
I'm not 100% sure we never do it). However since it could arguably be
masking a real bug if we do, I agree with the change.

Cheers,
Muli