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

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

To: Steven Rostedt <srostedt@xxxxxxxxxx>
Subject: Re: [Xen-devel] [PATCH] wrong accounting in direct_remap_pfn_range
From: Steven Rostedt <srostedt@xxxxxxxxxx>
Date: Tue, 29 Aug 2006 14:22:27 -0400
Cc: xen-devel@xxxxxxxxxxxxxxxxxxx, quintela@xxxxxxxxxx
Delivery-date: Tue, 29 Aug 2006 11:22:00 -0700
Envelope-to: www-data@xxxxxxxxxxxxxxxxxx
In-reply-to: <44F46A7E.9010906@xxxxxxxxxx>
List-help: <mailto:xen-devel-request@lists.xensource.com?subject=help>
List-id: Xen developer discussion <xen-devel.lists.xensource.com>
List-post: <mailto:xen-devel@lists.xensource.com>
List-subscribe: <http://lists.xensource.com/cgi-bin/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=subscribe>
List-unsubscribe: <http://lists.xensource.com/cgi-bin/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=unsubscribe>
References: <44F46A7E.9010906@xxxxxxxxxx>
Sender: xen-devel-bounces@xxxxxxxxxxxxxxxxxxx
User-agent: Thunderbird 1.5.0.4 (X11/20060614)
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