From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:56959) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Vq8Aw-0005uS-3A for qemu-devel@nongnu.org; Mon, 09 Dec 2013 16:15:40 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Vq8Aq-0003Fx-1W for qemu-devel@nongnu.org; Mon, 09 Dec 2013 16:15:33 -0500 Received: from mx1.redhat.com ([209.132.183.28]:44203) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Vq8Ap-0003Fi-Pd for qemu-devel@nongnu.org; Mon, 09 Dec 2013 16:15:27 -0500 Date: Mon, 9 Dec 2013 22:15:14 +0100 From: Igor Mammedov Message-ID: <20131209221514.18acca99@thinkpad> In-Reply-To: <52A5FB72.8080307@redhat.com> References: <1386349395-5710-1-git-send-email-imammedo@redhat.com> <1386349395-5710-8-git-send-email-imammedo@redhat.com> <52A20996.8020308@redhat.com> <20131209144139.7f9ac562@nial.usersys.redhat.com> <52A5C9D4.7080404@redhat.com> <20131209151431.57d49bd4@nial.usersys.redhat.com> <52A5D573.7010803@redhat.com> <20131209160834.59bfdc98@nial.usersys.redhat.com> <52A5DEE9.9030901@redhat.com> <20131209174856.5609760d@nial.usersys.redhat.com> <52A5FB72.8080307@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 7/7] hw/pci: convert PCI bus to use "hotplug-device" interface. List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Paolo Bonzini Cc: peter.maydell@linaro.org, ehabkost@redhat.com, mst@redhat.com, marcel.a@redhat.com, qemu-devel@nongnu.org, blauwirbel@gmail.com, alex.williamson@redhat.com, anthony@codemonkey.ws, afaerber@suse.de On Mon, 09 Dec 2013 18:18:42 +0100 Paolo Bonzini wrote: > Il 09/12/2013 17:48, Igor Mammedov ha scritto: > >> > > >> > We could have separate check/plug methods. Only check can fail, it must > >> > be idempotent, and it can be invoked while the device is still unrealized. > > Reasons I've stated before apply to 'check' as well, for only specific device knows > > when it's fine to call it. That would just replace "plug" with "check" in realize() > > for not much of benefit. > > Check is idempotent, and can be called before realize makes any change > (it could also be called after the device is added to > /machine/unattached, it's not a big difference). > > Plug is called after realize. PCIE case: "check" before realize will work since code that does check depends only on hotplug device (i.e. PCIE slot) and do not access not yet realized device at all. however SHPC case: check code access pci_slot that is derived from PCIDevice.devfn, which in turn could be initialized in realize() (see pci_qdev_init() devfn auto allocation). So it's not possible to call check before realize() it should be called from realize(). Perhaps other hotplug buses/devices have similar limitations, where it's not fine to access device state from outside before calling it's realize(), so it should be some post_realize() hook then to make it generic which leads to the following: if ->plug() called after realize() fails, all we need to do is to fail "realize" property setter. That should cause qdev_device_add() -> object_unparent() -> device_unparent() -> unrealize() doing all necessary cleanup. > > >> > > >> > The reason I liked the interface, is because it removes the need for > >> > each bus to add its own plug/unplug handling. > > ^^^ > > Have you meant device instead of bus? > > I meant each bus-specific abstract class (PCIDevice, SCSIDevice, etc.). > > > It's still improvement over current PCI hotplug code and allows to simplify > > other buses as well by removing callbacks from them. > > Exactly. But so far you don't get any benefit: no removal of PCI > hotplug code, no removal of allow_hotunplug. What I'm proposing, I > believe, has a small cost and already starts the transition (which I > believe we can complete for 2.0). > > > The only way to call callbacks from DEVICE.realize()/unplug(), I see, is if we make > > them all nofail, then it would be safe to call them in "realize" property setter. > > But we have at least 2 callbacks that can fail: > > pcie_cap_slot_hotplug() and shpc_device_hotplug() > > Both of them can be handled by a "check" step in the handler. > > > Goal of this series was to add and demonstrate reusable hotplug interface as > > opposed to PCI specific or SCSI-bus specific ones, so it could be used for memory > > hotplug as well. It might not do everything we would like but it looks like a move > > the right direction. > > If it's wrong direction, I could drop the idea and fallback to an original > > less intrusive approach, taking in account your comment to move type definitions > > into separate header. > > No, absolutely. I think it's the right direction, I just think more > aspects of it should be made generic. > > Paolo > -- Regards, Igor