Il 04/06/2013 16:11, Peter Maydell ha scritto: > We've already got a working implementation, in the shape > of sysbus_mmio_get_region(). This is exactly the right way > to do this API -- we have one API which says "give me a > MemoryRegion*" and one which says "I have a MemoryRegion*, > please expose it". All I'm asking you to do is not break it.
I can add a conditional to sysbus_add_mmio if you prefer. I think it's uglier, but I can live with it. > I still don't understand them. Why should "hey, please use > this MMIO region as a PCI BAR" imply "and by the way set the > ownership"? Why does "here's an MMIO region which should be > exposed to users of this device" imply "and by the way > set the ownership"? Because both places are _already_ tying a MemoryRegion to a device. >> By contrast, changing 800 >> invocations of the functions would be impossible to review seriously, it >> would have to be redone when boards are qdev/QOM-ified, would be worse >> for submitters of new boards. > > If it's not clear who ought to be the owner when mmio_init_region > is called then there are problems anyway. It is clear, but this doesn't make a mechanical-but-not-quite patch easy to review. It's not that I cannot do it. I simply believe it is a worse choice. >> There are an order of magnitude less calls to memory_region_set_owner >> than to memory_region_init_*. > > That's because you've optimised for "minimise number of places > to put calls". The downside is it's much harder to review new > patches. An owner parameter to the mmio_init_region* functions > means (a) it's impossible to forget to set the owner (b) it's > easy to check when looking at the patch whether the owner is > appropriate (c) you don't have to worry about weird cases > where something else might try to set the owner later. That's true, I cannot deny that. > As a concrete example, if somebody submitted cirrus_vga > as a new driver, I have no idea how to tell that it needs > to set the owner for its memory regions, when 99% of > other devices don't. I think this is going to result in > "forgot to set owner" bugs. Because cirrus is adding regions directly to address_space_memory/io. As documented: * The device must set the owner itself * only if it uses memory_region_add_subregion directly on some address * space, or after the parent region is passed to the bus (for example * dynamically while the device runs). Paolo
