WARNING - OLD ARCHIVES

This is an archived copy of the Xen.org mailing list, which we have preserved to ensure that existing links to archives are not broken. The live archive, which contains the latest emails, can be found at http://lists.xen.org/
   
 
 
Xen 
 
Home Products Support Community News
 
   
 

xen-devel

[Xen-devel] Re: [PATCH 18 of 38] x86: unify pci iommu setup and allow sw

On Sat, 2008-11-22 at 10:49 +0900, FUJITA Tomonori wrote:
> On Fri, 21 Nov 2008 14:21:32 +0000
> Ian Campbell <Ian.Campbell@xxxxxxxxxx> wrote:
> 
> > On Wed, 2008-11-19 at 11:19 +0900, FUJITA Tomonori wrote:
> > > 
> > > The problem that I talked about in the previous mail:
> > > 
> > > >       max_slots = mask + 1
> > > >                   ? ALIGN(mask + 1, 1 << IO_TLB_SHIFT) >> IO_TLB_SHIFT
> > > >                   : 1UL << (BITS_PER_LONG - IO_TLB_SHIFT);
> > > 
> > > Since the popular value of the mask is 0xffffffff. So the above code
> > > (mask + 1 ?) works wrongly if the size of mask is 32bit (well,
> > > accidentally the result of max_slots is identical though).
> > 
> > I've just been looking at this again and I don't think it is an accident
> > that this evaluates to the correct value when mask + 1 == 0.
> > 
> > The patch which adds the "mask + 1 ? ... : 1UL << ..." stuff is:
> > 
> >         commit b15a3891c916f32a29832886a053a48be2741d4d
> >         Author: Jan Beulich <jbeulich@xxxxxxxxxx>
> >         Date:   Thu Mar 13 09:13:30 2008 +0000
> >         
> >             avoid endless loops in lib/swiotlb.c
> >             
> >             Commit 681cc5cd3efbeafca6386114070e0bfb5012e249 ("iommu sg 
> > merging:
> >             swiotlb: respect the segment boundary limits") introduced two
> >             possibilities for entering an endless loop in lib/swiotlb.c:
> >             
> >              - if max_slots is zero (possible if mask is ~0UL)
> >              [...]
> > 
> > I think the existing code is the nicest way to handle this corner case
> > and it is necessary anyway to handle the ~0UL case on 64 bit.
> 
> Ah, I vaguely remember this patch. The ~0ULL mask didn't happen here
> (nobody uses it) so the possibility was false. IMHO, if we use this
> code on 32bit architectures, the mask should be u64 and the overflow
> should be handled explicitly. But as you pointed out, looks like that
> this patch takes account of the overflow.

Something like this?

Ian.
--- 

swiotlb: explicitly handle segment boundary mask overflow.

When swiotlb is used on 32 bit we can overflow mask + 1 in the common
case where mask is 0xffffffffUL. This overflow was previously caught
by the case which attempts to handle a mask of ~0UL on 64 bit.

Signed-off-by: Ian Campbell <ian.campbell@xxxxxxxxxx>

diff -r 5fa30e5284dd lib/swiotlb.c
--- a/lib/swiotlb.c     Mon Nov 24 09:39:50 2008 +0000
+++ b/lib/swiotlb.c     Mon Nov 24 11:37:39 2008 +0000
@@ -303,7 +303,7 @@
        unsigned int nslots, stride, index, wrap;
        int i;
        unsigned long start_dma_addr;
-       unsigned long mask;
+       u64 mask;
        unsigned long offset_slots;
        unsigned long max_slots;
 
@@ -314,6 +314,7 @@
        max_slots = mask + 1
                    ? ALIGN(mask + 1, 1 << IO_TLB_SHIFT) >> IO_TLB_SHIFT
                    : 1UL << (BITS_PER_LONG - IO_TLB_SHIFT);
+       BUG_ON(max_slots > 1UL << (BITS_PER_LONG - IO_TLB_SHIFT));
 
        /*
         * For mappings greater than a page, we limit the stride (and




_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel

<Prev in Thread] Current Thread [Next in Thread>