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

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

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");
---