From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:49963) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XqK5w-0002kz-Ss for qemu-devel@nongnu.org; Mon, 17 Nov 2014 06:03:49 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XqK5s-0000uH-EO for qemu-devel@nongnu.org; Mon, 17 Nov 2014 06:03:44 -0500 Received: from mx1.redhat.com ([209.132.183.28]:28526) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XqK5s-0000u8-2W for qemu-devel@nongnu.org; Mon, 17 Nov 2014 06:03:40 -0500 Message-ID: <5469D604.2010800@redhat.com> Date: Mon, 17 Nov 2014 12:03:32 +0100 From: Paolo Bonzini MIME-Version: 1.0 References: <1416167061-13203-1-git-send-email-peter.maydell@linaro.org> In-Reply-To: <1416167061-13203-1-git-send-email-peter.maydell@linaro.org> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH] exec: Handle multipage ranges in invalidate_and_set_dirty() List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Peter Maydell , qemu-devel@nongnu.org Cc: Stefan Hajnoczi On 16/11/2014 20:44, Peter Maydell wrote: > The code in invalidate_and_set_dirty() needs to handle addr/length > combinations which cross guest physical page boundaries. This can happen, > for example, when disk I/O reads large blocks into guest RAM which previously > held code that we have cached translations for. Unfortunately we were only > checking the clean/dirty status of the first page in the range, and then > were calling a tb_invalidate function which only handles ranges that don't > cross page boundaries. Fix the function to deal with multipage ranges. > > The symptoms of this bug were that guest code would misbehave (eg segfault), > in particular after a guest reboot but potentially any time the guest > reused a page of its physical RAM for new code. > > Cc: qemu-stable@nongnu.org > Signed-off-by: Peter Maydell > --- > This seems pretty nasty, and I have no idea why it hasn't been wreaking > more havoc than this before. I'm not entirely sure why we invalidate TBs > if any of the dirty bits is set rather than only if the code bit is set, > but I left that logic as it is. I think it's a remain of when we had a single bitmap with three bits in it. We can clean up in 2.3. > Review appreciated -- it would be nice to get this into rc2 if > we can, I think. Reviewed-by: Paolo Bonzini > exec.c | 6 ++---- > include/exec/ram_addr.h | 25 +++++++++++++++++++++++++ > 2 files changed, 27 insertions(+), 4 deletions(-) > > diff --git a/exec.c b/exec.c > index 759055d..f0e2bd3 100644 > --- a/exec.c > +++ b/exec.c > @@ -2066,10 +2066,8 @@ int cpu_memory_rw_debug(CPUState *cpu, target_ulong addr, > static void invalidate_and_set_dirty(hwaddr addr, > hwaddr length) > { > - if (cpu_physical_memory_is_clean(addr)) { > - /* invalidate code */ > - tb_invalidate_phys_page_range(addr, addr + length, 0); > - /* set dirty bit */ > + if (cpu_physical_memory_range_includes_clean(addr, length)) { > + tb_invalidate_phys_range(addr, addr + length, 0); > cpu_physical_memory_set_dirty_range_nocode(addr, length); > } > xen_modified_memory(addr, length); > diff --git a/include/exec/ram_addr.h b/include/exec/ram_addr.h > index cf1d4c7..8fc75cd 100644 > --- a/include/exec/ram_addr.h > +++ b/include/exec/ram_addr.h > @@ -49,6 +49,21 @@ static inline bool cpu_physical_memory_get_dirty(ram_addr_t start, > return next < end; > } > > +static inline bool cpu_physical_memory_get_clean(ram_addr_t start, > + ram_addr_t length, > + unsigned client) > +{ > + unsigned long end, page, next; > + > + assert(client < DIRTY_MEMORY_NUM); > + > + end = TARGET_PAGE_ALIGN(start + length) >> TARGET_PAGE_BITS; > + page = start >> TARGET_PAGE_BITS; > + next = find_next_zero_bit(ram_list.dirty_memory[client], end, page); > + > + return next < end; > +} > + > static inline bool cpu_physical_memory_get_dirty_flag(ram_addr_t addr, > unsigned client) > { > @@ -64,6 +79,16 @@ static inline bool cpu_physical_memory_is_clean(ram_addr_t addr) > return !(vga && code && migration); > } > > +static inline bool cpu_physical_memory_range_includes_clean(ram_addr_t start, > + ram_addr_t length) > +{ > + bool vga = cpu_physical_memory_get_clean(start, length, DIRTY_MEMORY_VGA); > + bool code = cpu_physical_memory_get_clean(start, length, DIRTY_MEMORY_CODE); > + bool migration = > + cpu_physical_memory_get_clean(start, length, DIRTY_MEMORY_MIGRATION); > + return vga || code || migration; > +} > + > static inline void cpu_physical_memory_set_dirty_flag(ram_addr_t addr, > unsigned client) > { >