On Mon, Sep 26, 2011 at 9:45 AM, Avi Kivity <a...@redhat.com> wrote:
> 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<assert.h>
>> +#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.
monitor.c could pass a pointer to a fprintf-like function and a void
*opaque for Monitor *mon.
>> 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.
OK.
>> +
>> +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?
Leftover from previous version. I'll remove the 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.
Right, it looked a bit nicer this way but I'll remove that.
>> +
>> + } 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.
OK for both.
>> +
>> + /* 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.
Good idea, that could avoid the forever loop.
>> + 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
>
>
