Message ID | 1489345956-29167-1-git-send-email-mst@redhat.com |
---|---|
State | New |
Headers | show |
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
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
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"); }
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(-)