Re: [Qemu-devel] [PATCH 00/15] Memory/IOMMU patches part 4: region ownership

[Paolo Bonzini]

Il 03/06/2013 11:22, Peter Maydell ha scritto:
>>> Who owns it at that point? [That's a legitimate thing to do, I think,
>>> though I don't suppose anybody does it at the moment.
>>> Sysbus MMIOs aren't only for mapping in the system address
>>> space, they're a general way for one device to expose a
>>> MemoryRegion * for use by another device.]
>>
>> I don't think it is legitimate, MMIO regions are just for use via
>> sysbus_map_mmio.
> 
> This is definitely not true.

Indeed a wrong generalization.  (Though it is becomes almost true if you
replace sysbus_map_mmio with memory_region_add_subregion, for which
sysbus_map_mmio is a simple wrapper).

> We already make extensive use
> of MMIO regions other than simply directly via sysbus_map_mmio.
> Exposing a MemoryRegion* is just saying "here is something I
> have which is some kind of memory mapped IO, do whatever you
> need to with it" (which might be mapping it directly to the
> system address space, or might be passing it to some other
> device that wants a MemoryRegion*, or might be putting it in
> a container MR or otherwise managing it). For example,
> arm11mpcore.c does this:
>    sysbus_init_mmio(dev, sysbus_mmio_get_region(s->priv, 0));
> which I suspect will assert with your patches.

Thanks for the pointer.  All other occurrences of
sys_bus_mmio_get_region are either in non-qdevified OMAP code, or they
do what I said in my patch.

I'll fix a11mpcore to use an alias.

>> The right thing to do is to use a container or alias region, and put the
>> 1st region inside it.  Then the 1st region keeps its owner, and the
>> container/alias gets a new one.
> 
> I think the actual right fix is to make the creator of
> the MR specify its owner. Anything else is just going to
> have holes in it.

I think the rules I wrote down are easy to understand, and I'd really
like to avoid touching 783 instances of memory_region_init*.  The
patches say that devices doing their own stuff with regions are the
exception, not the rule.  Thus the bus functions (which already take a
DeviceState) are just as good a place to set ownership as
memory_region_init*.

Paolo