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