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.

Reported-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 include/qemu/int128.h | 15 +++++++++++++++
 memory.c              | 28 +++++++++++++++++-----------
 2 files changed, 32 insertions(+), 11 deletions(-)

diff --git a/include/qemu/int128.h b/include/qemu/int128.h
index 5c9890d..8be5328 100644
--- a/include/qemu/int128.h
+++ b/include/qemu/int128.h
@@ -302,4 +302,19 @@ static inline void int128_subfrom(Int128 *a, Int128 b)
 }
 
 #endif /* CONFIG_INT128 */
+
+#define INT128_FMT1_plx "0x%" PRIx64
+#define INT128_FMT2_plx "%015" PRIx64
+
+static inline uint64_t int128_printf1(Int128 a)
+{
+    /* We assume 4 highest bits are clear and safe to ignore */
+    return (int128_gethi(a) << 4) | (int128_getlo(a) >> 60);
+}
+
+static inline uint64_t int128_printf2(Int128 a)
+{
+    return (int128_getlo(a) << 4) >> 4;
+}
+
 #endif /* INT128_H */
diff --git a/memory.c b/memory.c
index d61caee..b73a671 100644
--- a/memory.c
+++ b/memory.c
@@ -2487,13 +2487,14 @@ typedef QTAILQ_HEAD(queue, MemoryRegionList) MemoryRegionListHead;
 
 static void mtree_print_mr(fprintf_function mon_printf, void *f,
                            const MemoryRegion *mr, unsigned int level,
-                           hwaddr base,
+                           Int128 base,
                            MemoryRegionListHead *alias_print_queue)
 {
     MemoryRegionList *new_ml, *ml, *next_ml;
     MemoryRegionListHead submr_print_queue;
     const MemoryRegion *submr;
     unsigned int i;
+    Int128 start, end;
 
     if (!mr) {
         return;
@@ -2503,6 +2504,9 @@ static void mtree_print_mr(fprintf_function mon_printf, void *f,
         mon_printf(f, MTREE_INDENT);
     }
 
+    start = int128_add(base, int128_make64(mr->addr));
+    end = int128_add(start, mr->size);
+
     if (mr->alias) {
         MemoryRegionList *ml;
         bool found = false;
@@ -2519,11 +2523,12 @@ static void mtree_print_mr(fprintf_function mon_printf, void *f,
             ml->mr = mr->alias;
             QTAILQ_INSERT_TAIL(alias_print_queue, ml, queue);
         }
-        mon_printf(f, TARGET_FMT_plx "-" TARGET_FMT_plx
+        mon_printf(f, INT128_FMT1_plx INT128_FMT2_plx
+		   "-" INT128_FMT1_plx INT128_FMT2_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),
+                   int128_printf1(start), int128_printf2(start),
+                   int128_printf1(end), int128_printf2(end),
                    mr->priority,
                    memory_region_type((MemoryRegion *)mr),
                    memory_region_name(mr),
@@ -2532,10 +2537,11 @@ static void mtree_print_mr(fprintf_function mon_printf, void *f,
                    mr->alias_offset + MR_SIZE(mr->size),
                    mr->enabled ? "" : " [disabled]");
     } 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),
+        mon_printf(f, INT128_FMT1_plx INT128_FMT2_plx
+		   "-" INT128_FMT1_plx INT128_FMT2_plx
+                   " (prio %d, %s): %s%s\n",
+                   int128_printf1(start), int128_printf2(start),
+                   int128_printf1(end), int128_printf2(end),
                    mr->priority,
                    memory_region_type((MemoryRegion *)mr),
                    memory_region_name(mr),
@@ -2562,7 +2568,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, start,
                        alias_print_queue);
     }
 
@@ -2620,14 +2626,14 @@ void mtree_info(fprintf_function mon_printf, void *f, bool flatview)
 
     QTAILQ_FOREACH(as, &address_spaces, address_spaces_link) {
         mon_printf(f, "address-space: %s\n", as->name);
-        mtree_print_mr(mon_printf, f, as->root, 1, 0, &ml_head);
+        mtree_print_mr(mon_printf, f, as->root, 1, int128_zero(), &ml_head);
         mon_printf(f, "\n");
     }
 
     /* print aliased regions */
     QTAILQ_FOREACH(ml, &ml_head, queue) {
         mon_printf(f, "memory-region: %s\n", memory_region_name(ml->mr));
-        mtree_print_mr(mon_printf, f, ml->mr, 1, 0, &ml_head);
+        mtree_print_mr(mon_printf, f, ml->mr, 1, int128_zero(), &ml_head);
         mon_printf(f, "\n");
     }
 
-- 
MST

On 12 March 2017 at 20:12, Michael S. Tsirkin <mst@redhat.com> 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.
>
> Reported-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>  include/qemu/int128.h | 15 +++++++++++++++
>  memory.c              | 28 +++++++++++++++++-----------
>  2 files changed, 32 insertions(+), 11 deletions(-)
>
> diff --git a/include/qemu/int128.h b/include/qemu/int128.h
> index 5c9890d..8be5328 100644
> --- a/include/qemu/int128.h
> +++ b/include/qemu/int128.h
> @@ -302,4 +302,19 @@ static inline void int128_subfrom(Int128 *a, Int128 b)
>  }
>
>  #endif /* CONFIG_INT128 */
> +
> +#define INT128_FMT1_plx "0x%" PRIx64
> +#define INT128_FMT2_plx "%015" PRIx64
> +
> +static inline uint64_t int128_printf1(Int128 a)
> +{
> +    /* We assume 4 highest bits are clear and safe to ignore */
> +    return (int128_gethi(a) << 4) | (int128_getlo(a) >> 60);
> +}
> +
> +static inline uint64_t int128_printf2(Int128 a)
> +{
> +    return (int128_getlo(a) << 4) >> 4;
> +}

I'm confused -- I was expecting these to just
be the two halves of the 128 bit integer to be
printed, but they seem to be doing something
more complicated...

thanks
-- PMM

On 12/03/2017 20:35, Peter Maydell wrote:
>> +
>> +static inline uint64_t int128_printf1(Int128 a)
>> +{
>> +    /* We assume 4 highest bits are clear and safe to ignore */
>> +    return (int128_gethi(a) << 4) | (int128_getlo(a) >> 60);
>> +}
>> +
>> +static inline uint64_t int128_printf2(Int128 a)
>> +{
>> +    return (int128_getlo(a) << 4) >> 4;
>> +}
> I'm confused -- I was expecting these to just
> be the two halves of the 128 bit integer to be
> printed, but they seem to be doing something
> more complicated...

Yeah, it tries to make a 16 hex-digit output unless the value doesn't
fit in 64 bits.  I'll merge peterx's patch instead.

Paolo

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

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
> 

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

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

Hi,

This series seems to have some coding style problems. See output below for
more information:

Type: series
Subject: [Qemu-devel] [PATCH] memory: use 128 bit in info mtree
Message-id: 1489345956-29167-1-git-send-email-mst@redhat.com

=== TEST SCRIPT BEGIN ===
#!/bin/bash

BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0

# Useful git options
git config --local diff.renamelimit 0
git config --local diff.renames True

commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
    echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
    if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
        failed=1
        echo
    fi
    n=$((n+1))
done

exit $failed
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
 * [new tag]         patchew/1489345956-29167-1-git-send-email-mst@redhat.com -> patchew/1489345956-29167-1-git-send-email-mst@redhat.com
Switched to a new branch 'test'
d2f72b7 memory: use 128 bit in info mtree

=== OUTPUT BEGIN ===
Checking PATCH 1/1: memory: use 128 bit in info mtree...
ERROR: code indent should never use tabs
#79: FILE: memory.c:2527:
+^I^I   "-" INT128_FMT1_plx INT128_FMT2_plx$

ERROR: code indent should never use tabs
#98: FILE: memory.c:2541:
+^I^I   "-" INT128_FMT1_plx INT128_FMT2_plx$

total: 2 errors, 0 warnings, 97 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

=== OUTPUT END ===

Test command exited with code: 1


---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@freelists.org