include/exec/memory.h | 2 +- memory.c | 80 ++++++++++++++++++++++++++++++++++++++++++++------- monitor.c | 4 ++- hmp-commands-info.hx | 7 +++-- 4 files changed, 78 insertions(+), 15 deletions(-)
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>
---
Does this look anything useful?
There are cases ("msi", "msix-table", "msix-pba" and probably more) when
it is not clear what owns an MR while they all have an owner (always? mostly?).
"info mtree" example:
address-space: memory
0000000000000000-ffffffffffffffff (prio 0, i/o): system parent:{obj}
0000000000000000-000000007fffffff (prio 0, ram): ppc_spapr.ram parent:{obj}
0000200000000000-000020000000ffff (prio 0, i/o): alias pci@800000020000000.io-alias @pci@800000020000000.io 0000000000000000-000000000000ffff owner:{dev path=/machine/unattached/device[3]}
0000200080000000-00002000ffffffff (prio 0, i/o): alias pci@800000020000000.mmio32-alias @pci@800000020000000.mmio 0000000080000000-00000000ffffffff owner:{dev path=/machine/unattached/device[3]}
0000210000000000-000021ffffffffff (prio 0, i/o): alias pci@800000020000000.mmio64-alias @pci@800000020000000.mmio 0000210000000000-000021ffffffffff owner:{dev path=/machine/unattached/device[3]}
address-space: I/O
0000000000000000-000000000000ffff (prio 0, i/o): io parent:{obj}
address-space: cpu-memory-0
0000000000000000-ffffffffffffffff (prio 0, i/o): system parent:{obj}
0000000000000000-000000007fffffff (prio 0, ram): ppc_spapr.ram parent:{obj}
0000200000000000-000020000000ffff (prio 0, i/o): alias pci@800000020000000.io-alias @pci@800000020000000.io 0000000000000000-000000000000ffff owner:{dev path=/machine/unattached/device[3]}
0000200080000000-00002000ffffffff (prio 0, i/o): alias pci@800000020000000.mmio32-alias @pci@800000020000000.mmio 0000000080000000-00000000ffffffff owner:{dev path=/machine/unattached/device[3]}
0000210000000000-000021ffffffffff (prio 0, i/o): alias pci@800000020000000.mmio64-alias @pci@800000020000000.mmio 0000210000000000-000021ffffffffff owner:{dev path=/machine/unattached/device[3]}
address-space: pci@800000020000000
0000000000000000-ffffffffffffffff (prio 0, i/o): pci@800000020000000.iommu-root owner:{dev path=/machine/unattached/device[3]}
0000000000000000-ffffffffffffffff (prio 0, i/o): tce-root-80000001 owner:{dev path=/machine/unattached/device[3]/tce-table-80000001}
0800000000000000-080000007fffffff (prio 0, i/o): tce-iommu-80000001 owner:{dev path=/machine/unattached/device[3]/tce-table-80000001}
0000000000000000-ffffffffffffffff (prio 0, i/o): tce-root-80000000 owner:{dev path=/machine/unattached/device[3]/tce-table-80000000}
0000000000000000-000000003fffffff (prio 0, i/o): tce-iommu-80000000 owner:{dev path=/machine/unattached/device[3]/tce-table-80000000}
0000040000000000-000004000000ffff (prio 0, i/o): msi owner:{dev path=/machine/unattached/device[3]}
address-space: vfio-pci
0000000000000000-ffffffffffffffff (prio 0, i/o): bus master container owner:{dev id=vfio0001_03_00_0}
0000000000000000-ffffffffffffffff (prio 0, i/o): alias bus master @pci@800000020000000.iommu-root 0000000000000000-ffffffffffffffff owner:{dev id=vfio0001_03_00_0}
memory-region: pci@800000020000000.io
0000000000000000-000000000000ffff (prio 0, i/o): pci@800000020000000.io owner:{dev path=/machine/unattached/device[3]}
memory-region: pci@800000020000000.mmio
0000000000000000-ffffffffffffffff (prio 0, i/o): pci@800000020000000.mmio owner:{dev path=/machine/unattached/device[3]}
0000210000000000-000021000000ffff (prio 1, i/o): 0001:03:00.0 base BAR 1 owner:{dev id=vfio0001_03_00_0}
0000210000000000-000021000000ffff (prio 0, i/o): 0001:03:00.0 BAR 1 owner:{dev id=vfio0001_03_00_0}
000021000000e000-000021000000e5ff (prio 0, i/o): msix-table owner:{dev id=vfio0001_03_00_0}
000021000000f000-000021000000f00f (prio 0, i/o): msix-pba [disabled] owner:{dev id=vfio0001_03_00_0}
0000210000040000-000021000007ffff (prio 1, i/o): 0001:03:00.0 base BAR 3 owner:{dev id=vfio0001_03_00_0}
0000210000040000-000021000007ffff (prio 0, i/o): 0001:03:00.0 BAR 3 owner:{dev id=vfio0001_03_00_0}
0000210000040000-000021000007ffff (prio 0, ramd): 0001:03:00.0 BAR 3 mmaps[0] owner:{dev id=vfio0001_03_00_0}
memory-region: pci@800000020000000.iommu-root
0000000000000000-ffffffffffffffff (prio 0, i/o): pci@800000020000000.iommu-root owner:{dev path=/machine/unattached/device[3]}
0000000000000000-ffffffffffffffff (prio 0, i/o): tce-root-80000001 owner:{dev path=/machine/unattached/device[3]/tce-table-80000001}
0800000000000000-080000007fffffff (prio 0, i/o): tce-iommu-80000001 owner:{dev path=/machine/unattached/device[3]/tce-table-80000001}
0000000000000000-ffffffffffffffff (prio 0, i/o): tce-root-80000000 owner:{dev path=/machine/unattached/device[3]/tce-table-80000000}
0000000000000000-000000003fffffff (prio 0, i/o): tce-iommu-80000000 owner:{dev path=/machine/unattached/device[3]/tce-table-80000000}
0000040000000000-000004000000ffff (prio 0, i/o): msi owner:{dev path=/machine/unattached/device[3]}
Flatviews:
FlatView #0
AS "pci@800000020000000", root: pci@800000020000000.iommu-root
AS "vfio-pci", root: bus master container
Root memory region: pci@800000020000000.iommu-root
0000000000000000-000000003fffffff (prio 0, i/o): tce-iommu-80000000 owner:{dev path=/machine/unattached/device[3]/tce-table-80000000}
0000040000000000-000004000000ffff (prio 0, i/o): msi owner:{dev path=/machine/unattached/device[3]}
0800000000000000-080000007fffffff (prio 0, i/o): tce-iommu-80000001 owner:{dev path=/machine/unattached/device[3]/tce-table-80000001}
FlatView #1
AS "memory", root: system
AS "cpu-memory-0", root: system
Root memory region: system
0000000000000000-000000007fffffff (prio 0, ram): ppc_spapr.ram parent:{obj}
0000210000000000-000021000000dfff (prio 0, i/o): 0001:03:00.0 BAR 1 owner:{dev id=vfio0001_03_00_0}
000021000000e000-000021000000e5ff (prio 0, i/o): msix-table owner:{dev id=vfio0001_03_00_0}
000021000000e600-000021000000ffff (prio 0, i/o): 0001:03:00.0 BAR 1 @000000000000e600 owner:{dev id=vfio0001_03_00_0}
0000210000040000-000021000007ffff (prio 0, ramd): 0001:03:00.0 BAR 3 mmaps[0] owner:{dev id=vfio0001_03_00_0}
FlatView #2
AS "I/O", root: io
Root memory region: io
0000000000000000-000000000000ffff (prio 0, i/o): io parent:{obj}
---
include/exec/memory.h | 2 +-
memory.c | 80 ++++++++++++++++++++++++++++++++++++++++++++-------
monitor.c | 4 ++-
hmp-commands-info.hx | 7 +++--
4 files changed, 78 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..538a57c 100644
--- a/memory.c
+++ b/memory.c
@@ -2830,10 +2830,57 @@ 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);
+ const char *name = object_property_print(obj, "name", true, NULL);
+
+ mon_printf(f, " %s:{", label);
+ if (dev) {
+ mon_printf(f, "dev");
+ if (dev->id) {
+ mon_printf(f, " id=%s", dev->id);
+ }
+ } else {
+ mon_printf(f, "obj");
+ }
+ if (name) {
+ mon_printf(f, " name=%s ", name);
+ }
+ if (id) {
+ mon_printf(f, " id=%s ", id);
+ }
+ if (dev && (!name && !id && !dev->id)) {
+ mon_printf(f, " path=%s", dev->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 +2926,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 +2935,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 +2973,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 +2986,7 @@ struct FlatViewInfo {
void *f;
int counter;
bool dispatch_tree;
+ bool owner;
};
static void mtree_print_flatview(gpointer key, gpointer value,
@@ -2972,7 +3027,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 +3036,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 +3072,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 +3084,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 +3117,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 51f4cf4..9d304a5 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
* Alexey Kardashevskiy (aik@ozlabs.ru) wrote:
> 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>
From the HMP side this looks fine to me, and if it's making it clearer
where mappings are coming from that's good.
(It's a bit long/wordy, but that's the nature of the information).
Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> ---
>
> Does this look anything useful?
>
> There are cases ("msi", "msix-table", "msix-pba" and probably more) when
> it is not clear what owns an MR while they all have an owner (always? mostly?).
>
>
> "info mtree" example:
>
> address-space: memory
> 0000000000000000-ffffffffffffffff (prio 0, i/o): system parent:{obj}
> 0000000000000000-000000007fffffff (prio 0, ram): ppc_spapr.ram parent:{obj}
> 0000200000000000-000020000000ffff (prio 0, i/o): alias pci@800000020000000.io-alias @pci@800000020000000.io 0000000000000000-000000000000ffff owner:{dev path=/machine/unattached/device[3]}
> 0000200080000000-00002000ffffffff (prio 0, i/o): alias pci@800000020000000.mmio32-alias @pci@800000020000000.mmio 0000000080000000-00000000ffffffff owner:{dev path=/machine/unattached/device[3]}
> 0000210000000000-000021ffffffffff (prio 0, i/o): alias pci@800000020000000.mmio64-alias @pci@800000020000000.mmio 0000210000000000-000021ffffffffff owner:{dev path=/machine/unattached/device[3]}
>
> address-space: I/O
> 0000000000000000-000000000000ffff (prio 0, i/o): io parent:{obj}
>
> address-space: cpu-memory-0
> 0000000000000000-ffffffffffffffff (prio 0, i/o): system parent:{obj}
> 0000000000000000-000000007fffffff (prio 0, ram): ppc_spapr.ram parent:{obj}
> 0000200000000000-000020000000ffff (prio 0, i/o): alias pci@800000020000000.io-alias @pci@800000020000000.io 0000000000000000-000000000000ffff owner:{dev path=/machine/unattached/device[3]}
> 0000200080000000-00002000ffffffff (prio 0, i/o): alias pci@800000020000000.mmio32-alias @pci@800000020000000.mmio 0000000080000000-00000000ffffffff owner:{dev path=/machine/unattached/device[3]}
> 0000210000000000-000021ffffffffff (prio 0, i/o): alias pci@800000020000000.mmio64-alias @pci@800000020000000.mmio 0000210000000000-000021ffffffffff owner:{dev path=/machine/unattached/device[3]}
>
> address-space: pci@800000020000000
> 0000000000000000-ffffffffffffffff (prio 0, i/o): pci@800000020000000.iommu-root owner:{dev path=/machine/unattached/device[3]}
> 0000000000000000-ffffffffffffffff (prio 0, i/o): tce-root-80000001 owner:{dev path=/machine/unattached/device[3]/tce-table-80000001}
> 0800000000000000-080000007fffffff (prio 0, i/o): tce-iommu-80000001 owner:{dev path=/machine/unattached/device[3]/tce-table-80000001}
> 0000000000000000-ffffffffffffffff (prio 0, i/o): tce-root-80000000 owner:{dev path=/machine/unattached/device[3]/tce-table-80000000}
> 0000000000000000-000000003fffffff (prio 0, i/o): tce-iommu-80000000 owner:{dev path=/machine/unattached/device[3]/tce-table-80000000}
> 0000040000000000-000004000000ffff (prio 0, i/o): msi owner:{dev path=/machine/unattached/device[3]}
>
> address-space: vfio-pci
> 0000000000000000-ffffffffffffffff (prio 0, i/o): bus master container owner:{dev id=vfio0001_03_00_0}
> 0000000000000000-ffffffffffffffff (prio 0, i/o): alias bus master @pci@800000020000000.iommu-root 0000000000000000-ffffffffffffffff owner:{dev id=vfio0001_03_00_0}
>
> memory-region: pci@800000020000000.io
> 0000000000000000-000000000000ffff (prio 0, i/o): pci@800000020000000.io owner:{dev path=/machine/unattached/device[3]}
>
> memory-region: pci@800000020000000.mmio
> 0000000000000000-ffffffffffffffff (prio 0, i/o): pci@800000020000000.mmio owner:{dev path=/machine/unattached/device[3]}
> 0000210000000000-000021000000ffff (prio 1, i/o): 0001:03:00.0 base BAR 1 owner:{dev id=vfio0001_03_00_0}
> 0000210000000000-000021000000ffff (prio 0, i/o): 0001:03:00.0 BAR 1 owner:{dev id=vfio0001_03_00_0}
> 000021000000e000-000021000000e5ff (prio 0, i/o): msix-table owner:{dev id=vfio0001_03_00_0}
> 000021000000f000-000021000000f00f (prio 0, i/o): msix-pba [disabled] owner:{dev id=vfio0001_03_00_0}
> 0000210000040000-000021000007ffff (prio 1, i/o): 0001:03:00.0 base BAR 3 owner:{dev id=vfio0001_03_00_0}
> 0000210000040000-000021000007ffff (prio 0, i/o): 0001:03:00.0 BAR 3 owner:{dev id=vfio0001_03_00_0}
> 0000210000040000-000021000007ffff (prio 0, ramd): 0001:03:00.0 BAR 3 mmaps[0] owner:{dev id=vfio0001_03_00_0}
>
> memory-region: pci@800000020000000.iommu-root
> 0000000000000000-ffffffffffffffff (prio 0, i/o): pci@800000020000000.iommu-root owner:{dev path=/machine/unattached/device[3]}
> 0000000000000000-ffffffffffffffff (prio 0, i/o): tce-root-80000001 owner:{dev path=/machine/unattached/device[3]/tce-table-80000001}
> 0800000000000000-080000007fffffff (prio 0, i/o): tce-iommu-80000001 owner:{dev path=/machine/unattached/device[3]/tce-table-80000001}
> 0000000000000000-ffffffffffffffff (prio 0, i/o): tce-root-80000000 owner:{dev path=/machine/unattached/device[3]/tce-table-80000000}
> 0000000000000000-000000003fffffff (prio 0, i/o): tce-iommu-80000000 owner:{dev path=/machine/unattached/device[3]/tce-table-80000000}
> 0000040000000000-000004000000ffff (prio 0, i/o): msi owner:{dev path=/machine/unattached/device[3]}
>
>
>
> Flatviews:
>
>
> FlatView #0
> AS "pci@800000020000000", root: pci@800000020000000.iommu-root
> AS "vfio-pci", root: bus master container
> Root memory region: pci@800000020000000.iommu-root
> 0000000000000000-000000003fffffff (prio 0, i/o): tce-iommu-80000000 owner:{dev path=/machine/unattached/device[3]/tce-table-80000000}
> 0000040000000000-000004000000ffff (prio 0, i/o): msi owner:{dev path=/machine/unattached/device[3]}
> 0800000000000000-080000007fffffff (prio 0, i/o): tce-iommu-80000001 owner:{dev path=/machine/unattached/device[3]/tce-table-80000001}
>
> FlatView #1
> AS "memory", root: system
> AS "cpu-memory-0", root: system
> Root memory region: system
> 0000000000000000-000000007fffffff (prio 0, ram): ppc_spapr.ram parent:{obj}
> 0000210000000000-000021000000dfff (prio 0, i/o): 0001:03:00.0 BAR 1 owner:{dev id=vfio0001_03_00_0}
> 000021000000e000-000021000000e5ff (prio 0, i/o): msix-table owner:{dev id=vfio0001_03_00_0}
> 000021000000e600-000021000000ffff (prio 0, i/o): 0001:03:00.0 BAR 1 @000000000000e600 owner:{dev id=vfio0001_03_00_0}
> 0000210000040000-000021000007ffff (prio 0, ramd): 0001:03:00.0 BAR 3 mmaps[0] owner:{dev id=vfio0001_03_00_0}
>
> FlatView #2
> AS "I/O", root: io
> Root memory region: io
> 0000000000000000-000000000000ffff (prio 0, i/o): io parent:{obj}
> ---
> include/exec/memory.h | 2 +-
> memory.c | 80 ++++++++++++++++++++++++++++++++++++++++++++-------
> monitor.c | 4 ++-
> hmp-commands-info.hx | 7 +++--
> 4 files changed, 78 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..538a57c 100644
> --- a/memory.c
> +++ b/memory.c
> @@ -2830,10 +2830,57 @@ 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);
> + const char *name = object_property_print(obj, "name", true, NULL);
> +
> + mon_printf(f, " %s:{", label);
> + if (dev) {
> + mon_printf(f, "dev");
> + if (dev->id) {
> + mon_printf(f, " id=%s", dev->id);
> + }
> + } else {
> + mon_printf(f, "obj");
> + }
> + if (name) {
> + mon_printf(f, " name=%s ", name);
> + }
> + if (id) {
> + mon_printf(f, " id=%s ", id);
> + }
> + if (dev && (!name && !id && !dev->id)) {
> + mon_printf(f, " path=%s", dev->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 +2926,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 +2935,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 +2973,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 +2986,7 @@ struct FlatViewInfo {
> void *f;
> int counter;
> bool dispatch_tree;
> + bool owner;
> };
>
> static void mtree_print_flatview(gpointer key, gpointer value,
> @@ -2972,7 +3027,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 +3036,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 +3072,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 +3084,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 +3117,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 51f4cf4..9d304a5 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
>
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
On 29/03/2018 05:21, Alexey Kardashevskiy wrote:
> + DeviceState *dev = (DeviceState *) object_dynamic_cast(obj, TYPE_DEVICE);
> + const char *id = object_property_print(obj, "id", true, NULL);
I learnt now about commit e1ff3c67e8544f41f1bea76ba76385faee0d2bb7 and I
find it a mistake. The "id" is available through
object_get_canonical_path_component. Can you change the "id" assignment
to use object_get_canonical_path_component instead?
> + const char *name = object_property_print(obj, "name", true, NULL);
Who defines a name property?
> + mon_printf(f, " %s:{", label);
> + if (dev) {
> + mon_printf(f, "dev");
> + if (dev->id) {
> + mon_printf(f, " id=%s", dev->id);
> + }
> + } else {
> + mon_printf(f, "obj");
> + }
> + if (name) {
> + mon_printf(f, " name=%s ", name);
> + }
> + if (id) {
> + mon_printf(f, " id=%s ", id);
> + }
> + if (dev && (!name && !id && !dev->id)) {
> + mon_printf(f, " path=%s", dev->canonical_path);
Why not print the path for the !dev case? I think we can just have
mon_prinf(f, " %s:{%s", label, dev ? "dev" : "obj");
if (dev ? dev->id : id) {
mon_printf(f, " id=%s", dev ? dev->id : id);
} else {
canonical_path = object_get_canonical_path(...)
mon_printf(f, " path=%s", canonical_path);
g_free(canonical_path);
}
Thanks,
Paolo
On Thu, 12 Apr 2018 11:15:04 +0200
Paolo Bonzini <pbonzini@redhat.com> wrote:
> On 29/03/2018 05:21, Alexey Kardashevskiy wrote:
> > + DeviceState *dev = (DeviceState *) object_dynamic_cast(obj, TYPE_DEVICE);
> > + const char *id = object_property_print(obj, "id", true, NULL);
>
> I learnt now about commit e1ff3c67e8544f41f1bea76ba76385faee0d2bb7 and I
> find it a mistake. The "id" is available through
> object_get_canonical_path_component. Can you change the "id" assignment
> to use object_get_canonical_path_component instead?
object_get_canonical_path_component() should be used with care as it
assumes that object HAS parent. It will crash if parent is NULL
or hasn't been assigned yet.
Generally object doesn't need to know its own name,
we use it only for debugging and nice error reporting so far.
I'd rather have 'id' property at Object level so we won't have
to fish out ID from parent /which we aren't supposed to do and
which doesn't work in some cases/ when it's needed within
object itself.
> > + const char *name = object_property_print(obj, "name", true, NULL);
>
> Who defines a name property?
>
> > + mon_printf(f, " %s:{", label);
> > + if (dev) {
> > + mon_printf(f, "dev");
> > + if (dev->id) {
> > + mon_printf(f, " id=%s", dev->id);
> > + }
> > + } else {
> > + mon_printf(f, "obj");
>
> > + }
> > + if (name) {
> > + mon_printf(f, " name=%s ", name);
> > + }
> > + if (id) {
> > + mon_printf(f, " id=%s ", id);
> > + }
> > + if (dev && (!name && !id && !dev->id)) {
> > + mon_printf(f, " path=%s", dev->canonical_path);
>
> Why not print the path for the !dev case? I think we can just have
>
> mon_prinf(f, " %s:{%s", label, dev ? "dev" : "obj");
> if (dev ? dev->id : id) {
> mon_printf(f, " id=%s", dev ? dev->id : id);
> } else {
> canonical_path = object_get_canonical_path(...)
> mon_printf(f, " path=%s", canonical_path);
> g_free(canonical_path);
> }
>
> Thanks,
>
> Paolo
On 16/04/2018 15:20, Igor Mammedov wrote: > Generally object doesn't need to know its own name, > we use it only for debugging and nice error reporting so far. > I'd rather have 'id' property at Object level so we won't have > to fish out ID from parent /which we aren't supposed to do and > which doesn't work in some cases/ when it's needed within > object itself. Having an 'id' at object level is also a mess, because that id is invalid after unparent. Since this is just for debugging use, object_get_canonical_path_component is the right function. We can just make it return NULL if there is no parent. Paolo
On Mon, 16 Apr 2018 15:30:39 +0200 Paolo Bonzini <pbonzini@redhat.com> wrote: > On 16/04/2018 15:20, Igor Mammedov wrote: > > Generally object doesn't need to know its own name, > > we use it only for debugging and nice error reporting so far. > > I'd rather have 'id' property at Object level so we won't have > > to fish out ID from parent /which we aren't supposed to do and > > which doesn't work in some cases/ when it's needed within > > object itself. > > Having an 'id' at object level is also a mess, because that id is > invalid after unparent. I'd just consider 'id' as object name which is valid even if there is no parent (during whole object lifecycle). That would allow for object to have a reachable name vs getting NULL when parent isn't set. Maybe Object::id is overkill, but we probably could use NamedObject where it's needed and avoid reverse engineering id from path. > Since this is just for debugging use, > object_get_canonical_path_component is the right function. We can just > make it return NULL if there is no parent. looking at current use it out-grew just debugging usecases and it's rather messy right now: ram_backend_memory_alloc, throttle_group_obj_complete, xlnx_zynqmp_create_rpu, spapr_drc.c:realize, iothread_complete, memory_region_name > Paolo
On 16/04/2018 16:27, Igor Mammedov wrote: > On Mon, 16 Apr 2018 15:30:39 +0200 > Paolo Bonzini <pbonzini@redhat.com> wrote: > >> On 16/04/2018 15:20, Igor Mammedov wrote: >>> Generally object doesn't need to know its own name, >>> we use it only for debugging and nice error reporting so far. >>> I'd rather have 'id' property at Object level so we won't have >>> to fish out ID from parent /which we aren't supposed to do and >>> which doesn't work in some cases/ when it's needed within >>> object itself. >> >> Having an 'id' at object level is also a mess, because that id is >> invalid after unparent. > I'd just consider 'id' as object name which is valid even if there > is no parent (during whole object lifecycle). What's the point of an object name if it cannot be unique? Paolo > That would allow for object to have a reachable name vs getting NULL > when parent isn't set. > Maybe Object::id is overkill, but we probably could use NamedObject > where it's needed and avoid reverse engineering id from path. > >> Since this is just for debugging use, >> object_get_canonical_path_component is the right function. We can just >> make it return NULL if there is no parent. > > looking at current use it out-grew just debugging usecases > and it's rather messy right now: > > ram_backend_memory_alloc, throttle_group_obj_complete, > xlnx_zynqmp_create_rpu, spapr_drc.c:realize, iothread_complete, > memory_region_name I agree, _but_ it's definitely okay for debugging usecases. Paolo
On Mon, 16 Apr 2018 17:29:23 +0200
Paolo Bonzini <pbonzini@redhat.com> wrote:
> On 16/04/2018 16:27, Igor Mammedov wrote:
> > On Mon, 16 Apr 2018 15:30:39 +0200
> > Paolo Bonzini <pbonzini@redhat.com> wrote:
> >
> >> On 16/04/2018 15:20, Igor Mammedov wrote:
> >>> Generally object doesn't need to know its own name,
> >>> we use it only for debugging and nice error reporting so far.
> >>> I'd rather have 'id' property at Object level so we won't have
> >>> to fish out ID from parent /which we aren't supposed to do and
> >>> which doesn't work in some cases/ when it's needed within
> >>> object itself.
> >>
> >> Having an 'id' at object level is also a mess, because that id is
> >> invalid after unparent.
> > I'd just consider 'id' as object name which is valid even if there
> > is no parent (during whole object lifecycle).
>
> What's the point of an object name if it cannot be unique?
It should be sufficient for it to be unique within parent's
scope and object_property_add_child() should make sure that
added object is unique within its parent's namespace.
Having named object from starters is useful as object
won't have to piggyback on parent (object_get_canonical_path_component)
when it need its own name. Then named object could use its name
freely anywhere including initfn, property setters/getters and
let object_property_add_child() take care of possible name
conflict.
From debugging/error reporting pov 'id' might be ambiguous
sometimes but it's way better than unanimous "NULL" string
or crash due to no set parent.
It sort of relates to discussion Peter started with
https://patchwork.kernel.org/patch/10224775/
"
Code that used to look like this:
object_initialize(&s->ic, sizeof(s->ic), TYPE_BCM2835_IC);
object_property_add_child(obj, "ic", OBJECT(&s->ic), NULL);
qdev_set_parent_bus(DEVICE(&s->ic), sysbus_get_default());
can now look like this:
sysbus_init_child(obj, "ic", &s->ic, sizeof(s->ic), TYPE_BCM2835_IC);
"
Considering that in most cases where named objects are used
we parent them. We might be better of with API like
named_object_new(TYPE, NAME, PARENT, ERROR)
named_object_initialize(PTR, SIZE, TYPE, NAME, PARENT, ERROR)
> Paolo
>
> > That would allow for object to have a reachable name vs getting NULL
> > when parent isn't set.
> > Maybe Object::id is overkill, but we probably could use NamedObject
> > where it's needed and avoid reverse engineering id from path.
> >
> >> Since this is just for debugging use,
> >> object_get_canonical_path_component is the right function. We can just
> >> make it return NULL if there is no parent.
> >
> > looking at current use it out-grew just debugging usecases
> > and it's rather messy right now:
> >
> > ram_backend_memory_alloc, throttle_group_obj_complete,
> > xlnx_zynqmp_create_rpu, spapr_drc.c:realize, iothread_complete,
> > memory_region_name
>
> I agree, _but_ it's definitely okay for debugging usecases.
yep, since there isn't other way to do it now,
it's fine to use object_get_canonical_path_component()
>
> Paolo
On 17/04/2018 14:18, Igor Mammedov wrote: >> What's the point of an object name if it cannot be unique? > It should be sufficient for it to be unique within parent's > scope and object_property_add_child() should make sure that > added object is unique within its parent's namespace. > Having named object from starters is useful as object > won't have to piggyback on parent (object_get_canonical_path_component) > when it need its own name. Then named object could use its name > freely anywhere including initfn, property setters/getters and > let object_property_add_child() take care of possible name > conflict. I agree that it looks nice, but I'm worried that people forget that the path component is only unique until object_unparent(). The use for DEVICE_DELETED events is already a bad thing... Paolo
Paolo Bonzini <pbonzini@redhat.com> writes: > On 17/04/2018 14:18, Igor Mammedov wrote: >>> What's the point of an object name if it cannot be unique? >> It should be sufficient for it to be unique within parent's >> scope and object_property_add_child() should make sure that >> added object is unique within its parent's namespace. >> Having named object from starters is useful as object >> won't have to piggyback on parent (object_get_canonical_path_component) >> when it need its own name. Then named object could use its name >> freely anywhere including initfn, property setters/getters and >> let object_property_add_child() take care of possible name >> conflict. > > I agree that it looks nice, but I'm worried that people forget that the > path component is only unique until object_unparent(). The use for > DEVICE_DELETED events is already a bad thing... Hmm. If the canonical QOM path isn't the proper way to identify a device in QMP, what else is? Honest question!
On 18/04/2018 08:32, Markus Armbruster wrote: > Paolo Bonzini <pbonzini@redhat.com> writes: > >> On 17/04/2018 14:18, Igor Mammedov wrote: >>>> What's the point of an object name if it cannot be unique? >>> It should be sufficient for it to be unique within parent's >>> scope and object_property_add_child() should make sure that >>> added object is unique within its parent's namespace. >>> Having named object from starters is useful as object >>> won't have to piggyback on parent (object_get_canonical_path_component) >>> when it need its own name. Then named object could use its name >>> freely anywhere including initfn, property setters/getters and >>> let object_property_add_child() take care of possible name >>> conflict. >> >> I agree that it looks nice, but I'm worried that people forget that the >> path component is only unique until object_unparent(). The use for >> DEVICE_DELETED events is already a bad thing... > > Hmm. If the canonical QOM path isn't the proper way to identify a > device in QMP, what else is? Honest question! The canonical path is; the last component alone is not. Paolo
On 12/4/18 7:15 pm, Paolo Bonzini wrote:
> On 29/03/2018 05:21, Alexey Kardashevskiy wrote:
>> + DeviceState *dev = (DeviceState *) object_dynamic_cast(obj, TYPE_DEVICE);
>> + const char *id = object_property_print(obj, "id", true, NULL);
>
> I learnt now about commit e1ff3c67e8544f41f1bea76ba76385faee0d2bb7 and I
> find it a mistake. The "id" is available through
> object_get_canonical_path_component. Can you change the "id" assignment
> to use object_get_canonical_path_component instead?
Uff. I did some tests and found that this is needed, at least. I can also
returning NULL from object_get_canonical_path_component(), it just seems to
be a real case and majority of object_get_canonical_path_component() users
do not test the returned pointer.
===================
diff --git a/qom/object.c b/qom/object.c
index 4677951..0c2d5c2 100644
--- a/qom/object.c
+++ b/qom/object.c
@@ -1645,7 +1645,9 @@ gchar *object_get_canonical_path_component(Object *obj)
GHashTableIter iter;
g_assert(obj);
- g_assert(obj->parent != NULL);
+ if (!obj->parent) {
+ return g_strdup("<no-parent>");
+ }
g_hash_table_iter_init(&iter, obj->parent->properties);
while (g_hash_table_iter_next(&iter, NULL, (gpointer *)&prop)) {
@@ -1668,7 +1670,7 @@ gchar *object_get_canonical_path(Object *obj)
Object *root = object_get_root();
char *newpath, *path = NULL;
- while (obj != root) {
+ while (obj && obj != root) {
char *component = object_get_canonical_path_component(obj);
if (path) {
@@ -1681,6 +1683,8 @@ gchar *object_get_canonical_path(Object *obj)
}
obj = obj->parent;
+ if (!obj)
+ printf("+++Q+++ (%u) %s %u\n", getpid(), __func__, __LINE__);
}
====================
as QMP crashes QEMU with this:
{"execute": "qom-get", "arguments": {"path":
"/machine/unattached/device[3]/smram[0]", "property": "container"}}
with the patch it returns:
{'return': '/<no-parent>/mem-container-smram[0]'}
Without the patch it crashes as:
(gdb) bt
#0 object_get_canonical_path (obj=0x0) at /home/aik/p/qemu/qom/object.c:1687
#1 0x0000555555829676 in memory_region_get_container (obj=0x555556d5c6e0,
v=0x555557a7c530, name=0x5555569a4fe0 "container", opaque=0x0,
errp=0x7fffffffda58) at /home/aik/p/qemu/memory.c:1172
#2 0x0000555555bf719a in object_property_get (obj=0x555556d5c6e0,
v=0x555557a7c530, name=0x5555569a4fe0 "container", errp=0x7fffffffda58) at
/home/aik/p/qemu/qom/object.c:1107
#3 0x0000555555bfa2de in object_property_get_qobject (obj=0x555556d5c6e0,
name=0x5555569a4fe0 "container", errp=0x7fffffffdae8) at
/home/aik/p/qemu/qom/qom-qobject.c:39
#4 0x00005555559963cf in qmp_qom_get (path=0x5555576e5680
"/machine/unattached/device[3]/smram[0]", property=0x5555569a4fe0
"container", errp=0x7fffffffdae8) at /home/aik/p/qemu/qmp.c:271
#5 0x000055555598de6c in qmp_marshal_qom_get (args=0x5555573b5720,
ret=0x7fffffffdb60, errp=0x7fffffffdb58) at qapi/qapi-commands-misc.c:1249
#6 0x0000555555d155ad in do_qmp_dispatch (cmds=0x55555645a3e0
<qmp_commands>, request=0x5555573aff80, errp=0x7fffffffdbb0) at
/home/aik/p/qemu/qapi/qmp-dispatch.c:111
#7 0x0000555555d1576d in qmp_dispatch (cmds=0x55555645a3e0 <qmp_commands>,
request=0x5555573aff80) at /home/aik/p/qemu/qapi/qmp-dispatch.c:160
#8 0x0000555555819361 in monitor_qmp_dispatch_one (req_obj=0x555556fd7ee0)
at /home/aik/p/qemu/monitor.c:4091
#9 0x0000555555819573 in monitor_qmp_bh_dispatcher (data=0x0) at
/home/aik/p/qemu/monitor.c:4149
#10 0x0000555555d23df9 in aio_bh_call (bh=0x555556a1a650) at
/home/aik/p/qemu/util/async.c:90
#11 0x0000555555d23e91 in aio_bh_poll (ctx=0x5555569f0ce0) at
/home/aik/p/qemu/util/async.c:118
#12 0x0000555555d28a92 in aio_dispatch (ctx=0x5555569f0ce0) at
/home/aik/p/qemu/util/aio-posix.c:436
#13 0x0000555555d2422c in aio_ctx_dispatch (source=0x5555569f0ce0,
callback=0x0, user_data=0x0) at /home/aik/p/qemu/util/async.c:261
#14 0x00007ffff4cc6b77 in ?? ()
#15 0x0000000000000000 in ?? ()
It is fc27/x86_64 both host and guest, QMP issues when guest booted to the
login prompt (so I'd not expect any hotplugs) and I have no idea what smram
is and what its parent should be.
>> + const char *name = object_property_print(obj, "name", true, NULL);
>
> Who defines a name property?
Nothing, not sure why I put it here in the first place, will remove.
>
>> + mon_printf(f, " %s:{", label);
>> + if (dev) {
>> + mon_printf(f, "dev");
>> + if (dev->id) {
>> + mon_printf(f, " id=%s", dev->id);
>> + }
>> + } else {
>> + mon_printf(f, "obj");
>
>> + }
>> + if (name) {
>> + mon_printf(f, " name=%s ", name);
>> + }
>> + if (id) {
>> + mon_printf(f, " id=%s ", id);
>> + }
>> + if (dev && (!name && !id && !dev->id)) {
>> + mon_printf(f, " path=%s", dev->canonical_path);
>
> Why not print the path for the !dev case? I think we can just have
>
> mon_prinf(f, " %s:{%s", label, dev ? "dev" : "obj");
> if (dev ? dev->id : id) {
> mon_printf(f, " id=%s", dev ? dev->id : id);
> } else {
> canonical_path = object_get_canonical_path(...)
> mon_printf(f, " path=%s", canonical_path);
> g_free(canonical_path);
> }
Makes sense, assuming the issue above is fixed.
>
> Thanks,
>
> Paolo
>
--
Alexey
On 19/04/2018 04:41, Alexey Kardashevskiy wrote:
> it just seems to
> be a real case and majority of object_get_canonical_path_component() users
> do not test the returned pointer.
>
> ===================
> diff --git a/qom/object.c b/qom/object.c
> index 4677951..0c2d5c2 100644
> --- a/qom/object.c
> +++ b/qom/object.c
> @@ -1645,7 +1645,9 @@ gchar *object_get_canonical_path_component(Object *obj)
> GHashTableIter iter;
>
> g_assert(obj);
> - g_assert(obj->parent != NULL);
I agree but why wouldn't the caller have crashed anyway on this assertion?
> as QMP crashes QEMU with this:
>
> {"execute": "qom-get", "arguments": {"path":
> "/machine/unattached/device[3]/smram[0]", "property": "container"}}
>
> with the patch it returns:
>
> {'return': '/<no-parent>/mem-container-smram[0]'}
The root cause of this is that the accelerator object is not added to
the QOM object tree.
Paolo
On 19/4/18 8:33 pm, Paolo Bonzini wrote:
> On 19/04/2018 04:41, Alexey Kardashevskiy wrote:
>> it just seems to
>> be a real case and majority of object_get_canonical_path_component() users
>> do not test the returned pointer.
>>
>> ===================
>> diff --git a/qom/object.c b/qom/object.c
>> index 4677951..0c2d5c2 100644
>> --- a/qom/object.c
>> +++ b/qom/object.c
>> @@ -1645,7 +1645,9 @@ gchar *object_get_canonical_path_component(Object *obj)
>> GHashTableIter iter;
>>
>> g_assert(obj);
>> - g_assert(obj->parent != NULL);
>
> I agree but why wouldn't the caller have crashed anyway on this assertion?
Not sure I am following. It did crash, this is why I replaced the assert to
make qom-get working.
>> as QMP crashes QEMU with this:
>>
>> {"execute": "qom-get", "arguments": {"path":
>> "/machine/unattached/device[3]/smram[0]", "property": "container"}}
>>
>> with the patch it returns:
>>
>> {'return': '/<no-parent>/mem-container-smram[0]'}
>
> The root cause of this is that the accelerator object is not added to
> the QOM object tree.
ookay.
--
Alexey
© 2016 - 2024 Red Hat, Inc.