From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:49767) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UjsDP-0003sT-Fi for qemu-devel@nongnu.org; Tue, 04 Jun 2013 10:28:00 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1UjsDO-0003Tw-1R for qemu-devel@nongnu.org; Tue, 04 Jun 2013 10:27:59 -0400 Received: from mx1.redhat.com ([209.132.183.28]:9908) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1UjsDN-0003Tp-Ou for qemu-devel@nongnu.org; Tue, 04 Jun 2013 10:27:57 -0400 Message-ID: <51ADF960.9030208@redhat.com> Date: Tue, 04 Jun 2013 16:27:44 +0200 From: Paolo Bonzini MIME-Version: 1.0 References: <1370348041-6768-1-git-send-email-pbonzini@redhat.com> <1370348041-6768-7-git-send-email-pbonzini@redhat.com> <51ADDE1A.4030709@redhat.com> <51ADEAA6.2050509@redhat.com> In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v2 06/17] sysbus: add sysbus_pass_mmio List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Peter Maydell Cc: qemu-devel@nongnu.org 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