On Mon, 23 Aug 2021 at 09:54, Philippe Mathieu-Daudé <phi...@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.mayd...@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
