From mboxrd@z Thu Jan 1 00:00:00 1970 From: Steven Rostedt Subject: Re: [PATCH] wrong accounting in direct_remap_pfn_range Date: Tue, 29 Aug 2006 14:22:27 -0400 Message-ID: <44F485E3.6010703@redhat.com> References: <44F46A7E.9010906@redhat.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="------------040102010106000807090505" Return-path: In-Reply-To: <44F46A7E.9010906@redhat.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xensource.com Errors-To: xen-devel-bounces@lists.xensource.com To: Steven Rostedt Cc: xen-devel@lists.xensource.com, quintela@redhat.com List-Id: xen-devel@lists.xenproject.org This is a multi-part message in MIME format. --------------040102010106000807090505 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit 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 --------------040102010106000807090505 Content-Type: text/x-patch; name="ioredirect.patch" Content-Transfer-Encoding: 7bit Content-Disposition: inline; filename="ioredirect.patch" 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, --------------040102010106000807090505 Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel --------------040102010106000807090505--