Re: [PATCH] wrong accounting in direct_remap_pfn_range

[Steven Rostedt <srostedt@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 1367 bytes --]

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@redhat.com>



[-- Attachment #2: ioredirect.patch --]
[-- Type: text/x-patch, Size: 1212 bytes --]

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,

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

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