From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([140.186.70.92]:54835) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1R87l7-0004Ic-2j for qemu-devel@nongnu.org; Mon, 26 Sep 2011 05:45:58 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1R87l5-0004ip-Po for qemu-devel@nongnu.org; Mon, 26 Sep 2011 05:45:57 -0400 Received: from mx1.redhat.com ([209.132.183.28]:39738) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1R87l5-0004il-Hy for qemu-devel@nongnu.org; Mon, 26 Sep 2011 05:45:55 -0400 Message-ID: <4E8049CD.90504@redhat.com> Date: Mon, 26 Sep 2011 12:45:49 +0300 From: Avi Kivity MIME-Version: 1.0 References: In-Reply-To: Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v2] memory: simple memory tree printer List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Blue Swirl Cc: qemu-devel On 09/25/2011 11:19 PM, Blue Swirl wrote: > Add a monitor command 'info mtree' to show the memory hierarchy > much like /proc/iomem in Linux. > > > diff --git a/memory.c b/memory.c > index ba74435..6b33fc4 100644 > --- a/memory.c > +++ b/memory.c > @@ -17,6 +17,7 @@ > #include "bitops.h" > #include "kvm.h" > #include > +#include "monitor.h" This is a unfortunate - now the monitor and memory.c are interdependent; this makes it harder to write unit tests (at least without ifdefs). I guess we can disentangle it later using some kind of generic walker. > unsigned memory_region_transaction_depth = 0; > > @@ -1256,3 +1257,103 @@ void set_system_io_map(MemoryRegion *mr) > address_space_io.root = mr; > memory_region_update_topology(); > } > + > +typedef struct MemoryRegionList MemoryRegionList; > +typedef struct MemoryRegionListHead MemoryRegionListHead; > + > +struct MemoryRegionList { > + const MemoryRegion *mr; > + QLIST_ENTRY(MemoryRegionList) queue; > +}; > + > +struct MemoryRegionListHead { > + QLIST_HEAD(queue, MemoryRegionList) head; > +}; Straight typedef of QLIST_HEAD(queue, MemoryRegionList) would be nicer. > + > +static void mtree_print_mr(Monitor *mon, const MemoryRegion *mr, > + unsigned int level, target_phys_addr_t base, > + MemoryRegionListHead *print_queue) > +{ > + const MemoryRegion *submr; > + unsigned int i; > + > + for (i = 0; i< level; i++) { > + monitor_printf(mon, " "); > + } > + > + if (mr->alias) { > + if (print_queue) { print_queue is never NULL, why test? > + MemoryRegionList *ml; > + bool found = false; > + > + /* check if the alias is already in the queue */ > + QLIST_FOREACH(ml,&print_queue->head, queue) { > + if (ml->mr == mr->alias) { > + found = true; > + } > + } > + > + if (!found) { > + ml = g_malloc(sizeof(*ml)); > + ml->mr = mr->alias; > + QLIST_INSERT_HEAD(&print_queue->head, ml, queue); > + } > + } > + monitor_printf(mon, TARGET_FMT_plx "-" TARGET_FMT_plx " : > alias %s @%s " > + TARGET_FMT_plx "-" TARGET_FMT_plx "\n", > + base + mr->addr, > + base + mr->addr + (target_phys_addr_t)mr->size - 1, > + mr->name, > + mr->alias->name, > + mr->alias_offset + mr->alias->addr, > + mr->alias_offset + mr->alias->addr + > + (target_phys_addr_t)mr->size - 1); Adding mr->alias->addr doesn't help much - it doesn't give you an absolute address, just relative to the alias target's container. If it's deep enough in the tree the address is meaningless. > + > + } else { > + monitor_printf(mon, TARGET_FMT_plx "-" TARGET_FMT_plx " : %s\n", > + base + mr->addr, > + base + mr->addr + (target_phys_addr_t)mr->size - 1, > + mr->name); > + } > + QTAILQ_FOREACH(submr,&mr->subregions, subregions_link) { > + mtree_print_mr(mon, submr, level + 1, base + mr->addr, print_queue); > + } > +} > + > +void mtree_info(Monitor *mon) > +{ > + MemoryRegionListHead *ml_head, *ml_head2, *ml_tmp; > + MemoryRegionList *ml, *ml2; > + > + ml_head = g_malloc(sizeof(*ml)); Wrong type. g_new() is much better for this. > + QLIST_INIT(&ml_head->head); > + > + monitor_printf(mon, "memory\n"); > + mtree_print_mr(mon, address_space_memory.root, 0, 0, ml_head); > + > + ml_head2 = g_malloc(sizeof(*ml)); Again. > + > + /* print aliased regions */ > + for (;;) { > + QLIST_INIT(&ml_head2->head); > + > + QLIST_FOREACH_SAFE(ml,&ml_head->head, queue, ml2) { > + monitor_printf(mon, "%s\n", ml->mr->name); > + mtree_print_mr(mon, ml->mr, 0, 0, ml_head2); > + g_free(ml); > + } I think you can eliminate more duplicates by adding a bool ->printed to the queue entry, and always using the same queue. Iterate until no un ->printed elements remain. > + if (QLIST_EMPTY(&ml_head->head)) { > + break; > + } > + ml_tmp = ml_head; > + ml_head = ml_head2; > + ml_head2 = ml_tmp; > + } > + > +#ifdef TARGET_I386 > + monitor_printf(mon, "I/O\n"); > + mtree_print_mr(mon, address_space_io.root, 0, 0, ml_head); > +#endif > + g_free(ml_head2); > + g_free(ml_head); > +} > diff --git a/memory.h b/memory.h > index 06b83ae..09d8e29 100644 > --- a/memory.h > +++ b/memory.h > @@ -500,6 +500,8 @@ void memory_region_transaction_begin(void); > */ > void memory_region_transaction_commit(void); > > +void mtree_info(Monitor *mon); > + > #endif > -- error compiling committee.c: too many arguments to function