From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:38740) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Vkvbv-0002bO-ND for qemu-devel@nongnu.org; Mon, 25 Nov 2013 07:50:01 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Vkvbp-0002Rp-O3 for qemu-devel@nongnu.org; Mon, 25 Nov 2013 07:49:55 -0500 Received: from mx1.redhat.com ([209.132.183.28]:12620) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Vkvbp-0002Rk-FW for qemu-devel@nongnu.org; Mon, 25 Nov 2013 07:49:49 -0500 Message-ID: <52934759.3020204@redhat.com> Date: Mon, 25 Nov 2013 13:49:29 +0100 From: Paolo Bonzini MIME-Version: 1.0 References: <1385001528-12003-1-git-send-email-imammedo@redhat.com> <1385001528-12003-3-git-send-email-imammedo@redhat.com> In-Reply-To: <1385001528-12003-3-git-send-email-imammedo@redhat.com> Content-Type: text/plain; charset=ISO-8859-15 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 02/27] rename pci_hotplug_fn to hotplug_fn and make it available for other devices List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Igor Mammedov Cc: peter.maydell@linaro.org, mdroth@linux.vnet.ibm.com, mst@redhat.com, mjt@tls.msk.ru, stefanb@linux.vnet.ibm.com, stefanha@redhat.com, armbru@redhat.com, qemu-devel@nongnu.org, vasilis.liaskovitis@profitbricks.com, quintela@redhat.com, chegu_vinod@hp.com, kraxel@redhat.com, aliguori@amazon.com, hutao@cn.fujitsu.com, marcel.a@redhat.com, lcapitulino@redhat.com, afaerber@suse.de Il 21/11/2013 03:38, Igor Mammedov ha scritto: > +typedef enum { > + HOTPLUG_DISABLED, > + HOTPLUG_ENABLED, > + COLDPLUG_ENABLED, > +} HotplugState; > + > +typedef int (*hotplug_fn)(DeviceState *hotplug_dev, DeviceState *dev, > + HotplugState state); I don't think this is a particularly useful interface, because it reinvents a lot of what already exists in qdev/qbus. The cold/hotplug_enabled differentiation is not particularly useful when dev->hotplugged already exists, and the interface leaves one to wonder why COLDPLUG_ENABLED doesn't have a matching COLDPLUG_DISABLED (until one looks at the code). Take for example this: static void enable_device(PIIX4PMState *s, int slot) { s->ar.gpe.sts[0] |= PIIX4_PCI_HOTPLUG_STATUS; s->pci0_slot_device_present |= (1U << slot); } static void disable_device(PIIX4PMState *s, int slot) { s->ar.gpe.sts[0] |= PIIX4_PCI_HOTPLUG_STATUS; s->pci0_status.down |= (1U << slot); } static int piix4_device_hotplug(DeviceState *qdev, PCIDevice *dev, PCIHotplugState state) { int slot = PCI_SLOT(dev->devfn); PIIX4PMState *s = PIIX4_PM(qdev); /* Don't send event when device is enabled during qemu machine creation: * it is present on boot, no hotplug event is necessary. We do send an * event when the device is disabled later. */ if (state == PCI_COLDPLUG_ENABLED) { s->pci0_slot_device_present |= (1U << slot); return 0; } if (state == PCI_HOTPLUG_ENABLED) { enable_device(s, slot); } else { disable_device(s, slot); } pm_update_sci(s); return 0; } In my opinion, it is much clearer this way---separating enable/disabled rather than hotplug/coldplug: static void piix4_pci_send_event(PIIX4PMState *s) { s->ar.gpe.sts[0] |= PIIX4_PCI_HOTPLUG_STATUS; pm_update_sci(s); } static int piix4_device_hotplug(DeviceState *qdev, DeviceState *dev) { PCIDevice *pci = PCI_DEVICE(dev); int slot = PCI_SLOT(pci->devfn); PIIX4PMState *s = PIIX4_PM(qdev); s->pci0_slot_device_present |= (1U << slot); /* Don't send event when device is enabled during qemu machine creation: * it is present on boot, no hotplug event is necessary. We do send an * event when the device is disabled later. */ if (dev->hotplugged) { piix4_pci_send_event(s); } return 0; } static int piix4_device_hot_unplug(DeviceState *qdev, DeviceState *dev) { PCIDevice *pci = PCI_DEVICE(dev); int slot = PCI_SLOT(pci->devfn); PIIX4PMState *s = PIIX4_PM(qdev); s->pci0_status.down |= (1U << slot); piix4_pci_send_event(s); return 0; } This is of course just an example, I'm not asking you to reimplement hotplug for all buses in QEMU. However, my point is that this shouldn't be a "core" enum and interface. If you want to have a core interface in BusClass, I'd rather add ->hotplug and ->hot_unplug to BusClass, similar to how the SCSI bus does it (note: I only maintain it, I didn't write that code) and so that BusClass's methods match ->init/->exit on the device side. Paolo