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

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

[Chao Liu]

Hi, all:

Thanks to philmd's review, I've made the following improvements in PATCH v3:

Use more maintainable c functions instead of macros, as per review comments.

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

PATCH v2 (reviewed):
https://lore.kernel.org/qemu-devel/72b2d911-112e-48e3-9ba4-017a11758060@linaro.org/T/#u

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 | 36 ++++++++++++++++++++++++++++++------
 1 file changed, 30 insertions(+), 6 deletions(-)

-- 
2.48.1

[PATCH v3 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 | 36 ++++++++++++++++++++++++++++++------
 1 file changed, 30 insertions(+), 6 deletions(-)

diff --git a/system/memory.c b/system/memory.c
index 71434e7ad0..cf91718bcc 100644
--- a/system/memory.c
+++ b/system/memory.c
@@ -3296,6 +3296,21 @@ 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,
+};
+
+static void mtree_print_node(enum mtree_node_type is_tail)
+{
+    qemu_printf((is_tail == MTREE_NODE_T_TAIL) ? "└── " : "├── ");
+}
+
+static void mtree_print_col(unsigned level)
+{
+    qemu_printf(level ? "│   " : "│  ");
+}
+
 static void mtree_expand_owner(const char *label, Object *obj)
 {
     DeviceState *dev = (DeviceState *) object_dynamic_cast(obj, TYPE_DEVICE);
@@ -3335,7 +3350,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 +3392,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);
+                mtree_print_col(i);
             }
+            mtree_print_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 +3415,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);
+                mtree_print_col(i);
             }
+            mtree_print_node(node_type);
             qemu_printf(HWADDR_FMT_plx "-" HWADDR_FMT_plx
                         " (prio %d, %s%s): %s%s",
                         cur_start, cur_end,
@@ -3435,8 +3453,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 +3636,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 +3682,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 v3 1/1] system: optimizing info mtree printing for monitors

[Philippe Mathieu-Daudé]

Hi Chao,

On 30/4/25 11:02, Chao Liu wrote:
> Make the hierarchical relationship between nodes clearer by adding characters

W.r.t. $Subject, it isn't really about "optimization" but making
output clearer, as you describe.

> 
> 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 | 36 ++++++++++++++++++++++++++++++------
>   1 file changed, 30 insertions(+), 6 deletions(-)
> 
> diff --git a/system/memory.c b/system/memory.c
> index 71434e7ad0..cf91718bcc 100644
> --- a/system/memory.c
> +++ b/system/memory.c
> @@ -3296,6 +3296,21 @@ 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,
> +};
> +
> +static void mtree_print_node(enum mtree_node_type is_tail)

Either rename 'is_tail' as 'node_type', or really use a boolean
(maybe '[is_]last_node' is clearer?), since the mtree_node_type enum
doesn't bring much (see [*] below).

> +{
> +    qemu_printf((is_tail == MTREE_NODE_T_TAIL) ? "└── " : "├── ");
> +}
> +
> +static void mtree_print_col(unsigned level)
> +{
> +    qemu_printf(level ? "│   " : "│  ");
> +}
> +
>   static void mtree_expand_owner(const char *label, Object *obj)
>   {
>       DeviceState *dev = (DeviceState *) object_dynamic_cast(obj, TYPE_DEVICE);
> @@ -3335,7 +3350,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 +3392,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);
> +                mtree_print_col(i);
>               }
> +            mtree_print_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 +3415,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);
> +                mtree_print_col(i);
>               }
> +            mtree_print_node(node_type);
>               qemu_printf(HWADDR_FMT_plx "-" HWADDR_FMT_plx
>                           " (prio %d, %s%s): %s%s",
>                           cur_start, cur_end,
> @@ -3435,8 +3453,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 +3636,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 +3682,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");
>       }
>   

Trying this patch:

$ ./qemu-system-aarch64 -S -monitor stdio -M raspi4b
QEMU 10.0.50 monitor - type 'help' for more information
(qemu) info mtree
address-space: cpu-memory-0
address-space: cpu-memory-1
address-space: cpu-memory-2
address-space: cpu-memory-3
address-space: cpu-secure-memory-0
address-space: cpu-secure-memory-1
address-space: cpu-secure-memory-2
address-space: cpu-secure-memory-3
address-space: memory
│  ├── 0000000000000000-ffffffffffffffff (prio 0, i/o): system
│  │   ├── 0000000000000000-000000007fffffff (prio 0, ram): ram
│  │   ├── 00000000fc000000-00000000fdffffff (prio 1, i/o): 
bcm2838-peripherals
│  │   ├── 00000000fe000000-00000000ff7fffff (prio 1, i/o): 
bcm2835-peripherals
│  │   │   ├── 00000000fe003000-00000000fe00301f (prio 0, i/o): 
bcm2835-sys-timer
│  │   │   ├── 00000000fe004000-00000000fe004fff (prio -1000, i/o): 
bcm2835-txp
│  │   │   ├── 00000000fe007000-00000000fe007fff (prio 0, i/o): bcm2835-dma
│  │   │   ├── 00000000fe00a000-00000000fe00a023 (prio -1000, i/o): 
bcm2838-asb
│  │   │   ├── 00000000fe00b200-00000000fe00b3ff (prio 0, i/o): alias 
mphi @mphi 0000000000000000-00000000000001ff
│  │   │   ├── 00000000fe00b200-00000000fe00b3ff (prio 0, i/o): bcm2835-ic
│  │   │   ├── 00000000fe00b400-00000000fe00b43f (prio -1000, i/o): 
bcm2835-sp804
│  │   │   ├── 00000000fe00b800-00000000fe00bbff (prio 0, i/o): bcm2835-mbox
│  │   │   ├── 00000000fe100000-00000000fe1001ff (prio 0, i/o): 
bcm2835-powermgt
│  │   │   ├── 00000000fe101000-00000000fe102fff (prio 0, i/o): 
bcm2835-cprman
│  │   │   ├── 00000000fe200000-00000000fe200fff (prio 0, i/o): bcm2838_gpio
│  │   │   ├── 00000000fe201000-00000000fe201fff (prio 0, i/o): pl011
│  │   │   ├── 00000000fe202000-00000000fe202fff (prio 0, i/o): 
bcm2835-sdhost
│  │   │   ├── 00000000fe203000-00000000fe2030ff (prio -1000, i/o): 
bcm2835-i2s
│  │   │   ├── 00000000fe204000-00000000fe204017 (prio 0, i/o): bcm2835-spi
│  │   │   ├── 00000000fe205000-00000000fe205023 (prio 0, i/o): bcm2835-i2c
│  │   │   ├── 00000000fe20f000-00000000fe20f07f (prio 0, i/o): bcm2835-otp
│  │   │   ├── 00000000fe214000-00000000fe2140ff (prio -1000, i/o): 
bcm2835-spis
│  │   │   ├── 00000000fe215000-00000000fe2150ff (prio 0, i/o): bcm2835-aux
│  │   │   ├── 00000000fe300000-00000000fe3000ff (prio 0, i/o): sdhci
│  │   │   ├── 00000000fe340000-00000000fe3400ff (prio 0, i/o): sdhci
│  │   │   ├── 00000000fe600000-00000000fe6000ff (prio -1000, i/o): 
bcm2835-smi
│  │   │   ├── 00000000fe804000-00000000fe804023 (prio 0, i/o): bcm2835-i2c
│  │   │   ├── 00000000fe805000-00000000fe805023 (prio 0, i/o): bcm2835-i2c
│  │   │   ├── 00000000fe900000-00000000fe907fff (prio -1000, i/o): 
bcm2835-dbus
│  │   │   ├── 00000000fe910000-00000000fe917fff (prio -1000, i/o): 
bcm2835-ave0
│  │   │   ├── 00000000fe980000-00000000fe990fff (prio 0, i/o): dwc2
│  │   │   │   ├── 00000000fe980000-00000000fe980fff (prio 0, i/o): dwc2-io
│  │   │   │   └── 00000000fe981000-00000000fe990fff (prio 0, i/o): 
dwc2-fifo
│  │   │   ├── 00000000fec00000-00000000fec00fff (prio -1000, i/o): 
bcm2835-v3d
│  │   │   ├── 00000000fec11000-00000000fec110ff (prio -1000, i/o): 
bcm2835-clkisp
│  │   │   ├── 00000000fee00000-00000000fee000ff (prio -1000, i/o): 
bcm2835-sdramc
│  │   │   └── 00000000fee05000-00000000fee050ff (prio 0, i/o): 
bcm2835-dma-chan15
│  │   ├── 00000000ff800000-00000000ff8000ff (prio 0, i/o): bcm2836-control
│  │   ├── 00000000ff841000-00000000ff841fff (prio 0, i/o): gic_dist
│  │   ├── 00000000ff842000-00000000ff843fff (prio 0, i/o): gic_cpu
│  │   ├── 00000000ff844000-00000000ff844fff (prio 0, i/o): gic_viface
│  │   ├── 00000000ff845000-00000000ff8450ff (prio 0, i/o): gic_cpu
│  │   ├── 00000000ff845200-00000000ff8452ff (prio 0, i/o): gic_cpu
│  │   ├── 00000000ff845400-00000000ff8454ff (prio 0, i/o): gic_cpu
│  │   ├── 00000000ff845600-00000000ff8456ff (prio 0, i/o): gic_cpu
│  │   └── 00000000ff846000-00000000ff847fff (prio 0, i/o): gic_vcpu

address-space: bcm2835-mbox-memory
│  ├── 0000000000000000-000000000000008f (prio 0, i/o): bcm2835-mbox
│  │   ├── 0000000000000010-000000000000001f (prio 0, i/o): bcm2835-fb
│  │   └── 0000000000000080-000000000000008f (prio 0, i/o): bcm2835-property

address-space: I/O
│  ├── 0000000000000000-000000000000ffff (prio 0, i/o): io

address-space: bcm2835-dma-memory
address-space: bcm2835-fb-memory
address-space: bcm2835-property-memory
address-space: dwc2
│  ├── 0000000000000000-00000000ffffffff (prio 0, i/o): bcm2835-gpu
│  │   ├── 0000000000000000-000000007fffffff (prio 0, ram): alias 
bcm2835-gpu-ram-alias[*] @ram 0000000000000000-000000007fffffff
│  │   ├── 0000000040000000-00000000bfffffff (prio 0, ram): alias 
bcm2835-gpu-ram-alias[*] @ram 0000000000000000-000000007fffffff
│  │   ├── 000000007c000000-000000007dffffff (prio 1, i/o): alias 
bcm2838-peripherals @bcm2838-peripherals 0000000000000000-0000000001ffffff
│  │   ├── 000000007e000000-000000007f7fffff (prio 1, i/o): alias 
bcm2835-peripherals @bcm2835-peripherals 0000000000000000-00000000017fffff
│  │   ├── 0000000080000000-00000000ffffffff (prio 0, ram): alias 
bcm2835-gpu-ram-alias[*] @ram 0000000000000000-000000007fffffff
│  │   └── 00000000c0000000-000000013fffffff (prio 0, ram): alias 
bcm2835-gpu-ram-alias[*] @ram 0000000000000000-000000007fffffff

memory-region: mphi
│  ├── 0000000000000000-0000000000000fff (prio 0, i/o): mphi

memory-region: ram
│  ├── 0000000000000000-000000007fffffff (prio 0, ram): ram

memory-region: bcm2838-peripherals
│  ├── 00000000fc000000-00000000fdffffff (prio 1, i/o): bcm2838-peripherals

memory-region: bcm2835-peripherals
│  ├── 00000000fe000000-00000000ff7fffff (prio 1, i/o): bcm2835-peripherals
│  │   ├── 00000000fe003000-00000000fe00301f (prio 0, i/o): 
bcm2835-sys-timer
│  │   ├── 00000000fe004000-00000000fe004fff (prio -1000, i/o): bcm2835-txp
│  │   ├── 00000000fe007000-00000000fe007fff (prio 0, i/o): bcm2835-dma
│  │   ├── 00000000fe00a000-00000000fe00a023 (prio -1000, i/o): bcm2838-asb
│  │   ├── 00000000fe00b200-00000000fe00b3ff (prio 0, i/o): alias mphi 
@mphi 0000000000000000-00000000000001ff
│  │   ├── 00000000fe00b200-00000000fe00b3ff (prio 0, i/o): bcm2835-ic
│  │   ├── 00000000fe00b400-00000000fe00b43f (prio -1000, i/o): 
bcm2835-sp804
│  │   ├── 00000000fe00b800-00000000fe00bbff (prio 0, i/o): bcm2835-mbox
│  │   ├── 00000000fe100000-00000000fe1001ff (prio 0, i/o): bcm2835-powermgt
│  │   ├── 00000000fe101000-00000000fe102fff (prio 0, i/o): bcm2835-cprman
│  │   ├── 00000000fe200000-00000000fe200fff (prio 0, i/o): bcm2838_gpio
│  │   ├── 00000000fe201000-00000000fe201fff (prio 0, i/o): pl011
│  │   ├── 00000000fe202000-00000000fe202fff (prio 0, i/o): bcm2835-sdhost
│  │   ├── 00000000fe203000-00000000fe2030ff (prio -1000, i/o): bcm2835-i2s
│  │   ├── 00000000fe204000-00000000fe204017 (prio 0, i/o): bcm2835-spi
│  │   ├── 00000000fe205000-00000000fe205023 (prio 0, i/o): bcm2835-i2c
│  │   ├── 00000000fe20f000-00000000fe20f07f (prio 0, i/o): bcm2835-otp
│  │   ├── 00000000fe214000-00000000fe2140ff (prio -1000, i/o): bcm2835-spis
│  │   ├── 00000000fe215000-00000000fe2150ff (prio 0, i/o): bcm2835-aux
│  │   ├── 00000000fe300000-00000000fe3000ff (prio 0, i/o): sdhci
│  │   ├── 00000000fe340000-00000000fe3400ff (prio 0, i/o): sdhci
│  │   ├── 00000000fe600000-00000000fe6000ff (prio -1000, i/o): bcm2835-smi
│  │   ├── 00000000fe804000-00000000fe804023 (prio 0, i/o): bcm2835-i2c
│  │   ├── 00000000fe805000-00000000fe805023 (prio 0, i/o): bcm2835-i2c
│  │   ├── 00000000fe900000-00000000fe907fff (prio -1000, i/o): bcm2835-dbus
│  │   ├── 00000000fe910000-00000000fe917fff (prio -1000, i/o): bcm2835-ave0
│  │   ├── 00000000fe980000-00000000fe990fff (prio 0, i/o): dwc2
│  │   │   ├── 00000000fe980000-00000000fe980fff (prio 0, i/o): dwc2-io
│  │   │   └── 00000000fe981000-00000000fe990fff (prio 0, i/o): dwc2-fifo
│  │   ├── 00000000fec00000-00000000fec00fff (prio -1000, i/o): bcm2835-v3d
│  │   ├── 00000000fec11000-00000000fec110ff (prio -1000, i/o): 
bcm2835-clkisp
│  │   ├── 00000000fee00000-00000000fee000ff (prio -1000, i/o): 
bcm2835-sdramc
│  │   └── 00000000fee05000-00000000fee050ff (prio 0, i/o): 
bcm2835-dma-chan15

(qemu)

1/ For leaves it is OK, but root branch looks odd being displayed all
    along without leaf.

2/ Why single region is displayed unfinished (├)? We shouldn't display
    any prefix at all, see:

address-space: I/O
│  ├── 0000000000000000-000000000000ffff (prio 0, i/o): io


[*] removing enum mtree_node_type:

-- >8 --
diff --git a/system/memory.c b/system/memory.c
index cf91718bcc1..20aa4a8e008 100644
--- a/system/memory.c
+++ b/system/memory.c
@@ -3298,10 +3298,5 @@ typedef QTAILQ_HEAD(, MemoryRegionList) 
MemoryRegionListHead;

-enum mtree_node_type {
-    MTREE_NODE_T_INNER,
-    MTREE_NODE_T_TAIL,
-};
-
-static void mtree_print_node(enum mtree_node_type is_tail)
+static void mtree_print_node(bool last_node)
  {
-    qemu_printf((is_tail == MTREE_NODE_T_TAIL) ? "└── " : "├── ");
+    qemu_printf(last_node ? "└── " : "├── ");
  }
@@ -3353,3 +3348,3 @@ static void mtree_print_mr(const MemoryRegion *mr, 
unsigned int level,
                             bool owner, bool display_disabled,
-                           enum mtree_node_type node_type)
+                           bool last_node)
  {
@@ -3396,3 +3391,3 @@ static void mtree_print_mr(const MemoryRegion *mr, 
unsigned int level,
              }
-            mtree_print_node(node_type);
+            mtree_print_node(last_node);
              qemu_printf(HWADDR_FMT_plx "-" HWADDR_FMT_plx
@@ -3419,3 +3414,3 @@ static void mtree_print_mr(const MemoryRegion *mr, 
unsigned int level,
              }
-            mtree_print_node(node_type);
+            mtree_print_node(last_node);
              qemu_printf(HWADDR_FMT_plx "-" HWADDR_FMT_plx
@@ -3456,3 +3451,3 @@ static void mtree_print_mr(const MemoryRegion *mr, 
unsigned int level,
          if (ml == QTAILQ_LAST(&submr_print_queue)) {
-            node_type = MTREE_NODE_T_TAIL;
+            last_node = true;
          }
@@ -3460,3 +3455,3 @@ static void mtree_print_mr(const MemoryRegion *mr, 
unsigned int level,
                         alias_print_queue, owner,
-                       display_disabled, node_type);
+                       display_disabled, last_node);
      }
@@ -3638,4 +3633,3 @@ static void mtree_print_as(gpointer key, gpointer 
value, gpointer 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_NODE_T_INNER);
+    mtree_print_mr(mr, 1, 0, asi->ml_head, asi->owner, asi->disabled, 
false);
      qemu_printf("\n");
@@ -3684,4 +3678,3 @@ static void mtree_info_as(bool dispatch_tree, bool 
owner, bool disabled)
          qemu_printf("memory-region: %s\n", memory_region_name(ml->mr));
-        mtree_print_mr(ml->mr, 1, 0, &ml_head, owner, disabled,
-                       MTREE_NODE_T_INNER);
+        mtree_print_mr(ml->mr, 1, 0, &ml_head, owner, disabled, false);
          qemu_printf("\n");
---

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

[David Hildenbrand]

On 30.04.25 11:58, Philippe Mathieu-Daudé wrote:
> Hi Chao,
> 
> On 30/4/25 11:02, Chao Liu wrote:
>> Make the hierarchical relationship between nodes clearer by adding characters
> 
> W.r.t. $Subject, it isn't really about "optimization" but making
> output clearer, as you describe.
> 
>>
>> 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 | 36 ++++++++++++++++++++++++++++++------
>>    1 file changed, 30 insertions(+), 6 deletions(-)
>>
>> diff --git a/system/memory.c b/system/memory.c
>> index 71434e7ad0..cf91718bcc 100644
>> --- a/system/memory.c
>> +++ b/system/memory.c
>> @@ -3296,6 +3296,21 @@ 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,
>> +};
>> +
>> +static void mtree_print_node(enum mtree_node_type is_tail)
> 
> Either rename 'is_tail' as 'node_type', or really use a boolean
> (maybe '[is_]last_node' is clearer?), since the mtree_node_type enum
> doesn't bring much (see [*] below).
> 

Note that I suggested that, because the passing or true/false in the 
code is horrible for readability.

-- 
Cheers,

David / dhildenb

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

[Philippe Mathieu-Daudé]

On 30/4/25 23:38, David Hildenbrand wrote:
> On 30.04.25 11:58, Philippe Mathieu-Daudé wrote:
>> Hi Chao,
>>
>> On 30/4/25 11:02, Chao Liu wrote:
>>> Make the hierarchical relationship between nodes clearer by adding 
>>> characters
>>
>> W.r.t. $Subject, it isn't really about "optimization" but making
>> output clearer, as you describe.
>>
>>>
>>> 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 | 36 ++++++++++++++++++++++++++++++------
>>>    1 file changed, 30 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/system/memory.c b/system/memory.c
>>> index 71434e7ad0..cf91718bcc 100644
>>> --- a/system/memory.c
>>> +++ b/system/memory.c
>>> @@ -3296,6 +3296,21 @@ 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,
>>> +};
>>> +
>>> +static void mtree_print_node(enum mtree_node_type is_tail)
>>
>> Either rename 'is_tail' as 'node_type', or really use a boolean
>> (maybe '[is_]last_node' is clearer?), since the mtree_node_type enum
>> doesn't bring much (see [*] below).
>>
> 
> Note that I suggested that, because the passing or true/false in the 
> code is horrible for readability.

Oh good point. Sorry I missed previous version reviews.

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

[Mario Casquero]

This patch has been successfully tested. Differences are clear,
without the patch:

/usr/libexec/qemu-kvm -m 4G -monitor stdio
(qemu) info mtree
...
memory-region: system
  0000000000000000-ffffffffffffffff (prio 0, i/o): system
    0000000000000000-00000000bfffffff (prio 0, ram): alias
ram-below-4g @pc.ram 0000000000000000-00000000bfffffff
    0000000000000000-ffffffffffffffff (prio -1, i/o): pci
...
After applying the patch

./qemu/build/qemu-system-x86_64 -m 4G -monitor stdio
(qemu) info mtree
...
memory-region: system
│  ├── 0000000000000000-ffffffffffffffff (prio 0, i/o): system
│  │   ├── 0000000000000000-00000000bfffffff (prio 0, ram): alias
ram-below-4g @pc.ram 0000000000000000-00000000bfffffff
│  │   ├── 0000000000000000-ffffffffffffffff (prio -1, i/o): pci
...

Tested-by: Mario Casquero <mcasquer@redhat.com>


On Wed, Apr 30, 2025 at 11:08 AM Chao Liu <lc00631@tecorigin.com> 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 | 36 ++++++++++++++++++++++++++++++------
>  1 file changed, 30 insertions(+), 6 deletions(-)
>
> diff --git a/system/memory.c b/system/memory.c
> index 71434e7ad0..cf91718bcc 100644
> --- a/system/memory.c
> +++ b/system/memory.c
> @@ -3296,6 +3296,21 @@ 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,
> +};
> +
> +static void mtree_print_node(enum mtree_node_type is_tail)
> +{
> +    qemu_printf((is_tail == MTREE_NODE_T_TAIL) ? "└── " : "├── ");
> +}
> +
> +static void mtree_print_col(unsigned level)
> +{
> +    qemu_printf(level ? "│   " : "│  ");
> +}
> +
>  static void mtree_expand_owner(const char *label, Object *obj)
>  {
>      DeviceState *dev = (DeviceState *) object_dynamic_cast(obj, TYPE_DEVICE);
> @@ -3335,7 +3350,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 +3392,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);
> +                mtree_print_col(i);
>              }
> +            mtree_print_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 +3415,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);
> +                mtree_print_col(i);
>              }
> +            mtree_print_node(node_type);
>              qemu_printf(HWADDR_FMT_plx "-" HWADDR_FMT_plx
>                          " (prio %d, %s%s): %s%s",
>                          cur_start, cur_end,
> @@ -3435,8 +3453,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 +3636,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 +3682,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
>
>