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<a...@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;