Patchwork exec.c: address_space_translate: handle access to addr 0 of 2^64 sized region

login
register
mail settings
Submitter Peter Maydell
Date June 20, 2013, 2:18 p.m.
Message ID <1371737884-26020-1-git-send-email-peter.maydell@linaro.org>
Download mbox | patch
Permalink /patch/252944/
State New
Headers show

Comments

Peter Maydell - June 20, 2013, 2:18 p.m.
The memory API allows a MemoryRegion's size to be 2^64, as a special
case (otherwise the size always fits in a 64 bit integer). This meant
that attempts to access address zero in a 2^64 sized region would
assert in address_space_translate():

  #3  0x00007ffff3e4d192 in __GI___assert_fail#(assertion=0x555555a43f32
    "!a.hi", file=0x555555a43ef0 "include/qemu/int128.h", line=18,
    function=0x555555a4439f "int128_get64") at assert.c:103
  #4  0x0000555555877642 in int128_get64 (a=...)
    at include/qemu/int128.h:18
  #5  0x00005555558782f2 in address_space_translate (as=0x55555668d140,
   /addr=0, xlat=0x7fffafac9918, plen=0x7fffafac9920, is_write=false)
    at exec.c:221

Fix this by doing the 'min' operation in 128 bit arithmetic
rather than 64 bit arithmetic (we know the result of the 'min'
definitely fits in 64 bits because one of the inputs did).

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
The other possible approach here would be:
    if (!diff.hi) {
        *plen = MIN(int128_get64(diff), *plen);
    }
(since if diff.hi is nonzero we know plen is smaller) but
it seems slightly cleaner not to "look inside" the Int128.

 exec.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Paolo Bonzini - June 20, 2013, 2:31 p.m.
Il 20/06/2013 16:18, Peter Maydell ha scritto:
> The memory API allows a MemoryRegion's size to be 2^64, as a special
> case (otherwise the size always fits in a 64 bit integer). This meant
> that attempts to access address zero in a 2^64 sized region would
> assert in address_space_translate():
> 
>   #3  0x00007ffff3e4d192 in __GI___assert_fail#(assertion=0x555555a43f32
>     "!a.hi", file=0x555555a43ef0 "include/qemu/int128.h", line=18,
>     function=0x555555a4439f "int128_get64") at assert.c:103
>   #4  0x0000555555877642 in int128_get64 (a=...)
>     at include/qemu/int128.h:18
>   #5  0x00005555558782f2 in address_space_translate (as=0x55555668d140,
>    /addr=0, xlat=0x7fffafac9918, plen=0x7fffafac9920, is_write=false)
>     at exec.c:221
> 
> Fix this by doing the 'min' operation in 128 bit arithmetic
> rather than 64 bit arithmetic (we know the result of the 'min'
> definitely fits in 64 bits because one of the inputs did).
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
> The other possible approach here would be:
>     if (!diff.hi) {
>         *plen = MIN(int128_get64(diff), *plen);
>     }
> (since if diff.hi is nonzero we know plen is smaller) but
> it seems slightly cleaner not to "look inside" the Int128.
> 
>  exec.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/exec.c b/exec.c
> index 5b8b40d..eb200d0 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -218,7 +218,7 @@ MemoryRegionSection *address_space_translate(AddressSpace *as, hwaddr addr,
>      *xlat = addr + section->offset_within_region;
>  
>      diff = int128_sub(section->mr->size, int128_make64(addr));
> -    *plen = MIN(int128_get64(diff), *plen);
> +    *plen = int128_get64(int128_min(diff, int128_make64(*plen)));
>      return section;
>  }
>  #endif
> 

Thanks, applied.

Paolo

Patch

diff --git a/exec.c b/exec.c
index 5b8b40d..eb200d0 100644
--- a/exec.c
+++ b/exec.c
@@ -218,7 +218,7 @@  MemoryRegionSection *address_space_translate(AddressSpace *as, hwaddr addr,
     *xlat = addr + section->offset_within_region;
 
     diff = int128_sub(section->mr->size, int128_make64(addr));
-    *plen = MIN(int128_get64(diff), *plen);
+    *plen = int128_get64(int128_min(diff, int128_make64(*plen)));
     return section;
 }
 #endif