[Qemu-devel] [PATCH] memory: simple memory tree printer

[Qemu-devel] [PATCH] memory: simple memory tree printer

[Blue Swirl]

[-- Attachment #1: Type: text/plain, Size: 7268 bytes --]

Add a monitor command 'info mtree' to show the memory hierarchy.

Signed-off-by: Blue Swirl <blauwirbel@gmail.com>
---
Example output:
qemu-system-i386 -monitor stdio
QEMU 0.15.50 monitor - type 'help' for more information
(qemu) info mtree
memory
system addr 0000000000000000 off 0000000000000000 size 7fffffffffffffff
-pam-ram addr 00000000000ec000 off 0000000000000000 size 4000
-pam-ram addr 00000000000e8000 off 0000000000000000 size 4000
-pam-ram addr 00000000000e4000 off 0000000000000000 size 4000
-pam-ram addr 00000000000e0000 off 0000000000000000 size 4000
-pam-ram addr 00000000000dc000 off 0000000000000000 size 4000
-pam-ram addr 00000000000d8000 off 0000000000000000 size 4000
-pam-ram addr 00000000000d4000 off 0000000000000000 size 4000
-pam-ram addr 00000000000d0000 off 0000000000000000 size 4000
-pam-ram addr 00000000000cc000 off 0000000000000000 size 4000
-pam-rom addr 00000000000c8000 off 0000000000000000 size 4000
-pam-rom addr 00000000000c4000 off 0000000000000000 size 4000
-pam-rom addr 00000000000c0000 off 0000000000000000 size 4000
-pam-rom addr 00000000000f0000 off 0000000000000000 size 10000
-smram-region addr 00000000000a0000 off 0000000000000000 size 20000
-pci-hole64 addr 4000000000000000 off 0000000000000000 size 4000000000000000
-pci-hole addr 0000000008000000 off 0000000000000000 size f8000000
-ram-below-4g addr 0000000000000000 off 0000000000000000 size 8000000
-apic addr 00000000fee00000 off 0000000000000000 size 100000
I/O
io addr 0000000000000000 off 0000000000000000 size 10000
-e1000-io addr 000000000000c000 off 0000000000000000 size 40
-piix-bmdma-container addr 000000000000c040 off 0000000000000000 size 10
--bmdma addr 000000000000000c off 0000000000000000 size 4
--piix-bmdma addr 0000000000000008 off 0000000000000000 size 4
--bmdma addr 0000000000000004 off 0000000000000000 size 4
--piix-bmdma addr 0000000000000000 off 0000000000000000 size 4
-pci-conf-data addr 0000000000000cfc off 0000000000000000 size 4
-pci-conf-idx addr 0000000000000cf8 off 0000000000000000 size 4

Sparc
memory
system addr 0000000000000000 off 0000000000000000 size 7fffffffffffffff
-escc addr 0000000071100000 off 0000000000000000 size 8
-escc addr 0000000071000000 off 0000000000000000 size 8
-lance-mmio addr 0000000078c00000 off 0000000000000000 size 4
I/O
io addr 0000000000000000 off 0000000000000000 size 10000

Sparc64
memory
system addr 0000000000000000 off 0000000000000000 size 7fffffffffffffff
-pci-mmio addr 000001ff00000000 off 0000000000000000 size 100000000
--vga.chain4 addr 00000000000a0000 off 0000000000000000 size 10000
--isa-mmio addr 0000000003000000 off 0000000000000000 size 800000
--isa-mmio addr 0000000002000000 off 0000000000000000 size 1000000
--vga.rom addr 0000000001000000 off 0000000000000000 size 10000
--vga.vram addr 0000000000800000 off 0000000000000000 size 800000
--vga-lowmem addr 000001fe020a0000 off 0000000000000000 size 20000
-apb-pci-ioport addr 000001fe02000000 off 0000000000000000 size 10000
-apb-pci-config addr 000001fe01000000 off 0000000000000000 size 1000000
-apb-config addr 000001fe00000000 off 0000000000000000 size 10000
I/O
io addr 0000000000000000 off 0000000000000000 size 10000
-cmd646-bmdma addr 0000000000000700 off 0000000000000000 size 10
--cmd646-bmdma-ioport addr 000000000000000c off 0000000000000000 size 4
--cmd646-bmdma-bus addr 0000000000000008 off 0000000000000000 size 4
--cmd646-bmdma-ioport addr 0000000000000004 off 0000000000000000 size 4
--cmd646-bmdma-bus addr 0000000000000000 off 0000000000000000 size 4
-cmd646-cmd addr 0000000000000680 off 0000000000000000 size 4
-cmd646-data addr 0000000000000600 off 0000000000000000 size 8
-cmd646-cmd addr 0000000000000580 off 0000000000000000 size 4
-cmd646-data addr 0000000000000500 off 0000000000000000 size 8
-ne2000 addr 0000000000000400 off 0000000000000000 size 100

PPC
memory
system addr 00000000 off 00000000 size 7fffffffffffffff
-vga.chain4 addr 000a0000 off 00000000 size 10000
-macio addr 80880000 off 00000000 size 80000
--macio-nvram addr 00060000 off 00000000 size 20000
--pmac-ide addr 00020000 off 00000000 size 1000
--(null) addr 00016000 off 00000000 size 0
--escc-bar addr 00013000 off 00000000 size 40
--dbdma addr 00008000 off 00000000 size 1000
--heathrow-pic addr 00000000 off 00000000 size 1000
-vga.rom addr 80800000 off 00000000 size 10000
-vga.vram addr 80000000 off 00000000 size 800000
-vga-lowmem addr 800a0000 off 00000000 size 20000
-escc addr 80013000 off 00000000 size 40
-pci-data-idx addr fee00000 off 00000000 size 1000
-pci-conf-idx addr fec00000 off 00000000 size 1000
-isa-mmio addr fe000000 off 00000000 size 200000
I/O
io addr 00000000 off 00000000 size 10000
-cmd646-bmdma addr 00000700 off 00000000 size 10
--cmd646-bmdma-ioport addr 0000000c off 00000000 size 4
--cmd646-bmdma-bus addr 00000008 off 00000000 size 4
--cmd646-bmdma-ioport addr 00000004 off 00000000 size 4
--cmd646-bmdma-bus addr 00000000 off 00000000 size 4
-cmd646-cmd addr 00000680 off 00000000 size 4
-cmd646-data addr 00000600 off 00000000 size 8
-cmd646-cmd addr 00000580 off 00000000 size 4
-cmd646-data addr 00000500 off 00000000 size 8
-ne2000 addr 00000400 off 00000000 size 100

NB: (null) does not look OK.

Field 'offset' is always zero, maybe that is not interesting. Will it
become one day?

---
 memory.c  |   27 +++++++++++++++++++++++++++
 memory.h  |    2 ++
 monitor.c |    7 +++++++
 3 files changed, 36 insertions(+), 0 deletions(-)

diff --git a/memory.c b/memory.c
index 57f0fa4..0bcef84 100644
--- a/memory.c
+++ b/memory.c
@@ -17,6 +17,7 @@
 #include "bitops.h"
 #include "kvm.h"
 #include <assert.h>
+#include "monitor.h"

 unsigned memory_region_transaction_depth = 0;

@@ -1253,3 +1254,29 @@ void set_system_io_map(MemoryRegion *mr)
     address_space_io.root = mr;
     memory_region_update_topology();
 }
+
+static void mtree_print_mr(Monitor *mon, MemoryRegion *mr, unsigned int level)
+{
+    MemoryRegion *submr;
+    unsigned int i;
+
+    for (i = 0; i < level; i++) {
+        monitor_printf(mon, "-");
+    }
+    monitor_printf(mon, "%s addr " TARGET_FMT_plx " off " TARGET_FMT_plx
+                   " size %" PRIx64 "\n",
+                   mr->name, mr->addr, mr->offset, mr->size);
+
+    QTAILQ_FOREACH(submr, &mr->subregions, subregions_link) {
+        mtree_print_mr(mon, submr, level + 1);
+    }
+}
+
+void mtree_info(Monitor *mon)
+{
+    monitor_printf(mon, "memory\n");
+    mtree_print_mr(mon, address_space_memory.root, 0);
+
+    monitor_printf(mon, "I/O\n");
+    mtree_print_mr(mon, address_space_io.root, 0);
+}
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

 #endif
diff --git a/monitor.c b/monitor.c
index 03ae997..0302446 100644
--- a/monitor.c
+++ b/monitor.c
@@ -2968,6 +2968,13 @@ static const mon_cmd_t info_cmds[] = {
     },
 #endif
     {
+        .name       = "mtree",
+        .args_type  = "",
+        .params     = "",
+        .help       = "show memory tree",
+        .mhandler.info = mtree_info,
+    },
+    {
         .name       = "jit",
         .args_type  = "",
         .params     = "",
-- 
1.6.2.4

[-- Attachment #2: 0001-memory-simple-memory-tree-printer.patch --]
[-- Type: text/x-diff, Size: 2398 bytes --]

From 5e01e21490994a538b70ff27f1caf9e4865aeba1 Mon Sep 17 00:00:00 2001
Message-Id: <5e01e21490994a538b70ff27f1caf9e4865aeba1.1315772597.git.blauwirbel@gmail.com>
From: Blue Swirl <blauwirbel@gmail.com>
Date: Sun, 11 Sep 2011 20:22:05 +0000
Subject: [PATCH] memory: simple memory tree printer

Add a monitor command 'info mtree' to show the memory hierarchy.

Signed-off-by: Blue Swirl <blauwirbel@gmail.com>
---
 memory.c  |   27 +++++++++++++++++++++++++++
 memory.h  |    2 ++
 monitor.c |    7 +++++++
 3 files changed, 36 insertions(+), 0 deletions(-)

diff --git a/memory.c b/memory.c
index 57f0fa4..0bcef84 100644
--- a/memory.c
+++ b/memory.c
@@ -17,6 +17,7 @@
 #include "bitops.h"
 #include "kvm.h"
 #include <assert.h>
+#include "monitor.h"
 
 unsigned memory_region_transaction_depth = 0;
 
@@ -1253,3 +1254,29 @@ void set_system_io_map(MemoryRegion *mr)
     address_space_io.root = mr;
     memory_region_update_topology();
 }
+
+static void mtree_print_mr(Monitor *mon, MemoryRegion *mr, unsigned int level)
+{
+    MemoryRegion *submr;
+    unsigned int i;
+
+    for (i = 0; i < level; i++) {
+        monitor_printf(mon, "-");
+    }
+    monitor_printf(mon, "%s addr " TARGET_FMT_plx " off " TARGET_FMT_plx
+                   " size %" PRIx64 "\n",
+                   mr->name, mr->addr, mr->offset, mr->size);
+
+    QTAILQ_FOREACH(submr, &mr->subregions, subregions_link) {
+        mtree_print_mr(mon, submr, level + 1);
+    }
+}
+
+void mtree_info(Monitor *mon)
+{
+    monitor_printf(mon, "memory\n");
+    mtree_print_mr(mon, address_space_memory.root, 0);
+
+    monitor_printf(mon, "I/O\n");
+    mtree_print_mr(mon, address_space_io.root, 0);
+}
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
 
 #endif
diff --git a/monitor.c b/monitor.c
index 03ae997..0302446 100644
--- a/monitor.c
+++ b/monitor.c
@@ -2968,6 +2968,13 @@ static const mon_cmd_t info_cmds[] = {
     },
 #endif
     {
+        .name       = "mtree",
+        .args_type  = "",
+        .params     = "",
+        .help       = "show memory tree",
+        .mhandler.info = mtree_info,
+    },
+    {
         .name       = "jit",
         .args_type  = "",
         .params     = "",
-- 
1.7.2.5

Re: [Qemu-devel] [PATCH] memory: simple memory tree printer

[Richard Henderson]

On 09/11/2011 09:31 PM, Blue Swirl wrote:
> Field 'offset' is always zero, maybe that is not interesting. Will it
> become one day?

It's not always zero, but only used by certain devices.


r~

Re: [Qemu-devel] [PATCH] memory: simple memory tree printer

[Jan Kiszka]

On 2011-09-12 08:43, Richard Henderson wrote:
> On 09/11/2011 09:31 PM, Blue Swirl wrote:
>> Field 'offset' is always zero, maybe that is not interesting. Will it
>> become one day?
> 
> It's not always zero, but only used by certain devices.

I do not see any users, neither upstream nor in Avi's tree.

To my (semi-)understanding, offset should correlate to region_offset of
cpu_register_physical_memory_offset: legacy device models require this
to be 0 as they expect an absolute memory address passed to their
handler, in contrast to a normal one that is relative to the regions
base. But I do not see how the memory region offset actually helps here.

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Re: [Qemu-devel] [PATCH] memory: simple memory tree printer

[Avi Kivity]

On 09/12/2011 12:01 PM, Jan Kiszka wrote:
> On 2011-09-12 08:43, Richard Henderson wrote:
> >  On 09/11/2011 09:31 PM, Blue Swirl wrote:
> >>  Field 'offset' is always zero, maybe that is not interesting. Will it
> >>  become one day?
> >
> >  It's not always zero, but only used by certain devices.
>
> I do not see any users, neither upstream nor in Avi's tree.

There aren't.

> To my (semi-)understanding, offset should correlate to region_offset of
> cpu_register_physical_memory_offset: legacy device models require this
> to be 0 as they expect an absolute memory address passed to their
> handler, in contrast to a normal one that is relative to the regions
> base. But I do not see how the memory region offset actually helps here.
>

mr->offset is added to the address in memory_region_{read,write}_thunk_n().

-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH] memory: simple memory tree printer

[Jan Kiszka]

On 2011-09-12 11:11, Avi Kivity wrote:
> On 09/12/2011 12:01 PM, Jan Kiszka wrote:
>> On 2011-09-12 08:43, Richard Henderson wrote:
>>>  On 09/11/2011 09:31 PM, Blue Swirl wrote:
>>>>  Field 'offset' is always zero, maybe that is not interesting. Will it
>>>>  become one day?
>>>
>>>  It's not always zero, but only used by certain devices.
>>
>> I do not see any users, neither upstream nor in Avi's tree.
> 
> There aren't.
> 
>> To my (semi-)understanding, offset should correlate to region_offset of
>> cpu_register_physical_memory_offset: legacy device models require this
>> to be 0 as they expect an absolute memory address passed to their
>> handler, in contrast to a normal one that is relative to the regions
>> base. But I do not see how the memory region offset actually helps here.
>>
> 
> mr->offset is added to the address in memory_region_{read,write}_thunk_n().

Ah, ok.

So the default address passed to the handler is now already relative? I
think we should keep it like this for all converted devices, ie. take
the chance, fix the remaining models, and drop the offset.

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Re: [Qemu-devel] [PATCH] memory: simple memory tree printer

[Richard Henderson]

On 09/12/2011 02:19 AM, Jan Kiszka wrote:
> On 2011-09-12 11:11, Avi Kivity wrote:
>> On 09/12/2011 12:01 PM, Jan Kiszka wrote:
>>> On 2011-09-12 08:43, Richard Henderson wrote:
>>>>  On 09/11/2011 09:31 PM, Blue Swirl wrote:
>>>>>  Field 'offset' is always zero, maybe that is not interesting. Will it
>>>>>  become one day?
>>>>
>>>>  It's not always zero, but only used by certain devices.
>>>
>>> I do not see any users, neither upstream nor in Avi's tree.
>>
>> There aren't.
>>
>>> To my (semi-)understanding, offset should correlate to region_offset of
>>> cpu_register_physical_memory_offset: legacy device models require this
>>> to be 0 as they expect an absolute memory address passed to their
>>> handler, in contrast to a normal one that is relative to the regions
>>> base. But I do not see how the memory region offset actually helps here.
>>>
>>
>> mr->offset is added to the address in memory_region_{read,write}_thunk_n().
> 
> Ah, ok.
> 
> So the default address passed to the handler is now already relative? I
> think we should keep it like this for all converted devices, ie. take
> the chance, fix the remaining models, and drop the offset.

It's non-zero for the isa portio conversion that I did, which
I thought was in Avi's tree.

This is required by at least the VGA and GUS ISA devices which
do expect absolute i/o addresses, and check them.  The offset is
used to convert the relative i/o address back into an absolute
address.

It would also be used when we split an ISA portio region, as 
with the FDC device.  There, we have 7 ports in 2 chunks.  The
offset would still be needed to convert the relative offset of
the second chuck to be relative to the "real" un-split region.

Feel free to convert all of these devices to a more "native" use
of the memory api, but I warn you it won't be trivial.


r~

Re: [Qemu-devel] [PATCH] memory: simple memory tree printer

[Avi Kivity]

On 09/14/2011 06:10 PM, Richard Henderson wrote:
> >
> >  So the default address passed to the handler is now already relative? I
> >  think we should keep it like this for all converted devices, ie. take
> >  the chance, fix the remaining models, and drop the offset.
>
> It's non-zero for the isa portio conversion that I did, which
> I thought was in Avi's tree.

That's MemoryRegionPortio::offset, not MemoryRegion::offset.


-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH] memory: simple memory tree printer

[Richard Henderson]

On 09/14/2011 08:23 AM, Avi Kivity wrote:
> On 09/14/2011 06:10 PM, Richard Henderson wrote:
>> >
>> >  So the default address passed to the handler is now already relative? I
>> >  think we should keep it like this for all converted devices, ie. take
>> >  the chance, fix the remaining models, and drop the offset.
>>
>> It's non-zero for the isa portio conversion that I did, which
>> I thought was in Avi's tree.
> 
> That's MemoryRegionPortio::offset, not MemoryRegion::offset.

No, look in isa_register_portio_list:

        memory_region_init_io(region, ops, opaque, name, off_high - off_low);
        memory_region_set_offset(region, start + off_low);

That last line sets MemoryRegion::offset.


r~

Re: [Qemu-devel] [PATCH] memory: simple memory tree printer

[Avi Kivity]

On 09/14/2011 06:27 PM, Richard Henderson wrote:
> On 09/14/2011 08:23 AM, Avi Kivity wrote:
> >  On 09/14/2011 06:10 PM, Richard Henderson wrote:
> >>  >
> >>  >   So the default address passed to the handler is now already relative? I
> >>  >   think we should keep it like this for all converted devices, ie. take
> >>  >   the chance, fix the remaining models, and drop the offset.
> >>
> >>  It's non-zero for the isa portio conversion that I did, which
> >>  I thought was in Avi's tree.
> >
> >  That's MemoryRegionPortio::offset, not MemoryRegion::offset.
>
> No, look in isa_register_portio_list:
>
>          memory_region_init_io(region, ops, opaque, name, off_high - off_low);
>          memory_region_set_offset(region, start + off_low);
>
> That last line sets MemoryRegion::offset.
>

That's not in my queue.  Which patchset was that?

-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH] memory: simple memory tree printer

[Richard Henderson]

On 09/14/2011 08:29 AM, Avi Kivity wrote:
> That's not in my queue.  Which patchset was that?

On the list beginning at

  https://lists.gnu.org/archive/html/qemu-devel/2011-08/msg02732.html

to which you responded positively.  It's also at

  git://repo.or.cz/qemu/rth.git mem-api-isa


r~

Re: [Qemu-devel] [PATCH] memory: simple memory tree printer

[Avi Kivity]

On 09/14/2011 06:36 PM, Richard Henderson wrote:
> On 09/14/2011 08:29 AM, Avi Kivity wrote:
> >  That's not in my queue.  Which patchset was that?
>
> On the list beginning at
>
>    https://lists.gnu.org/archive/html/qemu-devel/2011-08/msg02732.html
>
> to which you responded positively.  It's also at
>
>    git://repo.or.cz/qemu/rth.git mem-api-isa
>
>

Whoops - forgot about it.  Applied now, thanks.

-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH] memory: simple memory tree printer

[Jan Kiszka]

On 2011-09-14 17:10, Richard Henderson wrote:
> On 09/12/2011 02:19 AM, Jan Kiszka wrote:
>> On 2011-09-12 11:11, Avi Kivity wrote:
>>> On 09/12/2011 12:01 PM, Jan Kiszka wrote:
>>>> On 2011-09-12 08:43, Richard Henderson wrote:
>>>>>  On 09/11/2011 09:31 PM, Blue Swirl wrote:
>>>>>>  Field 'offset' is always zero, maybe that is not interesting. Will it
>>>>>>  become one day?
>>>>>
>>>>>  It's not always zero, but only used by certain devices.
>>>>
>>>> I do not see any users, neither upstream nor in Avi's tree.
>>>
>>> There aren't.
>>>
>>>> To my (semi-)understanding, offset should correlate to region_offset of
>>>> cpu_register_physical_memory_offset: legacy device models require this
>>>> to be 0 as they expect an absolute memory address passed to their
>>>> handler, in contrast to a normal one that is relative to the regions
>>>> base. But I do not see how the memory region offset actually helps here.
>>>>
>>>
>>> mr->offset is added to the address in memory_region_{read,write}_thunk_n().
>>
>> Ah, ok.
>>
>> So the default address passed to the handler is now already relative? I
>> think we should keep it like this for all converted devices, ie. take
>> the chance, fix the remaining models, and drop the offset.
> 
> It's non-zero for the isa portio conversion that I did, which
> I thought was in Avi's tree.

Hmm, I wasn't looking at PIO yet as it was out of scope of the original
MMIO offset. But good to known.

> 
> This is required by at least the VGA and GUS ISA devices which
> do expect absolute i/o addresses, and check them.  The offset is
> used to convert the relative i/o address back into an absolute
> address.

If those are the only users, let's adjust them.

> 
> It would also be used when we split an ISA portio region, as 
> with the FDC device.  There, we have 7 ports in 2 chunks.  The
> offset would still be needed to convert the relative offset of
> the second chuck to be relative to the "real" un-split region.

That sounds a lot like it only affects the flattened representation, and
there an offset does make sense. But we are discussing the original
memory regions here that should not require it (provided the above
cleanup is applicable).

> 
> Feel free to convert all of these devices to a more "native" use
> of the memory api, but I warn you it won't be trivial.

Do you mean VGA and GUS or FDC?

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Re: [Qemu-devel] [PATCH] memory: simple memory tree printer

[Jan Kiszka]

On 2011-09-14 19:58, Jan Kiszka wrote:
> On 2011-09-14 17:10, Richard Henderson wrote:
>> On 09/12/2011 02:19 AM, Jan Kiszka wrote:
>>> On 2011-09-12 11:11, Avi Kivity wrote:
>>>> On 09/12/2011 12:01 PM, Jan Kiszka wrote:
>>>>> On 2011-09-12 08:43, Richard Henderson wrote:
>>>>>>  On 09/11/2011 09:31 PM, Blue Swirl wrote:
>>>>>>>  Field 'offset' is always zero, maybe that is not interesting. Will it
>>>>>>>  become one day?
>>>>>>
>>>>>>  It's not always zero, but only used by certain devices.
>>>>>
>>>>> I do not see any users, neither upstream nor in Avi's tree.
>>>>
>>>> There aren't.
>>>>
>>>>> To my (semi-)understanding, offset should correlate to region_offset of
>>>>> cpu_register_physical_memory_offset: legacy device models require this
>>>>> to be 0 as they expect an absolute memory address passed to their
>>>>> handler, in contrast to a normal one that is relative to the regions
>>>>> base. But I do not see how the memory region offset actually helps here.
>>>>>
>>>>
>>>> mr->offset is added to the address in memory_region_{read,write}_thunk_n().
>>>
>>> Ah, ok.
>>>
>>> So the default address passed to the handler is now already relative? I
>>> think we should keep it like this for all converted devices, ie. take
>>> the chance, fix the remaining models, and drop the offset.
>>
>> It's non-zero for the isa portio conversion that I did, which
>> I thought was in Avi's tree.
> 
> Hmm, I wasn't looking at PIO yet as it was out of scope of the original
> MMIO offset. But good to known.

OK, let's try again: Do we have to model hierarchy in PIO address space
at all? I don't think so. Rather, devices dispatch the full address
range, thus are aware of the absolute addresses they listen to.

So we are supposed to pass absolute addresses down to the handlers
anyway, and offset is useless for PIO as well.

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Re: [Qemu-devel] [PATCH] memory: simple memory tree printer

[Avi Kivity]

On 09/14/2011 09:10 PM, Jan Kiszka wrote:
> OK, let's try again: Do we have to model hierarchy in PIO address space
> at all? I don't think so.


We do.  A device listens to addresses 0x100-0x110.  Another BAR (at 
0x106) clips this to 0x100-0x106.  The pci/pci bridge clips this to 
0x105-0x106.  The host pci bridge remaps this as 
0x1000000105-0x1000000106 in the memory address space space.  But 
someone configured a cpu-local region at this address, so the cpu can't 
reach it at all.

-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.

Re: [Qemu-devel] [PATCH] memory: simple memory tree printer

[Jan Kiszka]

On 2011-09-14 21:24, Avi Kivity wrote:
> On 09/14/2011 09:10 PM, Jan Kiszka wrote:
>> OK, let's try again: Do we have to model hierarchy in PIO address space
>> at all? I don't think so.
> 
> 
> We do.  A device listens to addresses 0x100-0x110.  Another BAR (at 
> 0x106) clips this to 0x100-0x106.  The pci/pci bridge clips this to 
> 0x105-0x106.

OK, but clipping does not require offsets as it does not register PIO
regions at different base addresses. It's a pure internal representation
when flatting the view.

>  The host pci bridge remaps this as 
> 0x1000000105-0x1000000106 in the memory address space space.  But 
> someone configured a cpu-local region at this address, so the cpu can't 
> reach it at all.

Mapping PIO into MMIO space is special as it needs an intermediate layer
(ie. translation handlers).

Anyway, the point is that there are device models out there
(specifically PCI devices) that already use relative PIO addresses and
models (specifically ISA) that still expect absolute addresses (/wrt to
the ISA base). I believe it is better to consolidate over one model
longterm, but we need a transition phase here as well. However, I'm
unsure yet if we really need MemoryRegion::offset for that and cannot
use MemoryRegionPortio::offset. Need to look into the details.

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Re: [Qemu-devel] [PATCH] memory: simple memory tree printer

[Avi Kivity]

On 09/15/2011 12:30 PM, Jan Kiszka wrote:
> On 2011-09-14 21:24, Avi Kivity wrote:
> >  On 09/14/2011 09:10 PM, Jan Kiszka wrote:
> >>  OK, let's try again: Do we have to model hierarchy in PIO address space
> >>  at all? I don't think so.
> >
> >
> >  We do.  A device listens to addresses 0x100-0x110.  Another BAR (at
> >  0x106) clips this to 0x100-0x106.  The pci/pci bridge clips this to
> >  0x105-0x106.
>
> OK, but clipping does not require offsets as it does not register PIO
> regions at different base addresses. It's a pure internal representation
> when flatting the view.

A hierarchy is needed.

>
> >   The host pci bridge remaps this as
> >  0x1000000105-0x1000000106 in the memory address space space.  But
> >  someone configured a cpu-local region at this address, so the cpu can't
> >  reach it at all.
>
> Mapping PIO into MMIO space is special as it needs an intermediate layer
> (ie. translation handlers).

Translation handlers aren't needed - you can simply add the pci pio 
region as a subregion of the mmio space.

> Anyway, the point is that there are device models out there
> (specifically PCI devices) that already use relative PIO addresses and
> models (specifically ISA) that still expect absolute addresses (/wrt to
> the ISA base). I believe it is better to consolidate over one model
> longterm, but we need a transition phase here as well. However, I'm
> unsure yet if we really need MemoryRegion::offset for that and cannot
> use MemoryRegionPortio::offset. Need to look into the details.

It would be good to get rid of MemoryRegion::offset.

-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH] memory: simple memory tree printer

[Jan Kiszka]

On 2011-09-15 11:53, Avi Kivity wrote:
>>>   The host pci bridge remaps this as
>>>  0x1000000105-0x1000000106 in the memory address space space.  But
>>>  someone configured a cpu-local region at this address, so the cpu can't
>>>  reach it at all.
>>
>> Mapping PIO into MMIO space is special as it needs an intermediate layer
>> (ie. translation handlers).
> 
> Translation handlers aren't needed - you can simply add the pci pio 
> region as a subregion of the mmio space.

>From the outer perspective. But internally, there is still
memory_region_iorange_read/write.

The point is that no use case actually justifies memory_region_set_offset.

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Re: [Qemu-devel] [PATCH] memory: simple memory tree printer

[Avi Kivity]

On 09/15/2011 01:18 PM, Jan Kiszka wrote:
> On 2011-09-15 11:53, Avi Kivity wrote:
> >>>    The host pci bridge remaps this as
> >>>   0x1000000105-0x1000000106 in the memory address space space.  But
> >>>   someone configured a cpu-local region at this address, so the cpu can't
> >>>   reach it at all.
> >>
> >>  Mapping PIO into MMIO space is special as it needs an intermediate layer
> >>  (ie. translation handlers).
> >
> >  Translation handlers aren't needed - you can simply add the pci pio
> >  region as a subregion of the mmio space.
>
>  From the outer perspective. But internally, there is still
> memory_region_iorange_read/write.

These will go away.

> The point is that no use case actually justifies memory_region_set_offset.
>

It's just a compatibility interface.  When everything is converted, and 
no users remain, we'll remove it.

-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH] memory: simple memory tree printer

[Richard Henderson]

On 09/14/2011 10:58 AM, Jan Kiszka wrote:
>> It would also be used when we split an ISA portio region, as 
>> with the FDC device.  There, we have 7 ports in 2 chunks.  The
>> offset would still be needed to convert the relative offset of
>> the second chuck to be relative to the "real" un-split region.
> 
> That sounds a lot like it only affects the flattened representation, and
> there an offset does make sense. But we are discussing the original
> memory regions here that should not require it (provided the above
> cleanup is applicable).

These *are* the original memory regions, not something flattened.
The legacy isa port numbers for FDC and IDE are, sadly, interleaved.

>> Feel free to convert all of these devices to a more "native" use
>> of the memory api, but I warn you it won't be trivial.
> 
> Do you mean VGA and GUS or FDC?

VGA and GUS are non-trivial.  FDC would be fairly easy.


r~

Re: [Qemu-devel] [PATCH] memory: simple memory tree printer

[Peter Maydell]

On 11 September 2011 21:31, Blue Swirl <blauwirbel@gmail.com> wrote:
> PPC
> memory
> system addr 00000000 off 00000000 size 7fffffffffffffff
> -vga.chain4 addr 000a0000 off 00000000 size 10000
> -macio addr 80880000 off 00000000 size 80000
> --macio-nvram addr 00060000 off 00000000 size 20000
> --pmac-ide addr 00020000 off 00000000 size 1000
> --(null) addr 00016000 off 00000000 size 0
> --escc-bar addr 00013000 off 00000000 size 40
> --dbdma addr 00008000 off 00000000 size 1000
> --heathrow-pic addr 00000000 off 00000000 size 1000

> NB: (null) does not look OK.

I think the NULL is the cuda memory region -- do you have
this patch applied?
http://patchwork.ozlabs.org/patch/113925/

-- PMM

Re: [Qemu-devel] [PATCH] memory: simple memory tree printer

[Avi Kivity]

On 09/11/2011 11:31 PM, Blue Swirl wrote:
> Add a monitor command 'info mtree' to show the memory hierarchy.
>

Does this turn the memory hierarchy into an ABI?  It shouldn't.  Things 
like BARs are immutable but if a BAR is internally composed of several 
regions, well that's no one's business.

I originally wanted to implement this via a gdb script.  This works even 
when all you have is a core dump.  But I can see it's useful on a live 
system.

-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH] memory: simple memory tree printer

[Jan Kiszka]

On 2011-09-12 10:46, Avi Kivity wrote:
> On 09/11/2011 11:31 PM, Blue Swirl wrote:
>> Add a monitor command 'info mtree' to show the memory hierarchy.
>>
> 
> Does this turn the memory hierarchy into an ABI?  It shouldn't.  Things
> like BARs are immutable but if a BAR is internally composed of several
> regions, well that's no one's business.

"info mtree" falls into the same category as "info qtree" or
"device_show": they expose useful but unstable internal structures. But
they also only exist for the human monitor, so their output is not an
ABI by our definition.

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Re: [Qemu-devel] [PATCH] memory: simple memory tree printer

[Avi Kivity]

On 09/12/2011 11:53 AM, Jan Kiszka wrote:
> On 2011-09-12 10:46, Avi Kivity wrote:
> >  On 09/11/2011 11:31 PM, Blue Swirl wrote:
> >>  Add a monitor command 'info mtree' to show the memory hierarchy.
> >>
> >
> >  Does this turn the memory hierarchy into an ABI?  It shouldn't.  Things
> >  like BARs are immutable but if a BAR is internally composed of several
> >  regions, well that's no one's business.
>
> "info mtree" falls into the same category as "info qtree" or
> "device_show": they expose useful but unstable internal structures. But
> they also only exist for the human monitor, so their output is not an
> ABI by our definition.

Fair enough.

-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH] memory: simple memory tree printer

[Gerd Hoffmann]

> I/O
> io addr 0000000000000000 off 0000000000000000 size 10000
> -e1000-io addr 000000000000c000 off 0000000000000000 size 40
> -piix-bmdma-container addr 000000000000c040 off 0000000000000000 size 10
> --bmdma addr 000000000000000c off 0000000000000000 size 4
> --piix-bmdma addr 0000000000000008 off 0000000000000000 size 4
> --bmdma addr 0000000000000004 off 0000000000000000 size 4
> --piix-bmdma addr 0000000000000000 off 0000000000000000 size 4
> -pci-conf-data addr 0000000000000cfc off 0000000000000000 size 4
> -pci-conf-idx addr 0000000000000cf8 off 0000000000000000 size 4

Could you put the (variable-length) name field last?  That should make 
the whole list more readable as the addresses are aligned then.

cheers,
   Gerd

Re: [Qemu-devel] [PATCH] memory: simple memory tree printer

[Avi Kivity]

On 09/12/2011 01:37 PM, Gerd Hoffmann wrote:
>> I/O
>> io addr 0000000000000000 off 0000000000000000 size 10000
>> -e1000-io addr 000000000000c000 off 0000000000000000 size 40
>> -piix-bmdma-container addr 000000000000c040 off 0000000000000000 size 10
>> --bmdma addr 000000000000000c off 0000000000000000 size 4
>> --piix-bmdma addr 0000000000000008 off 0000000000000000 size 4
>> --bmdma addr 0000000000000004 off 0000000000000000 size 4
>> --piix-bmdma addr 0000000000000000 off 0000000000000000 size 4
>> -pci-conf-data addr 0000000000000cfc off 0000000000000000 size 4
>> -pci-conf-idx addr 0000000000000cf8 off 0000000000000000 size 4
>
> Could you put the (variable-length) name field last?  That should make 
> the whole list more readable as the addresses are aligned then.
>

Makes sense.  Can adopt other features from /proc/iomem - like using 
start/end instead of start/length - makes it easier to see if an address 
fits in a range.

-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH] memory: simple memory tree printer

[Blue Swirl]

On Mon, Sep 12, 2011 at 10:53 AM, Avi Kivity <avi@redhat.com> wrote:
> On 09/12/2011 01:37 PM, Gerd Hoffmann wrote:
>>>
>>> I/O
>>> io addr 0000000000000000 off 0000000000000000 size 10000
>>> -e1000-io addr 000000000000c000 off 0000000000000000 size 40
>>> -piix-bmdma-container addr 000000000000c040 off 0000000000000000 size 10
>>> --bmdma addr 000000000000000c off 0000000000000000 size 4
>>> --piix-bmdma addr 0000000000000008 off 0000000000000000 size 4
>>> --bmdma addr 0000000000000004 off 0000000000000000 size 4
>>> --piix-bmdma addr 0000000000000000 off 0000000000000000 size 4
>>> -pci-conf-data addr 0000000000000cfc off 0000000000000000 size 4
>>> -pci-conf-idx addr 0000000000000cf8 off 0000000000000000 size 4
>>
>> Could you put the (variable-length) name field last?  That should make the
>> whole list more readable as the addresses are aligned then.
>>
>
> Makes sense.  Can adopt other features from /proc/iomem - like using
> start/end instead of start/length - makes it easier to see if an address
> fits in a range.

Nice ideas, thanks.