From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:36113) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RrVdQ-00010G-6a for qemu-devel@nongnu.org; Sun, 29 Jan 2012 09:21:37 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1RrVdP-0006Tm-0p for qemu-devel@nongnu.org; Sun, 29 Jan 2012 09:21:36 -0500 Received: from mx1.redhat.com ([209.132.183.28]:24400) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1RrVdO-0006Tg-QP for qemu-devel@nongnu.org; Sun, 29 Jan 2012 09:21:34 -0500 Message-ID: <4F2555E5.2060808@redhat.com> Date: Sun, 29 Jan 2012 16:21:25 +0200 From: Avi Kivity MIME-Version: 1.0 References: <4F2536B9.4070305@redhat.com> <4F254786.6070903@redhat.com> In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH] exec-obsolete: fix length handling List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Blue Swirl Cc: qemu-devel , Stefan Berger On 01/29/2012 03:39 PM, Blue Swirl wrote: > >> >> +++ b/exec-obsolete.h > >> >> @@ -81,11 +81,10 @@ static inline void > >> >> cpu_physical_memory_set_dirty_range(ram_addr_t start, > >> >> int dirty_flags) > >> >> { > >> >> uint8_t *p; > >> >> - ram_addr_t addr, end; > >> >> + ram_addr_t cur; > >> >> > >> >> - end = start + length; > >> >> p = ram_list.phys_dirty + (start >> TARGET_PAGE_BITS); > >> >> - for (addr = start; addr <= end; addr += TARGET_PAGE_SIZE) { > >> >> + for (cur = 0; cur < length; cur += TARGET_PAGE_SIZE) { > >> >> *p++ |= dirty_flags; > >> >> } > >> > > >> > I think this is still wrong - if length == 2 it will iterate once, but > >> > we need two iterations if start == 0xfff. > >> > >> Yes, tricky. We could do something like > >> for (cur = start & TARGET_PAGE_MASK; cur < length; cur += TARGET_PAGE_SIZE) { > >> but I'll send a new patch with just s/<=/ > > > That's broken too. > > Because length should be adjusted by -1? 0xfff/2 breaks. More generally, you can't have a loop that just looks at length, since 0/2 wants one iteration, and 0xfff/2 wants two. > > > I have: > > > > uint8_t *p; > > ram_addr_t addr, end; > > > > - end = start + length; > > + end = (start + length - 1) | (TARGET_PAGE_SIZE - 1); > > Why | (TARGET_PAGE_SIZE - 1), for length == 1? Yes. More generally, I wanted something that is easily understood - start/end addresses without trickery, given all the broken patches for fixing this. > TARGET_PAGE_ALIGN() > could be useful here. True, I'll respin. -- error compiling committee.c: too many arguments to function