diff mbox

memory: use 128 bit in info mtree

Message ID 20170313030201.GH17299@pxdev.xzpeter.org
State New
Headers show

Commit Message

Peter Xu March 13, 2017, 3:02 a.m. UTC 
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<-----------
Thanks,

-- peterx

Comments

Paolo Bonzini March 14, 2017, 10:26 a.m. UTC | #1 
On 13/03/2017 04:02, Peter Xu wrote:
> 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)

Looks good, can you submit it formally?

Paolo

> ---------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
>
Peter Xu March 14, 2017, 11:58 a.m. UTC | #2 
On Tue, Mar 14, 2017 at 11:26:19AM +0100, Paolo Bonzini wrote:
> 
> 
> On 13/03/2017 04:02, Peter Xu wrote:
> > 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)
> 
> Looks good, can you submit it formally?

Sure! Will do.

-- peterx
Michael S. Tsirkin March 14, 2017, 3:06 p.m. UTC | #3 
On Mon, Mar 13, 2017 at 11:02:01AM +0800, Peter Xu wrote:
> 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.
 
This is not what my patch is doing. It prints 16 digits if number fits.

> 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)

We don't use 64 bit addresses I don't really understand why does
it matter that 64 bit math would overflow.
It could be a valid configuration.

64 bit math overflow is just a particular case of a general issue where
all of child region is not visible through a parent.
IMHO if you are trying to report visible windows do just that:
add (visible through parent: AAA-BBB and maybe not visible through
parent).


> ---------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
diff mbox

Patch

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-----------