softmmu/memory.c | 64 +++++++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 60 insertions(+), 4 deletions(-)
Per Peter Maydell [*]:
'info mtree' monitor command was designed on the assumption that
there's really only one or two interesting address spaces, and
with more recent developments that's just not the case any more.
Similarly about how the FlatView are sorted using a GHashTable,
sort the AddressSpace objects to remove the duplications (AS
using the same root MemoryRegion).
This drastically reduce 'info mtree' on some boards.
Before:
$ (echo info mtree; echo q) \
| qemu-system-aarch64 -S -monitor stdio -M raspi3b \
| wc -l
423
After:
$ (echo info mtree; echo q) \
| qemu-system-aarch64 -S -monitor stdio -M raspi3b \
| wc -l
106
(qemu) info mtree
address-space: I/O
0000000000000000-000000000000ffff (prio 0, i/o): io
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-000000003fffffff (prio 0, ram): ram
000000003f000000-000000003fffffff (prio 1, i/o): bcm2835-peripherals
000000003f003000-000000003f00301f (prio 0, i/o): bcm2835-sys-timer
000000003f004000-000000003f004fff (prio -1000, i/o): bcm2835-txp
000000003f006000-000000003f006fff (prio 0, i/o): mphi
000000003f007000-000000003f007fff (prio 0, i/o): bcm2835-dma
000000003f00b200-000000003f00b3ff (prio 0, i/o): bcm2835-ic
000000003f00b400-000000003f00b43f (prio -1000, i/o): bcm2835-sp804
000000003f00b800-000000003f00bbff (prio 0, i/o): bcm2835-mbox
000000003f100000-000000003f1001ff (prio 0, i/o): bcm2835-powermgt
000000003f101000-000000003f102fff (prio 0, i/o): bcm2835-cprman
000000003f104000-000000003f10400f (prio 0, i/o): bcm2835-rng
000000003f200000-000000003f200fff (prio 0, i/o): bcm2835_gpio
000000003f201000-000000003f201fff (prio 0, i/o): pl011
000000003f202000-000000003f202fff (prio 0, i/o): bcm2835-sdhost
000000003f203000-000000003f2030ff (prio -1000, i/o): bcm2835-i2s
000000003f204000-000000003f20401f (prio -1000, i/o): bcm2835-spi0
000000003f205000-000000003f20501f (prio -1000, i/o): bcm2835-i2c0
000000003f20f000-000000003f20f07f (prio -1000, i/o): bcm2835-otp
000000003f212000-000000003f212007 (prio 0, i/o): bcm2835-thermal
000000003f214000-000000003f2140ff (prio -1000, i/o): bcm2835-spis
000000003f215000-000000003f2150ff (prio 0, i/o): bcm2835-aux
000000003f300000-000000003f3000ff (prio 0, i/o): sdhci
000000003f600000-000000003f6000ff (prio -1000, i/o): bcm2835-smi
000000003f804000-000000003f80401f (prio -1000, i/o): bcm2835-i2c1
000000003f805000-000000003f80501f (prio -1000, i/o): bcm2835-i2c2
000000003f900000-000000003f907fff (prio -1000, i/o): bcm2835-dbus
000000003f910000-000000003f917fff (prio -1000, i/o): bcm2835-ave0
000000003f980000-000000003f990fff (prio 0, i/o): dwc2
000000003f980000-000000003f980fff (prio 0, i/o): dwc2-io
000000003f981000-000000003f990fff (prio 0, i/o): dwc2-fifo
000000003fc00000-000000003fc00fff (prio -1000, i/o): bcm2835-v3d
000000003fe00000-000000003fe000ff (prio -1000, i/o): bcm2835-sdramc
000000003fe05000-000000003fe050ff (prio 0, i/o): bcm2835-dma-chan15
0000000040000000-00000000400000ff (prio 0, i/o): bcm2836-control
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-000000003fffffff (prio 0, ram): alias bcm2835-gpu-ram-alias[*] @ram 0000000000000000-000000003fffffff
0000000040000000-000000007fffffff (prio 0, ram): alias bcm2835-gpu-ram-alias[*] @ram 0000000000000000-000000003fffffff
000000007e000000-000000007effffff (prio 1, i/o): alias bcm2835-peripherals @bcm2835-peripherals 0000000000000000-0000000000ffffff
0000000080000000-00000000bfffffff (prio 0, ram): alias bcm2835-gpu-ram-alias[*] @ram 0000000000000000-000000003fffffff
00000000c0000000-00000000ffffffff (prio 0, ram): alias bcm2835-gpu-ram-alias[*] @ram 0000000000000000-000000003fffffff
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
memory-region: ram
0000000000000000-000000003fffffff (prio 0, ram): ram
memory-region: bcm2835-peripherals
000000003f000000-000000003fffffff (prio 1, i/o): bcm2835-peripherals
000000003f003000-000000003f00301f (prio 0, i/o): bcm2835-sys-timer
000000003f004000-000000003f004fff (prio -1000, i/o): bcm2835-txp
000000003f006000-000000003f006fff (prio 0, i/o): mphi
000000003f007000-000000003f007fff (prio 0, i/o): bcm2835-dma
000000003f00b200-000000003f00b3ff (prio 0, i/o): bcm2835-ic
000000003f00b400-000000003f00b43f (prio -1000, i/o): bcm2835-sp804
000000003f00b800-000000003f00bbff (prio 0, i/o): bcm2835-mbox
000000003f100000-000000003f1001ff (prio 0, i/o): bcm2835-powermgt
000000003f101000-000000003f102fff (prio 0, i/o): bcm2835-cprman
000000003f104000-000000003f10400f (prio 0, i/o): bcm2835-rng
000000003f200000-000000003f200fff (prio 0, i/o): bcm2835_gpio
000000003f201000-000000003f201fff (prio 0, i/o): pl011
000000003f202000-000000003f202fff (prio 0, i/o): bcm2835-sdhost
000000003f203000-000000003f2030ff (prio -1000, i/o): bcm2835-i2s
000000003f204000-000000003f20401f (prio -1000, i/o): bcm2835-spi0
000000003f205000-000000003f20501f (prio -1000, i/o): bcm2835-i2c0
000000003f20f000-000000003f20f07f (prio -1000, i/o): bcm2835-otp
000000003f212000-000000003f212007 (prio 0, i/o): bcm2835-thermal
000000003f214000-000000003f2140ff (prio -1000, i/o): bcm2835-spis
000000003f215000-000000003f2150ff (prio 0, i/o): bcm2835-aux
000000003f300000-000000003f3000ff (prio 0, i/o): sdhci
000000003f600000-000000003f6000ff (prio -1000, i/o): bcm2835-smi
000000003f804000-000000003f80401f (prio -1000, i/o): bcm2835-i2c1
000000003f805000-000000003f80501f (prio -1000, i/o): bcm2835-i2c2
000000003f900000-000000003f907fff (prio -1000, i/o): bcm2835-dbus
000000003f910000-000000003f917fff (prio -1000, i/o): bcm2835-ave0
000000003f980000-000000003f990fff (prio 0, i/o): dwc2
000000003f980000-000000003f980fff (prio 0, i/o): dwc2-io
000000003f981000-000000003f990fff (prio 0, i/o): dwc2-fifo
000000003fc00000-000000003fc00fff (prio -1000, i/o): bcm2835-v3d
000000003fe00000-000000003fe000ff (prio -1000, i/o): bcm2835-sdramc
000000003fe05000-000000003fe050ff (prio 0, i/o): bcm2835-dma-chan15
(qemu) q
[*] https://www.mail-archive.com/qemu-devel@nongnu.org/msg829821.html
Suggested-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
v3: Removed unused AddressSpaceInfo::counter
v2: List AS similarly to 'info mtree -f' (peterx)
---
softmmu/memory.c | 64 +++++++++++++++++++++++++++++++++++++++++++++---
1 file changed, 60 insertions(+), 4 deletions(-)
diff --git a/softmmu/memory.c b/softmmu/memory.c
index bfedaf9c4df..459d6246672 100644
--- a/softmmu/memory.c
+++ b/softmmu/memory.c
@@ -3246,11 +3246,55 @@ static gboolean mtree_info_flatview_free(gpointer key, gpointer value,
return true;
}
+struct AddressSpaceInfo {
+ MemoryRegionListHead *ml_head;
+ bool owner;
+ bool disabled;
+};
+
+/* Returns negative value if a < b; zero if a = b; positive value if a > b. */
+static gint address_space_compare_name(gconstpointer a, gconstpointer b)
+{
+ const AddressSpace *as_a = a;
+ const AddressSpace *as_b = b;
+
+ return g_strcmp0(as_a->name, as_b->name);
+}
+static void mtree_print_as_name(gpointer data, gpointer user_data)
+{
+ AddressSpace *as = data;
+
+ qemu_printf("address-space: %s\n", as->name);
+}
+
+static void mtree_print_as(gpointer key, gpointer value, gpointer user_data)
+{
+ MemoryRegion *mr = key;
+ GSList *as_same_root_mr_list = value;
+ 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);
+ qemu_printf("\n");
+}
+
+static gboolean mtree_info_as_free(gpointer key, gpointer value,
+ gpointer user_data)
+{
+ GSList *as_same_root_mr_list = value;
+
+ g_slist_free(as_same_root_mr_list);
+
+ return true;
+}
+
void mtree_info(bool flatview, bool dispatch_tree, bool owner, bool disabled)
{
MemoryRegionListHead ml_head;
MemoryRegionList *ml, *ml2;
AddressSpace *as;
+ GHashTable *views = g_hash_table_new(g_direct_hash, g_direct_equal);
+ GSList *as_same_root_mr_list;
if (flatview) {
FlatView *view;
@@ -3260,7 +3304,6 @@ void mtree_info(bool flatview, bool dispatch_tree, bool owner, bool disabled)
.owner = owner,
};
GArray *fv_address_spaces;
- GHashTable *views = g_hash_table_new(g_direct_hash, g_direct_equal);
AccelClass *ac = ACCEL_GET_CLASS(current_accel());
if (ac->has_memory) {
@@ -3293,11 +3336,24 @@ void mtree_info(bool flatview, bool dispatch_tree, bool owner, bool disabled)
QTAILQ_INIT(&ml_head);
QTAILQ_FOREACH(as, &address_spaces, address_spaces_link) {
- qemu_printf("address-space: %s\n", as->name);
- mtree_print_mr(as->root, 1, 0, &ml_head, owner, disabled);
- qemu_printf("\n");
+ /* Create hashtable, key=AS root MR, value = list of AS */
+ as_same_root_mr_list = g_hash_table_lookup(views, as->root);
+ as_same_root_mr_list = g_slist_insert_sorted(as_same_root_mr_list, as,
+ address_space_compare_name);
+ g_hash_table_insert(views, as->root, as_same_root_mr_list);
}
+ struct AddressSpaceInfo asi = {
+ .ml_head = &ml_head,
+ .owner = owner,
+ .disabled = disabled,
+ };
+
+ /* print address spaces */
+ g_hash_table_foreach(views, mtree_print_as, &asi);
+ g_hash_table_foreach_remove(views, mtree_info_as_free, 0);
+ g_hash_table_unref(views);
+
/* print aliased regions */
QTAILQ_FOREACH(ml, &ml_head, mrqueue) {
qemu_printf("memory-region: %s\n", memory_region_name(ml->mr));
--
2.31.1
On Mon, 23 Aug 2021 at 09:54, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
>
> Per Peter Maydell [*]:
>
> 'info mtree' monitor command was designed on the assumption that
> there's really only one or two interesting address spaces, and
> with more recent developments that's just not the case any more.
>
> Similarly about how the FlatView are sorted using a GHashTable,
> sort the AddressSpace objects to remove the duplications (AS
> using the same root MemoryRegion).
>
> diff --git a/softmmu/memory.c b/softmmu/memory.c
> index bfedaf9c4df..459d6246672 100644
> --- a/softmmu/memory.c
> +++ b/softmmu/memory.c
> @@ -3246,11 +3246,55 @@ static gboolean mtree_info_flatview_free(gpointer key, gpointer value,
> return true;
> }
>
> +struct AddressSpaceInfo {
> + MemoryRegionListHead *ml_head;
> + bool owner;
> + bool disabled;
> +};
> +
> +/* Returns negative value if a < b; zero if a = b; positive value if a > b. */
> +static gint address_space_compare_name(gconstpointer a, gconstpointer b)
> +{
> + const AddressSpace *as_a = a;
> + const AddressSpace *as_b = b;
> +
> + return g_strcmp0(as_a->name, as_b->name);
> +}
> +static void mtree_print_as_name(gpointer data, gpointer user_data)
> +{
> + AddressSpace *as = data;
> +
> + qemu_printf("address-space: %s\n", as->name);
> +}
> +
> +static void mtree_print_as(gpointer key, gpointer value, gpointer user_data)
> +{
> + MemoryRegion *mr = key;
> + GSList *as_same_root_mr_list = value;
> + 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);
> + qemu_printf("\n");
> +}
> +
> +static gboolean mtree_info_as_free(gpointer key, gpointer value,
> + gpointer user_data)
> +{
> + GSList *as_same_root_mr_list = value;
> +
> + g_slist_free(as_same_root_mr_list);
> +
> + return true;
> +}
> +
> void mtree_info(bool flatview, bool dispatch_tree, bool owner, bool disabled)
> {
> MemoryRegionListHead ml_head;
> MemoryRegionList *ml, *ml2;
> AddressSpace *as;
> + GHashTable *views = g_hash_table_new(g_direct_hash, g_direct_equal);
> + GSList *as_same_root_mr_list;
>
> if (flatview) {
> FlatView *view;
> @@ -3260,7 +3304,6 @@ void mtree_info(bool flatview, bool dispatch_tree, bool owner, bool disabled)
> .owner = owner,
> };
> GArray *fv_address_spaces;
> - GHashTable *views = g_hash_table_new(g_direct_hash, g_direct_equal);
> AccelClass *ac = ACCEL_GET_CLASS(current_accel());
>
> if (ac->has_memory) {
> @@ -3293,11 +3336,24 @@ void mtree_info(bool flatview, bool dispatch_tree, bool owner, bool disabled)
> QTAILQ_INIT(&ml_head);
>
> QTAILQ_FOREACH(as, &address_spaces, address_spaces_link) {
> - qemu_printf("address-space: %s\n", as->name);
> - mtree_print_mr(as->root, 1, 0, &ml_head, owner, disabled);
> - qemu_printf("\n");
> + /* Create hashtable, key=AS root MR, value = list of AS */
> + as_same_root_mr_list = g_hash_table_lookup(views, as->root);
> + as_same_root_mr_list = g_slist_insert_sorted(as_same_root_mr_list, as,
> + address_space_compare_name);
> + g_hash_table_insert(views, as->root, as_same_root_mr_list);
> }
>
> + struct AddressSpaceInfo asi = {
> + .ml_head = &ml_head,
> + .owner = owner,
> + .disabled = disabled,
> + };
Strictly speaking this is against our coding-style "no variable
declarations in the middle of a block".
> +
> + /* print address spaces */
> + g_hash_table_foreach(views, mtree_print_as, &asi);
> + g_hash_table_foreach_remove(views, mtree_info_as_free, 0);
> + g_hash_table_unref(views);
> +
> /* print aliased regions */
> QTAILQ_FOREACH(ml, &ml_head, mrqueue) {
> qemu_printf("memory-region: %s\n", memory_region_name(ml->mr));
> --
> 2.31.1
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
Side note: I wonder if it would be worth splitting mtree_info()
into a function for ASes and a function for flatviews? The two
cases share basically no code, and there's only one callsite.
thanks
-- PMM
On 8/31/21 10:27 PM, Peter Maydell wrote:
> On Mon, 23 Aug 2021 at 09:54, Philippe Mathieu-Daudé <philmd@redhat.com> wrote:
>>
>> Per Peter Maydell [*]:
>>
>> 'info mtree' monitor command was designed on the assumption that
>> there's really only one or two interesting address spaces, and
>> with more recent developments that's just not the case any more.
>>
>> Similarly about how the FlatView are sorted using a GHashTable,
>> sort the AddressSpace objects to remove the duplications (AS
>> using the same root MemoryRegion).
>>
>
>
>> diff --git a/softmmu/memory.c b/softmmu/memory.c
>> index bfedaf9c4df..459d6246672 100644
>> --- a/softmmu/memory.c
>> +++ b/softmmu/memory.c
>> @@ -3246,11 +3246,55 @@ static gboolean mtree_info_flatview_free(gpointer key, gpointer value,
>> return true;
>> }
>>
>> +struct AddressSpaceInfo {
>> + MemoryRegionListHead *ml_head;
>> + bool owner;
>> + bool disabled;
>> +};
>> +
>> +/* Returns negative value if a < b; zero if a = b; positive value if a > b. */
>> +static gint address_space_compare_name(gconstpointer a, gconstpointer b)
>> +{
>> + const AddressSpace *as_a = a;
>> + const AddressSpace *as_b = b;
>> +
>> + return g_strcmp0(as_a->name, as_b->name);
>> +}
>> +static void mtree_print_as_name(gpointer data, gpointer user_data)
>> +{
>> + AddressSpace *as = data;
>> +
>> + qemu_printf("address-space: %s\n", as->name);
>> +}
>> +
>> +static void mtree_print_as(gpointer key, gpointer value, gpointer user_data)
>> +{
>> + MemoryRegion *mr = key;
>> + GSList *as_same_root_mr_list = value;
>> + 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);
>> + qemu_printf("\n");
>> +}
>> +
>> +static gboolean mtree_info_as_free(gpointer key, gpointer value,
>> + gpointer user_data)
>> +{
>> + GSList *as_same_root_mr_list = value;
>> +
>> + g_slist_free(as_same_root_mr_list);
>> +
>> + return true;
>> +}
>> +
>> void mtree_info(bool flatview, bool dispatch_tree, bool owner, bool disabled)
>> {
>> MemoryRegionListHead ml_head;
>> MemoryRegionList *ml, *ml2;
>> AddressSpace *as;
>> + GHashTable *views = g_hash_table_new(g_direct_hash, g_direct_equal);
>> + GSList *as_same_root_mr_list;
>>
>> if (flatview) {
>> FlatView *view;
>> @@ -3260,7 +3304,6 @@ void mtree_info(bool flatview, bool dispatch_tree, bool owner, bool disabled)
>> .owner = owner,
>> };
>> GArray *fv_address_spaces;
>> - GHashTable *views = g_hash_table_new(g_direct_hash, g_direct_equal);
>> AccelClass *ac = ACCEL_GET_CLASS(current_accel());
>>
>> if (ac->has_memory) {
>> @@ -3293,11 +3336,24 @@ void mtree_info(bool flatview, bool dispatch_tree, bool owner, bool disabled)
>> QTAILQ_INIT(&ml_head);
>>
>> QTAILQ_FOREACH(as, &address_spaces, address_spaces_link) {
>> - qemu_printf("address-space: %s\n", as->name);
>> - mtree_print_mr(as->root, 1, 0, &ml_head, owner, disabled);
>> - qemu_printf("\n");
>> + /* Create hashtable, key=AS root MR, value = list of AS */
>> + as_same_root_mr_list = g_hash_table_lookup(views, as->root);
>> + as_same_root_mr_list = g_slist_insert_sorted(as_same_root_mr_list, as,
>> + address_space_compare_name);
>> + g_hash_table_insert(views, as->root, as_same_root_mr_list);
>> }
>>
>> + struct AddressSpaceInfo asi = {
>> + .ml_head = &ml_head,
>> + .owner = owner,
>> + .disabled = disabled,
>> + };
>
> Strictly speaking this is against our coding-style "no variable
> declarations in the middle of a block".
Right.
>> +
>> + /* print address spaces */
>> + g_hash_table_foreach(views, mtree_print_as, &asi);
>> + g_hash_table_foreach_remove(views, mtree_info_as_free, 0);
>> + g_hash_table_unref(views);
>> +
>> /* print aliased regions */
>> QTAILQ_FOREACH(ml, &ml_head, mrqueue) {
>> qemu_printf("memory-region: %s\n", memory_region_name(ml->mr));
>> --
>> 2.31.1
>
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
>
> Side note: I wonder if it would be worth splitting mtree_info()
> into a function for ASes and a function for flatviews? The two
> cases share basically no code, and there's only one callsite.
I noticed that too but wanted to get the dedup reviewed first.
I'll respin including the split (which should remove the mid-block
declaration).
Thanks for the review,
Phil.
On 23.08.21 10:54, Philippe Mathieu-Daudé wrote:
> Per Peter Maydell [*]:
>
> 'info mtree' monitor command was designed on the assumption that
> there's really only one or two interesting address spaces, and
> with more recent developments that's just not the case any more.
>
> Similarly about how the FlatView are sorted using a GHashTable,
> sort the AddressSpace objects to remove the duplications (AS
> using the same root MemoryRegion).
>
> This drastically reduce 'info mtree' on some boards.
s/reduce/reduces the output of/
>
> Before:
>
> $ (echo info mtree; echo q) \
> | qemu-system-aarch64 -S -monitor stdio -M raspi3b \
> | wc -l
> 423
>
> After:
>
> $ (echo info mtree; echo q) \
> | qemu-system-aarch64 -S -monitor stdio -M raspi3b \
> | wc -l
> 106
>
> (qemu) info mtree
> address-space: I/O
> 0000000000000000-000000000000ffff (prio 0, i/o): io
>
> 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
We can still distinguish from a completely empty AS, because we don't
have an empty line here, correct?
[...]
> ---
> softmmu/memory.c | 64 +++++++++++++++++++++++++++++++++++++++++++++---
> 1 file changed, 60 insertions(+), 4 deletions(-)
>
> diff --git a/softmmu/memory.c b/softmmu/memory.c
> index bfedaf9c4df..459d6246672 100644
> --- a/softmmu/memory.c
> +++ b/softmmu/memory.c
> @@ -3246,11 +3246,55 @@ static gboolean mtree_info_flatview_free(gpointer key, gpointer value,
> return true;
> }
>
> +struct AddressSpaceInfo {
> + MemoryRegionListHead *ml_head;
> + bool owner;
> + bool disabled;
> +};
> +
> +/* Returns negative value if a < b; zero if a = b; positive value if a > b. */
> +static gint address_space_compare_name(gconstpointer a, gconstpointer b)
> +{
> + const AddressSpace *as_a = a;
> + const AddressSpace *as_b = b;
> +
> + return g_strcmp0(as_a->name, as_b->name);
> +}
> +static void mtree_print_as_name(gpointer data, gpointer user_data)
> +{
> + AddressSpace *as = data;
> +
> + qemu_printf("address-space: %s\n", as->name);
> +}
> +
> +static void mtree_print_as(gpointer key, gpointer value, gpointer user_data)
> +{
> + MemoryRegion *mr = key;
> + GSList *as_same_root_mr_list = value;
> + struct AddressSpaceInfo *asi = user_data;
Reverse Christmas tree?
> +
> + 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);
> + qemu_printf("\n");
> +}
> +
> +static gboolean mtree_info_as_free(gpointer key, gpointer value,
> + gpointer user_data)
> +{
> + GSList *as_same_root_mr_list = value;
> +
> + g_slist_free(as_same_root_mr_list);
> +
> + return true;
> +}
> +
> void mtree_info(bool flatview, bool dispatch_tree, bool owner, bool disabled)
> {
> MemoryRegionListHead ml_head;
> MemoryRegionList *ml, *ml2;
> AddressSpace *as;
> + GHashTable *views = g_hash_table_new(g_direct_hash, g_direct_equal);
> + GSList *as_same_root_mr_list;
Reverse Christmas tree ?
>
> if (flatview) {
> FlatView *view;
> @@ -3260,7 +3304,6 @@ void mtree_info(bool flatview, bool dispatch_tree, bool owner, bool disabled)
> .owner = owner,
> };
> GArray *fv_address_spaces;
> - GHashTable *views = g_hash_table_new(g_direct_hash, g_direct_equal);
> AccelClass *ac = ACCEL_GET_CLASS(current_accel());
>
> if (ac->has_memory) {
> @@ -3293,11 +3336,24 @@ void mtree_info(bool flatview, bool dispatch_tree, bool owner, bool disabled)
> QTAILQ_INIT(&ml_head);
>
> QTAILQ_FOREACH(as, &address_spaces, address_spaces_link) {
> - qemu_printf("address-space: %s\n", as->name);
> - mtree_print_mr(as->root, 1, 0, &ml_head, owner, disabled);
> - qemu_printf("\n");
> + /* Create hashtable, key=AS root MR, value = list of AS */
> + as_same_root_mr_list = g_hash_table_lookup(views, as->root);
> + as_same_root_mr_list = g_slist_insert_sorted(as_same_root_mr_list, as,
> + address_space_compare_name);
> + g_hash_table_insert(views, as->root, as_same_root_mr_list);
> }
>
> + struct AddressSpaceInfo asi = {
> + .ml_head = &ml_head,
> + .owner = owner,
> + .disabled = disabled,
> + };
> +
> + /* print address spaces */
> + g_hash_table_foreach(views, mtree_print_as, &asi);
> + g_hash_table_foreach_remove(views, mtree_info_as_free, 0);
> + g_hash_table_unref(views);
> +
> /* print aliased regions */
> QTAILQ_FOREACH(ml, &ml_head, mrqueue) {
> qemu_printf("memory-region: %s\n", memory_region_name(ml->mr));
>
LGTM
--
Thanks,
David / dhildenb
On Mon, 23 Aug 2021 at 10:20, David Hildenbrand <david@redhat.com> wrote:
>
> On 23.08.21 10:54, Philippe Mathieu-Daudé wrote:
> > +static void mtree_print_as(gpointer key, gpointer value, gpointer user_data)
> > +{
> > + MemoryRegion *mr = key;
> > + GSList *as_same_root_mr_list = value;
> > + struct AddressSpaceInfo *asi = user_data;
>
> Reverse Christmas tree?
This has never been part of the QEMU style guidelines
and I would oppose our adding it. It would gain us very little,
the codebase doesn't consistently follow that rule today so
it wouldn't be preserving an existing consistency of style,
and it would be yet another weird stylistic issue that trips
people up and requires patch repins.
-- PMM
On 23.08.21 11:28, Peter Maydell wrote:
> On Mon, 23 Aug 2021 at 10:20, David Hildenbrand <david@redhat.com> wrote:
>>
>> On 23.08.21 10:54, Philippe Mathieu-Daudé wrote:
>>> +static void mtree_print_as(gpointer key, gpointer value, gpointer user_data)
>>> +{
>>> + MemoryRegion *mr = key;
>>> + GSList *as_same_root_mr_list = value;
>>> + struct AddressSpaceInfo *asi = user_data;
>>
>> Reverse Christmas tree?
>
> This has never been part of the QEMU style guidelines
> and I would oppose our adding it. It would gain us very little,
> the codebase doesn't consistently follow that rule today so
> it wouldn't be preserving an existing consistency of style,
> and it would be yet another weird stylistic issue that trips
> people up and requires patch repins.
Ah right, it used very inconsistently in the QEMU codebase and even in
this file (I spotted it's the case in the entry of mtree_info() and
wondered if it's the case for this file -- turns out it's absolutely not).
--
Thanks,
David / dhildenb
On 8/23/21 11:35 AM, David Hildenbrand wrote:
> On 23.08.21 11:28, Peter Maydell wrote:
>> On Mon, 23 Aug 2021 at 10:20, David Hildenbrand <david@redhat.com> wrote:
>>>
>>> On 23.08.21 10:54, Philippe Mathieu-Daudé wrote:
>>>> +static void mtree_print_as(gpointer key, gpointer value, gpointer
>>>> user_data)
>>>> +{
>>>> + MemoryRegion *mr = key;
>>>> + GSList *as_same_root_mr_list = value;
>>>> + struct AddressSpaceInfo *asi = user_data;
>>>
>>> Reverse Christmas tree?
I simply followed to order of the arguments as a no-brainer ;)
>> This has never been part of the QEMU style guidelines
>> and I would oppose our adding it. It would gain us very little,
>> the codebase doesn't consistently follow that rule today so
>> it wouldn't be preserving an existing consistency of style,
>> and it would be yet another weird stylistic issue that trips
>> people up and requires patch repins.
(in this particular case I've to respin for a typo).
> Ah right, it used very inconsistently in the QEMU codebase and even in
> this file (I spotted it's the case in the entry of mtree_info() and
> wondered if it's the case for this file -- turns out it's absolutely not).
On 8/23/21 11:20 AM, David Hildenbrand wrote:
> On 23.08.21 10:54, Philippe Mathieu-Daudé wrote:
>> Per Peter Maydell [*]:
>>
>> 'info mtree' monitor command was designed on the assumption that
>> there's really only one or two interesting address spaces, and
>> with more recent developments that's just not the case any more.
>>
>> Similarly about how the FlatView are sorted using a GHashTable,
>> sort the AddressSpace objects to remove the duplications (AS
>> using the same root MemoryRegion).
>>
>> This drastically reduce 'info mtree' on some boards.
>
> s/reduce/reduces the output of/
>
>>
>> Before:
>>
>> $ (echo info mtree; echo q) \
>> | qemu-system-aarch64 -S -monitor stdio -M raspi3b \
>> | wc -l
>> 423
>>
>> After:
>>
>> $ (echo info mtree; echo q) \
>> | qemu-system-aarch64 -S -monitor stdio -M raspi3b \
>> | wc -l
>> 106
>>
>> (qemu) info mtree
>> address-space: I/O
>> 0000000000000000-000000000000ffff (prio 0, i/o): io
>>
>> 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
>
> We can still distinguish from a completely empty AS, because we don't
> have an empty line here, correct?
Yes:
(qemu) info mtree
address-space: I/O
0000000000000000-000000000000ffff (prio 0, i/o): io
address-space shared 4 times:
- bcm2835-dma-memory
- bcm2835-fb-memory
- bcm2835-property-memory
- dwc2
0000000000000000-00000000ffffffff (prio 0, i/o): bcm2835-gpu
0000000000000000-000000003fffffff (prio 0, ram): alias
bcm2835-gpu-ram-alias[*] @ram 0000000000000000-000000003fffffff
0000000040000000-000000007fffffff (prio 0, ram): alias
bcm2835-gpu-ram-alias[*] @ram 0000000000000000-000000003fffffff
000000007e000000-000000007effffff (prio 1, i/o): alias
bcm2835-peripherals @bcm2835-peripherals 0000000000000000-0000000000ffffff
0000000080000000-00000000bfffffff (prio 0, ram): alias
bcm2835-gpu-ram-alias[*] @ram 0000000000000000-000000003fffffff
00000000c0000000-00000000ffffffff (prio 0, ram): alias
bcm2835-gpu-ram-alias[*] @ram 0000000000000000-000000003fffffff
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
[...]
On 9/1/21 6:01 PM, Philippe Mathieu-Daudé wrote: > On 8/23/21 11:20 AM, David Hildenbrand wrote: >> On 23.08.21 10:54, Philippe Mathieu-Daudé wrote: >>> Per Peter Maydell [*]: >>> >>> 'info mtree' monitor command was designed on the assumption that >>> there's really only one or two interesting address spaces, and >>> with more recent developments that's just not the case any more. >>> >>> Similarly about how the FlatView are sorted using a GHashTable, >>> sort the AddressSpace objects to remove the duplications (AS >>> using the same root MemoryRegion). >>> >>> This drastically reduce 'info mtree' on some boards. >> >> s/reduce/reduces the output of/ >> >>> >>> Before: >>> >>> $ (echo info mtree; echo q) \ >>> | qemu-system-aarch64 -S -monitor stdio -M raspi3b \ >>> | wc -l >>> 423 >>> >>> After: >>> >>> $ (echo info mtree; echo q) \ >>> | qemu-system-aarch64 -S -monitor stdio -M raspi3b \ >>> | wc -l >>> 106 >>> >>> (qemu) info mtree >>> address-space: I/O >>> 0000000000000000-000000000000ffff (prio 0, i/o): io >>> >>> 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 >> >> We can still distinguish from a completely empty AS, because we don't >> have an empty line here, correct? > > Yes: > > (qemu) info mtree > address-space: I/O > 0000000000000000-000000000000ffff (prio 0, i/o): io Wrong answer because there is 1 MR here. We can create address_space_init() with NULL MR, because memory_region_ref() checks for NULL mr, but QEMU aborts quickly: (gdb) bt #0 memory_region_get_flatview_root (mr=0x0) at softmmu/memory.c:685 #1 0x0000555555eec7ef in address_space_update_topology (as=0x5555567f2a20 <address_space_io>) at softmmu/memory.c:1073 #2 address_space_init (as=0x5555567f2a20 <address_space_io2>, root=0x0, name=<optimized out>) at softmmu/memory.c:2934 #3 0x0000555555edc7a9 in memory_map_init () at softmmu/physmem.c:2672 #4 cpu_exec_init_all () at softmmu/physmem.c:3070 #5 0x0000555555ef5480 in qemu_create_machine (qdict=0x7fffffffcec0) at softmmu/vl.c:2126 #6 qemu_init (argc=<optimized out>, argv=0x7fffffffd0e8, envp=<optimized out>) at softmmu/vl.c:3639 #7 0x00005555559c2fe9 in main (argc=<optimized out>, argv=<optimized out>, envp=<optimized out>) at softmmu/main.c:49 What is your "completely empty AS" case?
© 2016 - 2024 Red Hat, Inc.