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.

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

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

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().

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.

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

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~

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.

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~

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

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

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~

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.

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

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.

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

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.