Re: [Qemu-devel] [PATCH 07/22] memory: add address_space_translate

[Paolo Bonzini]

Il 20/06/2013 15:53, Peter Maydell ha scritto:
> On 30 May 2013 22:03, Paolo Bonzini <address@hidden> 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