From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:58233) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UpfiV-0007WD-93 for qemu-devel@nongnu.org; Thu, 20 Jun 2013 10:20:07 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UpfiQ-0006wE-9v for qemu-devel@nongnu.org; Thu, 20 Jun 2013 10:20:02 -0400 Received: from mx1.redhat.com ([209.132.183.28]:61563) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UpfiQ-0006wA-2e for qemu-devel@nongnu.org; Thu, 20 Jun 2013 10:19:58 -0400 Message-ID: <51C30F85.400@redhat.com> Date: Thu, 20 Jun 2013 16:19:49 +0200 From: Paolo Bonzini MIME-Version: 1.0 References: <1369947836-2638-1-git-send-email-pbonzini@redhat.com> <1369947836-2638-8-git-send-email-pbonzini@redhat.com> In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 07/22] memory: add address_space_translate List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Peter Maydell Cc: qemu-devel@nongnu.org Il 20/06/2013 15:53, Peter Maydell ha scritto: > On 30 May 2013 22:03, Paolo Bonzini wrote: >> +MemoryRegionSection *address_space_translate(AddressSpace *as, hwaddr addr, >> + hwaddr *xlat, hwaddr *plen, >> + bool is_write) >> +{ >> + MemoryRegionSection *section; >> + Int128 diff; >> + >> + section = phys_page_find(as->dispatch, addr >> TARGET_PAGE_BITS); >> + /* Compute offset within MemoryRegionSection */ >> + addr -= section->offset_within_address_space; >> + >> + /* Compute offset within MemoryRegion */ >> + *xlat = addr + section->offset_within_region; >> + >> + diff = int128_sub(section->mr->size, int128_make64(addr)); >> + *plen = MIN(int128_get64(diff), *plen); > > I've just run into a situation where the assertion in > int128_get64() that the value fits into a 64 bit integer > fires. This happened to me for an access to address zero > in the 'unassigned' region: > * io_mem_init() sets the size of these to UINT64_MAX > * memory_region_init() special-cases that size as meaning > 2^64, ie {hi=1,lo=0} > * since the addr is zero diff is also {hi=1,lo=0}, and > then int128_get64() asserts. This would be fixed by: *plen = int128_get64(int128_min(diff, int128_make64(*plen))); (We can optimize int128 so that it produces more than decent code for this). > There are other places in memory.c which do an int128_get64() > on mr->size, which also look suspicious... They are all on I/O regions so they are safe, except those in "info mtree". I'm going to squash this: diff --git a/memory.c b/memory.c index c500d8d..47b005a 100644 --- a/memory.c +++ b/memory.c @@ -1744,7 +1744,7 @@ static void mtree_print_mr(fprintf_function mon_printf, void *f, "-" TARGET_FMT_plx "\n", base + mr->addr, base + mr->addr - + (hwaddr)int128_get64(mr->size) - 1, + + (hwaddr)int128_get64(int128_sub(mr->size, int128_make64(1))), mr->priority, mr->romd_mode ? 'R' : '-', !mr->readonly && !(mr->rom_device && mr->romd_mode) ? 'W' @@ -1759,7 +1759,7 @@ static void mtree_print_mr(fprintf_function mon_printf, void *f, TARGET_FMT_plx "-" TARGET_FMT_plx " (prio %d, %c%c): %s\n", base + mr->addr, base + mr->addr - + (hwaddr)int128_get64(mr->size) - 1, + + (hwaddr)int128_get64(int128_sub(mr->size, int128_make64(1))), mr->priority, mr->romd_mode ? 'R' : '-', !mr->readonly && !(mr->rom_device && mr->romd_mode) ? 'W' Paolo