Re: [Qemu-devel] [PATCH] memory: use 128 bit in info mtree

[Peter Xu]

On Sun, Mar 12, 2017 at 09:12:43PM +0200, Michael S. Tsirkin wrote:
> info mtree is doing 64 bit math to figure out
> addresses from offsets, this does not work ncorrectly
> incase of overflow.
> 
> Overflow usually indicates a guest bug, so this is unusual
> but reporting correct addresses makes it easier to discover
> what is going on.

A tiny issue would be that we will always dump 128 bits even if
nothing went wrong. IMHO That's slightly awkward. Not sure whether
that will confuse people since they should be thinking why we need
that on 64bit systems...

Do you like below one instead? It'll keep the old interface, but just
warn user explicity when something wrong happens, and it's much easier
and obvious imho (along with a tiny cleanup):

(the code is not tested even for compile)

---------8<-----------
diff --git a/memory.c b/memory.c
index 284894b..64b0a60 100644
--- a/memory.c
+++ b/memory.c
@@ -2494,6 +2494,7 @@ static void mtree_print_mr(fprintf_function mon_printf, 
void *f,
     MemoryRegionListHead submr_print_queue;
     const MemoryRegion *submr;
     unsigned int i;
+    hwaddr cur_start, cur_end;

     if (!mr) {
         return;
@@ -2503,6 +2504,18 @@ static void mtree_print_mr(fprintf_function mon_printf, 
void *f,
         mon_printf(f, MTREE_INDENT);
     }

+    cur_start = base + mr->addr;
+    cur_end = cur_start + MR_SIZE(mr->size);
+
+    /*
+     * Try to detect overflow of memory ranges. This should never
+     * happen normally. When it happens, we dump something to warn the
+     * user who is observing this.
+     */
+    if (cur_start < base || cur_end < cur_start) {
+        mon_printf(f, "[DETECTED OVERFLOW!] ");
+    }
+
     if (mr->alias) {
         MemoryRegionList *ml;
         bool found = false;
@@ -2522,8 +2535,7 @@ static void mtree_print_mr(fprintf_function mon_printf, 
void *f,
         mon_printf(f, TARGET_FMT_plx "-" TARGET_FMT_plx
                    " (prio %d, %s): alias %s @%s " TARGET_FMT_plx
                    "-" TARGET_FMT_plx "%s\n",
-                   base + mr->addr,
-                   base + mr->addr + MR_SIZE(mr->size),
+                   cur_start, cur_end,
                    mr->priority,
                    memory_region_type((MemoryRegion *)mr),
                    memory_region_name(mr),
@@ -2534,8 +2546,7 @@ static void mtree_print_mr(fprintf_function mon_printf, 
void *f,
     } else {
         mon_printf(f,
                    TARGET_FMT_plx "-" TARGET_FMT_plx " (prio %d, %s): %s%s\n",
-                   base + mr->addr,
-                   base + mr->addr + MR_SIZE(mr->size),
+                   cur_start, cur_end,
                    mr->priority,
                    memory_region_type((MemoryRegion *)mr),
                    memory_region_name(mr),
@@ -2562,7 +2573,7 @@ static void mtree_print_mr(fprintf_function mon_printf, 
void *f,
     }

     QTAILQ_FOREACH(ml, &submr_print_queue, queue) {
-        mtree_print_mr(mon_printf, f, ml->mr, level + 1, base + mr->addr,
+        mtree_print_mr(mon_printf, f, ml->mr, level + 1, cur_start,
                        alias_print_queue);
     }
--------->8-----------

Thanks,

-- peterx