Re: [Xen-devel] [PATCH] wrong accounting in direct_remap_pfn_range

[Steven Rostedt <srostedt@xxxxxxxxxx>]

Steven Rostedt wrote:
> Looking into the code in linux-2.6-xen-sparse/arch/i386/mm/ioremap-xen.c
>
> I found some logic that did not make sense.  We have a loop than updates 
> the page tables in __direct_remap_pfn_range, and in the beginning of 
> that loop, there is a test that if we finished the page
> (v-u == PAGE_SIZE/sizeof(mmu_update_t)) we call the hypervisor to do our 
> update.  This is all fine and dandy but, but the code right after the 
> loop seems to be wrong.
> There is a check if (v != u) then do some more work.  I'm assuming that 
> this code is there in case we didn't reach the if statement at the top 
> of the loop.  But what is wrong is that this check is invalid.  Although 
> the loop if statement sets v = u, the following if statement ignores the 
> fact that v++ is done at the bottom of the loop.  So if we really want 
> to do this extra work if we didn't finish the loop, then the test really 
> needs to be.
> if ((v - u) != 1) { .... }
>
>
> Unless I'm missing something here, I've attached a patch.
I did miss something.  The fact that the loop may only go once.  This 
means that v - u will equal 1 and we miss that allocation.  So to handle 
 this, I'm submitting this patch that just keeps track of whether or 
not we need to do the final fixup.
-- Steve

Signed-off-by: Steven Rostedt <srostedt@xxxxxxxxxx>


Index: linux-2.6-xen-sparse/arch/i386/mm/ioremap-xen.c
===================================================================
--- linux-2.6-xen-sparse.orig/arch/i386/mm/ioremap-xen.c
+++ linux-2.6-xen-sparse/arch/i386/mm/ioremap-xen.c
@@ -55,6 +55,7 @@ static int __direct_remap_pfn_range(stru
        int rc;
        unsigned long i, start_address;
        mmu_update_t *u, *v, *w;
+       int fixup = 1;
 
        u = v = w = (mmu_update_t *)__get_free_page(GFP_KERNEL|__GFP_REPEAT);
        if (u == NULL)
@@ -65,6 +66,7 @@ static int __direct_remap_pfn_range(stru
        flush_cache_all();
 
        for (i = 0; i < size; i += PAGE_SIZE) {
+               fixup = 1;
                if ((v - u) == (PAGE_SIZE / sizeof(mmu_update_t))) {
                        /* Fill in the PTE pointers. */
                        rc = apply_to_page_range(mm, start_address, 
@@ -78,6 +80,7 @@ static int __direct_remap_pfn_range(stru
                                goto out;
                        v = u;
                        start_address = address;
+                       fixup = 0;
                }
 
                /*
@@ -91,7 +94,11 @@ static int __direct_remap_pfn_range(stru
                v++;
        }
 
-       if (v != u) {
+       /*
+        * If we didn't finish the page in the previous loop then we
+        * need to process it now.
+        */
+       if (fixup) {
                /* get the ptep's filled in */
                rc = apply_to_page_range(mm, start_address,
                                         address - start_address,

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