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/
Home Products Support Community News


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

To: xen-devel@xxxxxxxxxxxxxxxxxxx
Subject: [Xen-devel] [PATCH] wrong accounting in direct_remap_pfn_range
From: Steven Rostedt <srostedt@xxxxxxxxxx>
Date: Tue, 29 Aug 2006 12:25:34 -0400
Cc: quintela@xxxxxxxxxx
Delivery-date: Tue, 29 Aug 2006 09:25:12 -0700
Envelope-to: www-data@xxxxxxxxxxxxxxxxxx
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>
Sender: xen-devel-bounces@xxxxxxxxxxxxxxxxxxx
User-agent: Thunderbird (X11/20060614)
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.

-- 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
@@ -91,7 +91,13 @@ static int __direct_remap_pfn_range(stru
-       if (v != u) {
+       /*
+        * If we didn't finish the page in the previous loop then we
+        * need to process it now.  We take into account the v++
+        * at the end of the loop, so the test to know if we finished
+        * or not is really a +1 difference and not an equal.
+        */
+       if ((v - u) != 1) {
                /* get the ptep's filled in */
                rc = apply_to_page_range(mm, start_address,
                                         address - start_address,
Xen-devel mailing list