[Qemu-devel] [PATCH qemu v2 0/2] memory/hmp: Print owners/parents in "info mtree"

[Qemu-devel] [PATCH qemu v2 0/2] memory/hmp: Print owners/parents in "info mtree"

[Alexey Kardashevskiy]

This is a debug extension to "into mtree" to print a memory region owner/parent.

This is based on sha1
4743c23 Peter Maydell "Update version for v2.12.0 release".

Please comment. Thanks.



Alexey Kardashevskiy (2):
  memory/hmp: Print owners/parents in "info mtree"
  object: Handle objects with no parents

 include/exec/memory.h |  2 +-
 memory.c              | 69 +++++++++++++++++++++++++++++++++++++++++++--------
 monitor.c             |  4 ++-
 qom/object.c          |  2 +-
 hmp-commands-info.hx  |  7 +++---
 5 files changed, 68 insertions(+), 16 deletions(-)

-- 
2.11.0

[Qemu-devel] [PATCH qemu v2 1/2] memory/hmp: Print owners/parents in "info mtree"

[Alexey Kardashevskiy]

This adds owners/parents (which are the same, just occasionally
owner==NULL) printing for memory regions; a new '-o' flag
enabled new output.

Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
---
Changes:
v2:
* cleanups
---
 include/exec/memory.h |  2 +-
 memory.c              | 69 +++++++++++++++++++++++++++++++++++++++++++--------
 monitor.c             |  4 ++-
 hmp-commands-info.hx  |  7 +++---
 4 files changed, 67 insertions(+), 15 deletions(-)

diff --git a/include/exec/memory.h b/include/exec/memory.h
index 31eae0a..3e9d67c 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -1541,7 +1541,7 @@ void memory_global_dirty_log_start(void);
 void memory_global_dirty_log_stop(void);
 
 void mtree_info(fprintf_function mon_printf, void *f, bool flatview,
-                bool dispatch_tree);
+                bool dispatch_tree, bool owner);
 
 /**
  * memory_region_request_mmio_ptr: request a pointer to an mmio
diff --git a/memory.c b/memory.c
index e70b64b..230d905 100644
--- a/memory.c
+++ b/memory.c
@@ -2830,10 +2830,46 @@ typedef QTAILQ_HEAD(mrqueue, MemoryRegionList) MemoryRegionListHead;
                            int128_sub((size), int128_one())) : 0)
 #define MTREE_INDENT "  "
 
+static void mtree_expand_owner(fprintf_function mon_printf, void *f,
+                               const char *label, Object *obj)
+{
+    DeviceState *dev = (DeviceState *) object_dynamic_cast(obj, TYPE_DEVICE);
+    const char *id = object_property_print(obj, "id", true, NULL);
+
+    mon_printf(f, " %s:{%s", label, dev ? "dev" : "obj");
+    if (dev ? dev->id : id) {
+        mon_printf(f, " id=%s", dev ? dev->id : id);
+    } else {
+        gchar *canonical_path = object_get_canonical_path(obj);
+        mon_printf(f, " path=%s", canonical_path);
+        g_free(canonical_path);
+    }
+    mon_printf(f, "}");
+}
+
+static void mtree_print_mr_owner(fprintf_function mon_printf, void *f,
+                                 const MemoryRegion *mr)
+{
+    Object *owner = mr->owner;
+    Object *parent = memory_region_owner((MemoryRegion *)mr);
+
+    if (!owner && !parent) {
+        mon_printf(f, " orphan");
+        return;
+    }
+    if (owner) {
+        mtree_expand_owner(mon_printf, f, "owner", owner);
+    }
+    if (parent && parent != owner) {
+        mtree_expand_owner(mon_printf, f, "parent", parent);
+    }
+}
+
 static void mtree_print_mr(fprintf_function mon_printf, void *f,
                            const MemoryRegion *mr, unsigned int level,
                            hwaddr base,
-                           MemoryRegionListHead *alias_print_queue)
+                           MemoryRegionListHead *alias_print_queue,
+                           bool owner)
 {
     MemoryRegionList *new_ml, *ml, *next_ml;
     MemoryRegionListHead submr_print_queue;
@@ -2879,7 +2915,7 @@ static void mtree_print_mr(fprintf_function mon_printf, void *f,
         }
         mon_printf(f, TARGET_FMT_plx "-" TARGET_FMT_plx
                    " (prio %d, %s): alias %s @%s " TARGET_FMT_plx
-                   "-" TARGET_FMT_plx "%s\n",
+                   "-" TARGET_FMT_plx "%s",
                    cur_start, cur_end,
                    mr->priority,
                    memory_region_type((MemoryRegion *)mr),
@@ -2888,15 +2924,22 @@ static void mtree_print_mr(fprintf_function mon_printf, void *f,
                    mr->alias_offset,
                    mr->alias_offset + MR_SIZE(mr->size),
                    mr->enabled ? "" : " [disabled]");
+        if (owner) {
+            mtree_print_mr_owner(mon_printf, f, mr);
+        }
     } else {
         mon_printf(f,
-                   TARGET_FMT_plx "-" TARGET_FMT_plx " (prio %d, %s): %s%s\n",
+                   TARGET_FMT_plx "-" TARGET_FMT_plx " (prio %d, %s): %s%s",
                    cur_start, cur_end,
                    mr->priority,
                    memory_region_type((MemoryRegion *)mr),
                    memory_region_name(mr),
                    mr->enabled ? "" : " [disabled]");
+        if (owner) {
+            mtree_print_mr_owner(mon_printf, f, mr);
+        }
     }
+    mon_printf(f, "\n");
 
     QTAILQ_INIT(&submr_print_queue);
 
@@ -2919,7 +2962,7 @@ static void mtree_print_mr(fprintf_function mon_printf, void *f,
 
     QTAILQ_FOREACH(ml, &submr_print_queue, mrqueue) {
         mtree_print_mr(mon_printf, f, ml->mr, level + 1, cur_start,
-                       alias_print_queue);
+                       alias_print_queue, owner);
     }
 
     QTAILQ_FOREACH_SAFE(ml, &submr_print_queue, mrqueue, next_ml) {
@@ -2932,6 +2975,7 @@ struct FlatViewInfo {
     void *f;
     int counter;
     bool dispatch_tree;
+    bool owner;
 };
 
 static void mtree_print_flatview(gpointer key, gpointer value,
@@ -2972,7 +3016,7 @@ static void mtree_print_flatview(gpointer key, gpointer value,
         mr = range->mr;
         if (range->offset_in_region) {
             p(f, MTREE_INDENT TARGET_FMT_plx "-"
-              TARGET_FMT_plx " (prio %d, %s): %s @" TARGET_FMT_plx "\n",
+              TARGET_FMT_plx " (prio %d, %s): %s @" TARGET_FMT_plx,
               int128_get64(range->addr.start),
               int128_get64(range->addr.start) + MR_SIZE(range->addr.size),
               mr->priority,
@@ -2981,13 +3025,17 @@ static void mtree_print_flatview(gpointer key, gpointer value,
               range->offset_in_region);
         } else {
             p(f, MTREE_INDENT TARGET_FMT_plx "-"
-              TARGET_FMT_plx " (prio %d, %s): %s\n",
+              TARGET_FMT_plx " (prio %d, %s): %s",
               int128_get64(range->addr.start),
               int128_get64(range->addr.start) + MR_SIZE(range->addr.size),
               mr->priority,
               range->readonly ? "rom" : memory_region_type(mr),
               memory_region_name(mr));
         }
+        if (fvi->owner) {
+            mtree_print_mr_owner(p, f, mr);
+        }
+        p(f, "\n");
         range++;
     }
 
@@ -3013,7 +3061,7 @@ static gboolean mtree_info_flatview_free(gpointer key, gpointer value,
 }
 
 void mtree_info(fprintf_function mon_printf, void *f, bool flatview,
-                bool dispatch_tree)
+                bool dispatch_tree, bool owner)
 {
     MemoryRegionListHead ml_head;
     MemoryRegionList *ml, *ml2;
@@ -3025,7 +3073,8 @@ void mtree_info(fprintf_function mon_printf, void *f, bool flatview,
             .mon_printf = mon_printf,
             .f = f,
             .counter = 0,
-            .dispatch_tree = dispatch_tree
+            .dispatch_tree = dispatch_tree,
+            .owner = owner,
         };
         GArray *fv_address_spaces;
         GHashTable *views = g_hash_table_new(g_direct_hash, g_direct_equal);
@@ -3057,14 +3106,14 @@ void mtree_info(fprintf_function mon_printf, void *f, bool flatview,
 
     QTAILQ_FOREACH(as, &address_spaces, address_spaces_link) {
         mon_printf(f, "address-space: %s\n", as->name);
-        mtree_print_mr(mon_printf, f, as->root, 1, 0, &ml_head);
+        mtree_print_mr(mon_printf, f, as->root, 1, 0, &ml_head, owner);
         mon_printf(f, "\n");
     }
 
     /* print aliased regions */
     QTAILQ_FOREACH(ml, &ml_head, mrqueue) {
         mon_printf(f, "memory-region: %s\n", memory_region_name(ml->mr));
-        mtree_print_mr(mon_printf, f, ml->mr, 1, 0, &ml_head);
+        mtree_print_mr(mon_printf, f, ml->mr, 1, 0, &ml_head, owner);
         mon_printf(f, "\n");
     }
 
diff --git a/monitor.c b/monitor.c
index 39f8ee1..81a534d 100644
--- a/monitor.c
+++ b/monitor.c
@@ -1969,8 +1969,10 @@ static void hmp_info_mtree(Monitor *mon, const QDict *qdict)
 {
     bool flatview = qdict_get_try_bool(qdict, "flatview", false);
     bool dispatch_tree = qdict_get_try_bool(qdict, "dispatch_tree", false);
+    bool owner = qdict_get_try_bool(qdict, "owner", false);
 
-    mtree_info((fprintf_function)monitor_printf, mon, flatview, dispatch_tree);
+    mtree_info((fprintf_function)monitor_printf, mon, flatview, dispatch_tree,
+               owner);
 }
 
 static void hmp_info_numa(Monitor *mon, const QDict *qdict)
diff --git a/hmp-commands-info.hx b/hmp-commands-info.hx
index ddfcd5a..5956495 100644
--- a/hmp-commands-info.hx
+++ b/hmp-commands-info.hx
@@ -250,10 +250,11 @@ ETEXI
 
     {
         .name       = "mtree",
-        .args_type  = "flatview:-f,dispatch_tree:-d",
-        .params     = "[-f][-d]",
+        .args_type  = "flatview:-f,dispatch_tree:-d,owner:-o",
+        .params     = "[-f][-d][-o]",
         .help       = "show memory tree (-f: dump flat view for address spaces;"
-                      "-d: dump dispatch tree, valid with -f only)",
+                      "-d: dump dispatch tree, valid with -f only);"
+                      "-o: dump region owners/parents",
         .cmd        = hmp_info_mtree,
     },
 
-- 
2.11.0

Re: [Qemu-devel] [PATCH qemu v2 1/2] memory/hmp: Print owners/parents in "info mtree"

[Paolo Bonzini]

On 30/04/2018 08:25, Alexey Kardashevskiy wrote:
> +    DeviceState *dev = (DeviceState *) object_dynamic_cast(obj, TYPE_DEVICE);
> +    const char *id = object_property_print(obj, "id", true, NULL);

The only objects that have an "id" property are memdevs.  If you want to
special case their printing too, it's probably a good idea (that is,
print one of "dev id=ID"/"memdev id=ID"/"obj path=PATH").

Otherwise, I can also queue this patch as is, but I'd remove the "id"
property handling because I'm going to submit a small series to remove
the "id" property altogether.

Let me know what you prefer!

Thanks,

Paolo

> +    mon_printf(f, " %s:{%s", label, dev ? "dev" : "obj");
> +    if (dev ? dev->id : id) {
> +        mon_printf(f, " id=%s", dev ? dev->id : id);
> +    } else {
> +        gchar *canonical_path = object_get_canonical_path(obj);
> +        mon_printf(f, " path=%s", canonical_path);
> +        g_free(canonical_path);
> +    }
> +    mon_printf(f, "}");

Re: [Qemu-devel] [PATCH qemu v2 1/2] memory/hmp: Print owners/parents in "info mtree"

[Alexey Kardashevskiy]

On 30/4/18 7:53 pm, Paolo Bonzini wrote:
> On 30/04/2018 08:25, Alexey Kardashevskiy wrote:
>> +    DeviceState *dev = (DeviceState *) object_dynamic_cast(obj, TYPE_DEVICE);
>> +    const char *id = object_property_print(obj, "id", true, NULL);
> 
> The only objects that have an "id" property are memdevs.  If you want to
> special case their printing too, it's probably a good idea (that is,
> print one of "dev id=ID"/"memdev id=ID"/"obj path=PATH").
> 
> Otherwise, I can also queue this patch as is, but I'd remove the "id"
> property handling because I'm going to submit a small series to remove
> the "id" property altogether.
> 
> Let me know what you prefer!


I choose to wait and repost, thanks :)


> 
> Thanks,
> 
> Paolo
> 
>> +    mon_printf(f, " %s:{%s", label, dev ? "dev" : "obj");
>> +    if (dev ? dev->id : id) {
>> +        mon_printf(f, " id=%s", dev ? dev->id : id);
>> +    } else {
>> +        gchar *canonical_path = object_get_canonical_path(obj);
>> +        mon_printf(f, " path=%s", canonical_path);
>> +        g_free(canonical_path);
>> +    }
>> +    mon_printf(f, "}");
> 


-- 
Alexey

Re: [Qemu-devel] [PATCH qemu v2 1/2] memory/hmp: Print owners/parents in "info mtree"

[Alexey Kardashevskiy]

On 3/5/18 3:40 pm, Alexey Kardashevskiy wrote:
> On 30/4/18 7:53 pm, Paolo Bonzini wrote:
>> On 30/04/2018 08:25, Alexey Kardashevskiy wrote:
>>> +    DeviceState *dev = (DeviceState *) object_dynamic_cast(obj, TYPE_DEVICE);
>>> +    const char *id = object_property_print(obj, "id", true, NULL);
>>
>> The only objects that have an "id" property are memdevs.  If you want to
>> special case their printing too, it's probably a good idea (that is,
>> print one of "dev id=ID"/"memdev id=ID"/"obj path=PATH").
>>
>> Otherwise, I can also queue this patch as is, but I'd remove the "id"
>> property handling because I'm going to submit a small series to remove
>> the "id" property altogether.
>>
>> Let me know what you prefer!
> 
> 
> I choose to wait and repost, thanks :)

I waited, checked https://git.qemu.org/?p=qemu.git;a=commitdiff;h=29de4e
that "id" is no more and then grepped:

hw/intc/apic_common.c|489| object_property_add(obj, "id", "uint32",
hw/ppc/spapr_drc.c|557| object_property_add_uint32_ptr(obj, "id", &drc->id,
NULL);

This does not look like "remove the "id" property altogether" :) Does this
mean we still rather want to print QOM's "id"? spapr_drc does not own MRs
and APIC seems not to either.



> 
> 
>>
>> Thanks,
>>
>> Paolo
>>
>>> +    mon_printf(f, " %s:{%s", label, dev ? "dev" : "obj");
>>> +    if (dev ? dev->id : id) {
>>> +        mon_printf(f, " id=%s", dev ? dev->id : id);
>>> +    } else {
>>> +        gchar *canonical_path = object_get_canonical_path(obj);
>>> +        mon_printf(f, " path=%s", canonical_path);
>>> +        g_free(canonical_path);
>>> +    }
>>> +    mon_printf(f, "}");
>>
> 
> 


-- 
Alexey

Re: [Qemu-devel] [PATCH qemu v2 1/2] memory/hmp: Print owners/parents in "info mtree"

[Paolo Bonzini]

On 30/05/2018 06:57, Alexey Kardashevskiy wrote:
> hw/intc/apic_common.c|489| object_property_add(obj, "id", "uint32",
> hw/ppc/spapr_drc.c|557| object_property_add_uint32_ptr(obj, "id", &drc->id,
> NULL);
> 
> This does not look like "remove the "id" property altogether" :) Does this
> mean we still rather want to print QOM's "id"? spapr_drc does not own MRs
> and APIC seems not to either.

No, those properties are specific to some devices (and in fact they are
integers rather than strings).  The id property that mirrors the path
component is gone.

Paolo

[Qemu-devel] [PATCH qemu v2 2/2] object: Handle objects with no parents

[Alexey Kardashevskiy]

At the moment object_get_canonical_path_component() crashes on assert()
if the object does not have a parent. Usually it is not called for
orphan objects but various HMP/QMP commands can do that (info mtree,
qom-get).

This adds few more tests in object_get_canonical_path() to prevent QEMU
from crashing.

Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
---
 qom/object.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/qom/object.c b/qom/object.c
index 4677951..e0e300b 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -1668,7 +1668,7 @@ gchar *object_get_canonical_path(Object *obj)
     Object *root = object_get_root();
     char *newpath, *path = NULL;
 
-    while (obj != root) {
+    while (obj && obj->parent && obj != root) {
         char *component = object_get_canonical_path_component(obj);
 
         if (path) {
-- 
2.11.0

Re: [Qemu-devel] [PATCH qemu v2 2/2] object: Handle objects with no parents

[Paolo Bonzini]

On 30/04/2018 08:25, Alexey Kardashevskiy wrote:
> At the moment object_get_canonical_path_component() crashes on assert()
> if the object does not have a parent. Usually it is not called for
> orphan objects but various HMP/QMP commands can do that (info mtree,
> qom-get).
> 
> This adds few more tests in object_get_canonical_path() to prevent QEMU
> from crashing.
> 
> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> ---
>  qom/object.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/qom/object.c b/qom/object.c
> index 4677951..e0e300b 100644
> --- a/qom/object.c
> +++ b/qom/object.c
> @@ -1668,7 +1668,7 @@ gchar *object_get_canonical_path(Object *obj)
>      Object *root = object_get_root();
>      char *newpath, *path = NULL;
>  
> -    while (obj != root) {
> +    while (obj && obj->parent && obj != root) {
>          char *component = object_get_canonical_path_component(obj);
>  
>          if (path) {

I think the patch is a good idea, but as it is written it is incorrect,
because it will return an invalid canonical path.  You should return
NULL instead.

Also, checking both obj and obj->parent is unnecessary; if obj->parent
is NULL, obj will be NULL on the next iteration.

Thanks,

Paolo