[PATCH v2 0/1] Optimizing the print format of the QEMU monitor 'info mtree'

[PATCH v2 0/1] Optimizing the print format of the QEMU monitor 'info mtree'

[Chao Liu]

Hi, all:

Thanks to David's review, I've made the following improvements in PATCH v2:

1. Enrich the commit message, add 'info mtree' print example.
2. Optimize the code implementation according to the review comments.

PATCH v1 (reviewed):
https://lore.kernel.org/qemu-devel/210c69d9-803e-41a5-b40c-bc8372e582fa@redhat.com/T/#u

--
Regards,
Chao

Chao Liu (1):
  system: optimizing info mtree printing for monitors

 system/memory.c | 42 ++++++++++++++++++++++++++++++++++++------
 1 file changed, 36 insertions(+), 6 deletions(-)

-- 
2.48.1

[PATCH v2 1/1] system: optimizing info mtree printing for monitors

[Chao Liu]

Make the hierarchical relationship between nodes clearer by adding characters

e.g.

qemu-system-riscv64 -M virt -monitor stdio -display none

```
(qemu) info mtree
...
memory-region: system
│  ├── 0000000000000000-ffffffffffffffff (prio 0, i/o): system
│  │   ├── 0000000003000000-000000000300ffff (prio 0, i/o): gpex_ioport_window
│  │   │   └── 0000000003000000-000000000300ffff (prio 0, i/o): gpex_ioport
...
│  │   └── 0000000400000000-00000007ffffffff (prio 0, i/o): alias ...
```

Signed-off-by: Chao Liu <lc00631@tecorigin.com>
Reviewed-by: Qingze Zhao <zqz00548@tecorigin.com>
Reviewed-by: Tingjian Zhang <zhangtj@tecorigin.com>
---
 system/memory.c | 42 ++++++++++++++++++++++++++++++++++++------
 1 file changed, 36 insertions(+), 6 deletions(-)

diff --git a/system/memory.c b/system/memory.c
index 71434e7ad0..3a7faeb533 100644
--- a/system/memory.c
+++ b/system/memory.c
@@ -3296,6 +3296,27 @@ typedef QTAILQ_HEAD(, MemoryRegionList) MemoryRegionListHead;
                            int128_sub((size), int128_one())) : 0)
 #define MTREE_INDENT "  "
 
+enum mtree_node_type {
+    MTREE_NODE_T_INNER,
+    MTREE_NODE_T_TAIL,
+};
+
+#define PRINT_MTREE_NODE(node_type) do {  \
+    if (node_type == MTREE_NODE_T_TAIL) { \
+        qemu_printf("└── ");              \
+    } else {                              \
+        qemu_printf("├── ");              \
+    }                                     \
+} while (0)
+
+#define PRINT_MTREE_COL(level) do { \
+    if (level == 0) {               \
+        qemu_printf("│  ");         \
+    } else {                        \
+        qemu_printf("│   ");        \
+    }                               \
+} while (0)
+
 static void mtree_expand_owner(const char *label, Object *obj)
 {
     DeviceState *dev = (DeviceState *) object_dynamic_cast(obj, TYPE_DEVICE);
@@ -3335,7 +3356,8 @@ static void mtree_print_mr_owner(const MemoryRegion *mr)
 static void mtree_print_mr(const MemoryRegion *mr, unsigned int level,
                            hwaddr base,
                            MemoryRegionListHead *alias_print_queue,
-                           bool owner, bool display_disabled)
+                           bool owner, bool display_disabled,
+                           enum mtree_node_type node_type)
 {
     MemoryRegionList *new_ml, *ml, *next_ml;
     MemoryRegionListHead submr_print_queue;
@@ -3376,8 +3398,9 @@ static void mtree_print_mr(const MemoryRegion *mr, unsigned int level,
         }
         if (mr->enabled || display_disabled) {
             for (i = 0; i < level; i++) {
-                qemu_printf(MTREE_INDENT);
+                PRINT_MTREE_COL(i);
             }
+            PRINT_MTREE_NODE(node_type);
             qemu_printf(HWADDR_FMT_plx "-" HWADDR_FMT_plx
                         " (prio %d, %s%s): alias %s @%s " HWADDR_FMT_plx
                         "-" HWADDR_FMT_plx "%s",
@@ -3398,8 +3421,9 @@ static void mtree_print_mr(const MemoryRegion *mr, unsigned int level,
     } else {
         if (mr->enabled || display_disabled) {
             for (i = 0; i < level; i++) {
-                qemu_printf(MTREE_INDENT);
+                PRINT_MTREE_COL(i);
             }
+            PRINT_MTREE_NODE(node_type);
             qemu_printf(HWADDR_FMT_plx "-" HWADDR_FMT_plx
                         " (prio %d, %s%s): %s%s",
                         cur_start, cur_end,
@@ -3435,8 +3459,12 @@ static void mtree_print_mr(const MemoryRegion *mr, unsigned int level,
     }
 
     QTAILQ_FOREACH(ml, &submr_print_queue, mrqueue) {
+        if (ml == QTAILQ_LAST(&submr_print_queue)) {
+            node_type = MTREE_NODE_T_TAIL;
+        }
         mtree_print_mr(ml->mr, level + 1, cur_start,
-                       alias_print_queue, owner, display_disabled);
+                       alias_print_queue, owner,
+                       display_disabled, node_type);
     }
 
     QTAILQ_FOREACH_SAFE(ml, &submr_print_queue, mrqueue, next_ml) {
@@ -3614,7 +3642,8 @@ static void mtree_print_as(gpointer key, gpointer value, gpointer user_data)
     struct AddressSpaceInfo *asi = user_data;
 
     g_slist_foreach(as_same_root_mr_list, mtree_print_as_name, NULL);
-    mtree_print_mr(mr, 1, 0, asi->ml_head, asi->owner, asi->disabled);
+    mtree_print_mr(mr, 1, 0, asi->ml_head, asi->owner, asi->disabled,
+                   MTREE_NODE_T_INNER);
     qemu_printf("\n");
 }
 
@@ -3659,7 +3688,8 @@ static void mtree_info_as(bool dispatch_tree, bool owner, bool disabled)
     /* print aliased regions */
     QTAILQ_FOREACH(ml, &ml_head, mrqueue) {
         qemu_printf("memory-region: %s\n", memory_region_name(ml->mr));
-        mtree_print_mr(ml->mr, 1, 0, &ml_head, owner, disabled);
+        mtree_print_mr(ml->mr, 1, 0, &ml_head, owner, disabled,
+                       MTREE_NODE_T_INNER);
         qemu_printf("\n");
     }
 
-- 
2.48.1

Re: [PATCH v2 1/1] system: optimizing info mtree printing for monitors

[Philippe Mathieu-Daudé]

Hi Chao,

On 30/4/25 10:31, Chao Liu wrote:
> Make the hierarchical relationship between nodes clearer by adding characters
> 
> e.g.
> 
> qemu-system-riscv64 -M virt -monitor stdio -display none
> 
> ```
> (qemu) info mtree
> ...
> memory-region: system
> │  ├── 0000000000000000-ffffffffffffffff (prio 0, i/o): system
> │  │   ├── 0000000003000000-000000000300ffff (prio 0, i/o): gpex_ioport_window
> │  │   │   └── 0000000003000000-000000000300ffff (prio 0, i/o): gpex_ioport
> ...
> │  │   └── 0000000400000000-00000007ffffffff (prio 0, i/o): alias ...
> ```

Nice :)

> Signed-off-by: Chao Liu <lc00631@tecorigin.com>
> Reviewed-by: Qingze Zhao <zqz00548@tecorigin.com>
> Reviewed-by: Tingjian Zhang <zhangtj@tecorigin.com>
> ---
>   system/memory.c | 42 ++++++++++++++++++++++++++++++++++++------
>   1 file changed, 36 insertions(+), 6 deletions(-)
> 
> diff --git a/system/memory.c b/system/memory.c
> index 71434e7ad0..3a7faeb533 100644
> --- a/system/memory.c
> +++ b/system/memory.c
> @@ -3296,6 +3296,27 @@ typedef QTAILQ_HEAD(, MemoryRegionList) MemoryRegionListHead;
>                              int128_sub((size), int128_one())) : 0)
>   #define MTREE_INDENT "  "
>   
> +enum mtree_node_type {
> +    MTREE_NODE_T_INNER,
> +    MTREE_NODE_T_TAIL,
> +};
> +
> +#define PRINT_MTREE_NODE(node_type) do {  \
> +    if (node_type == MTREE_NODE_T_TAIL) { \
> +        qemu_printf("└── ");              \
> +    } else {                              \
> +        qemu_printf("├── ");              \
> +    }                                     \
> +} while (0)
> +
> +#define PRINT_MTREE_COL(level) do { \
> +    if (level == 0) {               \
> +        qemu_printf("│  ");         \
> +    } else {                        \
> +        qemu_printf("│   ");        \
> +    }                               \
> +} while (0)

Prefer tiny C functions over macros (easier to maintain):

static void mtree_print_node(bool is_tail)
{
     qemu_printf(is_tail ? "└── " : "├── ");
}

static void mtree_print_col(unsigned level)
{
     qemu_printf(level ? "│   " : "│  ");
}

Re: [PATCH v2 1/1] system: optimizing info mtree printing for monitors

[BALATON Zoltan]

[-- Attachment #1: Type: text/plain, Size: 5443 bytes --]



On Wed, 30 Apr 2025, Chao Liu wrote:

> Make the hierarchical relationship between nodes clearer by adding characters
>
> e.g.
>
> qemu-system-riscv64 -M virt -monitor stdio -display none
>
> ```
> (qemu) info mtree
> ...
> memory-region: system
> │  ├── 0000000000000000-ffffffffffffffff (prio 0, i/o): system
> │  │   ├── 0000000003000000-000000000300ffff (prio 0, i/o): gpex_ioport_window
> │  │   │   └── 0000000003000000-000000000300ffff (prio 0, i/o): gpex_ioport
> ...
> │  │   └── 0000000400000000-00000007ffffffff (prio 0, i/o): alias ...
> ```
>
> Signed-off-by: Chao Liu <lc00631@tecorigin.com>
> Reviewed-by: Qingze Zhao <zqz00548@tecorigin.com>
> Reviewed-by: Tingjian Zhang <zhangtj@tecorigin.com>
> ---
> system/memory.c | 42 ++++++++++++++++++++++++++++++++++++------
> 1 file changed, 36 insertions(+), 6 deletions(-)
>
> diff --git a/system/memory.c b/system/memory.c
> index 71434e7ad0..3a7faeb533 100644
> --- a/system/memory.c
> +++ b/system/memory.c
> @@ -3296,6 +3296,27 @@ typedef QTAILQ_HEAD(, MemoryRegionList) MemoryRegionListHead;
>                            int128_sub((size), int128_one())) : 0)
> #define MTREE_INDENT "  "
>
> +enum mtree_node_type {
> +    MTREE_NODE_T_INNER,
> +    MTREE_NODE_T_TAIL,
> +};
> +
> +#define PRINT_MTREE_NODE(node_type) do {  \
> +    if (node_type == MTREE_NODE_T_TAIL) { \
> +        qemu_printf("└──  );              \
> +    } else {                              \
> +        qemu_printf("├── ");              \

Could you remove one ─ to make the output less wide? Some memory region 
names can be long and with 64bit addresses it easily overflows the line 
length of narrower terminals which makes the output wrap and become less 
readable. Shorter lines there still shows the tree but takes up less 
horizontal space so I think shorter line here would be enough.

Regards,
BALATON Zoltan

> +    }                                     \
> +} while (0)
> +
> +#define PRINT_MTREE_COL(level) do { \
> +    if (level == 0) {               \
> +        qemu_printf("│  ");         \
> +    } else {                        \
> +        qemu_printf("│   ");        \
> +    }                               \
> +} while (0)
> +
> static void mtree_expand_owner(const char *label, Object *obj)
> {
>     DeviceState *dev = (DeviceState *) object_dynamic_cast(obj, TYPE_DEVICE);
> @@ -3335,7 +3356,8 @@ static void mtree_print_mr_owner(const MemoryRegion *mr)
> static void mtree_print_mr(const MemoryRegion *mr, unsigned int level,
>                            hwaddr base,
>                            MemoryRegionListHead *alias_print_queue,
> -                           bool owner, bool display_disabled)
> +                           bool owner, bool display_disabled,
> +                           enum mtree_node_type node_type)
> {
>     MemoryRegionList *new_ml, *ml, *next_ml;
>     MemoryRegionListHead submr_print_queue;
> @@ -3376,8 +3398,9 @@ static void mtree_print_mr(const MemoryRegion *mr, unsigned int level,
>         }
>         if (mr->enabled || display_disabled) {
>             for (i = 0; i < level; i++) {
> -                qemu_printf(MTREE_INDENT);
> +                PRINT_MTREE_COL(i);
>             }
> +            PRINT_MTREE_NODE(node_type);
>             qemu_printf(HWADDR_FMT_plx "-" HWADDR_FMT_plx
>                         " (prio %d, %s%s): alias %s @%s " HWADDR_FMT_plx
>                         "-" HWADDR_FMT_plx "%s",
> @@ -3398,8 +3421,9 @@ static void mtree_print_mr(const MemoryRegion *mr, unsigned int level,
>     } else {
>         if (mr->enabled || display_disabled) {
>             for (i = 0; i < level; i++) {
> -                qemu_printf(MTREE_INDENT);
> +                PRINT_MTREE_COL(i);
>             }
> +            PRINT_MTREE_NODE(node_type);
>             qemu_printf(HWADDR_FMT_plx "-" HWADDR_FMT_plx
>                         " (prio %d, %s%s): %s%s",
>                         cur_start, cur_end,
> @@ -3435,8 +3459,12 @@ static void mtree_print_mr(const MemoryRegion *mr, unsigned int level,
>     }
>
>     QTAILQ_FOREACH(ml, &submr_print_queue, mrqueue) {
> +        if (ml == QTAILQ_LAST(&submr_print_queue)) {
> +            node_type = MTREE_NODE_T_TAIL;
> +        }
>         mtree_print_mr(ml->mr, level + 1, cur_start,
> -                       alias_print_queue, owner, display_disabled);
> +                       alias_print_queue, owner,
> +                       display_disabled, node_type);
>     }
>
>     QTAILQ_FOREACH_SAFE(ml, &submr_print_queue, mrqueue, next_ml) {
> @@ -3614,7 +3642,8 @@ static void mtree_print_as(gpointer key, gpointer value, gpointer user_data)
>     struct AddressSpaceInfo *asi = user_data;
>
>     g_slist_foreach(as_same_root_mr_list, mtree_print_as_name, NULL);
> -    mtree_print_mr(mr, 1, 0, asi->ml_head, asi->owner, asi->disabled);
> +    mtree_print_mr(mr, 1, 0, asi->ml_head, asi->owner, asi->disabled,
> +                   MTREE_NODE_T_INNER);
>     qemu_printf("\n");
> }
>
> @@ -3659,7 +3688,8 @@ static void mtree_info_as(bool dispatch_tree, bool owner, bool disabled)
>     /* print aliased regions */
>     QTAILQ_FOREACH(ml, &ml_head, mrqueue) {
>         qemu_printf("memory-region: %s\n", memory_region_name(ml->mr));
> -        mtree_print_mr(ml->mr, 1, 0, &ml_head, owner, disabled);
> +        mtree_print_mr(ml->mr, 1, 0, &ml_head, owner, disabled,
> +                       MTREE_NODE_T_INNER);
>         qemu_printf("\n");
>     }
>
>

Re: [PATCH v2 1/1] system: optimizing info mtree printing for monitors

[Markus Armbruster]

Chao Liu <lc00631@tecorigin.com> writes:

> Make the hierarchical relationship between nodes clearer by adding characters
>
> e.g.
>
> qemu-system-riscv64 -M virt -monitor stdio -display none
>
> ```
> (qemu) info mtree
> ...
> memory-region: system
> │  ├── 0000000000000000-ffffffffffffffff (prio 0, i/o): system
> │  │   ├── 0000000003000000-000000000300ffff (prio 0, i/o): gpex_ioport_window
> │  │   │   └── 0000000003000000-000000000300ffff (prio 0, i/o): gpex_ioport
> ...
> │  │   └── 0000000400000000-00000007ffffffff (prio 0, i/o): alias ...
> ```

info qtree and info qom-tree could perhaps use similar treatment.

> Signed-off-by: Chao Liu <lc00631@tecorigin.com>
> Reviewed-by: Qingze Zhao <zqz00548@tecorigin.com>
> Reviewed-by: Tingjian Zhang <zhangtj@tecorigin.com>
> ---
>  system/memory.c | 42 ++++++++++++++++++++++++++++++++++++------
>  1 file changed, 36 insertions(+), 6 deletions(-)
>
> diff --git a/system/memory.c b/system/memory.c
> index 71434e7ad0..3a7faeb533 100644
> --- a/system/memory.c
> +++ b/system/memory.c
> @@ -3296,6 +3296,27 @@ typedef QTAILQ_HEAD(, MemoryRegionList) MemoryRegionListHead;
>                             int128_sub((size), int128_one())) : 0)
>  #define MTREE_INDENT "  "
>  
> +enum mtree_node_type {
> +    MTREE_NODE_T_INNER,
> +    MTREE_NODE_T_TAIL,
> +};
> +
> +#define PRINT_MTREE_NODE(node_type) do {  \
> +    if (node_type == MTREE_NODE_T_TAIL) { \
> +        qemu_printf("└── ");              \
> +    } else {                              \
> +        qemu_printf("├── ");              \
> +    }                                     \
> +} while (0)
> +
> +#define PRINT_MTREE_COL(level) do { \
> +    if (level == 0) {               \
> +        qemu_printf("│  ");         \
> +    } else {                        \
> +        qemu_printf("│   ");        \
> +    }                               \
> +} while (0)

Printing non-ASCII Unicode characters encoded in UTF-8 assumes the
monitor / stdout uses UTF-8.  Are we happy with such an assumption?

For what it's worth, such characters are exceedingly rare in C strings
so far.  I found:

    contrib/plugins/lockstep.c:        g_string_printf(out, "Δ too high, we have diverged, previous insns\n");

Intentional.  This is a "plugin for debugging TCG changes" (commit
c81950a2f19).

    hw/uefi/var-service-policy.c:    fprintf(stderr, "    name ´");

This looks like a typo to me.

    tests/tcg/plugins/insn.c:                               " Δ+%"PRId64 " since last match,"

Intentional.  Seems to be a plugin used for testing.

    ui/sdl2.c:            status = " - Press ⌃⌥⇧G to exit grab";
    ui/sdl2.c:            status = " - Press ⌃⌥G to exit grab";

These are #ifndef CONFIG_DARWIN.

None found in QEMU proper.

The common ASCII equivalent to

    │
    ├──

is

    |
    +--

Not as pretty, but quite readable.

[...]