[Qemu-devel] [PATCH v2] memory: use signed arithmetic

[Qemu-devel] [PATCH v2] memory: use signed arithmetic

[Avi Kivity]

When trying to map an alias of a ram region, where the alias starts at
address A and we map it into address B, and A > B, we had an arithmetic
underflow.  Because we use unsigned arithmetic, the underflow converted
into a large number which failed addrrange_intersects() tests.

The concrete example which triggered this was cirrus vga mapping
the framebuffer at offsets 0xc0000-0xc7fff (relative to the start of
the framebuffer) into offsets 0xa0000 (relative to system addres space
start).

With our favorite analogy of a windowing system, this is equivalent to
dragging a subwindow off the left edge of the screen, and failing to clip
it into its parent window which is on screen.

Fix by switching to signed arithmetic.

Signed-off-by: Avi Kivity <avi@redhat.com>
---

v2: add comment about physical address width limitation to 63 bits

 exec.c   |    2 +-
 memory.c |   23 ++++++++++++++---------
 2 files changed, 15 insertions(+), 10 deletions(-)

diff --git a/exec.c b/exec.c
index 476b507..751fd89 100644
--- a/exec.c
+++ b/exec.c
@@ -3818,7 +3818,7 @@ static void io_mem_init(void)
 static void memory_map_init(void)
 {
     system_memory = qemu_malloc(sizeof(*system_memory));
-    memory_region_init(system_memory, "system", UINT64_MAX);
+    memory_region_init(system_memory, "system", INT64_MAX);
     set_system_memory_map(system_memory);
 }
 
diff --git a/memory.c b/memory.c
index 5f20320..be891c6 100644
--- a/memory.c
+++ b/memory.c
@@ -22,12 +22,17 @@ unsigned memory_region_transaction_depth = 0;
 
 typedef struct AddrRange AddrRange;
 
+/*
+ * Note using signed integers limits us to physical addresses at most
+ * 63 bits wide.  They are needed for negative offsetting in aliases
+ * (large MemoryRegion::alias_offset).
+ */
 struct AddrRange {
-    uint64_t start;
-    uint64_t size;
+    int64_t start;
+    int64_t size;
 };
 
-static AddrRange addrrange_make(uint64_t start, uint64_t size)
+static AddrRange addrrange_make(int64_t start, int64_t size)
 {
     return (AddrRange) { start, size };
 }
@@ -37,7 +42,7 @@ static bool addrrange_equal(AddrRange r1, AddrRange r2)
     return r1.start == r2.start && r1.size == r2.size;
 }
 
-static uint64_t addrrange_end(AddrRange r)
+static int64_t addrrange_end(AddrRange r)
 {
     return r.start + r.size;
 }
@@ -56,9 +61,9 @@ static bool addrrange_intersects(AddrRange r1, AddrRange r2)
 
 static AddrRange addrrange_intersection(AddrRange r1, AddrRange r2)
 {
-    uint64_t start = MAX(r1.start, r2.start);
+    int64_t start = MAX(r1.start, r2.start);
     /* off-by-one arithmetic to prevent overflow */
-    uint64_t end = MIN(addrrange_end(r1) - 1, addrrange_end(r2) - 1);
+    int64_t end = MIN(addrrange_end(r1) - 1, addrrange_end(r2) - 1);
     return addrrange_make(start, end - start + 1);
 }
 
@@ -411,8 +416,8 @@ static void render_memory_region(FlatView *view,
     MemoryRegion *subregion;
     unsigned i;
     target_phys_addr_t offset_in_region;
-    uint64_t remain;
-    uint64_t now;
+    int64_t remain;
+    int64_t now;
     FlatRange fr;
     AddrRange tmp;
 
@@ -486,7 +491,7 @@ static FlatView generate_memory_topology(MemoryRegion *mr)
 
     flatview_init(&view);
 
-    render_memory_region(&view, mr, 0, addrrange_make(0, UINT64_MAX));
+    render_memory_region(&view, mr, 0, addrrange_make(0, INT64_MAX));
     flatview_simplify(&view);
 
     return view;
-- 
1.7.5.3

Re: [Qemu-devel] [PATCH v2] memory: use signed arithmetic

[Richard Henderson]

On 08/03/2011 01:56 AM, Avi Kivity wrote:
> When trying to map an alias of a ram region, where the alias starts at
> address A and we map it into address B, and A > B, we had an arithmetic
> underflow.  Because we use unsigned arithmetic, the underflow converted
> into a large number which failed addrrange_intersects() tests.
> 
> The concrete example which triggered this was cirrus vga mapping
> the framebuffer at offsets 0xc0000-0xc7fff (relative to the start of
> the framebuffer) into offsets 0xa0000 (relative to system addres space
> start).
> 
> With our favorite analogy of a windowing system, this is equivalent to
> dragging a subwindow off the left edge of the screen, and failing to clip
> it into its parent window which is on screen.
> 
> Fix by switching to signed arithmetic.
> 
> Signed-off-by: Avi Kivity <avi@redhat.com>

Signed-off-by: Richard Henderson <rth@twiddle.net>


r~

Re: [Qemu-devel] [PATCH v2] memory: use signed arithmetic

[Anthony Liguori]

On 08/03/2011 03:56 AM, Avi Kivity wrote:
> When trying to map an alias of a ram region, where the alias starts at
> address A and we map it into address B, and A>  B, we had an arithmetic
> underflow.  Because we use unsigned arithmetic, the underflow converted
> into a large number which failed addrrange_intersects() tests.
>
> The concrete example which triggered this was cirrus vga mapping
> the framebuffer at offsets 0xc0000-0xc7fff (relative to the start of
> the framebuffer) into offsets 0xa0000 (relative to system addres space
> start).
>
> With our favorite analogy of a windowing system, this is equivalent to
> dragging a subwindow off the left edge of the screen, and failing to clip
> it into its parent window which is on screen.
>
> Fix by switching to signed arithmetic.
>
> Signed-off-by: Avi Kivity<avi@redhat.com>

Applied.  Thanks.

Regards,

Anthony Liguori

> ---
>
> v2: add comment about physical address width limitation to 63 bits
>
>   exec.c   |    2 +-
>   memory.c |   23 ++++++++++++++---------
>   2 files changed, 15 insertions(+), 10 deletions(-)
>
> diff --git a/exec.c b/exec.c
> index 476b507..751fd89 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -3818,7 +3818,7 @@ static void io_mem_init(void)
>   static void memory_map_init(void)
>   {
>       system_memory = qemu_malloc(sizeof(*system_memory));
> -    memory_region_init(system_memory, "system", UINT64_MAX);
> +    memory_region_init(system_memory, "system", INT64_MAX);
>       set_system_memory_map(system_memory);
>   }
>
> diff --git a/memory.c b/memory.c
> index 5f20320..be891c6 100644
> --- a/memory.c
> +++ b/memory.c
> @@ -22,12 +22,17 @@ unsigned memory_region_transaction_depth = 0;
>
>   typedef struct AddrRange AddrRange;
>
> +/*
> + * Note using signed integers limits us to physical addresses at most
> + * 63 bits wide.  They are needed for negative offsetting in aliases
> + * (large MemoryRegion::alias_offset).
> + */
>   struct AddrRange {
> -    uint64_t start;
> -    uint64_t size;
> +    int64_t start;
> +    int64_t size;
>   };
>
> -static AddrRange addrrange_make(uint64_t start, uint64_t size)
> +static AddrRange addrrange_make(int64_t start, int64_t size)
>   {
>       return (AddrRange) { start, size };
>   }
> @@ -37,7 +42,7 @@ static bool addrrange_equal(AddrRange r1, AddrRange r2)
>       return r1.start == r2.start&&  r1.size == r2.size;
>   }
>
> -static uint64_t addrrange_end(AddrRange r)
> +static int64_t addrrange_end(AddrRange r)
>   {
>       return r.start + r.size;
>   }
> @@ -56,9 +61,9 @@ static bool addrrange_intersects(AddrRange r1, AddrRange r2)
>
>   static AddrRange addrrange_intersection(AddrRange r1, AddrRange r2)
>   {
> -    uint64_t start = MAX(r1.start, r2.start);
> +    int64_t start = MAX(r1.start, r2.start);
>       /* off-by-one arithmetic to prevent overflow */
> -    uint64_t end = MIN(addrrange_end(r1) - 1, addrrange_end(r2) - 1);
> +    int64_t end = MIN(addrrange_end(r1) - 1, addrrange_end(r2) - 1);
>       return addrrange_make(start, end - start + 1);
>   }
>
> @@ -411,8 +416,8 @@ static void render_memory_region(FlatView *view,
>       MemoryRegion *subregion;
>       unsigned i;
>       target_phys_addr_t offset_in_region;
> -    uint64_t remain;
> -    uint64_t now;
> +    int64_t remain;
> +    int64_t now;
>       FlatRange fr;
>       AddrRange tmp;
>
> @@ -486,7 +491,7 @@ static FlatView generate_memory_topology(MemoryRegion *mr)
>
>       flatview_init(&view);
>
> -    render_memory_region(&view, mr, 0, addrrange_make(0, UINT64_MAX));
> +    render_memory_region(&view, mr, 0, addrrange_make(0, INT64_MAX));
>       flatview_simplify(&view);
>
>       return view;