diff mbox

[07/22] memory: add address_space_translate

Message ID 51C30F85.400@redhat.com
State New
Headers show

Commit Message

Paolo Bonzini June 20, 2013, 2:19 p.m. UTC 
Il 20/06/2013 15:53, Peter Maydell ha scritto:
> On 30 May 2013 22:03, Paolo Bonzini <pbonzini@redhat.com> 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:



Paolo

Comments

Peter Maydell June 20, 2013, 2:43 p.m. UTC | #1 
On 20 June 2013 15:19, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Il 20/06/2013 15:53, Peter Maydell ha scritto:
>> 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

Not entirely sure I understand this. There's no particular
reason I can't create a 2^64 sized I/O memory region
and put it in an address space, is there?

thanks
-- PMM
Paolo Bonzini June 20, 2013, 2:53 p.m. UTC | #2 
Il 20/06/2013 16:43, Peter Maydell ha scritto:
>>> >> 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
> Not entirely sure I understand this. There's no particular
> reason I can't create a 2^64 sized I/O memory region
> and put it in an address space, is there?

I think there are problems in the core if you do that (probably part of
it is fixed now).  Still, in cases like this:

    memory_region_add_coalescing(mr, 0, int128_get64(mr->size));

the API simply doesn't support it.

Paolo
diff mbox

Patch

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'