[Qemu-devel] [PATCH v2 0/2] memory: extend "info mtree" with flat view dump

[Qemu-devel] [PATCH v2 0/2] memory: extend "info mtree" with flat view dump

[Peter Xu]

v2:
- fix a size error in patch 2
- add r-b for Marc-André in patch 1

Each address space has its own flatview. It's another way to observe
memory info besides the default memory region hierachy, for example,
if we want to know which memory region will handle the write to
specific address, a flatview will suite more here than the default
hierachical dump.

I used it to debug a vt-d memory region overlap issue. Do we need
this? I think we can at least consider patch 1, which is a cleanup of
existing codes. :)

Please review. Thanks,

Peter Xu (2):
  memory: provide common macros for mtree_print_mr()
  memory: hmp: dump flat view for 'info mtree'

 memory.c | 65 +++++++++++++++++++++++++++++++++++++++++++++-------------------
 1 file changed, 46 insertions(+), 19 deletions(-)

-- 
2.7.4

[Qemu-devel] [PATCH v2 1/2] memory: provide common macros for mtree_print_mr()

[Peter Xu]

mtree_print_mr() has some common codes. Generalize it.

Reviewed-by: Marc-André Lureau <marcandre.lureau@gmail.com>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 memory.c | 34 +++++++++++++++-------------------
 1 file changed, 15 insertions(+), 19 deletions(-)

diff --git a/memory.c b/memory.c
index 33110e9..5dcc2e1 100644
--- a/memory.c
+++ b/memory.c
@@ -2450,6 +2450,13 @@ struct MemoryRegionList {
 
 typedef QTAILQ_HEAD(queue, MemoryRegionList) MemoryRegionListHead;
 
+#define MR_CHAR_RD(mr) ((mr)->romd_mode ? 'R' : '-')
+#define MR_CHAR_WR(mr) (!(mr)->readonly && !((mr)->rom_device && \
+                                             (mr)->romd_mode) ? 'W' : '-')
+#define MR_SIZE(size) (int128_nz(size) ? (hwaddr)int128_get64( \
+                           int128_sub((size), int128_one())) : 0)
+#define MTREE_INDENT "  "
+
 static void mtree_print_mr(fprintf_function mon_printf, void *f,
                            const MemoryRegion *mr, unsigned int level,
                            hwaddr base,
@@ -2465,7 +2472,7 @@ static void mtree_print_mr(fprintf_function mon_printf, void *f,
     }
 
     for (i = 0; i < level; i++) {
-        mon_printf(f, "  ");
+        mon_printf(f, MTREE_INDENT);
     }
 
     if (mr->alias) {
@@ -2488,34 +2495,23 @@ static void mtree_print_mr(fprintf_function mon_printf, void *f,
                    " (prio %d, %c%c): alias %s @%s " TARGET_FMT_plx
                    "-" TARGET_FMT_plx "%s\n",
                    base + mr->addr,
-                   base + mr->addr
-                   + (int128_nz(mr->size) ?
-                      (hwaddr)int128_get64(int128_sub(mr->size,
-                                                      int128_one())) : 0),
+                   base + mr->addr + MR_SIZE(mr->size),
                    mr->priority,
-                   mr->romd_mode ? 'R' : '-',
-                   !mr->readonly && !(mr->rom_device && mr->romd_mode) ? 'W'
-                                                                       : '-',
+                   MR_CHAR_RD(mr),
+                   MR_CHAR_WR(mr),
                    memory_region_name(mr),
                    memory_region_name(mr->alias),
                    mr->alias_offset,
-                   mr->alias_offset
-                   + (int128_nz(mr->size) ?
-                      (hwaddr)int128_get64(int128_sub(mr->size,
-                                                      int128_one())) : 0),
+                   mr->alias_offset + MR_SIZE(mr->size),
                    mr->enabled ? "" : " [disabled]");
     } else {
         mon_printf(f,
                    TARGET_FMT_plx "-" TARGET_FMT_plx " (prio %d, %c%c): %s%s\n",
                    base + mr->addr,
-                   base + mr->addr
-                   + (int128_nz(mr->size) ?
-                      (hwaddr)int128_get64(int128_sub(mr->size,
-                                                      int128_one())) : 0),
+                   base + mr->addr + MR_SIZE(mr->size),
                    mr->priority,
-                   mr->romd_mode ? 'R' : '-',
-                   !mr->readonly && !(mr->rom_device && mr->romd_mode) ? 'W'
-                                                                       : '-',
+                   MR_CHAR_RD(mr),
+                   MR_CHAR_WR(mr),
                    memory_region_name(mr),
                    mr->enabled ? "" : " [disabled]");
     }
-- 
2.7.4

Re: [Qemu-devel] [PATCH v2 1/2] memory: provide common macros for mtree_print_mr()

[Paolo Bonzini]

On 21/12/2016 08:58, Peter Xu wrote:
> -                   mr->romd_mode ? 'R' : '-',
> -                   !mr->readonly && !(mr->rom_device && mr->romd_mode) ? 'W'
> -                                                                       : '-',
> +                   MR_CHAR_RD(mr),
> +                   MR_CHAR_WR(mr),

An alternative definition could be

	memory_access_is_direct(mr, false) ? 'R' : '-'
	memory_access_is_direct(mr, true) ? 'W' : '-'

for MR_CHAR_RD and MR_CHAR_WR.  With this change, I think the small code
duplication in the "? :" operator is tolerable and the code is clearer.

Paolo

Re: [Qemu-devel] [PATCH v2 1/2] memory: provide common macros for mtree_print_mr()

[Peter Xu]

On Wed, Jan 11, 2017 at 06:21:46PM +0100, Paolo Bonzini wrote:
> 
> 
> On 21/12/2016 08:58, Peter Xu wrote:
> > -                   mr->romd_mode ? 'R' : '-',
> > -                   !mr->readonly && !(mr->rom_device && mr->romd_mode) ? 'W'
> > -                                                                       : '-',
> > +                   MR_CHAR_RD(mr),
> > +                   MR_CHAR_WR(mr),
> 
> An alternative definition could be
> 
> 	memory_access_is_direct(mr, false) ? 'R' : '-'
> 	memory_access_is_direct(mr, true) ? 'W' : '-'
> 
> for MR_CHAR_RD and MR_CHAR_WR.  With this change, I think the small code
> duplication in the "? :" operator is tolerable and the code is clearer.

memory_access_is_direct() will check against whether mr is RAM, is
that what we want here? In that case we'll get most of the regions as
"--" as long as they are not RAM, while in fact IMHO we should want to
know the rw permission for all cases.

How about I add one more patch at the beginning to provide some more
memory_region_is_*() helpers (meanwhile refactor
memory_access_is_direct() a bit), like:

--------8<--------
diff --git a/include/exec/memory.h b/include/exec/memory.h
index bec9756..50974c8 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -1619,14 +1619,27 @@ MemTxResult address_space_read_full(AddressSpace *as, hwaddr addr,
                                     MemTxAttrs attrs, uint8_t *buf, int len);
 void *qemu_map_ram_ptr(RAMBlock *ram_block, ram_addr_t addr);

+static inline bool memory_region_is_readable(MemoryRegion *mr)
+{
+    return mr->rom_device ? mr->romd_mode : true;
+}
+
+static inline bool memory_region_is_writable(MemoryRegion *mr)
+{
+    return !mr->rom_device && !mr->readonly;
+}
+
+static inline bool memory_region_is_direct(MemoryRegion *mr)
+{
+    return memory_region_is_ram(mr) && !memory_region_is_ram_device(mr);
+}
+
 static inline bool memory_access_is_direct(MemoryRegion *mr, bool is_write)
 {
     if (is_write) {
-        return memory_region_is_ram(mr) &&
-               !mr->readonly && !memory_region_is_ram_device(mr);
+        return memory_region_is_direct(mr) && memory_region_is_writable(mr);
     } else {
-        return (memory_region_is_ram(mr) && !memory_region_is_ram_device(mr)) ||
-               memory_region_is_romd(mr);
+        return memory_region_is_direct(mr) && memory_region_is_readable(mr);
     }
 }
-------->8--------

Then, I can throw away MR_CHAR_* macros and use:

    memory_access_is_readable(mr, false) ? 'R' : '-'
    memory_access_is_writable(mr, true) ? 'W' : '-'

Do you like this approach?

-- peterx

Re: [Qemu-devel] [PATCH v2 1/2] memory: provide common macros for mtree_print_mr()

[Paolo Bonzini]

On 12/01/2017 06:50, Peter Xu wrote:
> On Wed, Jan 11, 2017 at 06:21:46PM +0100, Paolo Bonzini wrote:
>>
>>
>> On 21/12/2016 08:58, Peter Xu wrote:
>>> -                   mr->romd_mode ? 'R' : '-',
>>> -                   !mr->readonly && !(mr->rom_device && mr->romd_mode) ? 'W'
>>> -                                                                       : '-',
>>> +                   MR_CHAR_RD(mr),
>>> +                   MR_CHAR_WR(mr),
>>
>> An alternative definition could be
>>
>> 	memory_access_is_direct(mr, false) ? 'R' : '-'
>> 	memory_access_is_direct(mr, true) ? 'W' : '-'
>>
>> for MR_CHAR_RD and MR_CHAR_WR.  With this change, I think the small code
>> duplication in the "? :" operator is tolerable and the code is clearer.
> 
> memory_access_is_direct() will check against whether mr is RAM, is
> that what we want here? In that case we'll get most of the regions as
> "--" as long as they are not RAM, while in fact IMHO we should want to
> know the rw permission for all cases.

Indeed.  My idea was that the RW permission is not well defined for
non-RAM memory regions, and ROMD regions in MMIO mode shows as "--"
while MMIO regions show as "RW".  But perhaps it's confusing.

What about writing one of "ram", "rom", "ramd", "romd", "i/o" (with I/O
also covering rom_device && !romd_mode)?

If you disagree, the below patch looks good.

Paolo

> --------8<--------
> diff --git a/include/exec/memory.h b/include/exec/memory.h
> index bec9756..50974c8 100644
> --- a/include/exec/memory.h
> +++ b/include/exec/memory.h
> @@ -1619,14 +1619,27 @@ MemTxResult address_space_read_full(AddressSpace *as, hwaddr addr,
>                                      MemTxAttrs attrs, uint8_t *buf, int len);
>  void *qemu_map_ram_ptr(RAMBlock *ram_block, ram_addr_t addr);
> 
> +static inline bool memory_region_is_readable(MemoryRegion *mr)
> +{
> +    return mr->rom_device ? mr->romd_mode : true;
> +}
> +
> +static inline bool memory_region_is_writable(MemoryRegion *mr)
> +{
> +    return !mr->rom_device && !mr->readonly;
> +}
> +
> +static inline bool memory_region_is_direct(MemoryRegion *mr)
> +{
> +    return memory_region_is_ram(mr) && !memory_region_is_ram_device(mr);
> +}
> +
>  static inline bool memory_access_is_direct(MemoryRegion *mr, bool is_write)
>  {
>      if (is_write) {
> -        return memory_region_is_ram(mr) &&
> -               !mr->readonly && !memory_region_is_ram_device(mr);
> +        return memory_region_is_direct(mr) && memory_region_is_writable(mr);
>      } else {
> -        return (memory_region_is_ram(mr) && !memory_region_is_ram_device(mr)) ||
> -               memory_region_is_romd(mr);
> +        return memory_region_is_direct(mr) && memory_region_is_readable(mr);
>      }
>  }
> -------->8--------
> 
> Then, I can throw away MR_CHAR_* macros and use:
> 
>     memory_access_is_readable(mr, false) ? 'R' : '-'
>     memory_access_is_writable(mr, true) ? 'W' : '-'
> 
> Do you like this approach?
> 
> -- peterx
> 
> 

Re: [Qemu-devel] [PATCH v2 1/2] memory: provide common macros for mtree_print_mr()

[Peter Xu]

On Thu, Jan 12, 2017 at 09:54:24AM +0100, Paolo Bonzini wrote:
> 
> 
> On 12/01/2017 06:50, Peter Xu wrote:
> > On Wed, Jan 11, 2017 at 06:21:46PM +0100, Paolo Bonzini wrote:
> >>
> >>
> >> On 21/12/2016 08:58, Peter Xu wrote:
> >>> -                   mr->romd_mode ? 'R' : '-',
> >>> -                   !mr->readonly && !(mr->rom_device && mr->romd_mode) ? 'W'
> >>> -                                                                       : '-',
> >>> +                   MR_CHAR_RD(mr),
> >>> +                   MR_CHAR_WR(mr),
> >>
> >> An alternative definition could be
> >>
> >> 	memory_access_is_direct(mr, false) ? 'R' : '-'
> >> 	memory_access_is_direct(mr, true) ? 'W' : '-'
> >>
> >> for MR_CHAR_RD and MR_CHAR_WR.  With this change, I think the small code
> >> duplication in the "? :" operator is tolerable and the code is clearer.
> > 
> > memory_access_is_direct() will check against whether mr is RAM, is
> > that what we want here? In that case we'll get most of the regions as
> > "--" as long as they are not RAM, while in fact IMHO we should want to
> > know the rw permission for all cases.
> 
> Indeed.  My idea was that the RW permission is not well defined for
> non-RAM memory regions, and ROMD regions in MMIO mode shows as "--"
> while MMIO regions show as "RW".  But perhaps it's confusing.
> 
> What about writing one of "ram", "rom", "ramd", "romd", "i/o" (with I/O
> also covering rom_device && !romd_mode)?
> 
> If you disagree, the below patch looks good.

Yes, above suggestion makes sense to me, since after all the RW
permissions are derived from the type of memory regions, and the type
itself tells more things than the RW bits. So I totally agree we can
replace the "RW" chars with its type directly (if no one else
disagree, of course).

While for below patch, do you want me to include it as well as a
standalone patch, for the purpose of refactoring
memory_access_is_direct()? Since IMHO it's tiny clearer and more
readable than before.

Thanks!

> 
> Paolo
> 
> > --------8<--------
> > diff --git a/include/exec/memory.h b/include/exec/memory.h
> > index bec9756..50974c8 100644
> > --- a/include/exec/memory.h
> > +++ b/include/exec/memory.h
> > @@ -1619,14 +1619,27 @@ MemTxResult address_space_read_full(AddressSpace *as, hwaddr addr,
> >                                      MemTxAttrs attrs, uint8_t *buf, int len);
> >  void *qemu_map_ram_ptr(RAMBlock *ram_block, ram_addr_t addr);
> > 
> > +static inline bool memory_region_is_readable(MemoryRegion *mr)
> > +{
> > +    return mr->rom_device ? mr->romd_mode : true;
> > +}
> > +
> > +static inline bool memory_region_is_writable(MemoryRegion *mr)
> > +{
> > +    return !mr->rom_device && !mr->readonly;
> > +}
> > +
> > +static inline bool memory_region_is_direct(MemoryRegion *mr)
> > +{
> > +    return memory_region_is_ram(mr) && !memory_region_is_ram_device(mr);
> > +}
> > +
> >  static inline bool memory_access_is_direct(MemoryRegion *mr, bool is_write)
> >  {
> >      if (is_write) {
> > -        return memory_region_is_ram(mr) &&
> > -               !mr->readonly && !memory_region_is_ram_device(mr);
> > +        return memory_region_is_direct(mr) && memory_region_is_writable(mr);
> >      } else {
> > -        return (memory_region_is_ram(mr) && !memory_region_is_ram_device(mr)) ||
> > -               memory_region_is_romd(mr);
> > +        return memory_region_is_direct(mr) && memory_region_is_readable(mr);
> >      }
> >  }
> > -------->8--------
> > 
> > Then, I can throw away MR_CHAR_* macros and use:
> > 
> >     memory_access_is_readable(mr, false) ? 'R' : '-'
> >     memory_access_is_writable(mr, true) ? 'W' : '-'
> > 
> > Do you like this approach?
> > 
> > -- peterx
> > 
> > 

-- peterx

Re: [Qemu-devel] [PATCH v2 1/2] memory: provide common macros for mtree_print_mr()

[Paolo Bonzini]

On 12/01/2017 10:46, Peter Xu wrote:
> Yes, above suggestion makes sense to me, since after all the RW
> permissions are derived from the type of memory regions, and the type
> itself tells more things than the RW bits. So I totally agree we can
> replace the "RW" chars with its type directly (if no one else
> disagree, of course).
> 
> While for below patch, do you want me to include it as well as a
> standalone patch, for the purpose of refactoring
> memory_access_is_direct()? Since IMHO it's tiny clearer and more
> readable than before.

It is more readable, but my plan was to turn these fields into a single
field (with bits) to speed up memory_access_is_direct.  For that we'd
need to undo your change.  So I'm undecided.

Paolo

Re: [Qemu-devel] [PATCH v2 1/2] memory: provide common macros for mtree_print_mr()

[Peter Xu]

On Thu, Jan 12, 2017 at 12:19:21PM +0100, Paolo Bonzini wrote:
> 
> 
> On 12/01/2017 10:46, Peter Xu wrote:
> > Yes, above suggestion makes sense to me, since after all the RW
> > permissions are derived from the type of memory regions, and the type
> > itself tells more things than the RW bits. So I totally agree we can
> > replace the "RW" chars with its type directly (if no one else
> > disagree, of course).
> > 
> > While for below patch, do you want me to include it as well as a
> > standalone patch, for the purpose of refactoring
> > memory_access_is_direct()? Since IMHO it's tiny clearer and more
> > readable than before.
> 
> It is more readable, but my plan was to turn these fields into a single
> field (with bits) to speed up memory_access_is_direct.  For that we'd
> need to undo your change.  So I'm undecided.

No problem. Then I'll just ignore it and repost with above. Thanks!

-- peterx

[Qemu-devel] [PATCH v2 2/2] memory: hmp: dump flat view for 'info mtree'

[Peter Xu]

Dumping flat view will be useful to debug the memory rendering logic,
also it'll be much easier with it to know what memory region is handling
what address range.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 memory.c | 31 +++++++++++++++++++++++++++++++
 1 file changed, 31 insertions(+)

diff --git a/memory.c b/memory.c
index 5dcc2e1..91341d3 100644
--- a/memory.c
+++ b/memory.c
@@ -2545,6 +2545,36 @@ static void mtree_print_mr(fprintf_function mon_printf, void *f,
     }
 }
 
+static void mtree_print_flatview(fprintf_function p, void *f,
+                                 AddressSpace *as)
+{
+    FlatView *view = address_space_get_flatview(as);
+    FlatRange *range = &view->ranges[0];
+    MemoryRegion *mr;
+    int n = view->nr;
+
+    if (n <= 0) {
+        p(f, MTREE_INDENT "No rendered FlatView for "
+          "address space '%s'\n", as->name);
+        return;
+    }
+
+    p(f, MTREE_INDENT "FlatView (address space '%s'):\n", as->name);
+
+    while (n--) {
+        mr = range->mr;
+        p(f, MTREE_INDENT MTREE_INDENT TARGET_FMT_plx "-"
+          TARGET_FMT_plx " (prio %d, %c%c): %s\n",
+          int128_get64(range->addr.start),
+          int128_get64(range->addr.start) + MR_SIZE(range->addr.size),
+          mr->priority, MR_CHAR_RD(mr), MR_CHAR_WR(mr),
+          memory_region_name(mr));
+        range++;
+    }
+
+    flatview_unref(view);
+}
+
 void mtree_info(fprintf_function mon_printf, void *f)
 {
     MemoryRegionListHead ml_head;
@@ -2556,6 +2586,7 @@ void mtree_info(fprintf_function mon_printf, void *f)
     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_flatview(mon_printf, f, as);
         mon_printf(f, "\n");
     }
 
-- 
2.7.4

Re: [Qemu-devel] [PATCH v2 2/2] memory: hmp: dump flat view for 'info mtree'

[Paolo Bonzini]

On 21/12/2016 08:58, Peter Xu wrote:
> Dumping flat view will be useful to debug the memory rendering logic,
> also it'll be much easier with it to know what memory region is handling
> what address range.
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>

This is useful, but dumping both makes the output very long.  What about
adding a -f option to "info mtree"?

Paolo

> ---
>  memory.c | 31 +++++++++++++++++++++++++++++++
>  1 file changed, 31 insertions(+)
> 
> diff --git a/memory.c b/memory.c
> index 5dcc2e1..91341d3 100644
> --- a/memory.c
> +++ b/memory.c
> @@ -2545,6 +2545,36 @@ static void mtree_print_mr(fprintf_function mon_printf, void *f,
>      }
>  }
>  
> +static void mtree_print_flatview(fprintf_function p, void *f,
> +                                 AddressSpace *as)
> +{
> +    FlatView *view = address_space_get_flatview(as);
> +    FlatRange *range = &view->ranges[0];
> +    MemoryRegion *mr;
> +    int n = view->nr;
> +
> +    if (n <= 0) {
> +        p(f, MTREE_INDENT "No rendered FlatView for "
> +          "address space '%s'\n", as->name);
> +        return;
> +    }
> +
> +    p(f, MTREE_INDENT "FlatView (address space '%s'):\n", as->name);
> +
> +    while (n--) {
> +        mr = range->mr;
> +        p(f, MTREE_INDENT MTREE_INDENT TARGET_FMT_plx "-"
> +          TARGET_FMT_plx " (prio %d, %c%c): %s\n",
> +          int128_get64(range->addr.start),
> +          int128_get64(range->addr.start) + MR_SIZE(range->addr.size),
> +          mr->priority, MR_CHAR_RD(mr), MR_CHAR_WR(mr),
> +          memory_region_name(mr));
> +        range++;
> +    }
> +
> +    flatview_unref(view);
> +}
> +
>  void mtree_info(fprintf_function mon_printf, void *f)
>  {
>      MemoryRegionListHead ml_head;
> @@ -2556,6 +2586,7 @@ void mtree_info(fprintf_function mon_printf, void *f)
>      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_flatview(mon_printf, f, as);
>          mon_printf(f, "\n");
>      }
>  
> 

Re: [Qemu-devel] [PATCH v2 2/2] memory: hmp: dump flat view for 'info mtree'

[Peter Xu]

On Wed, Jan 11, 2017 at 06:13:11PM +0100, Paolo Bonzini wrote:
> 
> 
> On 21/12/2016 08:58, Peter Xu wrote:
> > Dumping flat view will be useful to debug the memory rendering logic,
> > also it'll be much easier with it to know what memory region is handling
> > what address range.
> > 
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> 
> This is useful, but dumping both makes the output very long.  What about
> adding a -f option to "info mtree"?

Sure. :)

After I confirm how I should improve on the first patch, I'll cook
another version for the series with "-f".

Thanks,

-- peterx

Re: [Qemu-devel] [PATCH v2 0/2] memory: extend "info mtree" with flat view dump

[Peter Xu]

On Wed, Dec 21, 2016 at 03:58:55PM +0800, Peter Xu wrote:
> v2:
> - fix a size error in patch 2
> - add r-b for Marc-André in patch 1

Ping? :)

-- peterx